Author: Berkay Sahin Date: 2026-07-03T08:57:39+08:00 New Revision: d7cd957bee261ce6e477f16c8768fe8889f599c5
URL: https://github.com/llvm/llvm-project/commit/d7cd957bee261ce6e477f16c8768fe8889f599c5 DIFF: https://github.com/llvm/llvm-project/commit/d7cd957bee261ce6e477f16c8768fe8889f599c5.diff LOG: [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 Added: Modified: clang-tools-extra/clangd/ModulesBuilder.cpp clang-tools-extra/clangd/ModulesBuilder.h clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 706fd459e15ec..6e47122bdd82b 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,20 @@ class ReusablePrerequisiteModules : public PrerequisiteModules { 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( 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..31f141f62eb7f 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/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) { 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
