[clang] [clang] Don't let an invalid redefinition replace an existing definition (PR #227358)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Akash Manna (akash-manna-sky)

<details>
<summary>Changes</summary>

Fixes #<!-- -->226183

`template <> struct S {};` for an already defined, non-template `S` is diagnosed, but `ActOnTag` recovers by treating it as a member specialization. That skips the usual redefinition check and leaves the error to `CheckMemberSpecialization`, but by then the new decl is already a redeclaration of `S`, and we still call `startDefinition()` on it. All redeclarations of a class share one `DefinitionData`, so this swaps the original definition for an empty one. It's been like that for a long time (`__is_empty(S)` comes out wrong as far back as Clang 14); it only started crashing in 22, where a type reaches its definition through that shared data, so the binding pack in `foo` was built from the original `S` at parse time and from the empty one at instantiation. A regular member specialization after instantiation, like `template <> struct A<int>::B {}`, goes down the same path.

`ActOnTag` now only starts the definition if the declaration is valid or nothing has defined the entity yet, so an invalid redefinition stays an invalid redeclaration and the existing definition is left alone. This is what `CheckClassTemplate` and `ActOnClassTemplateSpecialization` already do, both bail out before `startDefinition()` on a redefinition. I keyed it on `Invalid` rather than on member specializations, since a definition whose template header was already rejected skips the redefinition check the same way.


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


4 Files Affected:

- (modified) clang/docs/ReleaseNotes.md (+5) 
- (modified) clang/lib/Sema/SemaDecl.cpp (+3-1) 
- (modified) clang/test/SemaCXX/cxx2c-binding-pack.cpp (+17) 
- (modified) clang/test/SemaTemplate/explicit-specialization-member.cpp (+17) 


``````````diff
diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md
index 3c6acf353f93f..5ae90124f2861 100644
--- a/clang/docs/ReleaseNotes.md
+++ b/clang/docs/ReleaseNotes.md
@@ -707,6 +707,11 @@ features cannot lower the translation-unit ABI level;
 - Fixed merging of lambdas across modules in the case where neither lambda is
   imported from an AST file. (#GH214560)
 
+- Fixed a crash and spurious errors after an invalid explicit specialization of
+  a class that was already defined, such as `template <> struct S {};` for a
+  non-template `S`. The invalid definition no longer replaces the existing one.
+  (#GH226183)
+
 - Fixed a crash when a non-type template parameter of reference type is bound
   to a subobject and is used in a context that requires an implicit conversion.
   (#GH215900)
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index db5e66cb96c3e..d8fa3e9e71e99 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -19199,7 +19199,9 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
 
   if (TUK == TagUseKind::Definition) {
     if (!SkipBody || !SkipBody->ShouldSkip) {
-      New->startDefinition();
+      // An invalid redefinition must not replace the existing definition.
+      if (!Invalid || !PrevDecl || !PrevDecl->getDefinition())
+        New->startDefinition();
     } else {
       New->setCompleteDefinition();
       New->demoteThisDefinitionToDeclaration();
diff --git a/clang/test/SemaCXX/cxx2c-binding-pack.cpp b/clang/test/SemaCXX/cxx2c-binding-pack.cpp
index 455e09b7bdab8..908f43d21c71c 100644
--- a/clang/test/SemaCXX/cxx2c-binding-pack.cpp
+++ b/clang/test/SemaCXX/cxx2c-binding-pack.cpp
@@ -301,3 +301,20 @@ void i() {
 template void i<void>();
 
 } // namespace GH214160
+
+namespace GH226183 {
+struct S { // expected-note {{attempt to specialize declaration here}}
+  constexpr S() : i(1) {};
+  int i : 2;
+};
+
+void foo(auto) { constexpr auto [... p] = S(); }
+
+void bar() { foo(42); }
+
+template <> struct S {};
+// expected-error at -1 {{extraneous 'template<>' in declaration of struct 'S'}}
+// expected-error at -2 {{specialization of member 'GH226183::S' does not specialize an instantiated member}}
+
+static_assert(S().i == 1);
+} // namespace GH226183
diff --git a/clang/test/SemaTemplate/explicit-specialization-member.cpp b/clang/test/SemaTemplate/explicit-specialization-member.cpp
index c406fb3e21d40..f126b83d90b93 100644
--- a/clang/test/SemaTemplate/explicit-specialization-member.cpp
+++ b/clang/test/SemaTemplate/explicit-specialization-member.cpp
@@ -93,3 +93,20 @@ namespace PR41607 {
   static_assert(Outer<123>::f<>() == 123, "");
   static_assert(Outer<123>::f<>() != 125, "");
 }
+
+namespace GH226183 {
+  template <typename T> struct A {
+    struct B { int n = 1; };
+    struct C { int n = 2; };
+    enum E : int { e };
+  };
+
+  A<int>::B b; // expected-note 2 {{implicit instantiation first required here}}
+  template <> struct A<int>::B {}; // expected-error {{explicit specialization of 'B' after instantiation}}
+  template <> enum A<int>::E : int { f }; // expected-error {{explicit specialization of 'E' after instantiation}}
+  template <typename T> template <> struct A<T>::C {}; // expected-error {{cannot specialize (with 'template<>') a member of an unspecialized template}}
+
+  static_assert(A<int>::B().n == 1, "");
+  static_assert(sizeof(A<int>::E) == sizeof(int), "");
+  static_assert(A<int>::C().n == 2, "");
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/227358