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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline 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());
----------------
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?


================
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);
----------------
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.
```


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:36
     walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
-      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)) &&
+          SM.getDecomposedLoc(SM.getSpellingLoc(Loc)).first !=
----------------
VitaNuo wrote:
> kadircet wrote:
> > you can directly use expansion location instead of checking both and mentioning preamble file id here, eg: `!SM.isWrittenInMainFile(SM.getExpansionLoc(Loc))`
> This actually leads to both `BAR` and `Foo` in `#define BAR(x) Foo *x` in `foo.h` being recognized as missing and attaches 2 diagnostics to `[[BAR]](b)` in the main file. AFAIU, this is exactly what you're asking me to prevent in the previous comment.
> 
> 
no you're right, i think i was just confused here.

still might be nicer to clean it up a little:
```
auto FID = SM.getFileID(SM.getSpellingLoc(Loc));
if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID())
  return;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146727