[Clang] Fix "Unions cannot be dynamic classes" assertion

Project: llvm/llvm-project | Language: C++ | Concepts: error-handling, compiler-internals, testing-strategy | Difficulty: senior
Link: https://github.com/llvm/llvm-project/pull/217942

The problem

Clang would crash when encountering an ill-formed qualified member function definition (e.g., void B::foo() {}) inside a union. The parser would recover and keep the declaration, but it would incorrectly mark the union as polymorphic (having virtual functions), which is not allowed. This led to an assertion failure: "Unions cannot be dynamic classes." A similar issue in non-union classes would silently produce an incorrect vtable pointer.

Why it's non-trivial

A naive fix might just remove the declaration or ignore the error, but that could either lose useful error recovery (keeping the AST node for diagnostics and tooling) or fail to prevent the union from being marked as polymorphic. The real challenge is ensuring that error recovery does not pollute the semantic properties of the enclosing class/union, while still preserving the node for diagnostics and further parsing.

The approach

The fix is to mark the declaration as invalid during parsing, so that it doesn't affect the enclosing class's properties. This is done in the semantic analysis phase, specifically in HandleDeclarator.

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
@@ -6584,6 +6584,8 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
         if (DC->isRecord())
           return nullptr;
 
+        D.setInvalidType();
+      } else if (CurContext->isRecord() && !CurContext->Equals(DC)) {
         D.setInvalidType();
       }
     }

• This snippet ensures that if a qualified member definition names a different class than the current context, the declaration is marked invalid:
  • D.setInvalidType(); marks the declaration as invalid, so it won't affect the class's semantic properties (like being polymorphic).
  • This is analogous to how virtual functions in unions are already handled.

The regression test ensures the fix works and that unions and structs are not incorrectly marked as polymorphic:

diff --git a/clang/test/SemaCXX/GH213854.cpp b/clang/test/SemaCXX/GH213854.cpp
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+namespace reduced {
+union Union {
+  class A {
+    virtual void foo();
+  };
+  class B : public A {
+  };
+  void B::foo() {} // expected-error {{non-friend class member 'foo' cannot have a qualified name}}
+};
+
+static_assert(!__is_polymorphic(Union), "");
...
+struct Struct {
+  class A {
+    virtual void foo();
+  };
+  class B : public A {
+  };
+  void B::foo() {} // expected-error {{non-friend class member 'foo' cannot have a qualified name}}
+};
+
+static_assert(!__is_polymorphic(Struct), "");

• The test checks that the union and struct are not polymorphic after the invalid member is ignored.

A release note was also added:

diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md
@@ -528,6 +528,12 @@ features cannot lower the translation-unit ABI level;
+- Fixed a crash when an ill-formed qualified member function definition written
+  inside a class (e.g. a definition of a virtual member of a nested class)
+  incorrectly caused the enclosing class to be treated as a polymorphic class,
+  which asserted during record layout when the enclosing class was a union.
+  (#GH213854)

• This explains the bug and the fix for users.

From the review thread

TPPPP72: I think removing the node is feasible, and this point in the discussion reminded me of it. Since addedMember needs to skip these malformed nodes, and skipping them doesn't affect subsequent syntax analysis, then they should be deleted directly from the beginning. Or, we could just mark them as InvalidDecl, which I think might be even better, depending on whether we want to preserve an AST that is as consistent as possible with the source code or a more correct AST.

akash-manna-sky: Thanks, agreed. After analyzing the discussions, I've dropped the addedMember change and fixed the recovery in HandleDeclarator instead: when the qualifier names a different class, the declaration is now marked invalid, same as we already do for virtual in a union. addedMember skips invalid decls, so the union can't become polymorphic, the body is still parsed and the node stays in the AST. Dropping the node instead could be done if you'd prefer that.

Takeaways

• Marking invalid declarations during semantic analysis prevents them from affecting class properties, while still preserving them for diagnostics and tooling.
• Error recovery in compilers should avoid polluting the semantic state of AST nodes with ill-formed constructs.
• Regression tests for compiler bugs should check both for the absence of crashes and for correct semantic properties (e.g., not incorrectly marking a union as polymorphic).