diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h index 91f7990312c76a..04e67bc8df54f3 100644 --- a/clang-tools-extra/clangd/index/Index.h +++ b/clang-tools-extra/clangd/index/Index.h @@ -149,8 +149,9 @@ class SymbolIndex { /// Returns function which checks if the specified file was used to build this /// index or not. The function must only be called while the index is alive. - virtual llvm::unique_function - indexedFiles() const = 0; + using IndexedFiles = + llvm::unique_function; + virtual IndexedFiles indexedFiles() const = 0; /// Returns estimated size of index (in bytes). virtual size_t estimateMemoryUsage() const = 0; diff --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp index 54793cf566e880..54bb1ee06a05bb 100644 --- a/clang-tools-extra/clangd/index/Merge.cpp +++ b/clang-tools-extra/clangd/index/Merge.cpp @@ -22,6 +22,19 @@ namespace clang { namespace clangd { +namespace { + +// Returns true if file defining/declaring \p S is covered by \p Index. +bool isIndexAuthoritative(const SymbolIndex::IndexedFiles &Index, + 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. + const char *OwningFile = + S.Definition ? S.Definition.FileURI : S.CanonicalDeclaration.FileURI; + return (Index(OwningFile) & IndexContents::Symbols) != IndexContents::None; +} +} // namespace + bool MergedIndex::fuzzyFind( const FuzzyFindRequest &Req, llvm::function_ref Callback) const { @@ -37,36 +50,44 @@ bool MergedIndex::fuzzyFind( unsigned DynamicCount = 0; unsigned StaticCount = 0; unsigned MergedCount = 0; + // Number of results ignored due to staleness. + unsigned StaticDropped = 0; More |= Dynamic->fuzzyFind(Req, [&](const Symbol &S) { ++DynamicCount; DynB.insert(S); }); 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 - : S.CanonicalDeclaration.FileURI) & - IndexContents::Symbols) != IndexContents::None) - return; - auto DynS = Dyn.find(S.ID); ++StaticCount; - if (DynS == Dyn.end()) - return Callback(S); - ++MergedCount; - SeenDynamicSymbols.insert(S.ID); - Callback(mergeSymbol(*DynS, S)); + auto DynS = Dyn.find(S.ID); + // If symbol also exist in the dynamic index, just merge and report. + if (DynS != Dyn.end()) { + ++MergedCount; + ReportedDynSymbols.insert(S.ID); + return Callback(mergeSymbol(*DynS, S)); + } + + // Otherwise, if the dynamic index owns the symbol's file, it means static + // index is stale just drop the symbol. + if (isIndexAuthoritative(DynamicContainsFile, S)) { + ++StaticDropped; + return; + } + + // If not just report the symbol from static index as is. + return Callback(S); }); } SPAN_ATTACH(Tracer, "dynamic", DynamicCount); SPAN_ATTACH(Tracer, "static", StaticCount); + SPAN_ATTACH(Tracer, "static_dropped", StaticDropped); 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 +104,21 @@ void MergedIndex::lookup( { 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 - : S.CanonicalDeclaration.FileURI) & - IndexContents::Symbols) != IndexContents::None) + // 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. + if (isIndexAuthoritative(DynamicContainsFile, S)) 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 index 2b4b3af856131c..5b6223a0ddc565 100644 --- 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,28 @@ TEST(MergeIndexTest, LookupRemovedDefinition) { 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. + // This seems wrong, but is generic behavior we want for e.g. include headers + // which are always missing from the dynamic 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 +599,44 @@ TEST(MergeTest, MergeIncludesOnDifferentDefinitions) { 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