Revert "[clangd] [Modules] Fixes to correctly handle module dependencies"

[fleeting-xx]

Reverts #142090 due to build failures on arm64 windows.

I'll need someone with commit permission to apply this revert.

[llvmbot]

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: None (fleeting-xx)

Changes

Reverts llvm/llvm-project#142090 due to build failures on arm64 windows.

I'll need someone with commit permission to apply this revert.

---

Full diff: https://github.com/llvm/llvm-project/pull/142162.diff

3 Files Affected:

• (modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+13-14)
• (removed) clang-tools-extra/clangd/test/module_dependencies.test (-92)
• (modified) clang-tools-extra/clangd/test/modules.test (+5-5)

diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index 786fb88dbe318..bf77f43bd28bb 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -84,7 +84,8 @@ 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
@@ -429,21 +430,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<std::string> getAllRequiredModules(PathRef RequiredSource,
-                                                     CachingProjectModules &MDB,
-                                                     StringRef ModuleName) {
-  llvm::SmallVector<std::string> ModuleNames;
+llvm::SmallVector<StringRef> getAllRequiredModules(PathRef RequiredSource,
+                                                   CachingProjectModules &MDB,
+                                                   StringRef ModuleName) {
+  llvm::SmallVector<llvm::StringRef> ModuleNames;
   llvm::StringSet<> ModuleNamesSet;
 
   auto VisitDeps = [&](StringRef ModuleName, auto Visitor) -> void {
     ModuleNamesSet.insert(ModuleName);
 
-    for (const std::string &RequiredModuleName : MDB.getRequiredModules(
+    for (StringRef RequiredModuleName : MDB.getRequiredModules(
              MDB.getSourceForModuleName(ModuleName, RequiredSource)))
       if (ModuleNamesSet.insert(RequiredModuleName).second)
         Visitor(RequiredModuleName, Visitor);
 
-    ModuleNames.push_back(ModuleName.str());
+    ModuleNames.push_back(ModuleName);
   };
   VisitDeps(ModuleName, VisitDeps);
 
@@ -493,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(ReqModuleName))
+    if (BuiltModuleFiles.isModuleUnitBuilt(ModuleName))
       continue;
 
     if (auto Cached = Cache.getModule(ReqModuleName)) {
       if (IsModuleFileUpToDate(Cached->getModuleFilePath(), BuiltModuleFiles,
                                TFS.view(std::nullopt))) {
-        log("Reusing module {0} from {1}", ReqModuleName,
+        log("Reusing module {0} from {1}", ModuleName,
             Cached->getModuleFilePath());
         BuiltModuleFiles.addModuleFile(std::move(Cached));
         continue;
@@ -507,16 +508,14 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile(
       Cache.remove(ReqModuleName);
     }
 
-    std::string ReqFileName =
-        MDB.getSourceForModuleName(ReqModuleName, RequiredSource);
     llvm::Expected<ModuleFile> MF = buildModuleFile(
-        ReqModuleName, ReqFileName, getCDB(), TFS, BuiltModuleFiles);
+        ModuleName, ModuleUnitFileName, getCDB(), TFS, BuiltModuleFiles);
     if (llvm::Error Err = MF.takeError())
       return Err;
 
-    log("Built module {0} to {1}", ReqModuleName, MF->getModuleFilePath());
+    log("Built module {0} to {1}", ModuleName, MF->getModuleFilePath());
     auto BuiltModuleFile = std::make_shared<const ModuleFile>(std::move(*MF));
-    Cache.add(ReqModuleName, BuiltModuleFile);
+    Cache.add(ModuleName, 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
deleted file mode 100644
index ee1df7f8a35cc..0000000000000
--- a/clang-tools-extra/clangd/test/module_dependencies.test
+++ /dev/null
@@ -1,92 +0,0 @@
-# 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 6792352bb8e56..74280811a6cff 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.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: sed -e "s|DIR|%/t|g" %t/definition.jsonrpc.tmpl > %t/definition.jsonrpc
 #
 # RUN: clangd -experimental-modules-support -lit-test < %t/definition.jsonrpc \
 # RUN:      | FileCheck -strict-whitespace %t/definition.jsonrpc

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang-tools-extra/clangd/ModulesBuilder.cpp

View the diff from clang-format here.

diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index bf77f43bd..8a54b70a2 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -84,8 +84,7 @@ public:
 
   // 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