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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
