Skip to content

Commit d490526

Browse files
fleeting-xxDan Baker
fleeting-xx
and
Dan Baker
authored
[clangd] [Modules] Fixes to correctly handle module dependencies (#142090)
Simple module import dependencies, see [module_dependencies.test](llvm/llvm-project@main...fleeting-xx:llvm-project:fix_clangd_dependent_modules#diff-5510681cbe5b7ed3a72c5e683184e83fa66e911e9abb0e6670b01b87b3ca7b1a), were not being correctly handled due to a couple of issues. - The `MDB.getRequiredModules()` call returned a `std::vector<std::string>` and all `StringRefs` were to entries in that temporary value. So the `StringRef` elements in `getAllRequiredModules()`'s return value were bound to values that went out of scope. - `ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile()` was iterating over each module dependency name, but only using the original module name and path for various checks and module compilation. In addition to fixing the above issues I added support for Windows paths in modules.test and added a new unit test, module_dependencies.test, which demonstrates the failure in the previous state and works correctly after the fixes have been applied. Please let me know if I've missed anything. Co-authored-by: Dan Baker <[email protected]>
1 parent 2e82a17 commit d490526

File tree

3 files changed

+111
-18
lines changed

3 files changed

+111
-18
lines changed

‎clang-tools-extra/clangd/ModulesBuilder.cpp‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,7 @@ class FailedPrerequisiteModules : public PrerequisiteModules {
8484

8585
// We shouldn't adjust the compilation commands based on
8686
// FailedPrerequisiteModules.
87-
void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {
88-
}
87+
void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {}
8988

9089
// FailedPrerequisiteModules can never be reused.
9190
bool
@@ -430,21 +429,21 @@ class CachingProjectModules : public ProjectModules {
430429
/// Collect the directly and indirectly required module names for \param
431430
/// ModuleName in topological order. The \param ModuleName is guaranteed to
432431
/// be the last element in \param ModuleNames.
433-
llvm::SmallVector<StringRef> getAllRequiredModules(PathRef RequiredSource,
434-
CachingProjectModules &MDB,
435-
StringRef ModuleName) {
436-
llvm::SmallVector<llvm::StringRef> ModuleNames;
432+
llvm::SmallVector<std::string> getAllRequiredModules(PathRef RequiredSource,
433+
CachingProjectModules &MDB,
434+
StringRef ModuleName) {
435+
llvm::SmallVector<std::string> ModuleNames;
437436
llvm::StringSet<> ModuleNamesSet;
438437

439438
auto VisitDeps = [&](StringRef ModuleName, auto Visitor) -> void {
440439
ModuleNamesSet.insert(ModuleName);
441440

442-
for (StringRef RequiredModuleName : MDB.getRequiredModules(
441+
for (const std::string &RequiredModuleName : MDB.getRequiredModules(
443442
MDB.getSourceForModuleName(ModuleName, RequiredSource)))
444443
if (ModuleNamesSet.insert(RequiredModuleName).second)
445444
Visitor(RequiredModuleName, Visitor);
446445

447-
ModuleNames.push_back(ModuleName);
446+
ModuleNames.push_back(ModuleName.str());
448447
};
449448
VisitDeps(ModuleName, VisitDeps);
450449

@@ -494,28 +493,30 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
494493
// Get Required modules in topological order.
495494
auto ReqModuleNames = getAllRequiredModules(RequiredSource, MDB, ModuleName);
496495
for (llvm::StringRef ReqModuleName : ReqModuleNames) {
497-
if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
496+
if (BuiltModuleFiles.isModuleUnitBuilt(ReqModuleName))
498497
continue;
499498

500499
if (auto Cached = Cache.getModule(ReqModuleName)) {
501500
if (IsModuleFileUpToDate(Cached->getModuleFilePath(), BuiltModuleFiles,
502501
TFS.view(std::nullopt))) {
503-
log("Reusing module {0} from {1}", ModuleName,
502+
log("Reusing module {0} from {1}", ReqModuleName,
504503
Cached->getModuleFilePath());
505504
BuiltModuleFiles.addModuleFile(std::move(Cached));
506505
continue;
507506
}
508507
Cache.remove(ReqModuleName);
509508
}
510509

510+
std::string ReqFileName =
511+
MDB.getSourceForModuleName(ReqModuleName, RequiredSource);
511512
llvm::Expected<ModuleFile> MF = buildModuleFile(
512-
ModuleName, ModuleUnitFileName, getCDB(), TFS, BuiltModuleFiles);
513+
ReqModuleName, ReqFileName, getCDB(), TFS, BuiltModuleFiles);
513514
if (llvm::Error Err = MF.takeError())
514515
return Err;
515516

516-
log("Built module {0} to {1}", ModuleName, MF->getModuleFilePath());
517+
log("Built module {0} to {1}", ReqModuleName, MF->getModuleFilePath());
517518
auto BuiltModuleFile = std::make_shared<const ModuleFile>(std::move(*MF));
518-
Cache.add(ModuleName, BuiltModuleFile);
519+
Cache.add(ReqModuleName, BuiltModuleFile);
519520
BuiltModuleFiles.addModuleFile(std::move(BuiltModuleFile));
520521
}
521522

‎clang-tools-extra/clangd/test/module_dependencies.test‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
# A smoke test to check that a simple dependency chain for modules can work.
2+
#
3+
# RUN: rm -fr %t
4+
# RUN: mkdir -p %t
5+
# RUN: split-file %s %t
6+
#
7+
# RUN: sed -e "s|DIR|%/t|g" %t/compile_commands.json.tmpl > %t/compile_commands.json.tmp
8+
# RUN: sed -e "s|CLANG_CC|%clang|g" %t/compile_commands.json.tmp > %t/compile_commands.json
9+
# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc.tmp
10+
#
11+
# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
12+
# (with the extra slash in the front), so we add it here.
13+
# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %/t/definition.jsonrpc.tmp > %/t/definition.jsonrpc
14+
#
15+
# RUN: clangd -experimental-modules-support -lit-test < %t/definition.jsonrpc \
16+
# RUN: | FileCheck -strict-whitespace %t/definition.jsonrpc
17+
18+
#--- A-frag.cppm
19+
export module A:frag;
20+
export void printA() {}
21+
22+
#--- A.cppm
23+
export module A;
24+
export import :frag;
25+
26+
#--- Use.cpp
27+
import A;
28+
void foo() {
29+
print
30+
}
31+
32+
#--- compile_commands.json.tmpl
33+
[
34+
{
35+
"directory": "DIR",
36+
"command": "CLANG_CC -fprebuilt-module-path=DIR -std=c++20 -o DIR/main.cpp.o -c DIR/Use.cpp",
37+
"file": "DIR/Use.cpp"
38+
},
39+
{
40+
"directory": "DIR",
41+
"command": "CLANG_CC -std=c++20 DIR/A.cppm --precompile -o DIR/A.pcm",
42+
"file": "DIR/A.cppm"
43+
},
44+
{
45+
"directory": "DIR",
46+
"command": "CLANG_CC -std=c++20 DIR/A-frag.cppm --precompile -o DIR/A-frag.pcm",
47+
"file": "DIR/A-frag.cppm"
48+
}
49+
]
50+
51+
#--- definition.jsonrpc.tmpl
52+
{
53+
"jsonrpc": "2.0",
54+
"id": 0,
55+
"method": "initialize",
56+
"params": {
57+
"processId": 123,
58+
"rootPath": "clangd",
59+
"capabilities": {
60+
"textDocument": {
61+
"completion": {
62+
"completionItem": {
63+
"snippetSupport": true
64+
}
65+
}
66+
}
67+
},
68+
"trace": "off"
69+
}
70+
}
71+
---
72+
{
73+
"jsonrpc": "2.0",
74+
"method": "textDocument/didOpen",
75+
"params": {
76+
"textDocument": {
77+
"uri": "file://DIR/Use.cpp",
78+
"languageId": "cpp",
79+
"version": 1,
80+
"text": "import A;\nvoid foo() {\n print\n}\n"
81+
}
82+
}
83+
}
84+
85+
# CHECK: "message"{{.*}}printA{{.*}}(fix available)
86+
87+
---
88+
{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file://DIR/Use.cpp"},"context":{"triggerKind":1},"position":{"line":2,"character":6}}}
89+
---
90+
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
91+
---
92+
{"jsonrpc":"2.0","method":"exit"}

‎clang-tools-extra/clangd/test/modules.test‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
# A smoke test to check the modules can work basically.
22
#
3-
# Windows have different escaping modes.
4-
# FIXME: We should add one for windows.
5-
# UNSUPPORTED: system-windows
6-
#
73
# RUN: rm -fr %t
84
# RUN: mkdir -p %t
95
# RUN: split-file %s %t
106
#
117
# RUN: sed -e "s|DIR|%/t|g" %t/compile_commands.json.tmpl > %t/compile_commands.json.tmp
128
# RUN: sed -e "s|CLANG_CC|%clang|g" %t/compile_commands.json.tmp > %t/compile_commands.json
13-
# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc
9+
# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc.tmp
10+
#
11+
# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
12+
# (with the extra slash in the front), so we add it here.
13+
# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %/t/definition.jsonrpc.tmp > %/t/definition.jsonrpc
1414
#
1515
# RUN: clangd -experimental-modules-support -lit-test < %t/definition.jsonrpc \
1616
# RUN: | FileCheck -strict-whitespace %t/definition.jsonrpc

0 commit comments

Comments
 (0)