[C++] PR c++/79133

Message ID CAFk2RUY6GAaErLo-joHP+UWkQwCSFM5zwa5p5WoyQd419BrKsQ@mail.gmail.com
State New
Headers show
Series
  • [C++] PR c++/79133
Related show

Commit Message

Ville Voutilainen July 6, 2018, 11:50 p.m.
Tested on Linux-PPC64. Ok for trunk, perhaps with the change
that I move the test under cpp1y, since it's a c++14 test anyway?

I considered pushing the captures into the parameter scope. I don't
know how to do that; changing the pushdecl_outermost_localscope
to a pushdecl doesn't seem to cut it; I guess that I should add
a new function into name-lookup.[ch], but I wonder whether
that makes sense, considering that this is lambda-only functionality.
I also wonder whether it makes more sense than the solution
in this patch, considering that we handle packs here as well
and capturepack/parampack, capturepack/param, capture/parampack
and capture/param clashes.

Guidance welcome. This approach has the benefit that it, well,
seems to work. :)

2018-07-07  Ville Voutilainen  <ville.voutilainen@gmail.com>

    gcc/cp/

    PR c++/79133
    * lambda.c (start_lambda_function): Reject captures and parameters
    with the same name.

    testsuite/

    PR c++/79133
    * g++.dg/cpp0x/lambda/lambda-shadow3.C: New.

Comments

Jason Merrill July 7, 2018, 1:15 p.m. | #1
Did you consider handling this in check_local_shadow?

On Fri, Jul 6, 2018 at 7:50 PM, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> Tested on Linux-PPC64. Ok for trunk, perhaps with the change
> that I move the test under cpp1y, since it's a c++14 test anyway?
>
> I considered pushing the captures into the parameter scope. I don't
> know how to do that; changing the pushdecl_outermost_localscope
> to a pushdecl doesn't seem to cut it; I guess that I should add
> a new function into name-lookup.[ch], but I wonder whether
> that makes sense, considering that this is lambda-only functionality.
> I also wonder whether it makes more sense than the solution
> in this patch, considering that we handle packs here as well
> and capturepack/parampack, capturepack/param, capture/parampack
> and capture/param clashes.
>
> Guidance welcome. This approach has the benefit that it, well,
> seems to work. :)
>
> 2018-07-07  Ville Voutilainen  <ville.voutilainen@gmail.com>
>
>     gcc/cp/
>
>     PR c++/79133
>     * lambda.c (start_lambda_function): Reject captures and parameters
>     with the same name.
>
>     testsuite/
>
>     PR c++/79133
>     * g++.dg/cpp0x/lambda/lambda-shadow3.C: New.
Paolo Carlini July 7, 2018, 6:12 p.m. | #2
Hi,

On 07/07/2018 01:50, Ville Voutilainen wrote:
> +	    error_at (DECL_SOURCE_LOCATION (parms),
> +		      "capture %qE and lambda parameter %qE "
> +		      "have the same name",
> +		      cap, parms);
Should we really print the same name twice? Looks like we don't have 
available (yet) a location for cap - that would likely enable fancy 
things - but in that case too I don't think the user would find that 
interesting seeing the same name twice. Also, we are using %E, thus we 
are pretty printing expressions - which in general we don't want to do - 
I see that in the case of cap it gives pretty obfuscated results for the 
last two tests (what the heck is __lambda3?!?). So, all in all, maybe 
print the name once, as parms, or something like that, for the time 
being? Or try to avoid %E altogether?

Paolo.
Ville Voutilainen July 7, 2018, 6:54 p.m. | #3
On 7 July 2018 at 16:15, Jason Merrill <jason@redhat.com> wrote:
> Did you consider handling this in check_local_shadow?

Roughly like this, not fully tested yet:

2018-07-07  Ville Voutilainen  <ville.voutilainen@gmail.com>

    gcc/cp/

    PR c++/79133
    * name-lookup.c (check_local_shadow): Reject captures and parameters
    with the same name.

    testsuite/

    PR c++/79133
    * g++.dg/cpp0x/lambda/lambda-shadow3.C: New.
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 3aafb0f..cc2d3c0 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -2640,6 +2640,7 @@ check_local_shadow (tree decl)
 		  || TREE_CODE (decl) == TYPE_DECL)))
       && DECL_FUNCTION_SCOPE_P (old)
       && (!DECL_ARTIFICIAL (decl)
+	  || is_capture_proxy (decl)
 	  || DECL_IMPLICIT_TYPEDEF_P (decl)
 	  || (VAR_P (decl) && DECL_ANON_UNION_VAR_P (decl))))
     {
@@ -2648,7 +2649,8 @@ check_local_shadow (tree decl)
       /* Don't complain if it's from an enclosing function.  */
       if (DECL_CONTEXT (old) == current_function_decl
 	  && TREE_CODE (decl) != PARM_DECL
-	  && TREE_CODE (old) == PARM_DECL)
+	  && TREE_CODE (old) == PARM_DECL
+	  && !is_capture_proxy (decl))
 	{
 	  /* Go to where the parms should be and see if we find
 	     them there.  */
@@ -2665,6 +2667,20 @@ check_local_shadow (tree decl)
 	      return;
 	    }
 	}
+      /* DR 2211: check that captures and parameters
+	 do not have the same name. */
+      else if (current_lambda_expr ()
+	       && DECL_CONTEXT (old) == lambda_function (current_lambda_expr ())
+	       && TREE_CODE (old) == PARM_DECL
+	       && is_capture_proxy (decl))
+	{
+	  if (DECL_NAME (decl) != this_identifier)
+	    error_at (DECL_SOURCE_LOCATION (old),
+		      "capture %qE and lambda parameter %qE "
+		      "have the same name",
+		      decl, old);
+	  return;
+	}
 
       /* The local structure or class can't use parameters of
 	 the containing function anyway.  */
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
new file mode 100644
index 0000000..b006470
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
@@ -0,0 +1,12 @@
+// { dg-do compile { target c++14 } }
+
+int main() {
+  int x = 42;
+  auto lambda = [x](int x) {}; // { dg-error "have the same name" }
+  auto lambda2 = [x=x](int x) {}; // { dg-error "have the same name" }
+  auto lambda3 = [x](auto... x) {}; // { dg-error "have the same name" }
+  auto lambda4 = [](auto... x) {
+    auto lambda5 = [x...](auto... x) {};  // { dg-error "have the same name" }
+    auto lambda6 = [x...](int x) {};  // { dg-error "have the same name" }
+  };
+}
Ville Voutilainen July 7, 2018, 6:55 p.m. | #4
On 7 July 2018 at 21:12, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Should we really print the same name twice? Looks like we don't have
> available (yet) a location for cap - that would likely enable fancy things -
> but in that case too I don't think the user would find that interesting
> seeing the same name twice. Also, we are using %E, thus we are pretty
> printing expressions - which in general we don't want to do - I see that in
> the case of cap it gives pretty obfuscated results for the last two tests
> (what the heck is __lambda3?!?). So, all in all, maybe print the name once,
> as parms, or something like that, for the time being? Or try to avoid %E
> altogether?

What should I print instead of %E?
Ville Voutilainen July 7, 2018, 7:05 p.m. | #5
On 7 July 2018 at 21:55, Ville Voutilainen <ville.voutilainen@gmail.com> wrote:
> On 7 July 2018 at 21:12, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Should we really print the same name twice? Looks like we don't have
>> available (yet) a location for cap - that would likely enable fancy things -
>> but in that case too I don't think the user would find that interesting
>> seeing the same name twice. Also, we are using %E, thus we are pretty
>> printing expressions - which in general we don't want to do - I see that in
>> the case of cap it gives pretty obfuscated results for the last two tests
>> (what the heck is __lambda3?!?). So, all in all, maybe print the name once,
>> as parms, or something like that, for the time being? Or try to avoid %E
>> altogether?
>
> What should I print instead of %E?

%qD instead of %qE, presumably. That seems to do the same thing in the
new patch, but I can sure change it.
Attached.
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 3aafb0f..b883054 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -2640,6 +2640,7 @@ check_local_shadow (tree decl)
 		  || TREE_CODE (decl) == TYPE_DECL)))
       && DECL_FUNCTION_SCOPE_P (old)
       && (!DECL_ARTIFICIAL (decl)
+	  || is_capture_proxy (decl)
 	  || DECL_IMPLICIT_TYPEDEF_P (decl)
 	  || (VAR_P (decl) && DECL_ANON_UNION_VAR_P (decl))))
     {
@@ -2648,7 +2649,8 @@ check_local_shadow (tree decl)
       /* Don't complain if it's from an enclosing function.  */
       if (DECL_CONTEXT (old) == current_function_decl
 	  && TREE_CODE (decl) != PARM_DECL
-	  && TREE_CODE (old) == PARM_DECL)
+	  && TREE_CODE (old) == PARM_DECL
+	  && !is_capture_proxy (decl))
 	{
 	  /* Go to where the parms should be and see if we find
 	     them there.  */
@@ -2665,6 +2667,20 @@ check_local_shadow (tree decl)
 	      return;
 	    }
 	}
+      /* DR 2211: check that captures and parameters
+	 do not have the same name. */
+      else if (current_lambda_expr ()
+	       && DECL_CONTEXT (old) == lambda_function (current_lambda_expr ())
+	       && TREE_CODE (old) == PARM_DECL
+	       && is_capture_proxy (decl))
+	{
+	  if (DECL_NAME (decl) != this_identifier)
+	    error_at (DECL_SOURCE_LOCATION (old),
+		      "capture %qD and lambda parameter %qD "
+		      "have the same name",
+		      decl, old);
+	  return;
+	}
 
       /* The local structure or class can't use parameters of
 	 the containing function anyway.  */
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
new file mode 100644
index 0000000..b006470
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
@@ -0,0 +1,12 @@
+// { dg-do compile { target c++14 } }
+
+int main() {
+  int x = 42;
+  auto lambda = [x](int x) {}; // { dg-error "have the same name" }
+  auto lambda2 = [x=x](int x) {}; // { dg-error "have the same name" }
+  auto lambda3 = [x](auto... x) {}; // { dg-error "have the same name" }
+  auto lambda4 = [](auto... x) {
+    auto lambda5 = [x...](auto... x) {};  // { dg-error "have the same name" }
+    auto lambda6 = [x...](int x) {};  // { dg-error "have the same name" }
+  };
+}
Ville Voutilainen July 7, 2018, 9:20 p.m. | #6
Needed one more tweak; when dealing with a capture proxy, always bail
out and never fall through to the warning-handling
code below the DR 2211 check.
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 3aafb0f..fee5482 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -2640,6 +2640,7 @@ check_local_shadow (tree decl)
 		  || TREE_CODE (decl) == TYPE_DECL)))
       && DECL_FUNCTION_SCOPE_P (old)
       && (!DECL_ARTIFICIAL (decl)
+	  || is_capture_proxy (decl)
 	  || DECL_IMPLICIT_TYPEDEF_P (decl)
 	  || (VAR_P (decl) && DECL_ANON_UNION_VAR_P (decl))))
     {
@@ -2648,7 +2649,8 @@ check_local_shadow (tree decl)
       /* Don't complain if it's from an enclosing function.  */
       if (DECL_CONTEXT (old) == current_function_decl
 	  && TREE_CODE (decl) != PARM_DECL
-	  && TREE_CODE (old) == PARM_DECL)
+	  && TREE_CODE (old) == PARM_DECL
+	  && !is_capture_proxy (decl))
 	{
 	  /* Go to where the parms should be and see if we find
 	     them there.  */
@@ -2665,6 +2667,20 @@ check_local_shadow (tree decl)
 	      return;
 	    }
 	}
+      /* DR 2211: check that captures and parameters
+	 do not have the same name. */
+      else if (is_capture_proxy (decl))
+	{
+	  if (current_lambda_expr ()
+	      && DECL_CONTEXT (old) == lambda_function (current_lambda_expr ())
+	      && TREE_CODE (old) == PARM_DECL
+	      && DECL_NAME (decl) != this_identifier)
+	    error_at (DECL_SOURCE_LOCATION (old),
+		      "capture %qD and lambda parameter %qD "
+		      "have the same name",
+		      decl, old);
+	  return;
+	}
 
       /* The local structure or class can't use parameters of
 	 the containing function anyway.  */
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
new file mode 100644
index 0000000..b006470
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
@@ -0,0 +1,12 @@
+// { dg-do compile { target c++14 } }
+
+int main() {
+  int x = 42;
+  auto lambda = [x](int x) {}; // { dg-error "have the same name" }
+  auto lambda2 = [x=x](int x) {}; // { dg-error "have the same name" }
+  auto lambda3 = [x](auto... x) {}; // { dg-error "have the same name" }
+  auto lambda4 = [](auto... x) {
+    auto lambda5 = [x...](auto... x) {};  // { dg-error "have the same name" }
+    auto lambda6 = [x...](int x) {};  // { dg-error "have the same name" }
+  };
+}
Paolo Carlini July 7, 2018, 9:35 p.m. | #7
Hi,

On 07/07/2018 23:20, Ville Voutilainen wrote:
> +	    error_at (DECL_SOURCE_LOCATION (old),
> +		      "capture %qD and lambda parameter %qD "
> +		      "have the same name",
> +		      decl, old);
Let's consider, say (with -Wshadow):

int main() {
   int x = 42;
   auto lambda0 = [x]() { int x; };
}

I'm thinking that the new diagnostic should be more consistent with it.

Paolo.
Ville Voutilainen July 7, 2018, 10:09 p.m. | #8
On 8 July 2018 at 00:35, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 07/07/2018 23:20, Ville Voutilainen wrote:
>>
>> +           error_at (DECL_SOURCE_LOCATION (old),
>> +                     "capture %qD and lambda parameter %qD "
>> +                     "have the same name",
>> +                     decl, old);
>
> Let's consider, say (with -Wshadow):
>
> int main() {
>   int x = 42;
>   auto lambda0 = [x]() { int x; };
> }
>
> I'm thinking that the new diagnostic should be more consistent with it.

There are a couple of errors that do an "error: redeclaration of foo"
followed by an inform "previously declared here". I would suggest that I do
an "error: declaration of parameter foo"
followed by an inform "previously declared as a capture here". How
does that sound?
That would make this more consistent with such a shadow warning, but I
don't want
to use the shadowing wording (which would be easy to do; just set
'shadowed' and do
a 'goto inform'), because this isn't shadowing in the precise sense;
the shadowing cases
are warnings, whereas this is more like the redeclaration errors in
the same function.
Paolo Carlini July 7, 2018, 10:54 p.m. | #9
Hi,

On 08/07/2018 00:09, Ville Voutilainen wrote:
> On 8 July 2018 at 00:35, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Hi,
>>
>> On 07/07/2018 23:20, Ville Voutilainen wrote:
>>> +           error_at (DECL_SOURCE_LOCATION (old),
>>> +                     "capture %qD and lambda parameter %qD "
>>> +                     "have the same name",
>>> +                     decl, old);
>> Let's consider, say (with -Wshadow):
>>
>> int main() {
>>    int x = 42;
>>    auto lambda0 = [x]() { int x; };
>> }
>>
>> I'm thinking that the new diagnostic should be more consistent with it.
> There are a couple of errors that do an "error: redeclaration of foo"
> followed by an inform "previously declared here". I would suggest that I do
> an "error: declaration of parameter foo"
> followed by an inform "previously declared as a capture here". How
> does that sound?
Sounds pretty good to me, I think this is the way to go but I'm not sure 
about the details...
> That would make this more consistent with such a shadow warning, but I
> don't want
> to use the shadowing wording (which would be easy to do; just set
> 'shadowed' and do
> a 'goto inform'), because this isn't shadowing in the precise sense;
> the shadowing cases
> are warnings, whereas this is more like the redeclaration errors in
> the same function.
... indeed and that annoys me a bit. Not having studied at all c++/79133 
so far (sorry) it seems a little weird to me that according to the 
standard we have to handle the two types of "shadowing" in different 
ways, one more strict, one less. Thus I would suggest double checking 
the details of that, eventually with Jason too in terms of the actual 
patch you would like to apply.

Paolo.
Ville Voutilainen July 7, 2018, 11:08 p.m. | #10
On 8 July 2018 at 01:54, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> That would make this more consistent with such a shadow warning, but I
>> don't want
>> to use the shadowing wording (which would be easy to do; just set
>> 'shadowed' and do
>> a 'goto inform'), because this isn't shadowing in the precise sense;
>> the shadowing cases
>> are warnings, whereas this is more like the redeclaration errors in
>> the same function.
>
> ... indeed and that annoys me a bit. Not having studied at all c++/79133 so
> far (sorry) it seems a little weird to me that according to the standard we
> have to handle the two types of "shadowing" in different ways, one more
> strict, one less. Thus I would suggest double checking the details of that,
> eventually with Jason too in terms of the actual patch you would like to
> apply.

Well. The PR is about DR 2211 which, in simple terms, says that lambda
parameters
and captures cannot have the same name. See
http://open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#2211

That's stricter than -Wshadow, but otherwise equally strict as the
other error cases already handled
in check_local_shadow. So I'll make this error case more consistent
with the others. We already
handle redeclaration errors slightly differently from shadowing
warnings in that function.

Patch

diff --git a/gcc/cp/lambda.c b/gcc/cp/lambda.c
index 3776d6b..534434a 100644
--- a/gcc/cp/lambda.c
+++ b/gcc/cp/lambda.c
@@ -1424,7 +1424,28 @@  start_lambda_function (tree fco, tree lambda_expr)
   /* Push the proxies for any explicit captures.  */
   for (tree cap = LAMBDA_EXPR_CAPTURE_LIST (lambda_expr); cap;
        cap = TREE_CHAIN (cap))
-    build_capture_proxy (TREE_PURPOSE (cap), TREE_VALUE (cap));
+    {
+      /* DR 2211: check that captures and parameters
+	 do not have the same name. */
+      for (tree parms = DECL_ARGUMENTS (fco); parms;
+	   parms = TREE_CHAIN (parms))
+	{
+	  tree real_cap = TREE_VALUE (cap);
+	  tree real_parms = parms;
+	  if (PACK_EXPANSION_P (real_cap))
+	    real_cap = PACK_EXPANSION_PATTERN (real_cap);
+	  if (PACK_EXPANSION_P (parms))
+	    real_parms = PACK_EXPANSION_PATTERN (parms);
+	  if (DECL_P (real_cap)
+	      && DECL_NAME (real_cap) != this_identifier
+	      && DECL_NAME (real_cap) == DECL_NAME (real_parms))
+	    error_at (DECL_SOURCE_LOCATION (parms),
+		      "capture %qE and lambda parameter %qE "
+		      "have the same name",
+		      cap, parms);
+	}
+      build_capture_proxy (TREE_PURPOSE (cap), TREE_VALUE (cap));
+    }
 
   return body;
 }
diff --git a/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
new file mode 100644
index 0000000..b006470
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/lambda/lambda-shadow3.C
@@ -0,0 +1,12 @@ 
+// { dg-do compile { target c++14 } }
+
+int main() {
+  int x = 42;
+  auto lambda = [x](int x) {}; // { dg-error "have the same name" }
+  auto lambda2 = [x=x](int x) {}; // { dg-error "have the same name" }
+  auto lambda3 = [x](auto... x) {}; // { dg-error "have the same name" }
+  auto lambda4 = [](auto... x) {
+    auto lambda5 = [x...](auto... x) {};  // { dg-error "have the same name" }
+    auto lambda6 = [x...](int x) {};  // { dg-error "have the same name" }
+  };
+}