[clang] Inject IndirectFieldDecl even if name conflicts.

[keinflue]

This modifies InjectAnonymousStructOrUnionMembers to inject an IndirectFieldDecl and mark it invalid even if its name conflicts with another name in the scope.

This resolves a crash on a further diagnostic diag::err_multiple_mem_union_initialization which via findDefaultInitializer relies on these declarations being present.

Fixes #149985

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (keinflue)

Changes

This modifies InjectAnonymousStructOrUnionMembers to inject an IndirectFieldDecl and mark it invalid even if its name conflicts with another name in the scope.

This resolves a crash on a further diagnostic diag::err_multiple_mem_union_initialization which via findDefaultInitializer relies on these declarations being present.

Fixes #149985

---

Full diff: https://github.com/llvm/llvm-project/pull/153140.diff

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaDecl.cpp (+32-28)
• (modified) clang/test/CXX/class/class.mem/p13.cpp (+1-1)
• (modified) clang/test/CXX/class/class.union/class.union.anon/p4.cpp (+10)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0e9fcaa5fac6a..00b21471f3731 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -184,6 +184,8 @@ Bug Fixes to C++ Support
   (``[[assume(expr)]]``) creates temporary objects.
 - Fix the dynamic_cast to final class optimization to correctly handle
   casts that are guaranteed to fail (#GH137518).
+- Fix a crash if errors "member of anonymous [...] redeclares" and
+  "intializing multiple members of union" coincide (#GH149985).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index b5eb825eb52cc..052e026fa3ee3 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -5463,40 +5463,44 @@ InjectAnonymousStructOrUnionMembers(Sema &SemaRef, Scope *S, DeclContext *Owner,
         //   distinct from the names of any other entity in the
         //   scope in which the anonymous union is declared.
         Invalid = true;
-      } else {
-        // C++ [class.union]p2:
-        //   For the purpose of name lookup, after the anonymous union
-        //   definition, the members of the anonymous union are
-        //   considered to have been defined in the scope in which the
-        //   anonymous union is declared.
-        unsigned OldChainingSize = Chaining.size();
-        if (IndirectFieldDecl *IF = dyn_cast<IndirectFieldDecl>(VD))
-          Chaining.append(IF->chain_begin(), IF->chain_end());
-        else
-          Chaining.push_back(VD);
+      }
+      // Inject the IndirectFieldDecl even if invalid, because later
+      // diagnostics may depend on it being present.
+
+      // C++ [class.union]p2:
+      //   For the purpose of name lookup, after the anonymous union
+      //   definition, the members of the anonymous union are
+      //   considered to have been defined in the scope in which the
+      //   anonymous union is declared.
+      unsigned OldChainingSize = Chaining.size();
+      if (IndirectFieldDecl *IF = dyn_cast<IndirectFieldDecl>(VD))
+        Chaining.append(IF->chain_begin(), IF->chain_end());
+      else
+        Chaining.push_back(VD);
 
-        assert(Chaining.size() >= 2);
-        NamedDecl **NamedChain =
-          new (SemaRef.Context)NamedDecl*[Chaining.size()];
-        for (unsigned i = 0; i < Chaining.size(); i++)
-          NamedChain[i] = Chaining[i];
+      assert(Chaining.size() >= 2);
+      NamedDecl **NamedChain =
+          new (SemaRef.Context) NamedDecl *[Chaining.size()];
+      for (unsigned i = 0; i < Chaining.size(); i++)
+        NamedChain[i] = Chaining[i];
 
-        IndirectFieldDecl *IndirectField = IndirectFieldDecl::Create(
-            SemaRef.Context, Owner, VD->getLocation(), VD->getIdentifier(),
-            VD->getType(), {NamedChain, Chaining.size()});
+      IndirectFieldDecl *IndirectField = IndirectFieldDecl::Create(
+          SemaRef.Context, Owner, VD->getLocation(), VD->getIdentifier(),
+          VD->getType(), {NamedChain, Chaining.size()});
 
-        for (const auto *Attr : VD->attrs())
-          IndirectField->addAttr(Attr->clone(SemaRef.Context));
+      for (const auto *Attr : VD->attrs())
+        IndirectField->addAttr(Attr->clone(SemaRef.Context));
 
-        IndirectField->setAccess(AS);
-        IndirectField->setImplicit();
-        SemaRef.PushOnScopeChains(IndirectField, S);
+      IndirectField->setAccess(AS);
+      IndirectField->setImplicit();
+      IndirectField->setInvalidDecl(Invalid);
+      SemaRef.PushOnScopeChains(IndirectField, S);
 
-        // That includes picking up the appropriate access specifier.
-        if (AS != AS_none) IndirectField->setAccess(AS);
+      // That includes picking up the appropriate access specifier.
+      if (AS != AS_none)
+        IndirectField->setAccess(AS);
 
-        Chaining.resize(OldChainingSize);
-      }
+      Chaining.resize(OldChainingSize);
     }
   }
 
diff --git a/clang/test/CXX/class/class.mem/p13.cpp b/clang/test/CXX/class/class.mem/p13.cpp
index d947586c41940..4dc959cd0beb8 100644
--- a/clang/test/CXX/class/class.mem/p13.cpp
+++ b/clang/test/CXX/class/class.mem/p13.cpp
@@ -63,7 +63,7 @@ struct X4 { // expected-note{{previous}}
     int X;
     union {
       float Y;
-      unsigned X4; // expected-error{{redeclares 'X4'}}
+      unsigned X4; // expected-error{{redeclares 'X4'}} expected-error {{'X4' has the same name as its class}}
     };
   };
 };
diff --git a/clang/test/CXX/class/class.union/class.union.anon/p4.cpp b/clang/test/CXX/class/class.union/class.union.anon/p4.cpp
index a12ec38503fa8..710636d2235db 100644
--- a/clang/test/CXX/class/class.union/class.union.anon/p4.cpp
+++ b/clang/test/CXX/class/class.union/class.union.anon/p4.cpp
@@ -8,3 +8,13 @@ union U {
     int y = 1; // expected-error {{initializing multiple members of union}}
   };
 };
+
+namespace GH149985 {
+  union U {
+    int x; // expected-note {{previous declaration is here}}
+    union {
+      int x = {}; // expected-error {{member of anonymous union redeclares}} expected-note {{previous initialization is here}}
+    };
+    int y = {}; // expected-error {{initializing multiple members of union}}
+  };
+}

[keinflue]

I am not entirely sure that this doesn't have any further side effects, but I think it makes sense to have the indirect fields nodes in the final AST even if they are invalid.

This should however probably at least be mentioned in the release notes, I am not entirely sure in which section though.

[Fznamznon]

Adding more reviewers...

[erichkeane]

I think the patch in general is correct but the double-error needs to be dealt with. I suspect one of them needs to suppress its diagnostic if it is invalid. While I have a preference for which is first, we should just make sure they both ensure they aren't diagnosing on an invalid IndirectFieldDecl.

[keinflue]

@erichkeane Thank you for approving the PR. I do not have commit access, if you could land it for, that would be appreciated.