[Clang][Sema] Fix invalid redefinition error in if/switch/for statement

[junaire]

Summary

Because lambda will create its own scope, so we shouldn't consider variables
defined with the same name as redefinition error, if the new one is in a
lambda.

After this patch, clang is supposed to accept code below:

void foo() {

for (int x = [] { int x = 0; return x; }(); ;) ;

}

Fixes https://github.com/llvm/llvm-project/issues/54913

[junaire]

Hi @erichkeane , thanks for accepting this! But do you think we need to add a release note about it? (I'm a totally beginner so sometimes I'm confused about when we should do it.) Also, Do you have any concerns about the wording? In fact, I am pretty embarrassed by the current wording in my commit message...

[erichkeane]

Ah, right, we SHOULD have a release note, please add one and I'll help review the wording. Commit message is fine for a commit message, particularly with a good release note.

[junaire]

Add a release note.

[junaire]

In D123840#3453903, @erichkeane wrote:

Ah, right, we SHOULD have a release note, please add one and I'll help review the wording. Commit message is fine for a commit message, particularly with a good release note.

Thanks! I have updated it :)

[junaire]

I also find the code below really awkward:

S->getParent()->getFlags() & Scope::ControlScope

I find that we have helpers like Scope::isBlockScope() Scope::isTryScope() and etc, but no Scope::isControlScope and Scope::isFnTryCatchScope

I will add these helpers as a NFC patch after this one, that won't need a release note or review right?

[erichkeane]

I will add these helpers as a NFC patch after this one, that won't need a release note or review right?

I found that awkward as well, and almost requested that you fix this, but decided against it. I'm ok with this being an NFC patch. It would be nice to grep for all uses of the getFlags that compares with ControlScope and FnTryCatchScope to see if we can replace those uses as well.

clang/docs/ReleaseNotes.rst

122

I think this isn't accurate, right? I thought this had to do with lambdas alone? Is the following accurate?

`Clang should no longer incorrectly diagnose a variable declaration inside of a lambda expression inside the scope of a if/while/for/switch init statement as a redeclaration. This fixes ...'.

[junaire]

Fix bad wording.

[erichkeane]

clang/docs/ReleaseNotes.rst

123

Sorry, re-read it:

Clang should no longer incorrectly diagnose a variable declaration inside of a lambda expression that shares the name of a variable in a containing if/while/for/switch init statement as a redeclaration.

Feel free to commit with that wording.

[junaire]

clang/docs/ReleaseNotes.rst

122

I think this isn't accurate, right? I thought this had to do with lambdas alone? Is the following accurate?

`Clang should no longer incorrectly diagnose a variable declaration inside of a lambda expression inside the scope of a if/while/for/switch init statement as a redeclaration. This fixes ...'.

Thanks for the suggestion, It's my fault, I really shouldn't be sleeping in my English grammar class...

[junaire]

Update release note.

[paulkirth]

Hi I think this patch is the root cause of https://github.com/llvm/llvm-project/issues/54968

The issue first appears in our builders here:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8816803488685972465/overview

based on the blamelist (https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8816803488685972465/blamelist) I'm fairly certain that this change is responsible.

I'm guessing that there is some additional case the new branch condition is missing that causes the assertion failure to manifest.

If this will be hard to fix, can you revert this patch until one is ready?

clang/docs/ReleaseNotes.rst

 - Statement expressions are now disabled in default arguments in general.
   This fixes Issue `Issue 53488 <https://github.com/llvm/llvm-project/issues/53488>`_.
 - According to `CWG 1394 <https://wg21.link/cwg1394>`_ and 
   `C++20 [dcl.fct.def.general]p2 <https://timsong-cpp.github.io/cppwp/n4868/dcl.fct.def#general-2.sentence-3>`_,
   Clang should not diagnose incomplete types in function definitions if the function body is "= delete;".
   This fixes Issue `Issue 52802 <https://github.com/llvm/llvm-project/issues/52802>`_.
 - Unknown type attributes with a ``[[]]`` spelling are no longer diagnosed twice.
   This fixes Issue `Issue 54817 <https://github.com/llvm/llvm-project/issues/54817>`_.
+- Clang should no longer incorrectly diagnose a variable declaration inside of
+  a lambda expression that shares the name of a variable in a containing
+  if/while/for/switch init statement as a redeclaration.
+  This fixes `Issue 54913 <https://github.com/llvm/llvm-project/issues/54913>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/lib/Sema/IdentifierResolver.cpp

if (LangOpt.CPlusPlus) {
       // Names declared in the for-init-statement, and in the condition of if,
       // while, for, and switch statements are local to the if, while, for, or
       // switch statement (including the controlled statement), and shall not be
       // redeclared in a subsequent condition of that statement nor in the
       // outermost block (or, for the if statement, any of the outermost blocks)
       // of the controlled statement.
       //
       assert(S->getParent() && "No TUScope?");
-      if (S->getParent()->getFlags() & Scope::ControlScope) {
+      // If the current decl is in a lambda, we shouldn't consider this is a
+      // redefinition as lambda has its own scope.
+      if (S->getParent()->getFlags() & Scope::ControlScope &&
+          !S->isFunctionScope()) {
         S = S->getParent();
         if (S->isDeclScope(D))
           return true;
       }
       if (S->getFlags() & Scope::FnTryCatchScope)
         return S->getParent()->isDeclScope(D);
     }
     return false;

clang/test/SemaCXX/cxx1z-init-statement.cpp

void test_constexpr_init_stmt() {
   static_assert(!a, "");
   static_assert(constexpr_if_init(1), "");
 
   constexpr int b = constexpr_switch_init(-1);
   static_assert(b == 1, "");
   static_assert(constexpr_switch_init(-2) == 0, "");
   static_assert(constexpr_switch_init(-5) == -1, "");
 }
+
+int test_lambda_init() {
+  if (int x = []() {int x = 42; return x; }(); x) {
+  };
+
+  switch (int y = []() {int y = 42; return y; }(); y) {
+  case 42:
+    return 1;
+  }
+
+  for (int x = [] { int x = 0; return x; }();;)
+    ;
+
+  return 0;
+}