This revision was automatically updated to reflect the committed changes. Closed by commit rG2522fa053b62: [clangd] Do not take stale definition from the static index. (authored by ArcsinX).
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93683/new/ https://reviews.llvm.org/D93683 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 @@ -290,6 +290,40 @@ 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()), 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,13 @@ 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)
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,40 @@ 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()), 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,13 @@ 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)
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits