[llvm-branch-commits] [clang-tools-extra] 2522fa0 - [clangd] Do not take stale definition from the static index.

[Aleksandr Platonov via llvm-branch-commits]

Author: Aleksandr Platonov
Date: 2020-12-23T18:21:38+03:00
New Revision: 2522fa053b62520ae48b4b27117ca003a2c878ab

URL: https://github.com/llvm/llvm-project/commit/2522fa053b62520ae48b4b27117ca003a2c878ab
DIFF: https://github.com/llvm/llvm-project/commit/2522fa053b62520ae48b4b27117ca003a2c878ab.diff

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

This is follow up to D93393.
Without this patch clangd takes the symbol definition from the static index if this definition was removed from the dynamic index.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D93683

Added: 
    

Modified: 
    clang-tools-extra/clangd/index/Merge.cpp
    clang-tools-extra/clangd/unittests/IndexTests.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clangd/index/Merge.cpp b/clang-tools-extra/clangd/index/Merge.cpp
index 97babacf2b38..f66f47499624 100644
--- a/clang-tools-extra/clangd/index/Merge.cpp
+++ b/clang-tools-extra/clangd/index/Merge.cpp
@@ -76,7 +76,13 @@ void MergedIndex::lookup(
   Dynamic->lookup(Req, [&](const Symbol &S) { B.insert(S); });
 
   auto RemainingIDs = Req.IDs;
+  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))
+      return;
     const Symbol *Sym = B.find(S.ID);
     RemainingIDs.erase(S.ID);
     if (!Sym)

diff  --git a/clang-tools-extra/clangd/unittests/IndexTests.cpp b/clang-tools-extra/clangd/unittests/IndexTests.cpp
index 8efc637d1250..ce4845e3e144 100644
--- a/clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ b/clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -290,6 +290,40 @@ TEST(MergeIndexTest, Lookup) {
   EXPECT_THAT(lookup(M, {}), UnorderedElementsAre());
 }
 
+TEST(MergeIndexTest, LookupRemovedDefinition) {
+  FileIndex DynamicIndex, 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};
+  unsigned SymbolCounter = 0;
+  Merge.lookup(LookupReq, [&](const Symbol &Sym) {
+    ++SymbolCounter;
+    EXPECT_FALSE(Sym.Definition);
+  });
+  EXPECT_EQ(SymbolCounter, 1u);
+}
+
 TEST(MergeIndexTest, FuzzyFind) {
   auto I = MemIndex::build(generateSymbols({"ns::A", "ns::B"}), RefSlab(),
                            RelationSlab()),