[include-cleaner] Filter out references that not spelled in the main file.

[hokein]

Summary

A HTML report of gtest after this patch:
https://gist.githubusercontent.com/hokein/73eee6f65a803e5702d8388c187524a6/raw/cf05a503519703a2fb87840bb0b270fe11a7a9fd/RecordTest.html

[sammccall]

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

58 ↗

(On Diff #478156)

this seems like unneccesary indirection:

• the argument is just SM.getSpellingLoc(Loc), no isMacroID check needed
• and given this, maybe just inline?

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

231

I find this test very hard to follow because it combines a lot of complicated and separate test cases, fixtures etc.
Consider a table-based test.

301

all this stuff around name-based lookup can be much simpler if you make your tests narrower, and require the ref target to have a fixed name (use llvm::to_string(Ref.Target))

int ^target = 1;
#define M target
#define USE_M M
int y = ^USE_M;

[kadircet]

This comment has been deleted.

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

38

as discussed offline we're introducing this extra constraint on references being provided, so let's mention that in the documentation as well. apart from that I agree with rest of the comments from Sam

[hokein]

address comments:

• polish and simplify the tests
• inline the spelling-loc-check logic
• add a comment for the walkUsed

[hokein]

rebase

[hokein]

this patch is ready for another review.

[kadircet]

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h

43

maybe rather say Only reports references from main file ?

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

243

i think it's better to make declarations for target always part of the header. otherwise there are changes to both target and extra step in between (e.g. CALL_FUNC) between tests (so multiple things could go wrong and cancel each other).

259

nit: i think you can still have int target() and convert PLUS_ONE to X() + 1 (same for other examples). (or you can do it the other way around, always have an static int target = 0;)

321

nit: no need for optional SourceLocation will be invalid by default.

[hokein]

address review comments

[kadircet]

thanks!

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

292

nit: this and the following test case is pretty much the same. maybe drop this one?

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h

 /// References occur at a particular location, refer to a single symbol, and
 /// that symbol may be provided by several headers.
 /// FIXME: Provide signals about the providing headers so the caller can filter
 /// and rank the results.
 using UsedSymbolCB = llvm::function_ref<void(const SymbolReference &SymRef,
                                              llvm::ArrayRef<Header> Providers)>;
 
 /// Find and report all references to symbols in a region of code.
+/// It only reports references from main file.
 ///
 /// The AST traversal is rooted at ASTRoots - typically top-level declarations
 /// of a single source file.
 /// The references to macros must be recorded separately and provided.
 ///
 /// This is the main entrypoint of the include-cleaner library, and can be used:
 ///  - to diagnose missing includes: a referenced symbol is provided by
 ///    headers which don't match any #include in the main file

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

llvm::SmallVector<Header> headersForSymbol(const Symbol &S,
                                            const PragmaIncludes *PI) {
   llvm::SmallVector<Header> Headers;
   for (auto &Loc : locateSymbol(S))
     Headers.append(findHeaders(Loc, SM, PI));
   return Headers;
 }
 } // namespace
 
 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) {
-    auto &SM = Root->getASTContext().getSourceManager();
     walkAST(*Root, [&](SourceLocation Loc, NamedDecl &ND, RefType RT) {
+      if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+        return;
       // FIXME: Most of the work done here is repetative. 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);
-    CB(MacroRef, headersForSymbol(MacroRef.Target, SM, PI));
+    if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)))
+      continue;
+    CB(MacroRef, findHeaders(MacroRef.Target.macro().Definition, SM, PI));
   }
 }
 
 static std::string spellHeader(const Header &H, HeaderSearch &HS,
                                const FileEntry *Main) {
   switch (H.kind()) {
   case Header::Physical: {
     bool IsSystem = false;

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

 } // namespace
 
 void writeHTMLReport(FileID File, const include_cleaner::Includes &Includes,
                      llvm::ArrayRef<Decl *> Roots,
                      llvm::ArrayRef<SymbolReference> MacroRefs, ASTContext &Ctx,
                      HeaderSearch &HS, PragmaIncludes *PI,
                      llvm::raw_ostream &OS) {
   Reporter R(OS, Ctx, HS, Includes, PI, File);
+  const auto& SM = Ctx.getSourceManager();
   for (Decl *Root : Roots)
     walkAST(*Root, [&](SourceLocation Loc, const NamedDecl &D, RefType T) {
+      if(!SM.isWrittenInMainFile(SM.getSpellingLoc(Loc)))
+        return;
       R.addRef(SymbolReference{Loc, D, T});
     });
-  for (const SymbolReference &Ref : MacroRefs)
+  for (const SymbolReference &Ref : MacroRefs) {
+    if (!SM.isWrittenInMainFile(SM.getSpellingLoc(Ref.RefLocation)))
+      continue;
     R.addRef(Ref);
+  }
   R.write();
 }
 
 } // namespace clang::include_cleaner

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp

 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Testing/TestAST.h"
 #include "clang/Tooling/Inclusions/StandardLibrary.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Testing/Support/Annotations.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cstddef>
 
 namespace clang::include_cleaner {
 namespace {
 using testing::Contains;
 using testing::ElementsAre;
+using testing::AllOf;
 using testing::Pair;
 using testing::UnorderedElementsAre;
 
 std::string guard(llvm::StringRef Code) {
   return "#pragma once\n" + Code.str();
 }
 
 class WalkUsedTest : public testing::Test {
TEST(FixIncludes, Basic) {
   llvm::StringRef Code = R"cpp(
 #include "a.h"
 #include "b.h"
 #include <c.h>
 )cpp";
 
   Includes Inc;
   Include I;
   I.Spelled = "a.h";
   I.Line = 2;
   Inc.add(I);
   I.Spelled = "b.h";
   I.Line = 3;
   Inc.add(I);
   I.Spelled = "c.h";
   I.Line = 4;
   I.Angled = true;
   Inc.add(I);
 
   AnalysisResults Results;
   Results.Missing.push_back("\"aa.h\"");
   Results.Missing.push_back("\"ab.h\"");
   Results.Missing.push_back("<e.h>");
   Results.Unused.push_back(Inc.atLine(3));
   Results.Unused.push_back(Inc.atLine(4));
 
   EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp(
 #include "a.h"
 #include "aa.h"
 #include "ab.h"
 #include <e.h>
 )cpp");
 }
 
+MATCHER_P3(expandedAt, FileID, Offset, SM, "") {
+  auto [ExpanedFileID, ExpandedOffset] = SM->getDecomposedExpansionLoc(arg);
+  return ExpanedFileID == FileID && ExpandedOffset == Offset;
+}
+MATCHER_P3(spelledAt, FileID, Offset, SM, "") {
+  auto [SpelledFileID, SpelledOffset] = SM->getDecomposedSpellingLoc(arg);
+  return SpelledFileID == FileID && SpelledOffset == Offset;
+}
+TEST(WalkUsed, FilterRefsNotSpelledInMainFile) {
+  // Each test is expected to have a single expected ref of `target` symbol
+  // (or have none).
+  // The location in the reported ref is a macro location. $expand points to
+  // the macro location, and $spell points to the spelled location.
+  struct {
+    llvm::StringRef Header;
+    llvm::StringRef Main;
+  } TestCases[] = {
+      // Tests for decl references.
+      {
+          /*Header=*/"int target();",
+          R"cpp(
+            #define CALL_FUNC $spell^target()
+
+            int b = $expand^CALL_FUNC;
+          )cpp",
+      },
+      {/*Header=*/R"cpp(
+           int target();
+           #define CALL_FUNC target()
+           )cpp",
+       // No ref of `target` being reported, as it is not spelled in main file.
+       "int a = CALL_FUNC;"},
+      {
+          /*Header=*/R"cpp(
+            int target();
+            #define PLUS_ONE(X) X() + 1
+          )cpp",
+          R"cpp(
+            int a = $expand^PLUS_ONE($spell^target);
+          )cpp",
+      },
+      {
+          /*Header=*/R"cpp(
+            int target();
+            #define PLUS_ONE(X) X() + 1
+          )cpp",
+          R"cpp(
+            int a = $expand^PLUS_ONE($spell^target);
+          )cpp",
+      },
+      // Tests for macro references
+      {/*Header=*/"#define target 1",
+       R"cpp(
+          #define USE_target $spell^target
+          int b = $expand^USE_target;
+        )cpp"},
+      {/*Header=*/R"cpp(
+          #define target 1
+          #define USE_target target
+        )cpp",
+       // No ref of `target` being reported, it is not spelled in main file.
+       R"cpp(
+          int a = USE_target;
+        )cpp"},
+  };
+
+  for (const auto &T : TestCases) {
+    llvm::Annotations Main(T.Main);
+    TestInputs Inputs(Main.code());
+    Inputs.ExtraFiles["header.h"] = guard(T.Header);
+    RecordedPP Recorded;
+    Inputs.MakeAction = [&]() {
+      struct RecordAction : public SyntaxOnlyAction {
+        RecordedPP &Out;
+        RecordAction(RecordedPP &Out) : Out(Out) {}
+        bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
+          auto &PP = CI.getPreprocessor();
+          PP.addPPCallbacks(Out.record(PP));
+          return true;
+        }
+      };
+      return std::make_unique<RecordAction>(Recorded);
+    };
+    Inputs.ExtraArgs.push_back("-include");
+    Inputs.ExtraArgs.push_back("header.h");
+    TestAST AST(Inputs);
+    llvm::SmallVector<Decl *> TopLevelDecls;
+    for (Decl *D : AST.context().getTranslationUnitDecl()->decls())
+      TopLevelDecls.emplace_back(D);
+    auto &SM = AST.sourceManager();
+
+    SourceLocation RefLoc;
+    walkUsed(TopLevelDecls, Recorded.MacroReferences,
+             /*PragmaIncludes=*/nullptr, SM,
+             [&](const SymbolReference &Ref, llvm::ArrayRef<Header>) {
+               if (!Ref.RefLocation.isMacroID())
+                 return;
+               if (llvm::to_string(Ref.Target) == "target") {
+                 ASSERT_TRUE(RefLoc.isInvalid())
+                     << "Expected only one 'target' ref loc per testcase";
+                 RefLoc = Ref.RefLocation;
+               }
+             });
+    FileID MainFID = SM.getMainFileID();
+    if (RefLoc.isValid()) {
+      EXPECT_THAT(RefLoc, AllOf(expandedAt(MainFID, Main.point("expand"), &SM),
+                                 spelledAt(MainFID, Main.point("spell"), &SM)))
+          << T.Main;
+    } else {
+      EXPECT_THAT(Main.points(), testing::IsEmpty());
+    }
+  }
+}
+
 } // namespace
 } // namespace clang::include_cleaner