[PATCH] D146727: [clangd] Use expansion location for missing include diagnostics.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added a comment.

thanks for the comments!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:380
+        auto Loc = SM.getFileLoc(Ref.RefLocation);
+        auto Range = Lexer::makeFileCharRange(
+            CharSourceRange{SourceRange{Loc}, true}, SM, AST.getLangOpts());
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > I don't think you need to re-lex the token here, as `Loc` is already a non-macro location in main file, you can still use `Tokens.spelledTokenAt(Loc)`. Am i missing something?
> > It's for consistency with clangd diagnostic generation [here](http://google3/third_party/llvm/llvm-project/clang-tools-extra/clangd/Diagnostics.cpp;l=114;rcl=512058308) (according to Sam), but both should work. 
> > Would you rather prefer `Tokens.spelledTokenAt(Loc)`?
> I'd rather go with `spelledTokenAt` here, diagnostics code deals with locations outside of the main file, hence it makes sense for that logic to re-run the lexer. but i feel like re-running lexer here will actually create more confusion, as we tend to re-use results in token buffers rather than using raw lexer in rest of the codebase, as it confused me above.
thank you!


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:382
 
-        auto &Tokens = AST.getTokens();
-        auto SpelledForExpanded =
-            Tokens.spelledForExpanded(Tokens.expandedTokens(Ref.RefLocation));
-        if (!SpelledForExpanded)
+        auto ExpansionLoc = SM.getExpansionLoc(Ref.RefLocation);
+        const auto *Token = AST.getTokens().spelledTokenAt(ExpansionLoc);
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > can you actually get the spelling location first and use expansion location only when it isn't inside the main file?
> > > that way we get to preserve the behaviour for macro arguments, which warrants a test like:
> > > ```
> > > #define FOO(X) const X x
> > > 
> > > FOO([[Foo]]); // we should have a missing include for symbol Foo here
> > > ```
> > > 
> > > and can we have a comment about reasons and implications, e.g. something like:
> > > ```
> > > Spelling locations can point into preamble section at times. We don't want to emit diagnostics there as the offsets might be off when preamble is stale.
> > > In such cases we fallback to expansion locations, as they're always in the main file. This means we fail to attach diagnostics to spelling of symbol names inside macro bodies.
> > > // FIXME: Use presumed locations to make sure we can correctly generate diagnostics even in such cases.
> > > ```
> > Sam suggested a magic solution for this, which is `SM.getFileLoc(Ref.RefLocation)`. Its documentation says:
> > 
> > ```
> > /// Given \p Loc, if it is a macro location return the expansion
> > /// location or the spelling location, depending on if it comes from a
> > /// macro argument or not.
> > ```
> > 
> > Sounds like what we need, right?
> Rather than doing spelling & falling back to expansion, just doing `getFileLoc` definitely makes sense.
> But what we actually want here is always map to spelling (e.g. we want `Foo` in macro body, rather than macro invocation to be highlighted).
> It's just we can't have it all the time because accessing offsets coming from stale preamble might result in crashes. Hence can you also add the comment i mentioned above so that we don't forget? maybe with a rewording like:
> ```
> // We actually always want to map usages to their spellings, but spelling locations can point into preamble section.
> // Using these offsets could lead into crashes in presence of stale preambles. Hence we use `getFileLoc` instead to make sure it always points into main file.
> // FIXME: Use presumed locations to map such usages back to patched locations safely.
> ```
thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146727