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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D93683#2469553 <https://reviews.llvm.org/D93683#2469553>, @ArcsinX wrote:

> In D93683#2468842 <https://reviews.llvm.org/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.



================
Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:434
+      PreambleSymbols.update(
+          *FilePath, std::make_unique<SymbolSlab>(std::move(*IF->Symbols)),
+          std::make_unique<RefSlab>(),
----------------
ArcsinX wrote:
> sammccall wrote:
> > ArcsinX wrote:
> > > 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.
> > Yeah... nevertheless the key is totally arbitrary here, so we might as well use whatever is most convenient/cheapest.
> 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`?)
> 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, ...)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93683/new/

https://reviews.llvm.org/D93683