[clang-tools-extra] [clangd] [Modules] Fixes to correctly handle module dependencies (PR #142090)
via cfe-commits
cfe-commits at lists.llvm.org
Thu May 29 23:03:30 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clangd
Author: None (fleeting-xx)
<details>
<summary>Changes</summary>
Simple module import dependencies, see [module_dependencies.test](https://github.com/llvm/llvm-project/compare/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.
---
Full diff: https://github.com/llvm/llvm-project/pull/142090.diff
3 Files Affected:
- (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+14-13)
- (added) clang-tools-extra/clangd/test/module_dependencies.test (+92)
- (modified) clang-tools-extra/clangd/test/modules.test (+5-5)
``````````diff
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index bf77f43bd28bb..786fb88dbe318 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -84,8 +84,7 @@ class FailedPrerequisiteModules : public PrerequisiteModules {
// We shouldn't adjust the compilation commands based on
// FailedPrerequisiteModules.
- void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {
- }
+ void adjustHeaderSearchOptions(HeaderSearchOptions &Options) const override {}
// FailedPrerequisiteModules can never be reused.
bool
@@ -430,21 +429,21 @@ class CachingProjectModules : public ProjectModules {
/// Collect the directly and indirectly required module names for \param
/// ModuleName in topological order. The \param ModuleName is guaranteed to
/// be the last element in \param ModuleNames.
-llvm::SmallVector<StringRef> getAllRequiredModules(PathRef RequiredSource,
- CachingProjectModules &MDB,
- StringRef ModuleName) {
- llvm::SmallVector<llvm::StringRef> ModuleNames;
+llvm::SmallVector<std::string> getAllRequiredModules(PathRef RequiredSource,
+ CachingProjectModules &MDB,
+ StringRef ModuleName) {
+ llvm::SmallVector<std::string> ModuleNames;
llvm::StringSet<> ModuleNamesSet;
auto VisitDeps = [&](StringRef ModuleName, auto Visitor) -> void {
ModuleNamesSet.insert(ModuleName);
- for (StringRef RequiredModuleName : MDB.getRequiredModules(
+ for (const std::string &RequiredModuleName : MDB.getRequiredModules(
MDB.getSourceForModuleName(ModuleName, RequiredSource)))
if (ModuleNamesSet.insert(RequiredModuleName).second)
Visitor(RequiredModuleName, Visitor);
- ModuleNames.push_back(ModuleName);
+ ModuleNames.push_back(ModuleName.str());
};
VisitDeps(ModuleName, VisitDeps);
@@ -494,13 +493,13 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
// Get Required modules in topological order.
auto ReqModuleNames = getAllRequiredModules(RequiredSource, MDB, ModuleName);
for (llvm::StringRef ReqModuleName : ReqModuleNames) {
- if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
+ if (BuiltModuleFiles.isModuleUnitBuilt(ReqModuleName))
continue;
if (auto Cached = Cache.getModule(ReqModuleName)) {
if (IsModuleFileUpToDate(Cached->getModuleFilePath(), BuiltModuleFiles,
TFS.view(std::nullopt))) {
- log("Reusing module {0} from {1}", ModuleName,
+ log("Reusing module {0} from {1}", ReqModuleName,
Cached->getModuleFilePath());
BuiltModuleFiles.addModuleFile(std::move(Cached));
continue;
@@ -508,14 +507,16 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
Cache.remove(ReqModuleName);
}
+ std::string ReqFileName =
+ MDB.getSourceForModuleName(ReqModuleName, RequiredSource);
llvm::Expected<ModuleFile> MF = buildModuleFile(
- ModuleName, ModuleUnitFileName, getCDB(), TFS, BuiltModuleFiles);
+ ReqModuleName, ReqFileName, getCDB(), TFS, BuiltModuleFiles);
if (llvm::Error Err = MF.takeError())
return Err;
- log("Built module {0} to {1}", ModuleName, MF->getModuleFilePath());
+ log("Built module {0} to {1}", ReqModuleName, MF->getModuleFilePath());
auto BuiltModuleFile = std::make_shared<const ModuleFile>(std::move(*MF));
- Cache.add(ModuleName, BuiltModuleFile);
+ Cache.add(ReqModuleName, BuiltModuleFile);
BuiltModuleFiles.addModuleFile(std::move(BuiltModuleFile));
}
diff --git a/clang-tools-extra/clangd/test/module_dependencies.test b/clang-tools-extra/clangd/test/module_dependencies.test
new file mode 100644
index 0000000000000..ee1df7f8a35cc
--- /dev/null
+++ b/clang-tools-extra/clangd/test/module_dependencies.test
@@ -0,0 +1,92 @@
+# A smoke test to check that a simple dependency chain for modules can work.
+#
+# RUN: rm -fr %t
+# RUN: mkdir -p %t
+# RUN: split-file %s %t
+#
+# RUN: sed -e "s|DIR|%/t|g" %t/compile_commands.json.tmpl > %t/compile_commands.json.tmp
+# RUN: sed -e "s|CLANG_CC|%clang|g" %t/compile_commands.json.tmp > %t/compile_commands.json
+# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc.tmp
+#
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %/t/definition.jsonrpc.tmp > %/t/definition.jsonrpc
+#
+# RUN: clangd -experimental-modules-support -lit-test < %t/definition.jsonrpc \
+# RUN: | FileCheck -strict-whitespace %t/definition.jsonrpc
+
+#--- A-frag.cppm
+export module A:frag;
+export void printA() {}
+
+#--- A.cppm
+export module A;
+export import :frag;
+
+#--- Use.cpp
+import A;
+void foo() {
+ print
+}
+
+#--- compile_commands.json.tmpl
+[
+ {
+ "directory": "DIR",
+ "command": "CLANG_CC -fprebuilt-module-path=DIR -std=c++20 -o DIR/main.cpp.o -c DIR/Use.cpp",
+ "file": "DIR/Use.cpp"
+ },
+ {
+ "directory": "DIR",
+ "command": "CLANG_CC -std=c++20 DIR/A.cppm --precompile -o DIR/A.pcm",
+ "file": "DIR/A.cppm"
+ },
+ {
+ "directory": "DIR",
+ "command": "CLANG_CC -std=c++20 DIR/A-frag.cppm --precompile -o DIR/A-frag.pcm",
+ "file": "DIR/A-frag.cppm"
+ }
+]
+
+#--- definition.jsonrpc.tmpl
+{
+ "jsonrpc": "2.0",
+ "id": 0,
+ "method": "initialize",
+ "params": {
+ "processId": 123,
+ "rootPath": "clangd",
+ "capabilities": {
+ "textDocument": {
+ "completion": {
+ "completionItem": {
+ "snippetSupport": true
+ }
+ }
+ }
+ },
+ "trace": "off"
+ }
+}
+---
+{
+ "jsonrpc": "2.0",
+ "method": "textDocument/didOpen",
+ "params": {
+ "textDocument": {
+ "uri": "file://DIR/Use.cpp",
+ "languageId": "cpp",
+ "version": 1,
+ "text": "import A;\nvoid foo() {\n print\n}\n"
+ }
+ }
+}
+
+# CHECK: "message"{{.*}}printA{{.*}}(fix available)
+
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file://DIR/Use.cpp"},"context":{"triggerKind":1},"position":{"line":2,"character":6}}}
+---
+{"jsonrpc":"2.0","id":2,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
diff --git a/clang-tools-extra/clangd/test/modules.test b/clang-tools-extra/clangd/test/modules.test
index 74280811a6cff..6792352bb8e56 100644
--- a/clang-tools-extra/clangd/test/modules.test
+++ b/clang-tools-extra/clangd/test/modules.test
@@ -1,16 +1,16 @@
# A smoke test to check the modules can work basically.
#
-# Windows have different escaping modes.
-# FIXME: We should add one for windows.
-# UNSUPPORTED: system-windows
-#
# RUN: rm -fr %t
# RUN: mkdir -p %t
# RUN: split-file %s %t
#
# RUN: sed -e "s|DIR|%/t|g" %t/compile_commands.json.tmpl > %t/compile_commands.json.tmp
# RUN: sed -e "s|CLANG_CC|%clang|g" %t/compile_commands.json.tmp > %t/compile_commands.json
-# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc
+# RUN: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc.tmp
+#
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %/t/definition.jsonrpc.tmp > %/t/definition.jsonrpc
#
# RUN: clangd -experimental-modules-support -lit-test < %t/definition.jsonrpc \
# RUN: | FileCheck -strict-whitespace %t/definition.jsonrpc
``````````
</details>
https://github.com/llvm/llvm-project/pull/142090
More information about the cfe-commits
mailing list