kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: usaxena95, arphaman.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D98538
Files:
clang-tools-extra/clangd/index/Merge.cpp
clang-tools-extra/clangd/unittests/IndexTests.cpp
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ 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,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({"<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
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -43,30 +43,40 @@
});
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
+ 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)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
