https://github.com/berkaysahiin created https://github.com/llvm/llvm-project/pull/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 fixes: #126350 >From d9a83f3220811a4bade49807b83ddb3e8d69971d Mon Sep 17 00:00:00 2001 From: Berkay Sahin <[email protected]> Date: Mon, 25 May 2026 02:18:13 +0300 Subject: [PATCH 1/2] [clangd] Invalidate preamble when new module imports are added 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 --- clang-tools-extra/clangd/ModulesBuilder.cpp | 25 +++++++++ clang-tools-extra/clangd/ModulesBuilder.h | 7 +++ clang-tools-extra/clangd/Preamble.cpp | 12 +++- .../unittests/PrerequisiteModulesTest.cpp | 56 +++++++++++++++++++ 4 files changed, 99 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 706fd459e15ec..2fe4b3b278a8f 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/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 @@ class FailedPrerequisiteModules : public PrerequisiteModules { 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,21 @@ class ReusablePrerequisiteModules : public PrerequisiteModules { RequiredModules.emplace_back(std::move(MF)); } + void setDirectModuleNames(std::vector<std::string> Names) { + for (auto &Name : Names) + DirectModuleNames.insert(std::move(Name)); + } + + 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 +1242,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 +1269,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( diff --git a/clang-tools-extra/clangd/ModulesBuilder.h b/clang-tools-extra/clangd/ModulesBuilder.h index b0e110b92b6a7..d8c8c446fd592 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.h +++ b/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 @@ class PrerequisiteModules { 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 @@ class ModulesBuilder { 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; diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 58da7edcf3b93..10e0c5eaef47c 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -728,11 +728,21 @@ bool isPreambleCompatible(const PreambleData &Preamble, auto Bounds = computePreambleBounds(CI.getLangOpts(), *ContentsBuffer, Inputs.Opts.SkipPreambleBuild); auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory); + auto ModuleNamesAreEqual = [&]() -> bool { + if (!Inputs.ModulesManager || !Preamble.RequiredModules) + return true; + auto NewNames = Inputs.ModulesManager->getRequiredModuleNames(FileName); + llvm::StringSet<> NewNameSet; + for (const auto &Name : NewNames) + NewNameSet.insert(Name); + 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)) && + ModuleNamesAreEqual(); } void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) { diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index b88213decea44..f163e248c383a 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/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 >From 510ca026f4486f0d6a0bfc5f91e7941b91275d20 Mon Sep 17 00:00:00 2001 From: Berkay Sahin <[email protected]> Date: Mon, 25 May 2026 02:30:04 +0300 Subject: [PATCH 2/2] [clangd] Prefer insert_range, rename local lambda --- clang-tools-extra/clangd/ModulesBuilder.cpp | 3 +-- clang-tools-extra/clangd/Preamble.cpp | 7 +++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 2fe4b3b278a8f..6e47122bdd82b 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -506,8 +506,7 @@ class ReusablePrerequisiteModules : public PrerequisiteModules { } void setDirectModuleNames(std::vector<std::string> Names) { - for (auto &Name : Names) - DirectModuleNames.insert(std::move(Name)); + DirectModuleNames.insert_range(Names); } llvm::StringSet<> getRequiredModuleNames() const override { diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 10e0c5eaef47c..f8d05b3dc0ded 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -728,13 +728,12 @@ bool isPreambleCompatible(const PreambleData &Preamble, auto Bounds = computePreambleBounds(CI.getLangOpts(), *ContentsBuffer, Inputs.Opts.SkipPreambleBuild); auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory); - auto ModuleNamesAreEqual = [&]() -> bool { + auto RequiredModulesMatch = [&]() -> bool { if (!Inputs.ModulesManager || !Preamble.RequiredModules) return true; auto NewNames = Inputs.ModulesManager->getRequiredModuleNames(FileName); llvm::StringSet<> NewNameSet; - for (const auto &Name : NewNames) - NewNameSet.insert(Name); + NewNameSet.insert_range(NewNames); return NewNameSet == Preamble.RequiredModules->getRequiredModuleNames(); }; return compileCommandsAreEqual(Inputs.CompileCommand, @@ -742,7 +741,7 @@ bool isPreambleCompatible(const PreambleData &Preamble, Preamble.Preamble.CanReuse(CI, *ContentsBuffer, Bounds, *VFS) && (!Preamble.RequiredModules || Preamble.RequiredModules->canReuse(CI, VFS)) && - ModuleNamesAreEqual(); + RequiredModulesMatch(); } void escapeBackslashAndQuotes(llvm::StringRef Text, llvm::raw_ostream &OS) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
