Author: fleeting-xx Date: 2025-06-06T09:33:11+08:00 New Revision: 93b0bf635a287aac55f6da39f38c1cf257efa824
URL: https://github.com/llvm/llvm-project/commit/93b0bf635a287aac55f6da39f38c1cf257efa824 DIFF: https://github.com/llvm/llvm-project/commit/93b0bf635a287aac55f6da39f38c1cf257efa824.diff LOG: [clangd] [Modules] Fix to correctly handle module dependencies (#142828) This is a re-application of llvm/llvm-project#142090 without the unit test changes. A subsequent PR will follow that adds a unit test for module dependencies. ### Changes - Fix dangling string references in the return value of getAllRequiredModules() - Change a couple of calls in getOrBuildModuleFile() to use the loop variable instead of the ModuleName parameter. @ChuanqiXu9 for review Added: clang-tools-extra/clangd/test/module_dependencies.test Modified: clang-tools-extra/clangd/ModulesBuilder.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index bf77f43bd28bb..d88aa01aad05d 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -430,10 +430,10 @@ 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 { @@ -444,7 +444,7 @@ llvm::SmallVector<StringRef> getAllRequiredModules(PathRef RequiredSource, if (ModuleNamesSet.insert(RequiredModuleName).second) Visitor(RequiredModuleName, Visitor); - ModuleNames.push_back(ModuleName); + ModuleNames.push_back(ModuleName.str()); }; VisitDeps(ModuleName, VisitDeps); @@ -494,13 +494,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 +508,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..1023b2363c9fa --- /dev/null +++ b/clang-tools-extra/clangd/test/module_dependencies.test @@ -0,0 +1,96 @@ +# A smoke test to check that a simple dependency chain for modules can work. +# +# FIXME: This fails on the Windows ARM64 build server. Not entirely sure why as it has been tested on +# an ARM64 Windows VM and appears to work there. +# UNSUPPORTED: host=aarch64-pc-windows-msvc +# +# 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"} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
