[clangd] Use expansion location for missing include diagnostics.

[VitaNuo]

Remove unrelated diff.

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

378–379

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

237

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

36–38

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

378–379

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

36–38

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

378–379

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.

380

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

36–38

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

380

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

380

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

378–379

thanks!

380

thank you!

[kadircet]

thanks!

clang-tools-extra/clangd/IncludeCleaner.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDECLEANER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDECLEANER_H
 
 #include "Headers.h"
 #include "ParsedAST.h"
+#include "Protocol.h"
 #include "clang-include-cleaner/Types.h"
-#include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringSet.h"
 #include <tuple>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 // Data needed for missing include diagnostics.
 struct MissingIncludeDiagInfo {
   include_cleaner::Symbol Symbol;
-  syntax::FileRange SymRefRange;
+  clangd::Range SymRefRange;
   std::vector<include_cleaner::Header> Providers;
 
   bool operator==(const MissingIncludeDiagInfo &Other) const {
     return std::tie(SymRefRange, Providers, Symbol) ==
            std::tie(Other.SymRefRange, Other.Providers, Other.Symbol);
   }
 };
 

clang-tools-extra/clangd/IncludeCleaner.cpp

 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Inclusions/HeaderIncludes.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
for (const auto &SymbolWithMissingInclude : MissingIncludes) {
     D.Message =
         llvm::formatv("No header providing \"{0}\" is directly included",
                       getSymbolName(SymbolWithMissingInclude.Symbol));
     D.Name = "missing-includes";
     D.Source = Diag::DiagSource::Clangd;
     D.File = AST.tuPath();
     D.InsideMainFile = true;
     D.Severity = DiagnosticsEngine::Warning;
-    D.Range = clangd::Range{
-        offsetToPosition(Code,
-                         SymbolWithMissingInclude.SymRefRange.beginOffset()),
-        offsetToPosition(Code,
-                         SymbolWithMissingInclude.SymRefRange.endOffset())};
+    D.Range = std::move(SymbolWithMissingInclude.SymRefRange);
     auto &F = D.Fixes.emplace_back();
     F.Message = "#include " + Spelling;
     TextEdit Edit = replacementToEdit(Code, *Replacement);
     F.Edits.emplace_back(std::move(Edit));
   }
   return Result;
 }
 
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 Loc = SM.getFileLoc(Ref.RefLocation);
+        auto Range = Lexer::makeFileCharRange(
+            CharSourceRange{SourceRange{Loc}, true}, SM, AST.getLangOpts());
 
-        auto Range = syntax::Token::range(SM, SpelledForExpanded->front(),
-                                          SpelledForExpanded->back());
-        MissingIncludeDiagInfo DiagInfo{Ref.Target, Range, Providers};
+        MissingIncludeDiagInfo DiagInfo{Ref.Target,  halfOpenToRange(SM, Range),
+                                        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

for (Decl *D : AST.getASTContext().getTranslationUnitDecl()->decls()) {
     std::string Name = CandidateDecl->getQualifiedNameAsString();
     if (Name != "b")
       continue;
     BDecl = CandidateDecl;
   }
   ASSERT_TRUE(BDecl);
   include_cleaner::Symbol B{*BDecl};
   auto Range = MainFile.range("b");
-  size_t Start = llvm::cantFail(positionToOffset(MainFile.code(), Range.start));
-  size_t End = llvm::cantFail(positionToOffset(MainFile.code(), Range.end));
-  syntax::FileRange BRange{SM.getMainFileID(), static_cast<unsigned int>(Start),
-                           static_cast<unsigned int>(End)};
   include_cleaner::Header Header{*SM.getFileManager().getFile("b.h")};
-  MissingIncludeDiagInfo BInfo{B, BRange, {Header}};
+  MissingIncludeDiagInfo BInfo{B, Range, {Header}};
   EXPECT_THAT(Findings.MissingIncludes, ElementsAre(BInfo));
 }
 
 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)))
+      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)) &&
+          SM.getDecomposedLoc(SM.getSpellingLoc(Loc)).first !=
+              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)))