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

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX marked an inline comment as done.
ArcsinX added a comment.

In D93683#2469584 <https://reviews.llvm.org/D93683#2469584>, @sammccall wrote:

> 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.

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

Thank you for review.



================
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>(),
----------------
sammccall wrote:
> 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, ...)?
This sounds reasonable. I will try to implement a solution for this problem based on your suggestion (in a separate patch)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93683