This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ArcsinX on Dec 22 2020, 3:22 AM.

Details

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.

Diff Detail

Event Timeline

ArcsinX created this revision.Dec 22 2020, 3:22 AM
ArcsinX requested review of this revision.Dec 22 2020, 3:22 AM

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 updated this revision to Diff 313320.Dec 22 2020, 6:31 AM
  • Simplify test.
  • Revert PreambleSymbols.update() first argument change.
sammccall accepted this revision.Dec 22 2020, 1:09 PM

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)

This revision is now accepted and ready to land.Dec 22 2020, 1:09 PM

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?)

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 updated this revision to Diff 313504.Dec 23 2020, 1:25 AM

Add comment.
Fix possible MergeIndexTest.LookupRemovedDefinition test failure.

ArcsinX marked an inline comment as done.Dec 23 2020, 1:31 AM

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)