Skip to content

Commit d3df2a8

Browse files
ChuanqiXu9ChuanqiXu9
committed
[C++20] [Modules] Handle transitive import in the module properly
Close #84002 Per [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. However, we only tried to implement it during the implicit import of primary module interface for module implementation unit. Also we didn't implement the last sentence from [module.import]p7 completely: > These rules can in turn lead to the importation of yet more > translation units. This patch tries to care the both issues.
1 parent d3e79e4 commit d3df2a8

File tree

5 files changed

+210
-17
lines changed

5 files changed

+210
-17
lines changed

‎clang/docs/ReleaseNotes.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,11 @@ C++20 Feature Support
8686
- Implemented the `__is_layout_compatible` intrinsic to support
8787
`P0466R5: Layout-compatibility and Pointer-interconvertibility Traits <https://wg21.link/P0466R5>`_.
8888

89+
- Clang now implements [module.import]p7 fully. Clang now will import module
90+
units transitively for the module units coming from the same module of the
91+
current module units.
92+
Fixes `#84002 <https://github.com/llvm/llvm-project/issues/84002>`_.
93+
8994
C++23 Feature Support
9095
^^^^^^^^^^^^^^^^^^^^^
9196

‎clang/include/clang/Basic/Module.h

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,11 @@ class alignas(8) Module {
598598
Kind == ModulePartitionImplementation;
599599
}
600600

601+
/// Is this a module partition implementation unit.
602+
bool isModulePartitionImplementation() const {
603+
return Kind == ModulePartitionImplementation;
604+
}
605+
601606
/// Is this a module implementation.
602607
bool isModuleImplementation() const {
603608
return Kind == ModuleImplementationUnit;
@@ -853,12 +858,6 @@ class VisibleModuleSet {
853858
VisibleCallback Vis = [](Module *) {},
854859
ConflictCallback Cb = [](ArrayRef<Module *>, Module *,
855860
StringRef) {});
856-
857-
/// Make transitive imports visible for [module.import]/7.
858-
void makeTransitiveImportsVisible(
859-
Module *M, SourceLocation Loc, VisibleCallback Vis = [](Module *) {},
860-
ConflictCallback Cb = [](ArrayRef<Module *>, Module *, StringRef) {});
861-
862861
private:
863862
/// Import locations for each visible module. Indexed by the module's
864863
/// VisibilityID.

‎clang/lib/Basic/Module.cpp

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -722,14 +722,6 @@ void VisibleModuleSet::setVisible(Module *M, SourceLocation Loc,
722722
VisitModule({M, nullptr});
723723
}
724724

725-
void VisibleModuleSet::makeTransitiveImportsVisible(Module *M,
726-
SourceLocation Loc,
727-
VisibleCallback Vis,
728-
ConflictCallback Cb) {
729-
for (auto *I : M->Imports)
730-
setVisible(I, Loc, Vis, Cb);
731-
}
732-
733725
ASTSourceDescriptor::ASTSourceDescriptor(Module &M)
734726
: Signature(M.Signature), ClangModule(&M) {
735727
if (M.Directory)

‎clang/lib/Sema/SemaModule.cpp

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,90 @@ static std::string stringFromPath(ModuleIdPath Path) {
7373
return Name;
7474
}
7575

76+
/// Helper function for makeTransitiveImportsVisible to decide whether
77+
/// the \param Imported module unit is in the same module with the \param
78+
/// CurrentModule.
79+
/// \param FoundPrimaryModuleInterface is a helper parameter to record the
80+
/// primary module interface unit corresponding to the module \param
81+
/// CurrentModule. Since currently it is expensive to decide whether two module
82+
/// units come from the same module by comparing the module name.
83+
static bool
84+
isImportingModuleUnitFromSameModule(Module *Imported, Module *CurrentModule,
85+
Module *&FoundPrimaryModuleInterface) {
86+
if (!Imported->isNamedModule())
87+
return false;
88+
89+
// The a partition unit we're importing must be in the same module of the
90+
// current module.
91+
if (Imported->isModulePartition())
92+
return true;
93+
94+
// If we found the primary module interface during the search process, we can
95+
// return quickly to avoid expensive string comparison.
96+
if (FoundPrimaryModuleInterface)
97+
return Imported == FoundPrimaryModuleInterface;
98+
99+
if (!CurrentModule)
100+
return false;
101+
102+
// Then the imported module must be a primary module interface unit. It
103+
// is only allowed to import the primary module interface unit from the same
104+
// module in the implementation unit and the implementation partition unit.
105+
106+
// Since we'll handle implementation unit above. We can only care
107+
// about the implementation partition unit here.
108+
if (!CurrentModule->isModulePartitionImplementation())
109+
return false;
110+
111+
if (Imported->getPrimaryModuleInterfaceName() ==
112+
CurrentModule->getPrimaryModuleInterfaceName()) {
113+
assert(!FoundPrimaryModuleInterface ||
114+
FoundPrimaryModuleInterface == Imported);
115+
FoundPrimaryModuleInterface = Imported;
116+
return true;
117+
}
118+
119+
return false;
120+
}
121+
122+
/// [module.import]p7:
123+
/// Additionally, when a module-import-declaration in a module unit of some
124+
/// module M imports another module unit U of M, it also imports all
125+
/// translation units imported by non-exported module-import-declarations in
126+
/// the module unit purview of U. These rules can in turn lead to the
127+
/// importation of yet more translation units.
128+
static void
129+
makeTransitiveImportsVisible(VisibleModuleSet &VisibleModules, Module *Imported,
130+
Module *CurrentModule, SourceLocation ImportLoc,
131+
bool IsImportingPrimaryModuleInterface = false) {
132+
assert(Imported->isNamedModule() &&
133+
"'makeTransitiveImportsVisible()' is intended for standard C++ named "
134+
"modules only.");
135+
136+
llvm::SmallVector<Module *, 4> Worklist;
137+
Worklist.push_back(Imported);
138+
139+
Module *FoundPrimaryModuleInterface =
140+
IsImportingPrimaryModuleInterface ? Imported : nullptr;
141+
142+
while (!Worklist.empty()) {
143+
Module *Importing = Worklist.pop_back_val();
144+
145+
if (VisibleModules.isVisible(Importing))
146+
continue;
147+
148+
// FIXME: The ImportLoc here is not meaningful. It may be problematic if we
149+
// use the sourcelocation loaded from the visible modules.
150+
VisibleModules.setVisible(Importing, ImportLoc);
151+
152+
if (isImportingModuleUnitFromSameModule(Importing, CurrentModule,
153+
FoundPrimaryModuleInterface))
154+
for (Module *TransImported : Importing->Imports)
155+
if (!VisibleModules.isVisible(TransImported))
156+
Worklist.push_back(TransImported);
157+
}
158+
}
159+
76160
Sema::DeclGroupPtrTy
77161
Sema::ActOnGlobalModuleFragmentDecl(SourceLocation ModuleLoc) {
78162
// We start in the global module;
@@ -396,8 +480,8 @@ Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
396480
// and return the import decl to be added to the current TU.
397481
if (Interface) {
398482

399-
VisibleModules.setVisible(Interface, ModuleLoc);
400-
VisibleModules.makeTransitiveImportsVisible(Interface, ModuleLoc);
483+
makeTransitiveImportsVisible(VisibleModules, Interface, Mod, ModuleLoc,
484+
/*IsImportingPrimaryModuleInterface=*/true);
401485

402486
// Make the import decl for the interface in the impl module.
403487
ImportDecl *Import = ImportDecl::Create(Context, CurContext, ModuleLoc,
@@ -554,7 +638,11 @@ DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
554638
if (Mod->isHeaderUnit())
555639
Diag(ImportLoc, diag::warn_experimental_header_unit);
556640

557-
VisibleModules.setVisible(Mod, ImportLoc);
641+
if (Mod->isNamedModule())
642+
makeTransitiveImportsVisible(VisibleModules, Mod, getCurrentModule(),
643+
ImportLoc);
644+
else
645+
VisibleModules.setVisible(Mod, ImportLoc);
558646

559647
checkModuleImportContext(*this, Mod, ImportLoc, CurContext);
560648

‎clang/test/Modules/transitive-import.cppm

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
// RUN: rm -rf %t
2+
// RUN: mkdir -p %t
3+
// RUN: split-file %s %t
4+
//
5+
// RUN: %clang_cc1 -std=c++20 %t/Invisible.cppm -emit-module-interface -o %t/Invisible.pcm
6+
// RUN: %clang_cc1 -std=c++20 %t/Other.cppm -emit-module-interface -fprebuilt-module-path=%t \
7+
// RUN: -o %t/Other.pcm
8+
// RUN: %clang_cc1 -std=c++20 %t/Another.cppm -emit-module-interface -o %t/Another.pcm
9+
// RUN: %clang_cc1 -std=c++20 %t/A-interface.cppm -emit-module-interface \
10+
// RUN: -fprebuilt-module-path=%t -o %t/A-interface.pcm
11+
// RUN: %clang_cc1 -std=c++20 %t/A-interface2.cppm -emit-module-interface \
12+
// RUN: -fprebuilt-module-path=%t -o %t/A-interface2.pcm
13+
// RUN: %clang_cc1 -std=c++20 %t/A-interface3.cppm -emit-module-interface \
14+
// RUN: -fprebuilt-module-path=%t -o %t/A-interface3.pcm
15+
// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface \
16+
// RUN: -fprebuilt-module-path=%t -o %t/A.pcm
17+
18+
// RUN: %clang_cc1 -std=c++20 %t/A.cpp -fprebuilt-module-path=%t -fsyntax-only -verify
19+
// RUN: %clang_cc1 -std=c++20 %t/A-impl.cppm -fprebuilt-module-path=%t -fsyntax-only -verify
20+
21+
// RUN: %clang_cc1 -std=c++20 %t/A-impl2.cppm -fprebuilt-module-path=%t -fsyntax-only -verify
22+
23+
//--- Invisible.cppm
24+
export module Invisible;
25+
export void invisible() {}
26+
27+
//--- Other.cppm
28+
export module Other;
29+
import Invisible;
30+
export void other() {}
31+
32+
//--- Another.cppm
33+
export module Another;
34+
export void another() {}
35+
36+
//--- A-interface.cppm
37+
export module A:interface;
38+
import Other;
39+
export void a_interface() {}
40+
41+
//--- A-interface2.cppm
42+
export module A:interface2;
43+
import Another;
44+
export void a_interface2() {}
45+
46+
//--- A-interface3.cppm
47+
export module A:interface3;
48+
import :interface;
49+
import :interface2;
50+
export void a_interface3() {}
51+
52+
//--- A.cppm
53+
export module A;
54+
import Another;
55+
import :interface;
56+
import :interface2;
57+
import :interface3;
58+
59+
export void a() {}
60+
export void impl();
61+
62+
//--- A.cpp
63+
module A;
64+
void impl() {
65+
a_interface();
66+
a_interface2();
67+
a_interface3();
68+
69+
other();
70+
another();
71+
72+
invisible(); // expected-error {{declaration of 'invisible' must be imported from module 'Invisible' before it is required}}
73+
// expected-note@* {{declaration here is not visible}}
74+
}
75+
76+
//--- A-impl.cppm
77+
module A:impl;
78+
import :interface3;
79+
80+
void impl_part() {
81+
a_interface();
82+
a_interface2();
83+
a_interface3();
84+
85+
other();
86+
another();
87+
88+
invisible(); // expected-error {{declaration of 'invisible' must be imported from module 'Invisible' before it is required}}
89+
// expected-note@* {{declaration here is not visible}}
90+
}
91+
92+
//--- A-impl2.cppm
93+
module A:impl2;
94+
import A;
95+
96+
void impl_part2() {
97+
a();
98+
impl();
99+
100+
a_interface();
101+
a_interface2();
102+
a_interface3();
103+
104+
other();
105+
another();
106+
107+
invisible(); // expected-error {{declaration of 'invisible' must be imported from module 'Invisible' before it is required}}
108+
// expected-note@* {{declaration here is not visible}}
109+
}

0 commit comments

Comments
 (0)