[clang-tools-extra] 6d2fb3c - [clangd] Perform merging for stale symbols in MergeIndex
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 30 02:10:16 PDT 2021
Author: Kadir Cetinkaya
Date: 2021-03-30T11:09:51+02:00
New Revision: 6d2fb3cefba618be0326bb3da85d7568a72fefc4
URL: https://github.com/llvm/llvm-project/commit/6d2fb3cefba618be0326bb3da85d7568a72fefc4
DIFF: https://github.com/llvm/llvm-project/commit/6d2fb3cefba618be0326bb3da85d7568a72fefc4.diff
LOG: [clangd] Perform merging for stale symbols in MergeIndex
Clangd drops symbols from static index whenever the dynamic index is
authoritative for the file. This results in regressions when static and
dynamic index contains different set of information, e.g.
IncludeHeaders.
After this patch, we'll choose to merge symbols from static index with
dynamic one rather than just dropping. This implies correctness problems
when the definition/documentation of the symbol is deleted. But seems
like it is worth having in more cases.
We still drop symbols if dynamic index owns the file and didn't report
the symbol, which means symbol is deleted.
Differential Revision: https://reviews.llvm.org/D98538
Added:
Modified:
clang-tools-extra/clangd/index/Index.h
clang-tools-extra/clangd/index/Merge.cpp
clang-tools-extra/clangd/unittests/IndexTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h
index 91f7990312c7..04e67bc8df54 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<IndexContents(llvm::StringRef) const>
- indexedFiles() const = 0;
+ using IndexedFiles =
+ llvm::unique_function<IndexContents(llvm::StringRef) const>;
+ 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 54793cf566e8..54bb1ee06a05 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<void(const Symbol &)> 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<SymbolID> SeenDynamicSymbols;
+ llvm::DenseSet<SymbolID> 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 2b4b3af85613..5b6223a0ddc5 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 <utility>
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({"<header>", 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("<header>", 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, "<header>");
+}
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list