c++, v2: Diagnose [basic.scope.block]/2 violations even for block externs [PR52953]

  On Thu, Aug 31, 2023 at 05:46:28PM -0400, Jason Merrill wrote:
> I've suggested this to Core.

Thanks.

> > So, I'm not really sure what to do.  Intuitively the patch seems right
> > because even block externs redeclare stuff and change meaning of the
> > identifiers and void foo () { int i; extern int i (int); } is rejected
> > by all compilers.
> 
> I think this direction makes sense, though we might pedwarn on these rather
> than error to reduce possible breakage.

It wasn't clear to me whether you want to make those pedwarns just for the
DECL_EXTERNAL cases, ones that actually changed, or all others as well
(which were errors or permerrors depending on the case).
I've implemented the former, kept existing behavior of !DECL_EXTERNAL.

> > 2023-08-31  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR c++/52953
> > 	* name-lookup.cc (check_local_shadow): Defer punting on
> > 	DECL_EXTERNAL (decl) from the start of function to right before
> > 	the -Wshadow* checks.
> 
> Don't we want to consider externs for the -Wshadow* checks as well?

I think that is a good idea (though dunno how much it will trigger in
real-world), but there is one case I've excluded, the global variable
shadowing case, because warning that
int z;
void foo () { extern int z; z = 1; }
shadows the global var would be incorrect, it is the same var.
It is true that
int y; namespace N { void bar () { extern int y; y = 1; } }
shadows ::y but it is unclear how to differentiate those two cases with
the information we have at check_local_shadow time.

I've also found one spot which wasn't using auto_diagnostic_group d;
on a pair of error_at/inform.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

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

	PR c++/52953
	* name-lookup.cc (check_local_shadow): Don't punt early for
	DECL_EXTERNAL decls, instead just disable the shadowing of namespace
	decls check for those and emit a pedwarn rather than error_at for
	those.  Add missing auto_diagnostic_group.  Formatting fix.

	* g++.dg/diagnostic/redeclaration-4.C: New test.
	* g++.dg/diagnostic/redeclaration-5.C: New test.
	* g++.dg/warn/Wshadow-19.C: New test.



	Jakub
  

On 9/1/23 09:34, Jakub Jelinek wrote:
> On Thu, Aug 31, 2023 at 05:46:28PM -0400, Jason Merrill wrote:
>> I've suggested this to Core.
> 
> Thanks.
> 
>>> So, I'm not really sure what to do.  Intuitively the patch seems right
>>> because even block externs redeclare stuff and change meaning of the
>>> identifiers and void foo () { int i; extern int i (int); } is rejected
>>> by all compilers.
>>
>> I think this direction makes sense, though we might pedwarn on these rather
>> than error to reduce possible breakage.
> 
> It wasn't clear to me whether you want to make those pedwarns just for the
> DECL_EXTERNAL cases, ones that actually changed, or all others as well
> (which were errors or permerrors depending on the case).
> I've implemented the former, kept existing behavior of !DECL_EXTERNAL.
> 
>>> 2023-08-31  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR c++/52953
>>> 	* name-lookup.cc (check_local_shadow): Defer punting on
>>> 	DECL_EXTERNAL (decl) from the start of function to right before
>>> 	the -Wshadow* checks.
>>
>> Don't we want to consider externs for the -Wshadow* checks as well?
> 
> I think that is a good idea (though dunno how much it will trigger in
> real-world), but there is one case I've excluded, the global variable
> shadowing case, because warning that
> int z;
> void foo () { extern int z; z = 1; }
> shadows the global var would be incorrect, it is the same var.
> It is true that
> int y; namespace N { void bar () { extern int y; y = 1; } }
> shadows ::y but it is unclear how to differentiate those two cases with
> the information we have at check_local_shadow time.
> 
> I've also found one spot which wasn't using auto_diagnostic_group d;
> on a pair of error_at/inform.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

> 2023-09-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/52953
> 	* name-lookup.cc (check_local_shadow): Don't punt early for
> 	DECL_EXTERNAL decls, instead just disable the shadowing of namespace
> 	decls check for those and emit a pedwarn rather than error_at for
> 	those.  Add missing auto_diagnostic_group.  Formatting fix.
> 
> 	* g++.dg/diagnostic/redeclaration-4.C: New test.
> 	* g++.dg/diagnostic/redeclaration-5.C: New test.
> 	* g++.dg/warn/Wshadow-19.C: New test.
> 
> --- gcc/cp/name-lookup.cc.jj	2023-09-01 10:21:03.658118594 +0200
> +++ gcc/cp/name-lookup.cc	2023-09-01 11:30:10.868516494 +0200
> @@ -3096,10 +3096,6 @@ check_local_shadow (tree decl)
>     if (TREE_CODE (decl) == PARM_DECL && !DECL_CONTEXT (decl))
>       return;
>   
> -  /* External decls are something else.  */
> -  if (DECL_EXTERNAL (decl))
> -    return;
> -
>     tree old = NULL_TREE;
>     cp_binding_level *old_scope = NULL;
>     if (cxx_binding *binding = outer_binding (DECL_NAME (decl), NULL, true))
> @@ -3130,11 +3126,9 @@ check_local_shadow (tree decl)
>   	      && DECL_CONTEXT (old) == lambda_function (current_lambda_expr ())
>   	      && TREE_CODE (old) == PARM_DECL
>   	      && DECL_NAME (decl) != this_identifier)
> -	    {
> -	      error_at (DECL_SOURCE_LOCATION (old),
> -			"lambda parameter %qD "
> -			"previously declared as a capture", old);
> -	    }
> +	    error_at (DECL_SOURCE_LOCATION (old),
> +		      "lambda parameter %qD "
> +		      "previously declared as a capture", old);
>   	  return;
>   	}
>         /* Don't complain if it's from an enclosing function.  */
> @@ -3156,10 +3150,18 @@ check_local_shadow (tree decl)
>   	     in the outermost block of the function definition.  */
>   	  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);
> +	      auto_diagnostic_group d;
> +	      bool emit = true;
> +	      if (DECL_EXTERNAL (decl))
> +		emit = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wpedantic,
> +				"declaration of %q#D shadows a parameter",
> +				decl);
> +	      else
> +		error_at (DECL_SOURCE_LOCATION (decl),
> +			  "declaration of %q#D shadows a parameter", decl);
> +	      if (emit)
> +		inform (DECL_SOURCE_LOCATION (old),
> +			"%q#D previously declared here", old);
>   	      return;
>   	    }
>   	}
> @@ -3185,10 +3187,16 @@ check_local_shadow (tree decl)
>   	       && (old_scope->kind == sk_cond || old_scope->kind == sk_for))
>   	{
>   	  auto_diagnostic_group d;
> -	  error_at (DECL_SOURCE_LOCATION (decl),
> -		    "redeclaration of %q#D", decl);
> -	  inform (DECL_SOURCE_LOCATION (old),
> -		  "%q#D previously declared here", old);
> +	  bool emit = true;
> +	  if (DECL_EXTERNAL (decl))
> +	    emit = pedwarn (DECL_SOURCE_LOCATION (decl), OPT_Wpedantic,
> +			    "redeclaration of %q#D", decl);
> +	  else
> +	    error_at (DECL_SOURCE_LOCATION (decl),
> +		      "redeclaration of %q#D", decl);
> +	  if (emit)
> +	    inform (DECL_SOURCE_LOCATION (old),
> +		    "%q#D previously declared here", old);
>   	  return;
>   	}
>         /* C++11:
> @@ -3314,6 +3322,7 @@ check_local_shadow (tree decl)
>   	  || (TREE_CODE (old) == TYPE_DECL
>   	      && (!DECL_ARTIFICIAL (old)
>   		  || TREE_CODE (decl) == TYPE_DECL)))
> +      && !DECL_EXTERNAL (decl)
>         && !instantiating_current_function_p ()
>         && !warning_suppressed_p (decl, OPT_Wshadow))
>       /* XXX shadow warnings in outer-more namespaces */
> --- gcc/testsuite/g++.dg/diagnostic/redeclaration-4.C.jj	2023-09-01 10:46:15.646025458 +0200
> +++ gcc/testsuite/g++.dg/diagnostic/redeclaration-4.C	2023-09-01 10:46:15.646025458 +0200
> @@ -0,0 +1,167 @@
> +// PR c++/52953
> +// { dg-do compile }
> +// { dg-options "-pedantic-errors -Wno-switch-unreachable" }
> +
> +void
> +foo (int x)				// { dg-message "'int x' previously declared here" }
> +{
> +  extern int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
> +}
> +
> +void
> +bar (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +  extern int x;				// { dg-error "declaration of 'int x' shadows a parameter" }
> +}
> +catch (...)
> +{
> +}
> +
> +volatile int v;
> +
> +void
> +baz ()
> +{
> +#if __cplusplus >= 201103L
> +  auto f = [] (int x) { extern 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" }
> +    {
> +      extern int x;			// { dg-error "redeclaration of 'int x'" }
> +    }
> +  if (int x = 0)			// { dg-message "'int x' previously declared here" }
> +    ;
> +  else
> +    {
> +      extern int x;			// { dg-error "redeclaration of 'int x'" }
> +    }
> +  if (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    extern int x;			// { dg-error "redeclaration of 'int x'" }
> +  if (int x = 0)			// { dg-message "'int x' previously declared here" }
> +    ;
> +  else
> +    extern int x;			// { dg-error "redeclaration of 'int x'" }
> +  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    {
> +      extern int x;			// { dg-error "redeclaration of 'int x'" }
> +    default:;
> +    }
> +  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    extern int x;			// { dg-error "redeclaration of 'int x'" }
> +  while (int x = v)
> +    {
> +      extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
> +    }
> +  while (int x = v)
> +    extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
> +  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
> +    {
> +      extern int x;			// { dg-error "redeclaration of 'int x'" }
> +    }
> +  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
> +    extern int x;			// { dg-error "redeclaration of 'int x'" }
> +  for (; int x = v; )
> +    {
> +      extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
> +    }
> +  for (; int x = v; )
> +    extern int x;			// { dg-error "'int x' conflicts with a previous declaration" }
> +  try
> +    {
> +    }
> +  catch (int x)				// { dg-message "'int x' previously declared here" }
> +    {
> +      extern int x;			// { dg-error "redeclaration of 'int x'" }
> +    }
> +}
> +
> +void
> +corge (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (...)
> +{
> +  extern int x;				// { dg-error "redeclaration of 'int x'" }
> +}
> +
> +void
> +fred (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (int)
> +{
> +}
> +catch (long)
> +{
> +  extern int x;				// { dg-error "redeclaration of 'int x'" }
> +}
> +
> +void
> +garply (int x)
> +{
> +  try
> +    {
> +      extern int x;
> +    }
> +  catch (...)
> +    {
> +      extern int x;
> +    }
> +}
> +
> +struct S
> +{
> +  S (int x)				// { dg-message "'int x' previously declared here" }
> +  try : s (x)
> +  {
> +    extern 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 (...)
> +  {
> +    extern int x;				// { dg-error "redeclaration of 'int x'" }
> +  }
> +  int t;
> +};
> +
> +struct U
> +{
> +  U (int x) : u (x)
> +  {
> +    try
> +    {
> +      extern int x;
> +    }
> +    catch (...)
> +    {
> +      extern int x;
> +    }
> +  }
> +  int u;
> +};
> +
> +struct V
> +{
> +  V (int x) : v (x)
> +  {
> +    {
> +      extern int x;
> +    }
> +  }
> +  int v;
> +};
> --- gcc/testsuite/g++.dg/diagnostic/redeclaration-5.C.jj	2023-09-01 10:46:15.646025458 +0200
> +++ gcc/testsuite/g++.dg/diagnostic/redeclaration-5.C	2023-09-01 10:46:15.646025458 +0200
> @@ -0,0 +1,167 @@
> +// PR c++/52953
> +// { dg-do compile }
> +// { dg-options "-pedantic-errors -Wno-switch-unreachable" }
> +
> +void
> +foo (int x)				// { dg-message "'int x' previously declared here" }
> +{
> +  extern int x (int);			// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" }
> +}
> +
> +void
> +bar (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +  extern int x (int);			// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" }
> +}
> +catch (...)
> +{
> +}
> +
> +volatile int v;
> +
> +void
> +baz ()
> +{
> +#if __cplusplus >= 201103L
> +  auto f = [] (int x) { extern int x (int); };// { dg-error "declaration of 'int x\\\(int\\\)' 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" }
> +    {
> +      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +    }
> +  if (int x = 0)			// { dg-message "'int x' previously declared here" }
> +    ;
> +  else
> +    {
> +      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +    }
> +  if (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +  if (int x = 0)			// { dg-message "'int x' previously declared here" }
> +    ;
> +  else
> +    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    {
> +      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +    default:;
> +    }
> +  switch (int x = 1)			// { dg-message "'int x' previously declared here" }
> +    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +  while (int x = v)
> +    {
> +      extern int x (int);		// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
> +    }
> +  while (int x = v)
> +    extern int x (int);			// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
> +  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
> +    {
> +      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +    }
> +  for (int x = v; x; ++x)		// { dg-message "'int x' previously declared here" }
> +    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +  for (; int x = v; )
> +    {
> +      extern int x (int);		// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
> +    }
> +  for (; int x = v; )
> +    extern int x (int);			// { dg-error "'int x\\\(int\\\)' redeclared as different kind of entity" }
> +  try
> +    {
> +    }
> +  catch (int x)				// { dg-message "'int x' previously declared here" }
> +    {
> +      extern int x (int);		// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +    }
> +}
> +
> +void
> +corge (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (...)
> +{
> +  extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +}
> +
> +void
> +fred (int x)				// { dg-message "'int x' previously declared here" }
> +try
> +{
> +}
> +catch (int)
> +{
> +}
> +catch (long)
> +{
> +  extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +}
> +
> +void
> +garply (int x)
> +{
> +  try
> +    {
> +      extern int x (int);
> +    }
> +  catch (...)
> +    {
> +      extern int x (int);
> +    }
> +}
> +
> +struct S
> +{
> +  S (int x)				// { dg-message "'int x' previously declared here" }
> +  try : s (x)
> +  {
> +    extern int x (int);			// { dg-error "declaration of 'int x\\\(int\\\)' shadows a parameter" }
> +  }
> +  catch (...)
> +  {
> +  }
> +  int s;
> +};
> +
> +struct T
> +{
> +  T (int x)				// { dg-message "'int x' previously declared here" }
> +  try : t (x)
> +  {
> +  }
> +  catch (...)
> +  {
> +    extern int x (int);			// { dg-error "redeclaration of 'int x\\\(int\\\)'" }
> +  }
> +  int t;
> +};
> +
> +struct U
> +{
> +  U (int x) : u (x)
> +  {
> +    try
> +    {
> +      extern int x (int);
> +    }
> +    catch (...)
> +    {
> +      extern int x (int);
> +    }
> +  }
> +  int u;
> +};
> +
> +struct V
> +{
> +  V (int x) : v (x)
> +  {
> +    {
> +      extern int x (int);
> +    }
> +  }
> +  int v;
> +};
> --- gcc/testsuite/g++.dg/warn/Wshadow-19.C.jj	2023-09-01 11:35:21.092200057 +0200
> +++ gcc/testsuite/g++.dg/warn/Wshadow-19.C	2023-09-01 11:37:15.997598483 +0200
> @@ -0,0 +1,27 @@
> +// { dg-do compile }
> +// { dg-options "-Wshadow" }
> +
> +void
> +foo (int x)
> +{
> +  int y = 1;
> +  {
> +    extern int x;				// { dg-warning "declaration of 'int x' shadows a parameter" }
> +    extern int y;				// { dg-warning "declaration of 'y' shadows a previous local" }
> +  }
> +#if __cplusplus >= 201102L
> +  auto fn = [x] () { extern int x; return 0; };	// { dg-warning "declaration of 'x' shadows a lambda capture" "" { target c++11 } }
> +#endif
> +}
> +
> +int z;
> +
> +struct S
> +{
> +  int x;
> +  void foo ()
> +  {
> +    extern int x;				// { dg-warning "declaration of 'x' shadows a member of 'S'" }
> +    extern int z;
> +  }
> +};
> 
> 
> 	Jakub
>