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

[ArcsinX]

Summary

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.

[ArcsinX]

I also want to propose the similar patch for fuzzyFind() (as a part of this patch or a separate one), but I faced the following problem:

Test.cpp

#define FOO 1

This example creates empty dynamic index for main file symbols and static index for preamble symbols (FOO). The location of FOO is inside Test.cpp, so (according to the logic that the static index could be stale) we should toss FOO from the static index (i.e. preamble symbols), but this behaviour is incorrect... Any advice?

clang-tools-extra/clangd/index/FileIndex.cpp

434 ↗

(On Diff #313280)

We do not need this change to fix behaviour for removed definition, but this is the only one place where we use URI instead of path as a key.

[ArcsinX]

• Simplify test.
• Revert PreambleSymbols.update() first argument change.

[sammccall]

In D93683#2467631, @ArcsinX wrote:

I also want to propose the similar patch for fuzzyFind() (as a part of this patch or a separate one), but I faced the following problem:

Test.cpp

#define FOO 1

This example creates empty dynamic index for main file symbols and static index for preamble symbols (FOO). The location of FOO is inside Test.cpp, so (according to the logic that the static index could be stale) we should toss FOO from the static index (i.e. preamble symbols), but this behaviour is incorrect... Any advice?

Ugh, this case manages to fall through the cracks of our model every time.

Originally the idea was:

• features traverse the AST for the current file, and consult the index for headers
• uh... macros do something analogous to that
• this lines up with the preamble/parsedAST split

But macros defined in the preamble section aren't in headers but also aren't parsed in parsedAST, so they're really awkward to handle. We end up playing whack-a-mole with these bugs. (See loadMainFilePreambleMacros in CodeComplete.cpp).

In this case, it seems reasonable to include the macros in the dynamic index for the file. This is a bit of work (requires handling MainFileMacros::Name in SymbolCollector::handleMacros I guess) but is probably less work than excluding the symbols from the index entirely and having to work around this in every feature.

It *also* seems reasonable to exclude these main-file symbols from the preamble index. For one thing, the preamble index will include all the files with any symbol defined in indexedFiles(), and the whole file isn't indexed!

I'm not 100% sure this bug would block adding the equivalent change for fuzzyFind though - it'll affect documentSymbol but not code-completion IIUC (because of the workaround I mentioned above). And these symbols are pretty rare.

clang-tools-extra/clangd/index/FileIndex.cpp

434 ↗

(On Diff #313280)

Yeah... nevertheless the key is totally arbitrary here, so we might as well use whatever is most convenient/cheapest.

clang-tools-extra/clangd/index/Merge.cpp

81

this seems OK because we expect the definition to see the canonical declaration - subtle enough to be worth a comment.
(The dumb version that always checks CanonicalDeclaration, and sometimes also checks Definition, would be a bit more obvious, but saving the lookup is probably worthwhile here)

[ArcsinX]

In D93683#2468842, @sammccall wrote:

I'm not 100% sure this bug would block adding the equivalent change for fuzzyFind though - it'll affect documentSymbol but not code-completion IIUC (because of the workaround I mentioned above). And these symbols are pretty rare.

With the similar change for fuzzyFind(), WorkspaceSymbols.Macros test fails. But maybe I can create one more review for this and we will discuss it there.

clang-tools-extra/clangd/index/FileIndex.cpp

434 ↗

(On Diff #313280)

It's not arbitrary after D93393 (we use these keys to create the list of indexed files).

But as it turns, the problem is deeper... I will try to explain.
Preamble index does not contain references, so with that change our logic about "file X is in the dynamic index, so toss references with location in X from the static index" breaks here, seems we can not think about preamble index that it is a part of the dynamic index for the background index (static). I wonder why tests do not catch this, but with this change find-all-references does not contain refs from headers for which we have preamble index, that's why I reverted this. Maybe we need to add some comment here about this behavior.
Example:
test.h

void test();

test.c

#include "test.h"
void te^st {}

• didOpen test.c
• references (where ^ is)

With this change only 1 element will be returned (definition of test in test.c and nothing from test.h)

I am not sure what we can do here, but I think we need some special merge for preamble and main file indexes (another merge class for this case instead of MergedIndex?)

[sammccall]

In D93683#2469553, @ArcsinX wrote:

In D93683#2468842, @sammccall wrote:

I'm not 100% sure this bug would block adding the equivalent change for fuzzyFind though - it'll affect documentSymbol but not code-completion IIUC (because of the workaround I mentioned above). And these symbols are pretty rare.

With the similar change for fuzzyFind(), WorkspaceSymbols.Macros test fails. But maybe I can create one more review for this and we will discuss it there.

Sorry, incomplete thought. I meant workspace/symbols would be broken for such macros, but maybe that was acceptable (it's not a terrible common feature nor symbol kind). Not great but we're trading one bug for another.
In particular, if we plan to fix both I don't think the *sequencing* matters.

clang-tools-extra/clangd/index/FileIndex.cpp

434 ↗

(On Diff #313280)

Preamble index does not contain references, so with that change our logic about "file X is in the dynamic index, so toss references with location in X from the static index" breaks

Hmm, what if we made the return value of the indexedFiles functor a bitmask instead of a single boolean (contains symbols, contains refs, ...)?

[ArcsinX]

Add comment.
Fix possible MergeIndexTest.LookupRemovedDefinition test failure.

[ArcsinX]

In D93683#2469584, @sammccall wrote:

In D93683#2469553, @ArcsinX wrote:

In D93683#2468842, @sammccall wrote:

I'm not 100% sure this bug would block adding the equivalent change for fuzzyFind though - it'll affect documentSymbol but not code-completion IIUC (because of the workaround I mentioned above). And these symbols are pretty rare.

With the similar change for fuzzyFind(), WorkspaceSymbols.Macros test fails. But maybe I can create one more review for this and we will discuss it there.

Sorry, incomplete thought. I meant workspace/symbols would be broken for such macros, but maybe that was acceptable (it's not a terrible common feature nor symbol kind). Not great but we're trading one bug for another.
In particular, if we plan to fix both I don't think the *sequencing* matters.

Ok. I will propose a separate patch for fuzzyFind() similar to this one.

Thank you for review.

clang-tools-extra/clangd/index/FileIndex.cpp

434 ↗

(On Diff #313280)

This sounds reasonable. I will try to implement a solution for this problem based on your suggestion (in a separate patch)

clang-tools-extra/clangd/index/Merge.cpp

void MergedIndex::lookup(
     const LookupRequest &Req,
     llvm::function_ref<void(const Symbol &)> Callback) const {
   trace::Span Tracer("MergedIndex lookup");
   SymbolSlab::Builder B;
 
   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)
       Callback(S);
     else
       Callback(mergeSymbol(*Sym, S));
   });
   for (const auto &ID : RemainingIDs)

clang-tools-extra/clangd/unittests/IndexTests.cpp

TEST(MergeIndexTest, Lookup) {
   EXPECT_THAT(lookup(M, {SymbolID("ns::A"), SymbolID("ns::B")}),
               UnorderedElementsAre("ns::A", "ns::B"));
   EXPECT_THAT(lookup(M, {SymbolID("ns::A"), SymbolID("ns::C")}),
               UnorderedElementsAre("ns::A", "ns::C"));
   EXPECT_THAT(lookup(M, SymbolID("ns::D")), UnorderedElementsAre());
   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()),
        J = MemIndex::build(generateSymbols({"ns::B", "ns::C"}), RefSlab(),
                            RelationSlab());
   FuzzyFindRequest Req;
   Req.Scopes = {"ns::"};
   EXPECT_THAT(match(MergedIndex(I.get(), J.get()), Req),