[clangd] Invalidate preamble when new module imports are added (#199460)

When using `SkipPreambleBuild`, adding a new `import` statement to a
file did
not invalidate the existing preamble because `isPreambleCompatible` only
checked whether existing prerequisite modules were up-to-date, not
whether
the set of required modules itself had changed.

Fixes: #199389
Partially addresses: #126350

clang-tools-extra/clangd/ModulesBuilder.cpp

@@ -16,6 +16,7 @@
 #include "clang/Serialization/ModuleCache.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/LockFileManager.h"
@@ -322,6 +323,8 @@ public:
            llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) const override {
     return false;
   }
+
+  llvm::StringSet<> getRequiredModuleNames() const override { return {}; }
 };
 
 /// Represents a reference to a module file (*.pcm).
@@ -502,10 +505,20 @@ public:
     RequiredModules.emplace_back(std::move(MF));
   }
 
+  void setDirectModuleNames(std::vector<std::string> Names) {
+    DirectModuleNames.insert_range(Names);
+  }
+
+  llvm::StringSet<> getRequiredModuleNames() const override {
+    return DirectModuleNames;
+  }
+
 private:
   llvm::SmallVector<std::shared_ptr<const ModuleFile>, 8> RequiredModules;
   // A helper class to speedup the query if a module is built.
   llvm::StringSet<> BuiltModuleNames;
+  // The directly required module names as scanned from the source file.
+  llvm::StringSet<> DirectModuleNames;
 };
 
 bool IsModuleFileUpToDate(PathRef ModuleFilePath,
@@ -1228,6 +1241,16 @@ bool ModulesBuilder::hasRequiredModules(PathRef File) {
   return !CachedMDB.getRequiredModules(File).empty();
 }
 
+std::vector<std::string> ModulesBuilder::getRequiredModuleNames(PathRef File) {
+  std::unique_ptr<ProjectModules> MDB = Impl->getCDB().getProjectModules(File);
+  if (!MDB)
+    return {};
+
+  CachingProjectModules CachedMDB(std::move(MDB),
+                                  Impl->getProjectModulesCache());
+  return CachedMDB.getRequiredModules(File);
+}
+
 std::unique_ptr<PrerequisiteModules>
 ModulesBuilder::buildPrerequisiteModulesFor(PathRef File,
                                             const ThreadsafeFS &TFS) {
@@ -1245,6 +1268,7 @@ ModulesBuilder::buildPrerequisiteModulesFor(PathRef File,
     return std::make_unique<ReusablePrerequisiteModules>();
 
   auto RequiredModules = std::make_unique<ReusablePrerequisiteModules>();
+  RequiredModules->setDirectModuleNames(RequiredModuleNames);
   for (llvm::StringRef RequiredModuleName : RequiredModuleNames) {
     // Return early if there is any error.
     if (llvm::Error Err = Impl->getOrBuildModuleFile(

clang-tools-extra/clangd/ModulesBuilder.h

@@ -26,6 +26,7 @@
 #include "support/ThreadsafeFS.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringSet.h"
 #include <memory>
 
 namespace clang {
@@ -76,6 +77,9 @@ public:
   canReuse(const CompilerInvocation &CI,
            llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>) const = 0;
 
+  /// Returns the set of directly required module names
+  virtual llvm::StringSet<> getRequiredModuleNames() const = 0;
+
   virtual ~PrerequisiteModules() = default;
 };
 
@@ -99,6 +103,9 @@ public:
 
   bool hasRequiredModules(PathRef File);
 
+  /// Returns the list of directly required module names
+  std::vector<std::string> getRequiredModuleNames(PathRef File);
+
 private:
   class ModulesBuilderImpl;
   std::unique_ptr<ModulesBuilderImpl> Impl;

clang-tools-extra/clangd/Preamble.cpp

@@ -728,11 +728,26 @@ bool isPreambleCompatible(const PreambleData &Preamble,
   auto Bounds = computePreambleBounds(CI.getLangOpts(), *ContentsBuffer,
                                       Inputs.Opts.SkipPreambleBuild);
   auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
+
+  // Check that the set of required modules hasn't changed.
+  // Preamble.RequiredModules->canReuse only verifies that the previously
+  // built module files are still up-to-date, but it cannot detect when a new
+  // import has been introduced.
+  auto RequiredModulesMatch = [&]() -> bool {
+    if (!Inputs.ModulesManager || !Preamble.RequiredModules)
+      return true;
+    auto NewNames = Inputs.ModulesManager->getRequiredModuleNames(FileName);
+    llvm::StringSet<> NewNameSet;
+    NewNameSet.insert_range(NewNames);
+    return NewNameSet == Preamble.RequiredModules->getRequiredModuleNames();
+  };
+
   return compileCommandsAreEqual(Inputs.CompileCommand,
                                  Preamble.CompileCommand) &&
          Preamble.Preamble.CanReuse(CI, *ContentsBuffer, Bounds, *VFS) &&
          (!Preamble.RequiredModules ||
-          Preamble.RequiredModules->canReuse(CI, VFS));
+          Preamble.RequiredModules->canReuse(CI, VFS)) &&
+         RequiredModulesMatch();
 }
 
 void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) {

clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp

@@ -15,6 +15,7 @@
 #include "CodeComplete.h"
 #include "Compiler.h"
 #include "ModulesBuilder.h"
+#include "Preamble.h"
 #include "ProjectModules.h"
 #include "TestTU.h"
 #include "support/Path.h"
@@ -1613,6 +1614,61 @@ struct TypeFromHeader {};
                                             named("TypeFromHeader")));
 }
 
+TEST_F(PrerequisiteModulesTests,
+       SkipPreambleBuildInvalidatedByNewModuleImport) {
+  MockDirectoryCompilationDatabase CDB(TestDir, FS);
+
+  CDB.addFile("Dep.cppm", R"cpp(
+export module Dep;
+)cpp");
+
+  CDB.addFile("Consumer.cpp", R"cpp(
+import Dep;
+void use() {}
+)cpp");
+
+  ModulesBuilder Builder(CDB);
+
+  auto Inputs = getInputs("Consumer.cpp", CDB);
+  Inputs.ModulesManager = &Builder;
+  Inputs.Opts.SkipPreambleBuild = true;
+
+  auto CI = buildCompilerInvocation(Inputs, DiagConsumer);
+  ASSERT_TRUE(CI);
+
+  auto Preamble = buildPreamble(getFullPath("Consumer.cpp"), *CI, Inputs,
+                                /*StoreInMemory=*/true,
+                                /*PreambleCallback=*/nullptr);
+  ASSERT_TRUE(Preamble);
+  EXPECT_EQ(Preamble->Preamble.getBounds().Size, 0u);
+  ASSERT_TRUE(Preamble->RequiredModules);
+
+  CDB.addFile("NewDep.cppm", R"cpp(
+export module NewDep;
+)cpp");
+
+  // Add a new import.
+  Inputs.Contents = R"cpp(
+import Dep;
+import NewDep;
+void use() {}
+)cpp";
+
+  {
+    std::error_code EC;
+    llvm::raw_fd_ostream OS(getFullPath("Consumer.cpp"), EC,
+                            llvm::sys::fs::OF_None);
+    ASSERT_FALSE(EC);
+    OS << Inputs.Contents;
+  }
+
+  auto NewCI = buildCompilerInvocation(Inputs, DiagConsumer);
+  ASSERT_TRUE(NewCI);
+
+  EXPECT_FALSE(isPreambleCompatible(*Preamble, Inputs,
+                                    getFullPath("Consumer.cpp"), *NewCI));
+}
+
 } // namespace
 } // namespace clang::clangd