c++, v2: Diagnose [basic.scope.block]/2 violations even in compound-stmt of function-try-block [PR52953]

  On Thu, Aug 31, 2023 at 03:52:22PM -0400, Jason Merrill wrote:
> On 8/31/23 03:20, Jakub Jelinek wrote:
> > As the following testcase shows, while check_local_shadow diagnoses most of
> > the [basic.scope.block]/2 violations, it doesn't diagnose when parameter's
> > name is redeclared inside of the compound-stmt of a function-try-block.
> > 
> > There is in that case an extra scope (sk_try with parent artificial
> > sk_block with for FUNCTION_NEEDS_BODY_BLOCK another sk_block and only then
> > sk_function_param).
> > 
> > The following patch fixes that.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > 2023-08-31  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c++/52953
> > 	* cp-tree.h (struct language_function): Add x_in_function_try_block
> > 	member.
> 
> How about adding a flag to cp_binding_level instead?  Maybe to mark the
> artificial sk_block level as such, which we could use for both this case and
> the FUNCTION_NEEDS_BODY_BLOCK cases.

So like this?

It actually changes behaviour on the
void foo (int x) try {} catch (int x) {} case, where previously
this triggered the
               || (TREE_CODE (old) == PARM_DECL
                   && (current_binding_level->kind == sk_catch
                       || current_binding_level->level_chain->kind == sk_catch)
                   && in_function_try_handler))
        {
          auto_diagnostic_group d;
          if (permerror (DECL_SOURCE_LOCATION (decl),
                         "redeclaration of %q#D", decl))
            inform (DECL_SOURCE_LOCATION (old),
                    "%q#D previously declared here", old);
diagnostics (note, just the current_binding_level->kind == sk_catch
case), while now it triggers already the earlier
          if (b->kind == sk_function_parms)
            {
              error_at (DECL_SOURCE_LOCATION (decl),
                        "declaration of %q#D shadows a parameter", decl);
              inform (DECL_SOURCE_LOCATION (old),
                      "%q#D previously declared here", old);
error.  If you think it is important to differentiate that,
I guess I could guard the while (b->artificial) loop with say
+	  if (!in_function_try_handler
+	      || current_binding_level->kind != sk_catch)
	    while (b->artificial)
	      b = b->level_chain;
and adjust the 2 testcases.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
or with modification?

2023-09-01  Jakub Jelinek  <jakub@redhat.com>

	PR c++/52953
	* name-lookup.h (struct cp_binding_level): Add artificial bit-field.
	Formatting fixes.
	* name-lookup.cc (check_local_shadow): Skip artificial bindings when
	checking if parameter scope is parent scope.  Don't special case
	FUNCTION_NEEDS_BODY_BLOCK.
	* decl.cc (begin_function_body): Set
	current_binding_level->artificial.
	* semantics.cc (begin_function_try_block): Likewise.

	* g++.dg/diagnostic/redeclaration-3.C: New test.
	* g++.dg/parse/pr31952-3.C: Expect different diagnostic wording.



	Jakub
  

On 9/1/23 09:24, Jakub Jelinek wrote:
> On Thu, Aug 31, 2023 at 03:52:22PM -0400, Jason Merrill wrote:
>> On 8/31/23 03:20, Jakub Jelinek wrote:
>>> As the following testcase shows, while check_local_shadow diagnoses most of
>>> the [basic.scope.block]/2 violations, it doesn't diagnose when parameter's
>>> name is redeclared inside of the compound-stmt of a function-try-block.
>>>
>>> There is in that case an extra scope (sk_try with parent artificial
>>> sk_block with for FUNCTION_NEEDS_BODY_BLOCK another sk_block and only then
>>> sk_function_param).
>>>
>>> The following patch fixes that.
>>>
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>>
>>> 2023-08-31  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR c++/52953
>>> 	* cp-tree.h (struct language_function): Add x_in_function_try_block
>>> 	member.
>>
>> How about adding a flag to cp_binding_level instead?  Maybe to mark the
>> artificial sk_block level as such, which we could use for both this case and
>> the FUNCTION_NEEDS_BODY_BLOCK cases.
> 
> So like this?
> 
> It actually changes behaviour on the
> void foo (int x) try {} catch (int x) {} case, where previously
> this triggered the
>                 || (TREE_CODE (old) == PARM_DECL
>                     && (current_binding_level->kind == sk_catch
>                         || current_binding_level->level_chain->kind == sk_catch)
>                     && in_function_try_handler))
>          {
>            auto_diagnostic_group d;
>            if (permerror (DECL_SOURCE_LOCATION (decl),
>                           "redeclaration of %q#D", decl))
>              inform (DECL_SOURCE_LOCATION (old),
>                      "%q#D previously declared here", old);
> diagnostics (note, just the current_binding_level->kind == sk_catch
> case), while now it triggers already the earlier
>            if (b->kind == sk_function_parms)
>              {
>                error_at (DECL_SOURCE_LOCATION (decl),
>                          "declaration of %q#D shadows a parameter", decl);
>                inform (DECL_SOURCE_LOCATION (old),
>                        "%q#D previously declared here", old);
> error.  If you think it is important to differentiate that,
> I guess I could guard the while (b->artificial) loop with say
> +	  if (!in_function_try_handler
> +	      || current_binding_level->kind != sk_catch)
> 	    while (b->artificial)
> 	      b = b->level_chain;
> and adjust the 2 testcases.

I don't mind the change of diagnostic.  Can we remove the 
in_function_try_handler test, then, if it's no longer reachable?  OK 
with that change.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk
> or with modification?
> 
> 2023-09-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/52953
> 	* name-lookup.h (struct cp_binding_level): Add artificial bit-field.
> 	Formatting fixes.
> 	* name-lookup.cc (check_local_shadow): Skip artificial bindings when
> 	checking if parameter scope is parent scope.  Don't special case
> 	FUNCTION_NEEDS_BODY_BLOCK.
> 	* decl.cc (begin_function_body): Set
> 	current_binding_level->artificial.
> 	* semantics.cc (begin_function_try_block): Likewise.
> 
> 	* g++.dg/diagnostic/redeclaration-3.C: New test.
> 	* g++.dg/parse/pr31952-3.C: Expect different diagnostic wording.
> 
> --- gcc/cp/name-lookup.h.jj	2023-08-21 11:57:33.105460770 +0200
> +++ gcc/cp/name-lookup.h	2023-09-01 10:15:20.137943395 +0200
> @@ -292,11 +292,11 @@ struct GTY(()) cp_binding_level {
>         only valid if KIND == SK_TEMPLATE_PARMS.  */
>     BOOL_BITFIELD explicit_spec_p : 1;
>   
> -  /* true means make a BLOCK for this level regardless of all else.  */
> +  /* True means make a BLOCK for this level regardless of all else.  */
>     unsigned keep : 1;
>   
>     /* Nonzero if this level can safely have additional
> -      cleanup-needing variables added to it.  */
> +     cleanup-needing variables added to it.  */
>     unsigned more_cleanups_ok : 1;
>     unsigned have_cleanups : 1;
>   
> @@ -308,9 +308,13 @@ struct GTY(()) cp_binding_level {
>     unsigned defining_class_p : 1;
>   
>     /* True for SK_FUNCTION_PARMS of a requires-expression.  */
> -  unsigned requires_expression: 1;
> +  unsigned requires_expression : 1;
>   
> -  /* 22 bits left to fill a 32-bit word.  */
> +  /* True for artificial blocks which should be ignored when finding
> +     parent scope.  */
> +  unsigned artificial : 1;
> +
> +  /* 21 bits left to fill a 32-bit word.  */
>   };
>   
>   /* The binding level currently in effect.  */
> --- gcc/cp/name-lookup.cc.jj	2023-08-31 14:31:06.055762306 +0200
> +++ gcc/cp/name-lookup.cc	2023-09-01 10:21:03.658118594 +0200
> @@ -3146,8 +3146,10 @@ check_local_shadow (tree decl)
>   	     them there.  */
>   	  cp_binding_level *b = current_binding_level->level_chain;
>   
> -	  if (FUNCTION_NEEDS_BODY_BLOCK (current_function_decl))
> -	    /* Skip the ctor/dtor cleanup level.  */
> +	  /* Skip artificially added scopes which aren't present
> +	     in the C++ standard, e.g. for function-try-block or
> +	     ctor/dtor cleanups.  */
> +	  while (b->artificial)
>   	    b = b->level_chain;
>   
>   	  /* [basic.scope.param] A parameter name shall not be redeclared
> --- gcc/cp/decl.cc.jj	2023-08-31 20:48:21.127722180 +0200
> +++ gcc/cp/decl.cc	2023-09-01 10:18:40.125134543 +0200
> @@ -18002,6 +18002,7 @@ begin_function_body (void)
>       keep_next_level (true);
>   
>     tree stmt = begin_compound_stmt (BCS_FN_BODY);
> +  current_binding_level->artificial = 1;
>   
>     if (processing_template_decl)
>       /* Do nothing now.  */;
> --- gcc/cp/semantics.cc.jj	2023-08-31 14:31:06.102761661 +0200
> +++ gcc/cp/semantics.cc	2023-09-01 10:17:49.044851974 +0200
> @@ -1624,6 +1624,7 @@ begin_function_try_block (tree *compound
>     /* This outer scope does not exist in the C++ standard, but we need
>        a place to put __FUNCTION__ and similar variables.  */
>     *compound_stmt = begin_compound_stmt (0);
> +  current_binding_level->artificial = 1;
>     r = begin_try_block ();
>     FN_TRY_BLOCK_P (r) = 1;
>     return r;
> --- gcc/testsuite/g++.dg/diagnostic/redeclaration-3.C.jj	2023-09-01 09:40:12.061707730 +0200
> +++ gcc/testsuite/g++.dg/diagnostic/redeclaration-3.C	2023-09-01 10:32:36.315429575 +0200
> @@ -0,0 +1,204 @@
> +// PR c++/52953
> +// { dg-do compile }
> +// { dg-options "-pedantic-errors -Wno-switch-unreachable" }
> +
> +void
> +foo (int x)				// { dg-message "'int x' previously declared here" }
> +{
> +  int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
> +}
> +
> +void
> +bar (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +  int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
> +}
> +catch (...)
> +{
> +}
> +
> +volatile int v;
> +
> +void
> +baz ()
> +{
> +#if __cplusplus >= 201103L
> +  auto f = [] (int x) { int x; };	// { dg-error "declaration of 'int x' shadows a parameter" "" { target c++11 } }
> +					// { dg-message "'int x' previously declared here" "" { target c++11 } .-1 }
> +#endif
> +  if (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    }
> +  if (int x = 0)			// { dg-message "'int x' previously declared here" }
> +    ;
> +  else
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    }
> +  if (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  if (int x = 0)			// { dg-message "'int x' previously declared here" }
> +    ;
> +  else
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    default:;
> +    }
> +  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  while (int x = v)			// { dg-message "'int x' previously declared here" }
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    }
> +  while (int x = v)			// { dg-message "'int x' previously declared here" }
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    }
> +  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  for (; int x = v; )			// { dg-message "'int x' previously declared here" }
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    }
> +  for (; int x = v; )			// { dg-message "'int x' previously declared here" }
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  try
> +    {
> +    }
> +  catch (int x)				// { dg-message "'int x' previously declared here" }
> +    {
> +      int x;				// { dg-error "redeclaration of 'int x'" }
> +    }
> +  if (int x = 1)
> +    if (int x = 1)
> +      ;
> +  if (int x = 0)
> +    ;
> +  else
> +    if (int x = 1)
> +      ;
> +  if (int x = 1)
> +    switch (int x = 1)
> +      ;
> +  if (int x = 0)
> +    while (int x = v)
> +      ;
> +  if (int x = 0)
> +    for (int x = v; x; ++x)
> +      ;
> +  switch (int x = 1)
> +    switch (int x = 1)
> +      {
> +      case 1:;
> +      }
> +  while (int x = 0)
> +    if (int x = 1)
> +      ;
> +  for (int x = v; x; ++x)
> +    for (int x = v; x; ++x)
> +      ;
> +}
> +
> +void
> +qux (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (int x)				// { dg-error "declaration of 'int x' shadows a parameter" }
> +{
> +}
> +
> +void
> +corge (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (...)
> +{
> +  int x;				// { dg-error "redeclaration of 'int x'" }
> +}
> +
> +void
> +fred (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (int)
> +{
> +}
> +catch (long)
> +{
> +  int x;				// { dg-error "redeclaration of 'int x'" }
> +}
> +
> +void
> +garply (int x)
> +{
> +  try
> +    {
> +      int x;
> +    }
> +  catch (...)
> +    {
> +      int x;
> +    }
> +}
> +
> +struct S
> +{
> +  S (int x)				// { dg-message "'int x' previously declared here" }
> +  try : s (x)
> +  {
> +    int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
> +  }
> +  catch (...)
> +  {
> +  }
> +  int s;
> +};
> +
> +struct T
> +{
> +  T (int x)				// { dg-message "'int x' previously declared here" }
> +  try : t (x)
> +  {
> +  }
> +  catch (...)
> +  {
> +    int x;				// { dg-error "redeclaration of 'int x'" }
> +  }
> +  int t;
> +};
> +
> +struct U
> +{
> +  U (int x) : u (x)
> +  {
> +    try
> +    {
> +      int x;
> +    }
> +    catch (...)
> +    {
> +      int x;
> +    }
> +  }
> +  int u;
> +};
> +
> +struct V
> +{
> +  V (int x) : v (x)
> +  {
> +    {
> +      int x;
> +    }
> +  }
> +  int v;
> +};
> --- gcc/testsuite/g++.dg/parse/pr31952-3.C.jj	2020-01-14 20:02:46.918607812 +0100
> +++ gcc/testsuite/g++.dg/parse/pr31952-3.C	2023-09-01 14:41:12.306964831 +0200
> @@ -6,7 +6,7 @@ try
>   {
>     return 0;
>   }
> -catch (int bar)  // { dg-error "redeclaration" }
> +catch (int bar)  // { dg-error "shadows a parameter" }
>   {
>     return 1;
>   }
> 
> 
> 	Jakub
>