https://github.com/avikivity updated https://github.com/llvm/llvm-project/pull/188865
>From 0aa3ed1696045933bee76fc040936c33cfbe3d6c 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. Fixes #188853 --- clang/lib/Sema/SemaDecl.cpp | 55 ++++++++++++++++++- .../include-between-imports-enums.cppm | 47 ++++++++++++++++ 2 files changed, 100 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..e01e8db814b85 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,38 @@ 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. + DeclContext *RedeclCtx = ED->getDeclContext()->getRedeclContext(); + if (auto *NewED = dyn_cast<EnumDecl>(SkipBody.New)) { + for (auto *ECD : NewED->enumerators()) { + 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..547848e4057a9 --- /dev/null +++ b/clang/test/Modules/include-between-imports-enums.cppm @@ -0,0 +1,47 @@ +// 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; + +namespace ns { +auto y = Value1; +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
