[clangd] Use expansion location for missing include diagnostics.

[VitaNuo]

Remove unrelated diff.

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

391–392

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.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

242

can you also put a #define BAR(x) Foo *x here and a usage like BAR(b); inside the main file and check that we only get include for symbol for BAR inside the main file at that location?

clang-tools-extra/include-cleaner/lib/Analysis.cpp

39–40

you can directly use expansion location instead of checking both and mentioning preamble file id here, eg: !SM.isWrittenInMainFile(SM.getExpansionLoc(Loc))

[VitaNuo]

Address review comments. Use spelling locations for macro arguments, and expansion locations for everything else.

[VitaNuo]

Thanks for the comments!

clang-tools-extra/clangd/IncludeCleaner.cpp

391–392

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?

clang-tools-extra/include-cleaner/lib/Analysis.cpp

39–40

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.

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

391–392

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.

392

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?

clang-tools-extra/include-cleaner/lib/Analysis.cpp

39–40

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;

[VitaNuo]

clang-tools-extra/clangd/IncludeCleaner.cpp

392

It's for consistency with clangd diagnostic generation here (according to Sam), but both should work.
Would you rather prefer Tokens.spelledTokenAt(Loc)?

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

392

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.

[VitaNuo]

Address review comments.

[VitaNuo]

Format.

[VitaNuo]

thanks for the comments!

clang-tools-extra/clangd/IncludeCleaner.cpp

391–392

thanks!

392

thank you!

[kadircet]

thanks!

clang-tools-extra/clangd/IncludeCleaner.cpp

clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned HashOffset) {
   // Span the warning until the EOL or EOF.
   Result.end.character +=
       lspLength(Code.drop_front(HashOffset).take_until([](char C) {
         return C == '\n' || C == '\r';
       }));
   return Result;
 }
 
-
 bool isFilteredByConfig(const Config &Cfg, llvm::StringRef HeaderPath) {
   // Convert the path to Unix slashes and try to match against the filter.
   llvm::SmallString<64> NormalizedPath(HeaderPath);
   llvm::sys::path::native(NormalizedPath, llvm::sys::path::Style::posix);
   for (auto &Filter : Cfg.Diagnostics.Includes.IgnoreHeader) {
     if (Filter(NormalizedPath))
       return true;
   }
include_cleaner::walkUsed(
                    "ConvertedIncludes only contains resolved includes.");
             Used.insert(*HeaderID);
           }
         }
 
         if (Satisfied || Providers.empty() ||
             Ref.RT != include_cleaner::RefType::Explicit)
           return;
 
-        auto &Tokens = AST.getTokens();
-        auto SpelledForExpanded =
-            Tokens.spelledForExpanded(Tokens.expandedTokens(Ref.RefLocation));
-        if (!SpelledForExpanded)
-          return;
-
-        auto Range = syntax::Token::range(SM, SpelledForExpanded->front(),
-                                          SpelledForExpanded->back());
-        MissingIncludeDiagInfo DiagInfo{Ref.Target, Range, Providers};
+        // 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.
+        auto Loc = SM.getFileLoc(Ref.RefLocation);
+        const auto *Token = AST.getTokens().spelledTokenAt(Loc);
+        MissingIncludeDiagInfo DiagInfo{Ref.Target, Token->range(SM),
+                                        Providers};
         MissingIncludes.push_back(std::move(DiagInfo));
       });
   std::vector<const Inclusion *> UnusedIncludes =
       getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
   return {std::move(UnusedIncludes), std::move(MissingIncludes)};
 }
 
 std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

 TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
   Config Cfg;
   Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict;
   Cfg.Diagnostics.Includes.IgnoreHeader = {
       [](llvm::StringRef Header) { return Header.ends_with("buzz.h"); }};
   WithContextValue Ctx(Config::Key, std::move(Cfg));
   Annotations MainFile(R"cpp(
 #include "a.h"
+#include "all.h"
 $insert_b[[]]#include "baz.h"
 #include "dir/c.h"
-$insert_d[[]]#include "fuzz.h"
+$insert_d[[]]$insert_foo[[]]#include "fuzz.h"
 #include "header.h"
 $insert_foobar[[]]#include <e.h>
 $insert_f[[]]$insert_vector[[]]
 
-    void foo() {
-      $b[[b]]();
+#define DEF(X) const Foo *X;
+#define BAZ(X) const X x
+
 
-      ns::$bar[[Bar]] bar;
-      bar.d();
-      $f[[f]](); 
 
-      // this should not be diagnosed, because it's ignored in the config
-      buzz(); 
 
-      $foobar[[foobar]]();
 
-      std::$vector[[vector]] v;
-    })cpp");
+
+    int var = $FOO[[FOO]];
+
+    $DEF[[DEF]](a);
+
+    $BAR[[BAR]](b);
+
+    BAZ($Foo[[Foo]]);
 
   TestTU TU;
   TU.Filename = "foo.cpp";
   TU.AdditionalFiles["a.h"] = guard("#include \"b.h\"");
   TU.AdditionalFiles["b.h"] = guard("void b();");
 
   TU.AdditionalFiles["dir/c.h"] = guard("#include \"d.h\"");
   TU.AdditionalFiles["dir/d.h"] =
#define BAZ(X) const X x
   TU.AdditionalFiles["private.h"] = guard(R"cpp(
     // IWYU pragma: private, include "public.h"
     void foobar();
   )cpp");
   TU.AdditionalFiles["header.h"] = guard(R"cpp(
   namespace std { class vector {}; }
   )cpp");
 
+  TU.AdditionalFiles["all.h"] = guard("#include \"foo.h\"");
+  TU.AdditionalFiles["foo.h"] = guard(R"cpp(
+    #define BAR(x) Foo *x
+    #define FOO 1
+    struct Foo{}; 
+  )cpp");
+
   TU.Code = MainFile.code();
   ParsedAST AST = TU.build();
 
   std::vector<clangd::Diag> Diags =
       issueIncludeCleanerDiagnostics(AST, TU.Code);
   EXPECT_THAT(
       Diags,
       UnorderedElementsAre(
EXPECT_THAT(
               Diag(MainFile.range("foobar"),
                    "No header providing \"foobar\" is directly included"),
               withFix(Fix(MainFile.range("insert_foobar"),
                           "#include \"public.h\"\n", "#include \"public.h\""))),
           AllOf(
               Diag(MainFile.range("vector"),
                    "No header providing \"std::vector\" is directly included"),
               withFix(Fix(MainFile.range("insert_vector"),
-                          "#include <vector>\n", "#include <vector>")))));
+                          "#include <vector>\n", "#include <vector>"))),
+          AllOf(Diag(MainFile.range("FOO"),
+                     "No header providing \"FOO\" is directly included"),
+                withFix(Fix(MainFile.range("insert_foo"),
+                            "#include \"foo.h\"\n", "#include \"foo.h\""))),
+          AllOf(Diag(MainFile.range("DEF"),
+                     "No header providing \"Foo\" is directly included"),
+                withFix(Fix(MainFile.range("insert_foo"),
+                            "#include \"foo.h\"\n", "#include \"foo.h\""))),
+          AllOf(Diag(MainFile.range("BAR"),
+                     "No header providing \"BAR\" is directly included"),
+                withFix(Fix(MainFile.range("insert_foo"),
+                            "#include \"foo.h\"\n", "#include \"foo.h\""))),
+          AllOf(Diag(MainFile.range("Foo"),
+                     "No header providing \"Foo\" is directly included"),
+                withFix(Fix(MainFile.range("insert_foo"),
+                            "#include \"foo.h\"\n", "#include \"foo.h\"")))));
 }
 
 TEST(IncludeCleaner, IWYUPragmas) {
   TestTU TU;
   TU.Code = R"cpp(
     #include "behind_keep.h" // IWYU pragma: keep
     #include "exported.h" // IWYU pragma: export
     #include "public.h"

clang-tools-extra/include-cleaner/lib/Analysis.cpp

 void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
               llvm::ArrayRef<SymbolReference> MacroRefs,
               const PragmaIncludes *PI, const SourceManager &SM,
               UsedSymbolCB CB) {
   // This is duplicated in writeHTMLReport, changes should be mirrored there.
   tooling::stdlib::Recognizer Recognizer;
   for (auto *Root : ASTRoots) {
     walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
-      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+      auto FID = SM.getFileID(SM.getSpellingLoc(Loc));
+      if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID())
         return;
-      // FIXME: Most of the work done here is repetative. It might be useful to
+      // FIXME: Most of the work done here is repetitive. It might be useful to
       // have a cache/batching.
       SymbolReference SymRef{Loc, ND, RT};
       return CB(SymRef, headersForSymbol(ND, SM, PI));
     });
   }
   for (const SymbolReference &MacroRef : MacroRefs) {
     assert(MacroRef.Target.kind() == Symbol::Macro);
     if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)))