[clang] ce2257d - [C++20] [Modules] Judge current module correctly
Chuanqi Xu via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 20 20:17:55 PDT 2022
Author: Chuanqi Xu
Date: 2022-04-21T11:09:55+08:00
New Revision: ce2257d69fd02d0c34164ce3de9ab8dbe6991e0c
URL: https://github.com/llvm/llvm-project/commit/ce2257d69fd02d0c34164ce3de9ab8dbe6991e0c
DIFF: https://github.com/llvm/llvm-project/commit/ce2257d69fd02d0c34164ce3de9ab8dbe6991e0c.diff
LOG: [C++20] [Modules] Judge current module correctly
Now the implementation would accept following code:
```
//--- impl.cppm
module M:impl;
class A {};
//--- M.cppm
export module M;
import :impl;
//--- Use.cpp
import M;
void test() {
A a; // Expected error. A is not visible here.
}
```
which is clearly wrong. The root cause is the implementation of
`isInCurrentModule` would return true if the module is a partition! So
in the above example, although Use.cpp is not a module unit,
`isInCurrentModule ` would still return true when the compiler tries to
see if the owning module of `A` is the current module. I believe this is
an oversight. This patch tries to fix this problem.
Reviewed By: iains
Differential Revision: https://reviews.llvm.org/D123837
Added:
clang/test/CXX/module/module.import/p2.cpp
Modified:
clang/include/clang/Basic/Module.h
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaLookup.cpp
clang/test/Modules/cxx20-10-1-ex2.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 45c23d5b7988e..5ce5fea45c670 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -168,6 +168,8 @@ class Module {
/// some C++ module.
bool isGlobalModule() const { return Kind == GlobalModuleFragment; }
+ bool isPrivateModule() const { return Kind == PrivateModuleFragment; }
+
private:
/// The submodules of this module, indexed by name.
std::vector<Module *> SubModules;
@@ -536,10 +538,23 @@ class Module {
/// Get the primary module interface name from a partition.
StringRef getPrimaryModuleInterfaceName() const {
+ // Technically, global module fragment belongs to global module. And global
+ // module has no name: [module.unit]p6:
+ // The global module has no name, no module interface unit, and is not
+ // introduced by any module-declaration.
+ //
+ // <global> is the default name showed in module map.
+ if (isGlobalModule())
+ return "<global>";
+
if (isModulePartition()) {
auto pos = Name.find(':');
return StringRef(Name.data(), pos);
}
+
+ if (isPrivateModule())
+ return getTopLevelModuleName();
+
return Name;
}
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 7ec794d18e825..74b6f21284fc6 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2239,7 +2239,7 @@ class Sema final {
/// Namespace definitions that we will export when they finish.
llvm::SmallPtrSet<const NamespaceDecl*, 8> DeferredExportedNamespaces;
- /// Get the module whose scope we are currently within.
+ /// Get the module unit whose scope we are currently within.
Module *getCurrentModule() const {
return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
}
@@ -2257,6 +2257,11 @@ class Sema final {
VisibleModuleSet VisibleModules;
+ /// Cache for module units which is usable for current module.
+ llvm::DenseSet<const Module *> UsableModuleUnitsCache;
+
+ bool isUsableModule(const Module *M);
+
public:
/// Get the module owning an entity.
Module *getOwningModule(const Decl *Entity) {
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 604c29c36fd2c..2a378741e7aa6 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -1558,17 +1558,39 @@ llvm::DenseSet<Module*> &Sema::getLookupModules() {
return LookupModulesCache;
}
-/// Determine whether the module M is part of the current module from the
-/// perspective of a module-private visibility check.
-static bool isInCurrentModule(const Module *M, const LangOptions &LangOpts) {
+/// Determine if we could use all the declarations in the module.
+bool Sema::isUsableModule(const Module *M) {
+ assert(M && "We shouldn't check nullness for module here");
+ // Return quickly if we cached the result.
+ if (UsableModuleUnitsCache.count(M))
+ return true;
+
// If M is the global module fragment of a module that we've not yet finished
- // parsing, then it must be part of the current module.
- // If it's a partition, then it must be visible to an importer (since only
- // another partition or the named module can import it).
- return M->getTopLevelModuleName() == LangOpts.CurrentModule ||
- (M->Kind == Module::GlobalModuleFragment && !M->Parent) ||
- M->Kind == Module::ModulePartitionInterface ||
- M->Kind == Module::ModulePartitionImplementation;
+ // parsing, then M should be the global module fragment in current TU. So it
+ // should be usable.
+ // [module.global.frag]p1:
+ // The global module fragment can be used to provide declarations that are
+ // attached to the global module and usable within the module unit.
+ if ((M->isGlobalModule() && !M->Parent) ||
+ // If M is the private module fragment, it is usable only if it is within
+ // the current module unit. And it must be the current parsing module unit
+ // if it is within the current module unit according to the grammar of the
+ // private module fragment.
+ // NOTE: This is covered by the following condition. The intention of the
+ // check is to avoid string comparison as much as possible.
+ (M->isPrivateModule() && M == getCurrentModule()) ||
+ // The module unit which is in the same module with the current module
+ // unit is usable.
+ //
+ // FIXME: Here we judge if they are in the same module by comparing the
+ // string. Is there any better solution?
+ (M->getPrimaryModuleInterfaceName() ==
+ llvm::StringRef(getLangOpts().CurrentModule).split(':').first)) {
+ UsableModuleUnitsCache.insert(M);
+ return true;
+ }
+
+ return false;
}
bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
@@ -1580,7 +1602,7 @@ bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
bool Sema::hasMergedDefinitionInCurrentModule(NamedDecl *Def) {
for (const Module *Merged : Context.getModulesWithMergedDefinition(Def))
- if (isInCurrentModule(Merged, getLangOpts()))
+ if (isUsableModule(Merged))
return true;
return false;
}
@@ -1762,7 +1784,7 @@ bool Sema::isModuleVisible(const Module *M, bool ModulePrivate) {
// means it is part of the current module. For any other query, that means it
// is in our visible module set.
if (ModulePrivate) {
- if (isInCurrentModule(M, getLangOpts()))
+ if (isUsableModule(M))
return true;
else if (M->Kind == Module::ModuleKind::ModulePartitionImplementation &&
isModuleDirectlyImported(M))
diff --git a/clang/test/CXX/module/module.import/p2.cpp b/clang/test/CXX/module/module.import/p2.cpp
new file mode 100644
index 0000000000000..7aa54e38f8c3f
--- /dev/null
+++ b/clang/test/CXX/module/module.import/p2.cpp
@@ -0,0 +1,73 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -std=c++20 %t/impl.cppm -emit-module-interface -o %t/M-impl.pcm
+// RUN: %clang_cc1 -std=c++20 %t/M.cppm -emit-module-interface -fprebuilt-module-path=%t -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/UseInPartA.cppm -fprebuilt-module-path=%t -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/UseInAnotherModule.cppm -fprebuilt-module-path=%t -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/Private.cppm -emit-module-interface -fprebuilt-module-path=%t -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 %t/TryUseFromPrivate.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+// RUN: %clang_cc1 -std=c++20 %t/Global.cppm -emit-module-interface -fprebuilt-module-path=%t -o %t/C.pcm
+// RUN: %clang_cc1 -std=c++20 %t/UseGlobal.cpp -fprebuilt-module-path=%t -verify -fsyntax-only
+
+//--- impl.cppm
+module M:impl;
+class A {};
+
+//--- M.cppm
+export module M;
+import :impl;
+export A f();
+
+//--- Use.cpp
+import M;
+void test() {
+ A a; // expected-error {{unknown type name 'A'}}
+}
+
+//--- UseInPartA.cppm
+// expected-no-diagnostics
+export module M:partA;
+import :impl;
+void test() {
+ A a;
+}
+
+//--- UseInAnotherModule.cppm
+export module B;
+import M;
+void test() {
+ A a; // expected-error {{unknown type name 'A'}}
+}
+
+//--- Private.cppm
+export module A;
+module :private;
+
+class A {};
+void test() {
+ A a;
+}
+
+//--- TryUseFromPrivate.cpp
+
+import A;
+void test() {
+ A a; // expected-error {{unknown type name 'A'}}
+}
+
+//--- Global.cppm
+module;
+class A{};
+export module C;
+void test() {
+ A a;
+}
+
+//--- UseGlobal.cpp
+import C;
+void test() {
+ A a; // expected-error {{'A' must be declared before it is used}}
+ // expected-note at Global.cppm:2 {{declaration here is not visible}}
+}
diff --git a/clang/test/Modules/cxx20-10-1-ex2.cpp b/clang/test/Modules/cxx20-10-1-ex2.cpp
index 6b47fb0f41eec..88ec945d6a848 100644
--- a/clang/test/Modules/cxx20-10-1-ex2.cpp
+++ b/clang/test/Modules/cxx20-10-1-ex2.cpp
@@ -56,7 +56,14 @@ import B;
int &c = n; // expected-error {{use of undeclared identifier}}
//--- std10-1-ex2-tu7.cpp
+// expected-no-diagnostics
module B:X3; // does not implicitly import B
-import :X2; // X2 is an implementation so exports nothing.
- // error: n not visible here.
-int &c = n; // expected-error {{use of undeclared identifier }}
+import :X2; // X2 is an implementation unit import B.
+// According to [module.import]p7:
+// Additionally, when a module-import-declaration in a module unit of some
+// module M imports another module unit U of M, it also imports all
+// translation units imported by non-exported module-import-declarations in
+// the module unit purview of U.
+//
+// So B is imported in B:X3 due to B:X2 imported B. So n is visible here.
+int &c = n;
More information about the cfe-commits
mailing list