https://github.com/avikivity updated https://github.com/llvm/llvm-project/pull/188865
>From 792a850ab88a3412f243e80097149827993480ee Mon Sep 17 00:00:00 2001 From: Avi Kivity <[email protected]> Date: Fri, 27 Mar 2026 00:24:31 +0200 Subject: [PATCH] Fix enum enumerator visibility loss when #include is between module imports When a textual #include of a header containing an unscoped enum is sandwiched between two import declarations of C++20 modules that both include the same header in their Global Module Fragments (GMFs), Clang would lose visibility of the enum's enumerators. The root cause was a 5-step failure chain: 1. ASTWriter categorizes GMF enum constants as TU-local 2. ASTReader discards TU-local entries from module files 3. makeMergedDefinitionVisible injects old AST-file enumerators into the namespace when #include detects the duplicate enum 4. A subsequent module import triggers reconcileExternalVisibleStorage 5. replaceExternalDecls removes the AST-file enumerators because external lookup can't find them (filtered as TU-local) The fix changes how duplicate unscoped enums from the GMF are handled in C++. Instead of completely skipping the enum body (ShouldSkip) and relying on AST-file enumerators that can later be evicted, we now use the CheckSameAsPrevious path: the enum body is parsed into a throwaway decl, structurally validated, and the locally-parsed enumerators are kept in scope and added to the enclosing namespace's lookup table. These local enumerators are immune to replaceExternalDecls since they are not AST-file declarations. The new enumerators are also retyped to the original (module) enum's canonical type, so that name lookup produces enumerators whose type matches the original enum declaration. Without this, initializing a variable declared with the original enum type using an enumerator name would fail with a type mismatch. Fixes #188853 Disclaimers: - the bug was identified and minimized by Claude Opus 4.6 - the fix and the regression tests were written by Claude Opus 4.6 - Unfortunately it is too deep for me to review and state with confidence that this is the correct fix --- clang/lib/Sema/SemaDecl.cpp | 60 ++++++++++++++++++- .../include-between-imports-enums.cppm | 55 +++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 clang/test/Modules/include-between-imports-enums.cppm diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 2951fd09294d8..92ad55e5ac767 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -18073,8 +18073,9 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc, /// Create a new tag decl in C/ObjC. Since the ODR-like semantics for ObjC/C /// implemented asks for structural equivalence checking, the returned decl /// here is passed back to the parser, allowing the tag body to be parsed. - auto createTagFromNewDecl = [&]() -> TagDecl * { - assert(!getLangOpts().CPlusPlus && "not meant for C++ usage"); + auto createTagFromNewDecl = [&](bool EnumOnly = false) -> TagDecl * { + assert((!getLangOpts().CPlusPlus || EnumOnly) && + "not meant for C++ usage except for enums"); // If there is an identifier, use the location of the identifier as the // location of the decl, otherwise use the location of the struct/union // keyword. @@ -18587,6 +18588,24 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc, return Def; } + // For unscoped enums from the global module fragment, + // don't skip the body. Instead, parse it into a throwaway + // decl and keep the locally-parsed enumerators. This + // prevents the enumerators from being lost when a + // subsequent module import triggers + // reconcileExternalVisibleStorage and + // replaceExternalDecls removes the AST-file entries that + // makeMergedDefinitionVisible would have injected. + if (auto *ED = dyn_cast<EnumDecl>(Def); + ED && !ED->isScoped() && ED->isFromGlobalModule()) { + SkipBody->CheckSameAsPrevious = true; + SkipBody->New = createTagFromNewDecl(/*EnumOnly=*/true); + SkipBody->Previous = Def; + + ProcessDeclAttributeList(S, SkipBody->New, Attrs); + return Def; + } + SkipBody->ShouldSkip = true; SkipBody->Previous = Def; if (!HiddenDefVisible && Hidden) @@ -18991,6 +19010,43 @@ bool Sema::ActOnDuplicateDefinition(Scope *S, Decl *Prev, if (!hasStructuralCompatLayout(Prev, SkipBody.New)) return false; + // For unscoped enums from the global module fragment in C++, keep the + // locally-parsed enumerators from the new (throwaway) definition rather + // than injecting the old AST-file enumerators. The AST-file enumerators + // are categorized as TU-local in the PCM and can be silently removed by + // replaceExternalDecls when a subsequent module import triggers + // reconcileExternalVisibleStorage. By keeping the local enumerators, we + // ensure they survive that process. + if (auto *ED = dyn_cast<EnumDecl>(SkipBody.Previous); + ED && !ED->isScoped() && ED->isFromGlobalModule() && + getLangOpts().CPlusPlus) { + // First, add new (local) enumerators into the enclosing DeclContext's + // lookup table so that both qualified and unqualified lookups find them. + // Retype them to the old enum's type so that name lookup produces + // enumerators whose type matches the original (module) enum, avoiding + // type mismatches when user code references the enum type by name. + DeclContext *RedeclCtx = ED->getDeclContext()->getRedeclContext(); + QualType OldEnumType = Context.getCanonicalTagType(ED); + if (auto *NewED = dyn_cast<EnumDecl>(SkipBody.New)) { + for (auto *ECD : NewED->enumerators()) { + ECD->setType(OldEnumType); + ECD->setVisibleDespiteOwningModule(); + if (RedeclCtx->lookup(ECD->getDeclName()).empty()) + RedeclCtx->makeDeclVisibleInContext(ECD); + } + } + + // Now make the previous enum type itself visible. Since we already + // populated the namespace with the new enumerators above, + // makeMergedDefinitionVisible will see them and skip injecting the + // old AST-file enumerators (due to its non-empty lookup check). + makeMergedDefinitionVisible(SkipBody.Previous); + + // Do NOT call CleanupMergedEnum — we want the new enumerators to + // remain in scope. + return true; + } + // Make the previous decl visible. makeMergedDefinitionVisible(SkipBody.Previous); CleanupMergedEnum(S, SkipBody.New); diff --git a/clang/test/Modules/include-between-imports-enums.cppm b/clang/test/Modules/include-between-imports-enums.cppm new file mode 100644 index 0000000000000..bc109870c899b --- /dev/null +++ b/clang/test/Modules/include-between-imports-enums.cppm @@ -0,0 +1,55 @@ +// RUN: rm -rf %t +// RUN: mkdir -p %t +// RUN: split-file %s %t +// +// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm +// RUN: %clang_cc1 -std=c++20 %t/B.cppm -emit-module-interface -o %t/B.pcm +// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only +// +// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-reduced-module-interface -o %t/A.pcm +// RUN: %clang_cc1 -std=c++20 %t/B.cppm -emit-reduced-module-interface -o %t/B.pcm +// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only + +// Test that a textual #include sandwiched between two import declarations +// of modules that both include the same header in their GMFs does not lose +// enum declarations. See https://github.com/llvm/llvm-project/issues/188853 + +//--- enum.h +#ifndef ENUM_H +#define ENUM_H +namespace ns { +enum E { Value1, Value2, Value3 }; +} +#endif + +//--- A.cppm +module; +#include "enum.h" +export module A; +export auto a = ns::Value1; + +//--- B.cppm +module; +#include "enum.h" +export module B; +export auto b = ns::Value2; + +//--- use.cpp +// expected-no-diagnostics +import A; +#include "enum.h" +import B; + +auto x = ns::Value3; + +// Verify that the enumerators' types match the original enum type. +// A previous version of this fix produced enumerators whose type was a +// throwaway enum, causing "cannot initialize a variable of type 'const E' +// with an rvalue of type 'ns::E'" errors. +ns::E z1 = ns::Value1; +ns::E z2 = ns::Value3; + +namespace ns { +auto y = Value1; +E w = Value2; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
