diff --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp --- a/clang-tools-extra/clangd/index/Merge.cpp +++ b/clang-tools-extra/clangd/index/Merge.cpp @@ -43,30 +43,40 @@ }); SymbolSlab Dyn = std::move(DynB).build(); - llvm::DenseSet SeenDynamicSymbols; + llvm::DenseSet ReportedDynSymbols; { auto DynamicContainsFile = Dynamic->indexedFiles(); More |= Static->fuzzyFind(Req, [&](const Symbol &S) { - // We expect the definition to see the canonical declaration, so it seems - // to be enough to check only the definition if it exists. - if ((DynamicContainsFile(S.Definition ? S.Definition.FileURI + auto DynS = Dyn.find(S.ID); + // If symbol also exist in the dynamic index, just merge and report. + if (DynS != Dyn.end()) { + ++StaticCount; + ++MergedCount; + ReportedDynSymbols.insert(S.ID); + return Callback(mergeSymbol(*DynS, S)); + } + + bool DynamicIndexIsAuthoritative = + // We expect the definition to see the canonical declaration, so it + // seems to be enough to check only the definition if it exists. + (DynamicContainsFile(S.Definition ? S.Definition.FileURI : S.CanonicalDeclaration.FileURI) & - IndexContents::Symbols) != IndexContents::None) + IndexContents::Symbols) != IndexContents::None; + // Otherwise, if the dynamic index owns the symbol's file, it means static + // index is stale just drop the symbol. + if (DynamicIndexIsAuthoritative) return; - auto DynS = Dyn.find(S.ID); + + // If not just report the symbol from static index as is. ++StaticCount; - if (DynS == Dyn.end()) - return Callback(S); - ++MergedCount; - SeenDynamicSymbols.insert(S.ID); - Callback(mergeSymbol(*DynS, S)); + return Callback(S); }); } SPAN_ATTACH(Tracer, "dynamic", DynamicCount); SPAN_ATTACH(Tracer, "static", StaticCount); SPAN_ATTACH(Tracer, "merged", MergedCount); for (const Symbol &S : Dyn) - if (!SeenDynamicSymbols.count(S.ID)) + if (!ReportedDynSymbols.count(S.ID)) Callback(S); return More; } @@ -83,18 +93,27 @@ { auto DynamicContainsFile = Dynamic->indexedFiles(); Static->lookup(Req, [&](const Symbol &S) { - // We expect the definition to see the canonical declaration, so it seems - // to be enough to check only the definition if it exists. - if ((DynamicContainsFile(S.Definition ? S.Definition.FileURI + // If we've seen the symbol before, just merge. + if (const Symbol *Sym = B.find(S.ID)) { + RemainingIDs.erase(S.ID); + return Callback(mergeSymbol(*Sym, S)); + } + + // If symbol is missing in dynamic index, and dynamic index owns the + // symbol's file. Static index is stale, just drop the symbol. + bool DynamicIndexIsAuthoritative = + // We expect the definition to see the canonical declaration, so it + // seems to be enough to check only the definition if it exists. + (DynamicContainsFile(S.Definition ? S.Definition.FileURI : S.CanonicalDeclaration.FileURI) & - IndexContents::Symbols) != IndexContents::None) + IndexContents::Symbols) != IndexContents::None; + if (DynamicIndexIsAuthoritative) return; - const Symbol *Sym = B.find(S.ID); + + // Dynamic index doesn't know about this file, just use the symbol from + // static index. RemainingIDs.erase(S.ID); - if (!Sym) - Callback(S); - else - Callback(mergeSymbol(*Sym, S)); + Callback(S); }); } for (const auto &ID : RemainingIDs) diff --git a/clang-tools-extra/clangd/unittests/IndexTests.cpp b/clang-tools-extra/clangd/unittests/IndexTests.cpp --- a/clang-tools-extra/clangd/unittests/IndexTests.cpp +++ b/clang-tools-extra/clangd/unittests/IndexTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "SyncAPI.h" #include "TestIndex.h" #include "TestTU.h" #include "index/FileIndex.h" @@ -17,6 +18,7 @@ #include "clang/Index/IndexSymbol.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include using ::testing::_; using ::testing::AllOf; @@ -312,16 +314,26 @@ AST = Test.build(); DynamicIndex.updateMain(testPath(Test.Filename), AST); - // Merged index should not return the symbol definition if this definition - // location is inside a file from the dynamic index. + // Even though the definition is actually deleted in the newer version of the + // file, we still chose to merge with information coming from static index. LookupRequest LookupReq; LookupReq.IDs = {Foo.ID}; unsigned SymbolCounter = 0; Merge.lookup(LookupReq, [&](const Symbol &Sym) { ++SymbolCounter; - EXPECT_FALSE(Sym.Definition); + EXPECT_TRUE(Sym.Definition); }); EXPECT_EQ(SymbolCounter, 1u); + + // Drop the symbol completely. + Test.Code = "class Bar {};"; + AST = Test.build(); + DynamicIndex.updateMain(testPath(Test.Filename), AST); + + // Now we don't expect to see the symbol at all. + SymbolCounter = 0; + Merge.lookup(LookupReq, [&](const Symbol &Sym) { ++SymbolCounter; }); + EXPECT_EQ(SymbolCounter, 0u); } TEST(MergeIndexTest, FuzzyFind) { @@ -585,6 +597,44 @@ IncludeHeaderWithRef("new", 1u))); } +TEST(MergeIndexTest, IncludeHeadersMerged) { + auto S = symbol("Z"); + S.Definition.FileURI = "unittest:///foo.cc"; + + SymbolSlab::Builder DynB; + S.IncludeHeaders.clear(); + DynB.insert(S); + SymbolSlab DynSymbols = std::move(DynB).build(); + RefSlab DynRefs; + auto DynSize = DynSymbols.bytes() + DynRefs.bytes(); + auto DynData = std::make_pair(std::move(DynSymbols), std::move(DynRefs)); + llvm::StringSet<> DynFiles = {S.Definition.FileURI}; + MemIndex DynIndex(std::move(DynData.first), std::move(DynData.second), + RelationSlab(), std::move(DynFiles), IndexContents::Symbols, + std::move(DynData), DynSize); + + SymbolSlab::Builder StaticB; + S.IncludeHeaders.push_back({"
", 0}); + StaticB.insert(S); + auto StaticIndex = + MemIndex::build(std::move(StaticB).build(), RefSlab(), RelationSlab()); + MergedIndex Merge(&DynIndex, StaticIndex.get()); + + EXPECT_THAT(runFuzzyFind(Merge, S.Name), + ElementsAre(testing::Field( + &Symbol::IncludeHeaders, + ElementsAre(IncludeHeaderWithRef("
", 0u))))); + + LookupRequest Req; + Req.IDs = {S.ID}; + std::string IncludeHeader; + Merge.lookup(Req, [&](const Symbol &S) { + EXPECT_TRUE(IncludeHeader.empty()); + ASSERT_EQ(S.IncludeHeaders.size(), 1u); + IncludeHeader = S.IncludeHeaders.front().IncludeHeader.str(); + }); + EXPECT_EQ(IncludeHeader, "
"); +} } // namespace } // namespace clangd } // namespace clang