[PATCH] D93683: [clangd] Do not take stale definition from the static index.

[Aleksandr Platonov via Phabricator via cfe-commits]

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 =


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D93683.313280.patch
Type: text/x-patch
Size: 3549 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201222/455f068c/attachment-0001.bin>