[C++20] [Modules] Don't make enum constant members always visible

Close #131058

See the comments in
ASTWriter.cpp:ASTDeclContextNameLookupTrait::getLookupVisibility and
SemaLookup.cpp:Sema::makeMergedDefinitionVisible for details.

Author: ChuanqiXu9

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);
 

clang/lib/Serialization/ASTWriter.cpp

@@ -4331,16 +4331,46 @@ 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;
 
 public:
   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

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;

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;