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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

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



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


================
Comment at: clang-tools-extra/clangd/index/Merge.cpp:81
   Static->lookup(Req, [&](const Symbol &S) {
+    if (DynamicContainsFile(S.Definition ? S.Definition.FileURI
+                                         : S.CanonicalDeclaration.FileURI))
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93683