ArcsinX created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
ArcsinX requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
This is follow up to D93393 <https://reviews.llvm.org/D93393>.
Without this patch clangd takes the symbol definition from the static index if
this definition was removed from the dynamic index.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93683
Files:
clang-tools-extra/clangd/index/FileIndex.cpp
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
@@ -290,6 +290,42 @@
EXPECT_THAT(lookup(M, {}), UnorderedElementsAre());
}
+TEST(MergeIndexTest, LookupRemovedDefinition) {
+ FileIndex DynamicIndex;
+ FileIndex StaticIndex;
+ MergedIndex Merge(&DynamicIndex, &StaticIndex);
+
+ const char *HeaderCode = "class Foo;";
+ auto HeaderSymbols = TestTU::withHeaderCode(HeaderCode).headerSymbols();
+ auto Foo = findSymbol(HeaderSymbols, "Foo");
+
+ // Build static index for test.cc with Foo definition
+ TestTU Test;
+ Test.HeaderCode = HeaderCode;
+ Test.Code = "class Foo {};";
+ Test.Filename = "test.cc";
+ auto AST = Test.build();
+ StaticIndex.updateMain(testPath(Test.Filename), AST);
+
+ // Remove Foo definition from test.cc, i.e. build dynamic index for test.cc
+ // without Foo definition.
+ Test.Code = "class Foo;";
+ 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.
+ LookupRequest LookupReq;
+ LookupReq.IDs = {Foo.ID};
+ std::vector<Symbol> Symbols;
+ Merge.lookup(LookupReq, [&](const Symbol &Sym) { Symbols.push_back(Sym); });
+ ASSERT_EQ(Symbols.size(), 1u);
+ const auto &Sym = Symbols.front();
+ EXPECT_FALSE(Sym.Definition);
+ EXPECT_EQ(std::string(Sym.CanonicalDeclaration.FileURI),
+ std::string("unittest:///TestTU.h"));
+}
+
TEST(MergeIndexTest, FuzzyFind) {
auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"}), RefSlab(),
RelationSlab()),
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -76,7 +76,11 @@
Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); });
auto RemainingIDs = Req.IDs;
+ auto DynamicContainsFile = Dynamic->indexedFiles();
Static->lookup(Req, [&](const Symbol &S) {
+ if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
+ : S.CanonicalDeclaration.FileURI))
+ return;
const Symbol *Sym = B.find(S.ID);
RemainingIDs.erase(S.ID);
if (!Sym)
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -428,11 +428,17 @@
// We are using the key received from ShardedIndex, so it should always
// exist.
assert(IF);
- PreambleSymbols.update(
- Uri, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
- std::make_unique<RefSlab>(),
- std::make_unique<RelationSlab>(std::move(*IF->Relations)),
- /*CountReferences=*/false);
+ auto FilePath = URI::resolve(Uri, Path);
+ if (FilePath) {
+ PreambleSymbols.update(
+ *FilePath, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
+ std::make_unique<RefSlab>(),
+ std::make_unique<RelationSlab>(std::move(*IF->Relations)),
+ /*CountReferences=*/false);
+ } else {
+ elog("Update preamble: could not resolve URI {0}: {1}", Uri,
+ FilePath.takeError());
+ }
}
size_t IndexVersion = 0;
auto NewIndex =
Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -290,6 +290,42 @@
EXPECT_THAT(lookup(M, {}), UnorderedElementsAre());
}
+TEST(MergeIndexTest, LookupRemovedDefinition) {
+ FileIndex DynamicIndex;
+ FileIndex StaticIndex;
+ MergedIndex Merge(&DynamicIndex, &StaticIndex);
+
+ const char *HeaderCode = "class Foo;";
+ auto HeaderSymbols = TestTU::withHeaderCode(HeaderCode).headerSymbols();
+ auto Foo = findSymbol(HeaderSymbols, "Foo");
+
+ // Build static index for test.cc with Foo definition
+ TestTU Test;
+ Test.HeaderCode = HeaderCode;
+ Test.Code = "class Foo {};";
+ Test.Filename = "test.cc";
+ auto AST = Test.build();
+ StaticIndex.updateMain(testPath(Test.Filename), AST);
+
+ // Remove Foo definition from test.cc, i.e. build dynamic index for test.cc
+ // without Foo definition.
+ Test.Code = "class Foo;";
+ 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.
+ LookupRequest LookupReq;
+ LookupReq.IDs = {Foo.ID};
+ std::vector<Symbol> Symbols;
+ Merge.lookup(LookupReq, [&](const Symbol &Sym) { Symbols.push_back(Sym); });
+ ASSERT_EQ(Symbols.size(), 1u);
+ const auto &Sym = Symbols.front();
+ EXPECT_FALSE(Sym.Definition);
+ EXPECT_EQ(std::string(Sym.CanonicalDeclaration.FileURI),
+ std::string("unittest:///TestTU.h"));
+}
+
TEST(MergeIndexTest, FuzzyFind) {
auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"}), RefSlab(),
RelationSlab()),
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -76,7 +76,11 @@
Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); });
auto RemainingIDs = Req.IDs;
+ auto DynamicContainsFile = Dynamic->indexedFiles();
Static->lookup(Req, [&](const Symbol &S) {
+ if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
+ : S.CanonicalDeclaration.FileURI))
+ return;
const Symbol *Sym = B.find(S.ID);
RemainingIDs.erase(S.ID);
if (!Sym)
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===================================================================
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -428,11 +428,17 @@
// We are using the key received from ShardedIndex, so it should always
// exist.
assert(IF);
- PreambleSymbols.update(
- Uri, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
- std::make_unique<RefSlab>(),
- std::make_unique<RelationSlab>(std::move(*IF->Relations)),
- /*CountReferences=*/false);
+ auto FilePath = URI::resolve(Uri, Path);
+ if (FilePath) {
+ PreambleSymbols.update(
+ *FilePath, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
+ std::make_unique<RefSlab>(),
+ std::make_unique<RelationSlab>(std::move(*IF->Relations)),
+ /*CountReferences=*/false);
+ } else {
+ elog("Update preamble: could not resolve URI {0}: {1}", Uri,
+ FilePath.takeError());
+ }
}
size_t IndexVersion = 0;
auto NewIndex =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
