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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
