[clang] Fix enum enumerator visibility loss when #include is between module imports (PR #188865)
Avi Kivity via cfe-commits
cfe-commits at lists.llvm.org
Sat Mar 28 06:59:33 PDT 2026
https://github.com/avikivity updated https://github.com/llvm/llvm-project/pull/188865
>From 0bb281dbc1c98d331ecb818d6f7ea58135cc16cb Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi at scylladb.com>
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
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 | 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;
+}
More information about the cfe-commits
mailing list