[clang] fccc6ee - [C++20] [Modules] Don't make enum constant members always visible
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Sun Jun 22 23:41:05 PDT 2025
Author: Chuanqi Xu
Date: 2025-06-23T14:39:08+08:00
New Revision: fccc6ee7021811a27ab1303d19407f703853ab92
URL: https://github.com/llvm/llvm-project/commit/fccc6ee7021811a27ab1303d19407f703853ab92
DIFF: https://github.com/llvm/llvm-project/commit/fccc6ee7021811a27ab1303d19407f703853ab92.diff
LOG: [C++20] [Modules] Don't make enum constant members always visible
Close https://github.com/llvm/llvm-project/issues/131058
See the comments in
ASTWriter.cpp:ASTDeclContextNameLookupTrait::getLookupVisibility and
SemaLookup.cpp:Sema::makeMergedDefinitionVisible for details.
Added:
clang/test/Modules/include-after-imports-enums.cppm
clang/test/Modules/pr131058.cppm
Modified:
clang/lib/Sema/SemaLookup.cpp
clang/lib/Serialization/ASTWriter.cpp
Removed:
################################################################################
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 5ad9dd8ed0d3e..aa7191d2814fe 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -1565,6 +1565,21 @@ void Sema::makeMergedDefinitionVisible(NamedDecl *ND) {
if (auto *TD = dyn_cast<TemplateDecl>(ND))
for (auto *Param : *TD->getTemplateParameters())
makeMergedDefinitionVisible(Param);
+
+ // If we import a named module which contains a header, and then we include a
+ // header which contains a definition of enums, we will skip parsing the enums
+ // in the current TU. But we need to ensure the visibility of the enum
+ // contants, since they are able to be found with the parents of their
+ // parents.
+ if (auto *ED = dyn_cast<EnumDecl>(ND);
+ ED && ED->isFromGlobalModule() && !ED->isScoped()) {
+ for (auto *ECD : ED->enumerators()) {
+ ECD->setVisibleDespiteOwningModule();
+ DeclContext *RedeclCtx = ED->getDeclContext()->getRedeclContext();
+ if (RedeclCtx->lookup(ECD->getDeclName()).empty())
+ RedeclCtx->makeDeclVisibleInContext(ECD);
+ }
+ }
}
/// Find the module in which the given declaration was defined.
@@ -2185,22 +2200,10 @@ bool LookupResult::isAvailableForLookup(Sema &SemaRef, NamedDecl *ND) {
// Class and enumeration member names can be found by name lookup in any
// context in which a definition of the type is reachable.
//
- // FIXME: The current implementation didn't consider about scope. For example,
- // ```
- // // m.cppm
- // export module m;
- // enum E1 { e1 };
- // // Use.cpp
- // import m;
- // void test() {
- // auto a = E1::e1; // Error as expected.
- // auto b = e1; // Should be error. namespace-scope name e1 is not visible
- // }
- // ```
- // For the above example, the current implementation would emit error for `a`
- // correctly. However, the implementation wouldn't diagnose about `b` now.
- // Since we only check the reachability for the parent only.
- // See clang/test/CXX/module/module.interface/p7.cpp for example.
+ // NOTE: The above wording may be problematic. See
+ // https://github.com/llvm/llvm-project/issues/131058 But it is much complext
+ // to adjust it in Sema's lookup process. Now we hacked it in ASTWriter. See
+ // the comments in ASTDeclContextNameLookupTrait::getLookupVisibility.
if (auto *TD = dyn_cast<TagDecl>(DC))
return SemaRef.hasReachableDefinition(TD);
diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index af7229d748872..c6487c5366a29 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4331,9 +4331,36 @@ class ASTDeclContextNameLookupTrait
return LookupVisibility::ModuleLocalVisible;
if (isTULocalInNamedModules(D))
return LookupVisibility::TULocal;
+
+ // A trick to handle enum constants. The enum constants is special since
+ // they can be found directly without their parent context. This makes it
+ // tricky to decide if an EnumConstantDecl is visible or not by their own
+ // visibilities. E.g., for a class member, we can assume it is visible if
+ // the user get its parent somehow. But for an enum constant, the users may
+ // access if without its parent context. Although we can fix the problem in
+ // Sema lookup process, it might be too complex, we just make a trick here.
+ // Note that we only removes enum constant from the lookup table from its
+ // parent of parent. We DON'T remove the enum constant from its parent. So
+ // we don't need to care about merging problems here.
+ if (auto *ECD = dyn_cast<EnumConstantDecl>(D);
+ ECD && DC.isFileContext() && ECD->getOwningModule() &&
+ ECD->getTopLevelOwningNamedModule()->isNamedModule()) {
+ if (llvm::all_of(
+ DC.noload_lookup(
+ cast<EnumDecl>(ECD->getDeclContext())->getDeclName()),
+ [](auto *Found) {
+ return Found->isInvisibleOutsideTheOwningModule();
+ }))
+ return ECD->isFromExplicitGlobalModule() ||
+ ECD->isInAnonymousNamespace()
+ ? LookupVisibility::TULocal
+ : LookupVisibility::ModuleLocalVisible;
+ }
+
return LookupVisibility::GenerallyVisibile;
}
+ DeclContext &DC;
ModuleLevelDeclsMapTy ModuleLocalDeclsMap;
TULocalDeclsMapTy TULocalDeclsMap;
@@ -4341,6 +4368,9 @@ class ASTDeclContextNameLookupTrait
using ASTDeclContextNameTrivialLookupTrait::
ASTDeclContextNameTrivialLookupTrait;
+ ASTDeclContextNameLookupTrait(ASTWriter &Writer, DeclContext &DC)
+ : ASTDeclContextNameTrivialLookupTrait(Writer), DC(DC) {}
+
template <typename Coll> data_type getData(const Coll &Decls) {
unsigned Start = DeclIDs.size();
for (NamedDecl *D : Decls) {
@@ -4612,7 +4642,7 @@ void ASTWriter::GenerateNameLookupTable(
MultiOnDiskHashTableGenerator<reader::ASTDeclContextNameLookupTrait,
ASTDeclContextNameLookupTrait>
Generator;
- ASTDeclContextNameLookupTrait Trait(*this);
+ ASTDeclContextNameLookupTrait Trait(*this, *DC);
// The first step is to collect the declaration names which we need to
// serialize into the name lookup table, and to collect them in a stable
diff --git a/clang/test/Modules/include-after-imports-enums.cppm b/clang/test/Modules/include-after-imports-enums.cppm
new file mode 100644
index 0000000000000..00affd98e299f
--- /dev/null
+++ b/clang/test/Modules/include-after-imports-enums.cppm
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+//
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-reduced-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+
+//--- enum.h
+enum E { Value };
+
+//--- M.cppm
+module;
+#include "enum.h"
+export module M;
+auto e = Value;
+
+//--- use.cpp
+// expected-no-diagnostics
+import M;
+#include "enum.h"
+
+auto e = Value;
diff --git a/clang/test/Modules/pr131058.cppm b/clang/test/Modules/pr131058.cppm
new file mode 100644
index 0000000000000..c5a626103373e
--- /dev/null
+++ b/clang/test/Modules/pr131058.cppm
@@ -0,0 +1,85 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-reduced-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+// RUN: %clang_cc1 -std=c++20 %t/M0.cppm -emit-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fsyntax-only -verify -fprebuilt-module-path=%t -DMODULE_LOCAL
+// RUN: %clang_cc1 -std=c++20 %t/M0.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+// RUN: %clang_cc1 -std=c++20 %t/M0.cppm -emit-reduced-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fsyntax-only -verify -fprebuilt-module-path=%t -DMODULE_LOCAL
+// RUN: %clang_cc1 -std=c++20 %t/M0.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+// RUN: %clang_cc1 -std=c++20 %t/M2.cppm -emit-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+// RUN: %clang_cc1 -std=c++20 %t/M2.cppm -emit-reduced-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+// RUN: %clang_cc1 -std=c++20 %t/M3.cppm -emit-reduced-module-interface -o %t/M.pcm
+// RUN: %clang_cc1 -std=c++20 %t/use2.cpp -fsyntax-only -verify -fprebuilt-module-path=%t
+
+//--- enum.h
+enum { SomeName, };
+
+//--- M.cppm
+module;
+#include "enum.h"
+export module M;
+export auto e = SomeName;
+
+//--- M0.cppm
+export module M;
+enum { SomeName, };
+export auto e = SomeName;
+
+//--- M0.cpp
+// expected-no-diagnostics
+module M;
+auto a = SomeName;
+
+//--- use.cpp
+import M;
+auto a = SomeName; // expected-error {{use of undeclared identifier 'SomeName'}}
+auto b = decltype(e)::SomeName;
+
+//--- enum1.h
+extern "C++" {
+enum { SomeName, };
+}
+
+//--- M2.cppm
+module;
+#include "enum1.h"
+export module M;
+export auto e = SomeName;
+
+//--- enums.h
+namespace nn {
+enum E { Value };
+enum E2 { VisibleEnum };
+enum AlwaysVisibleEnums { UnconditionallyVisible };
+}
+
+//--- M3.cppm
+module;
+#include "enums.h"
+export module M;
+export namespace nn {
+ using nn::E2::VisibleEnum;
+ using nn::AlwaysVisibleEnums;
+}
+auto e1 = nn::Value;
+auto e2 = nn::VisibleEnum;
+
+//--- use2.cpp
+import M;
+auto e = nn::Value1; // expected-error {{no member named 'Value1' in namespace 'nn'}}
+auto e2 = nn::VisibleEnum;
+auto e3 = nn::UnconditionallyVisible;
More information about the cfe-commits
mailing list