[clangd] Use expansion location for missing include diagnostics.

Differential Revision: https://reviews.llvm.org/D146727
llvm · Mar 27, 2023 · 481f888 · 481f888
1 parent cd7ab4b
commit 481f888
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 24 deletions.
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -78,7 +78,6 @@ clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned HashOffset) {
   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);
@@ -390,15 +389,17 @@ IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
             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 =

diff --git a/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp b/clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -178,27 +178,39 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
   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]](); 
+  void foo() {
+    $b[[b]]();
 
-      // this should not be diagnosed, because it's ignored in the config
-      buzz(); 
+    ns::$bar[[Bar]] bar;
+    bar.d();
+    $f[[f]](); 
 
-      $foobar[[foobar]]();
+    // this should not be diagnosed, because it's ignored in the config
+    buzz(); 
 
-      std::$vector[[vector]] v;
-    })cpp");
+    $foobar[[foobar]]();
+
+    std::$vector[[vector]] v;
+
+    int var = $FOO[[FOO]];
+
+    $DEF[[DEF]](a);
+
+    $BAR[[BAR]](b);
+
+    BAZ($Foo[[Foo]]);
+})cpp");
 
   TestTU TU;
   TU.Filename = "foo.cpp";
@@ -225,6 +237,13 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
   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();
 
@@ -254,7 +273,23 @@ TEST(IncludeCleaner, GenerateMissingHeaderDiags) {
               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) {

diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp
@@ -36,9 +36,10 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots,
   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));