https://github.com/TheOnionCutter1 created https://github.com/llvm/llvm-project/pull/215018
## Problem When a workspace contains two mutually exclusive implementations of the same function, only the active implementation is present in the compilation database. If the inactive source file is opened, clangd's dynamic index may cause Go to Definition to navigate to the inactive implementation instead. See clangd/clangd#1516 for more information. ## Approach - Add a navigation-specific index for resolving definitions. - Preserve information from the dynamic index for open files. - When the dynamic and project indexes contain conflicting definitions, prefer the project definition if the dynamic definition comes from outside the active project index. - Keep the existing index behavior unchanged for other features. ## Testing - Manually tested with a small multi-profile C project. - Added `ProjectDefinitionIndexTest.DefinitionPolicy`. - Added `ClangdServerTest.PrefersProjectDefinitionForNavigation`. - Ran the full `ClangdTests` suite. Fixes clangd/clangd#1516 Assisted-by: OpenAI Codex >From f6e8dd4b67561f6dc1a0a19c48849871d584f7e0 Mon Sep 17 00:00:00 2001 From: Omer <[email protected]> Date: Sat, 8 Aug 2026 12:49:21 +0300 Subject: [PATCH] [clangd] Prefer project definitions for navigation Prevent definitions from open files in the dynamic index from overriding project-index definitions when navigating from a project file. Preserve dynamic definitions for external files and AST definitions for unsaved edits. Assisted-by: OpenAI Codex Make a different approach --- clang-tools-extra/clangd/ClangdServer.cpp | 9 ++- clang-tools-extra/clangd/ClangdServer.h | 5 ++ clang-tools-extra/clangd/index/Merge.cpp | 29 +++++++ clang-tools-extra/clangd/index/Merge.h | 18 +++++ .../clangd/unittests/ClangdTests.cpp | 39 ++++++++++ .../clangd/unittests/IndexTests.cpp | 77 +++++++++++++++++++ .../clangd/unittests/XRefsTests.cpp | 1 + clang-tools-extra/docs/ReleaseNotes.md | 4 + 8 files changed, 181 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 37eb82116f3a9..0d0a343d2c585 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -268,8 +268,15 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, std::move(BGOpts)); AddIndex(BackgroundIdx.get()); } + const SymbolIndex *ProjectIndex = Index; if (DynamicIdx) AddIndex(DynamicIdx.get()); + NavigationIndex = Index; + if (DynamicIdx && ProjectIndex) { + ProjectDefinitionIdx = std::make_unique<ProjectDefinitionIndex>( + DynamicIdx.get(), ProjectIndex); + NavigationIndex = ProjectDefinitionIdx.get(); + } if (Opts.FeatureModules) { FeatureModule::Facilities F{ @@ -809,7 +816,7 @@ void ClangdServer::locateSymbolAt(PathRef File, Position Pos, this](llvm::Expected<InputsAndAST> InpAST) mutable { if (!InpAST) return CB(InpAST.takeError()); - CB(clangd::locateSymbolAt(InpAST->AST, Pos, Index)); + CB(clangd::locateSymbolAt(InpAST->AST, Pos, NavigationIndex)); }; WorkScheduler->runWithAST("Definitions", File, std::move(Action)); diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 264ab7437c248..f5e77ad370ac8 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -43,6 +43,7 @@ namespace clang { namespace clangd { +class ProjectDefinitionIndex; /// Manages a collection of source files and derived data (ASTs, indexes), /// and provides language-aware features such as code completion. /// @@ -494,12 +495,16 @@ class ClangdServer { // - the static index passed to the constructor // - a merged view of a static and dynamic index (MergedIndex) const SymbolIndex *Index = nullptr; + // Index used for navigation. Usually Index, but when both dynamic and + // project indexes exist this applies project-aware definition selection. + const SymbolIndex *NavigationIndex = nullptr; // If present, an index of symbols in open files. Read via *Index. std::unique_ptr<FileIndex> DynamicIdx; // If present, the new "auto-index" maintained in background threads. std::unique_ptr<BackgroundIndex> BackgroundIdx; // Storage for merged views of the various indexes. std::vector<std::unique_ptr<SymbolIndex>> MergedIdx; + std::unique_ptr<ProjectDefinitionIndex> ProjectDefinitionIdx; // Manage module files. ModulesBuilder *ModulesManager = nullptr; diff --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp index 625b1c6926a28..ee1ec2ddec65e 100644 --- a/clang-tools-extra/clangd/index/Merge.cpp +++ b/clang-tools-extra/clangd/index/Merge.cpp @@ -122,6 +122,35 @@ void MergedIndex::lookup( Callback(*Sym); } +void ProjectDefinitionIndex::lookup( + const LookupRequest &Req, + llvm::function_ref<void(const Symbol &)> Callback) const { + trace::Span Tracer("ProjectDefinitionIndex lookup"); + SymbolSlab::Builder DynamicSymbols; + Dynamic->lookup(Req, [&](const Symbol &S) { DynamicSymbols.insert(S); }); + + auto RemainingIDs = Req.IDs; + auto DynamicContainsFile = Dynamic->indexedFiles(); + auto ProjectContainsFile = Project->indexedFiles(); + Project->lookup(Req, [&](const Symbol &ProjectSymbol) { + RemainingIDs.erase(ProjectSymbol.ID); + if (const Symbol *DynamicSymbol = DynamicSymbols.find(ProjectSymbol.ID)) { + Symbol Result = mergeSymbol(*DynamicSymbol, ProjectSymbol); + if (ProjectSymbol.Definition && DynamicSymbol->Definition && + !isIndexAuthoritative(ProjectContainsFile, *DynamicSymbol)) + Result.Definition = ProjectSymbol.Definition; + return Callback(Result); + } + + if (isIndexAuthoritative(DynamicContainsFile, ProjectSymbol)) + return; + Callback(ProjectSymbol); + }); + for (const auto &ID : RemainingIDs) + if (const Symbol *S = DynamicSymbols.find(ID)) + Callback(*S); +} + bool MergedIndex::refs(const RefsRequest &Req, llvm::function_ref<void(const Ref &)> Callback) const { trace::Span Tracer("MergedIndex refs"); diff --git a/clang-tools-extra/clangd/index/Merge.h b/clang-tools-extra/clangd/index/Merge.h index 5910c27cab58a..5f8f2c1b09a41 100644 --- a/clang-tools-extra/clangd/index/Merge.h +++ b/clang-tools-extra/clangd/index/Merge.h @@ -55,6 +55,24 @@ class MergedIndex : public SymbolIndex { } }; +// A merged view for definition lookup that treats project files specially. +// Dynamic definitions from project files are preferred because they may contain +// unsaved edits. Definitions from dynamic files outside the project are +// superseded by project definitions, so merely opening an inactive source file +// cannot redirect navigation. +// +// The provided indexes must outlive this non-owning view. +class ProjectDefinitionIndex : public MergedIndex { + const SymbolIndex *Dynamic, *Project; + +public: + ProjectDefinitionIndex(const SymbolIndex *Dynamic, const SymbolIndex *Project) + : MergedIndex(Dynamic, Project), Dynamic(Dynamic), Project(Project) {} + + void lookup(const LookupRequest &, + llvm::function_ref<void(const Symbol &)>) const override; +}; + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp index 9ea7c3e02411d..433f7133466ef 100644 --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -368,6 +368,45 @@ TEST(ClangdServerTest, RespectsConfig) { EXPECT_NE(Result->front().PreferredDeclaration.range, Example.range()); } +TEST(ClangdServerTest, PrefersProjectDefinitionForNavigation) { + TestTU ActiveTU; + ActiveTU.HeaderCode = "void target();"; + ActiveTU.Code = "void target() {}"; + ActiveTU.Filename = "active.cpp"; + auto ProjectIndex = ActiveTU.index(); + + auto Opts = ClangdServer::optsForTest(); + Opts.BuildDynamicSymbolIndex = true; + Opts.StaticIndex = ProjectIndex.get(); + MockCompilationDatabase CDB; + MockFS FS; + ClangdServer Server(CDB, FS, Opts); + + runAddDocument(Server, testPath("inactive.cpp"), "void target() {}"); + Annotations Main(R"cpp( + void target(); + int main() { ^target(); } + )cpp"); + runAddDocument(Server, testPath("main.cpp"), Main.code()); + + auto Result = runLocateSymbolAt(Server, testPath("main.cpp"), Main.point()); + ASSERT_TRUE(bool(Result)) << Result.takeError(); + ASSERT_THAT(*Result, SizeIs(1)); + ASSERT_TRUE(Result->front().Definition); + EXPECT_EQ(Result->front().Definition->uri.file(), testPath("active.cpp")); + + Annotations Unsaved(R"cpp( + void target() {} + int main() { ^target(); } + )cpp"); + runAddDocument(Server, testPath("main.cpp"), Unsaved.code()); + Result = runLocateSymbolAt(Server, testPath("main.cpp"), Unsaved.point()); + ASSERT_TRUE(bool(Result)) << Result.takeError(); + ASSERT_THAT(*Result, SizeIs(1)); + ASSERT_TRUE(Result->front().Definition); + EXPECT_EQ(Result->front().Definition->uri.file(), testPath("main.cpp")); +} + TEST(ClangdServerTest, PropagatesVersion) { MockCompilationDatabase CDB; MockFS FS; diff --git a/clang-tools-extra/clangd/unittests/IndexTests.cpp b/clang-tools-extra/clangd/unittests/IndexTests.cpp index a66680d39c87d..6deed8f7fe156 100644 --- a/clang-tools-extra/clangd/unittests/IndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/IndexTests.cpp @@ -41,6 +41,20 @@ MATCHER_P(refRange, Range, "") { } MATCHER_P(fileURI, F, "") { return StringRef(arg.Location.FileURI) == F; } +class CountingIndex : public SwapIndex { +public: + explicit CountingIndex(std::unique_ptr<SymbolIndex> Index) + : SwapIndex(std::move(Index)) {} + + void lookup(const LookupRequest &Req, + llvm::function_ref<void(const Symbol &)> Callback) const override { + ++LookupCount; + SwapIndex::lookup(Req, Callback); + } + + mutable unsigned LookupCount = 0; +}; + TEST(SymbolLocation, Position) { using Position = SymbolLocation::Position; Position Pos; @@ -291,6 +305,69 @@ TEST(MergeIndexTest, Lookup) { EXPECT_THAT(lookup(M, {}), UnorderedElementsAre()); } +TEST(ProjectDefinitionIndexTest, DefinitionPolicy) { + constexpr llvm::StringLiteral DynamicURI("unittest:///dynamic.cpp"); + constexpr llvm::StringLiteral ProjectURI("unittest:///project.cpp"); + Symbol DynamicSymbol = symbol("target"); + DynamicSymbol.Definition.FileURI = DynamicURI.data(); + DynamicSymbol.Documentation = "dynamic documentation"; + Symbol ProjectSymbol = symbol("target"); + ProjectSymbol.Definition.FileURI = ProjectURI.data(); + + auto MakeIndex = [](const Symbol &S, + llvm::ArrayRef<llvm::StringRef> IndexedFiles) { + SymbolSlab::Builder Symbols; + Symbols.insert(S); + SymbolSlab SymbolData = std::move(Symbols).build(); + RefSlab RefData; + auto Size = SymbolData.bytes() + RefData.bytes(); + auto Data = std::make_pair(std::move(SymbolData), std::move(RefData)); + llvm::StringSet<> Files; + for (llvm::StringRef File : IndexedFiles) + Files.insert(File); + return std::make_unique<MemIndex>(Data.first, Data.second, RelationSlab(), + std::move(Files), IndexContents::Symbols, + std::move(Data), Size); + }; + auto DefinitionURI = [](const SymbolIndex &Index) { + std::string Result; + LookupRequest Req; + Req.IDs.insert(SymbolID("target")); + Index.lookup(Req, [&](const Symbol &S) { + Result = S.Definition.FileURI; + EXPECT_EQ(S.Documentation, "dynamic documentation"); + }); + return Result; + }; + + auto Dynamic = MakeIndex(DynamicSymbol, {}); + auto Project = MakeIndex(ProjectSymbol, {ProjectURI}); + ProjectDefinitionIndex UnlistedDynamic(Dynamic.get(), Project.get()); + EXPECT_EQ(DefinitionURI(UnlistedDynamic), ProjectURI); + + auto ProjectCoveringDynamic = + MakeIndex(ProjectSymbol, {ProjectURI, DynamicURI}); + ProjectDefinitionIndex ActiveDynamic(Dynamic.get(), + ProjectCoveringDynamic.get()); + EXPECT_EQ(DefinitionURI(ActiveDynamic), DynamicURI); + + ProjectSymbol.Definition = {}; + auto ProjectWithoutDefinition = MakeIndex(ProjectSymbol, {ProjectURI}); + ProjectDefinitionIndex DynamicFallback(Dynamic.get(), + ProjectWithoutDefinition.get()); + EXPECT_EQ(DefinitionURI(DynamicFallback), DynamicURI); + + MergedIndex OrdinaryMerge(Dynamic.get(), Project.get()); + EXPECT_EQ(DefinitionURI(OrdinaryMerge), DynamicURI); + + CountingIndex CountedDynamic(MakeIndex(DynamicSymbol, {})); + CountingIndex CountedProject(MakeIndex(ProjectSymbol, {ProjectURI})); + ProjectDefinitionIndex Counted(&CountedDynamic, &CountedProject); + DefinitionURI(Counted); + EXPECT_EQ(CountedDynamic.LookupCount, 1u); + EXPECT_EQ(CountedProject.LookupCount, 1u); +} + TEST(MergeIndexTest, LookupRemovedDefinition) { FileIndex DynamicIndex(true), StaticIndex(true); MergedIndex Merge(&DynamicIndex, &StaticIndex); diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index dce033af73c1a..5746b6eb75365 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -16,6 +16,7 @@ #include "TestWorkspace.h" #include "XRefs.h" #include "index/MemIndex.h" +#include "index/Merge.h" #include "clang/AST/Decl.h" #include "clang/Basic/SourceLocation.h" #include "llvm/ADT/StringRef.h" diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 5ded07934d906..bfd20e78d7446 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -76,6 +76,10 @@ infrastructure are described first, followed by tool-specific sections. #### Cross-references +- Improved Go to Definition in projects with alternative source + implementations. Definitions from files in the project index are now + preferred over definitions from open files outside the active project. + #### Objective-C #### Miscellaneous _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
