Bug 52953 - function parameter name redeclarations not detected
Summary: function parameter name redeclarations not detected
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.7.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: accepts-invalid, diagnostic
: 89616 111174 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-12 21:36 UTC by rigaje
Modified: 2023-09-06 13:44 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-08-19 00:00:00


Attachments
gcc14-pr52953-1.patch (1.53 KB, patch)
2023-08-30 18:22 UTC, Jakub Jelinek
Details | Diff
gcc14-pr52953-2.patch (1.48 KB, patch)
2023-08-30 18:28 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description rigaje 2012-04-12 21:36:29 UTC
source code demonstrating the problem.
------------------------------- BEGIN ----------------------------
void f (int i) try
{
 void i (); // 1
}
catch (...)
{
 void i (); // 2
}

int main ()
{
 return 0;
}
-------------------------------  END  ----------------------------

compiled with :
$HOME/gcc/4.7.0/bin/c++ -std=c++0x -Wall -O3 tt.cc

compiler output : 
nothing

g++-4.7.0 accepts the code as it is, issuing no warnings and no errors. I think this is wrong. According to c++11 3.3.3/2 
"A parameter name shall not be redeclared in the outermost block of the function definition nor in the outermost block of any handler associated with a function-try-block."

In the above example, i is the name of the parameter of function f. The name i, therefore, cannot be redeclared in the outermost block of the function definition (case 1) nor can it be redeclared in the outermost block of any associated handler (case 2). But my g++ accepts both cases while they should be rejected.
Comment 1 rigaje 2012-04-12 21:53:36 UTC
another example showing violation of c++11 3.3.3/4
---------------------------- BEGIN -------------------------------
int main ()
{
 if (int a = 1)
 {
  void a (); // 1
 }
 else
 {
  void a (); // 2
 }

 while (int a = 0)
 {
  void a (); // 3
 }

 for (int a = 0;;)
 {
  void a (); // 4
  break;
 }
 for (; int a = 0;)
 {
  void a (); // 5
 }

 return 0;
}
----------------------------  END  -------------------------------

According to my understanding, all numbered cases are illegal based on c++11 3.3.3/4.g++-4.7.0 correctly caught case 3 and 5 as compiler errors, the rest are accepted.
Comment 2 Paolo Carlini 2012-04-17 18:16:24 UTC
First blush, I'm not sure about 4, ICC doesn't error out either.
Comment 3 Eric Gallager 2017-08-19 18:32:22 UTC
(In reply to meng from comment #0)
> source code demonstrating the problem.
> ------------------------------- BEGIN ----------------------------
> void f (int i) try
> {
>  void i (); // 1
> }
> catch (...)
> {
>  void i (); // 2
> }
> 
> int main ()
> {
>  return 0;
> }
> -------------------------------  END  ----------------------------
> 
> compiled with :
> $HOME/gcc/4.7.0/bin/c++ -std=c++0x -Wall -O3 tt.cc
> 
> compiler output : 
> nothing
> 
> g++-4.7.0 accepts the code as it is, issuing no warnings and no errors. I
> think this is wrong. According to c++11 3.3.3/2 
> "A parameter name shall not be redeclared in the outermost block of the
> function definition nor in the outermost block of any handler associated
> with a function-try-block."
> 
> In the above example, i is the name of the parameter of function f. The name
> i, therefore, cannot be redeclared in the outermost block of the function
> definition (case 1) nor can it be redeclared in the outermost block of any
> associated handler (case 2). But my g++ accepts both cases while they should
> be rejected.

I can get some other warnings from this but not what you want:

$ /usr/local/bin/g++ -c -std=c++0x -Wall -O3 -Wextra -Wshadow -pedantic -Wmissing-declarations -Wredundant-decls 52953.c
52953.c:1:6: warning: no previous declaration for ‘void f(int)’ [-Wmissing-declarations]
 void f (int i) try
      ^
52953.c: In function ‘void f(int)’:
52953.c:1:13: warning: unused parameter ‘i’ [-Wunused-parameter]
 void f (int i) try
             ^
$

(In reply to meng from comment #1)
> another example showing violation of c++11 3.3.3/4
> ---------------------------- BEGIN -------------------------------
> int main ()
> {
>  if (int a = 1)
>  {
>   void a (); // 1
>  }
>  else
>  {
>   void a (); // 2
>  }
> 
>  while (int a = 0)
>  {
>   void a (); // 3
>  }
> 
>  for (int a = 0;;)
>  {
>   void a (); // 4
>   break;
>  }
>  for (; int a = 0;)
>  {
>   void a (); // 5
>  }
> 
>  return 0;
> }
> ----------------------------  END  -------------------------------
> 
> According to my understanding, all numbered cases are illegal based on c++11
> 3.3.3/4.g++-4.7.0 correctly caught case 3 and 5 as compiler errors, the rest
> are accepted.

Yup, 3 and 5 are still the only ones that error:

$ /usr/local/bin/g++ -c -std=c++0x -Wall -O3 -Wextra -Wshadow -pedantic -Wmissing-declarations -Wredundant-decls 52953_2.c
52953_2.c: In function ‘int main()’:
52953_2.c:3:10: warning: unused variable ‘a’ [-Wunused-variable]
  if (int a = 1)
          ^
52953_2.c:14:11: error: ‘void a()’ redeclared as different kind of symbol
   void a (); // 3
           ^
52953_2.c:12:13: note: previous declaration ‘int a’
  while (int a = 0)
             ^
52953_2.c:12:13: warning: unused variable ‘a’ [-Wunused-variable]
52953_2.c:17:11: warning: unused variable ‘a’ [-Wunused-variable]
  for (int a = 0;;)
           ^
52953_2.c:24:11: error: ‘void a()’ redeclared as different kind of symbol
   void a (); // 5
           ^
52953_2.c:22:13: note: previous declaration ‘int a’
  for (; int a = 0;)
             ^
52953_2.c:22:13: warning: unused variable ‘a’ [-Wunused-variable]
$

So, confirmed.
Comment 4 Eric Gallager 2019-11-20 05:45:05 UTC
cc-ing C++ FE maintainers
Comment 5 Andrew Pinski 2021-08-26 23:40:31 UTC
*** Bug 89616 has been marked as a duplicate of this bug. ***
Comment 6 Andrew Pinski 2023-08-27 18:25:19 UTC
*** Bug 111174 has been marked as a duplicate of this bug. ***
Comment 7 Jakub Jelinek 2023-08-30 18:22:35 UTC
Created attachment 55819 [details]
gcc14-pr52953-1.patch

Untested patch to diagnose invalid redeclaration of parameter in function-try-block.
Comment 8 Jakub Jelinek 2023-08-30 18:28:20 UTC
Created attachment 55820 [details]
gcc14-pr52953-2.patch

Untested incremental patch to diagnose even extern redeclarations in the
https://eel.is/c++draft/basic.scope#block-2 case.
I'm not convinced this second part is correct though and there seems to be
high implementation variance (g++ vs. clang++ vs. ICC vs. MSVC).
Because https://eel.is/c++draft/basic.scope#block-2 talks about declarations
whose target scope is the block scope of S, while
https://eel.is/c++draft/basic.scope#scope-2.10 says that extern block-scope
declarations target larger scope but bind a name in the immediate scope.
Comment 9 GCC Commits 2023-09-05 15:30:07 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:c24982689b8af19da9a64f2283fb99764a1c5db0

commit r14-3713-gc24982689b8af19da9a64f2283fb99764a1c5db0
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Sep 5 17:26:59 2023 +0200

    c++: Diagnose [basic.scope.block]/2 violations even in compound-stmt of function-try-block [PR52953]
    
    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 in_function_try_handler case doesn't work correctly
    void
    foo (int x)
    try {
    }
    catch (int)
    {
      try {
      } catch (int x)
      {
      }
      try {
      } catch (int)
      {
        int x;
      }
    }
    (which is valid) is rejected, because
                    || (TREE_CODE (old) == PARM_DECL
                        && (current_binding_level->kind == sk_catch
                            || current_binding_level->level_chain->kind == sk_catch)
                        && in_function_try_handler))
    is true but nothing verified that for the first case
    current_binding_level->level_chain->kind == sk_function_params
    (with perhaps artificial scopes in between and in the latter case
    with one extra level in between).
    
    The patch also changes behavior where for catch handlers of function-try-block
    the diagnostics will have the shadows function parameter wording as pedwarn
    rather than the old redeclaration permerror.
    
    2023-09-05  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.  Diagnose the in_function_try_handler
            cases in the b->kind == sk_function_parms test and verify no
            non-artificial intervening scopes.  Add missing auto_diagnostic_group.
            * decl.cc (begin_function_body): Set
            current_binding_level->artificial.
            * semantics.cc (begin_function_try_block): Likewise.
    
            * g++.dg/diagnostic/redeclaration-1.C: Expect different diagnostic
            wording.
            * g++.dg/diagnostic/redeclaration-3.C: New test.
            * g++.dg/parse/pr31952-1.C: Expect different diagnostic wording.
            * g++.dg/parse/pr31952-3.C: Likewise.
Comment 10 GCC Commits 2023-09-05 15:35:33 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:efafa66c294d261a4d964383674ab9ee51feaf88

commit r14-3714-gefafa66c294d261a4d964383674ab9ee51feaf88
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Tue Sep 5 17:31:12 2023 +0200

    c++: Diagnose [basic.scope.block]/2 violations even for block externs [PR52953]
    
    C++17 had in [basic.block.scope]/2
    "A parameter name shall not be redeclared in the outermost block of the function
    definition nor in the outermost block of any handler associated with a
    function-try-block."
    and in [basic.block.scope]/4 similar rule for selection/iteration
    statements.  My reading of that is that it applied even for block local
    externs in all those spots, while they declare something at namespace scope,
    the redeclaration happens in that outermost block etc. and introduces names
    into that.
    Those wordings seemed to have been moved somewhere else in C++20, but what's
    worse, they were moved back and completely rewritten in
    P1787R6: Declarations and where to find them
    which has been applied as a DR (but admittedly, we don't claim yet to
    implement that).
    The current wording at https://eel.is/c++draft/basic.scope#block-2
    and https://eel.is/c++draft/basic.scope#scope-2.10 seem to imply at least
    to me that it doesn't apply to extern block local decls because their
    target scope is the namespace scope and [basic.scope.block]/2 says
    "and whose target scope is the block scope"...
    Now, it is unclear if that is actually the intent or not.
    
    There seems to be quite large implementation divergence on this as well.
    
    Unpatched g++ e.g. on the redeclaration-5.C testcase diagnoses just
    lines 55,58,67,70 (i.e. where the previous declaration is in for's
    condition).
    
    clang++ trunk diagnoses just lines 8 and 27, i.e. redeclaration in the
    function body vs. parameter both in normal fn and lambda (but not e.g.
    function-try-block and others, including ctors, but it diagnoses those
    for non-extern decls).
    
    ICC 19 diagnoses lines 8,32,38,41,45,52,55,58,61,64,67,70,76.
    
    And MSCV trunk diagnoses 8,27,32,38,41,45,48,52,55,58,67,70,76,87,100,137
    although the last 4 are just warnings.
    
    g++ with the patch diagnoses
    8,15,27,32,38,41,45,48,52,55,58,61,64,67,70,76,87,100,121,137
    as the dg-error directives test.
    
    Jason said:
    > Yes, I suspect that should be
    >
    > If a declaration that is not a name-independent declaration and <del>whose
    > target scope is</del><ins>that binds a name in</ins> the block scope S of a
    >
    > which seems to also be needed to prohibit the already-diagnosed
    >
    > void f(int i) { union { int i; }; }
    > void g(int i) { enum { i }; }
    
    The following patch diagnoses DECL_EXTERNAL in check_local_shadow like
    !DECL_EXTERNAL, except that
    1) it uses pedwarn instead of errors for those cases
    2) it doesn't diagnose shadowing of namespace scope identifiers by block
       local externs, as they could be not actually shadowing but just redeclaring
       the same objects
    
    2023-09-05  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 or
            permerror for those.  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.
Comment 11 Jakub Jelinek 2023-09-05 15:42:16 UTC
Should be fixed now for GCC 14.
Comment 12 Gayathri Gottumukkala 2023-09-06 06:18:27 UTC
It is the duplicate of bug 111174. Arguments of function redeclared in the function definition which is not possible.
Comment 13 Jonathan Wakely 2023-09-06 13:44:49 UTC
Yes, Bug 111174 is already closed as a duplicate of this one.