[include-cleaner] Remove filtering from UsingDecl visit.

[VitaNuo]

Summary

Removes filtering from the VisitUsingDecl method for implementation files.

[VitaNuo]

Add another example.

[VitaNuo]

Edit commit message, add "[include-cleaner]".

[hokein]

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

80

we should report all references as explicit.

87

The member field IsHeader is no longer used anywhere, we can remove it, which will simplify a few places, @kadircet any thoughts?

[VitaNuo]

Address review comments: turn all references into explicit, remove the "isHeader" field.

[VitaNuo]

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

87

Ok, removed. Let me know if I should undo that eventually.

[hokein]

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

127

nit: update the comment.

137

I think the c++-header is not needed, we can even remove the special c++-header logic in testWalk.

140

nit: the comment is stale, we report all refs as explicit now. I think we can just remove this case.

[kadircet]

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

80

i think having Ambiguous here for unused symbols is fine. we'd like to consider such symbols for the purposes of saying "yeah this include is probably used" but we shouldn't be inserting headers for the unused ones.

do we have an example for the contrary?

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

133

you can drop this test now, having declaration in a header vs not doesn't make any difference.

141

sorry if i am being dense but what's the difference we're trying to grasp between this and the next test case exactly?
i guess it's meant to test the difference between used and non-used template patterns?

[VitaNuo]

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

80

@hokein so what would be the final conclusion then? Should I re-introduce the "isUsed" check?

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

133

sure

137

Ok, removed the header logic from testWalk, too.

141

Yes, you're right. It probably only made sense before I removed the "isUsed" check.

[VitaNuo]

Address review comments, remove excessive tests.

[hokein]

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

80

oh, right. Ambiguous is better, this is similar to OverloadExpr, we can't prove that the symbol is used. (sorry, I somewhat had an impression this should be explicit).

[VitaNuo]

Re-introduce the isUsed check.

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

80

Ok, thank you. I have re-introduced the isUsed check now.

[hokein]

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

127

this comment is stale, and the below one as well.

[VitaNuo]

Change/remove stale comments.

[VitaNuo]

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

127

oops, sorry.

141

I have re-introduced this test case now, since there's a differentiation between used and unused again.

[hokein]

Thanks.

You can add a Fix: https://github.com/llvm/llvm-project/issues/59147 in the commit message, it will automatically close the github issue when you commit the patch.

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

144

nit: merge the following line with this line, })cpp"

152

nit: fix the alignment

153

nit: and here as well.

[VitaNuo]

Formatting fixes.

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

 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 
 namespace clang::include_cleaner {
 namespace {
 using DeclCallback =
     llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>;
 
 class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
   DeclCallback Callback;
-  // Whether we're traversing declarations coming from a header file.
-  // This helps figure out whether certain symbols can be assumed as unused
-  // (e.g. overloads brought into an implementation file, but not used).
-  bool IsHeader = false;
 
   bool handleTemplateName(SourceLocation Loc, TemplateName TN) {
     // For using-templates, only mark the alias.
     if (auto *USD = TN.getAsUsingShadowDecl()) {
       report(Loc, USD);
       return true;
     }
     report(Loc, TN.getAsTemplateDecl());
     return true;
   }
 
   void report(SourceLocation Loc, NamedDecl *ND,
               RefType RT = RefType::Explicit) {
     if (!ND || Loc.isInvalid())
       return;
     Callback(Loc, *cast<NamedDecl>(ND->getCanonicalDecl()), RT);
   }
 
 public:
-  ASTWalker(DeclCallback Callback, bool IsHeader)
-      : Callback(Callback), IsHeader(IsHeader) {}
+  ASTWalker(DeclCallback Callback) : Callback(Callback) {}
 
   bool VisitDeclRefExpr(DeclRefExpr *DRE) {
     report(DRE->getLocation(), DRE->getFoundDecl());
     return true;
   }
 
   bool VisitMemberExpr(MemberExpr *E) {
     report(E->getMemberLoc(), E->getFoundDecl().getDecl());
bool VisitOverloadExpr(OverloadExpr *E) {
     });
     return true;
   }
 
   bool VisitUsingDecl(UsingDecl *UD) {
     for (const auto *Shadow : UD->shadows()) {
       auto *TD = Shadow->getTargetDecl();
       auto IsUsed = TD->isUsed() || TD->isReferenced();
-      // We ignore unused overloads inside implementation files, as the ones in
-      // headers might still be used by the dependents of the header.
-      if (!IsUsed && !IsHeader)
-        continue;
       report(UD->getLocation(), TD,
              IsUsed ? RefType::Explicit : RefType::Ambiguous);
     }
     return true;
   }
 
   bool VisitFunctionDecl(FunctionDecl *FD) {
     // Mark declaration from definition as it needs type-checking.
     if (FD->isThisDeclarationADefinition())
       report(FD->getLocation(), FD);
if (Arg.getKind() == TemplateArgument::Template ||
                                 Arg.getAsTemplateOrTemplatePattern());
     return RecursiveASTVisitor::TraverseTemplateArgumentLoc(TL);
   }
 };
 
 } // namespace
 
 void walkAST(Decl &Root, DeclCallback Callback) {
-  auto &AST = Root.getASTContext();
-  auto &SM = AST.getSourceManager();
-  // If decl isn't written in main file, assume it's coming from an include,
-  // hence written in a header.
-  bool IsRootedAtHeader =
-      AST.getLangOpts().IsHeaderFile ||
-      !SM.isWrittenInMainFile(SM.getSpellingLoc(Root.getLocation()));
-  ASTWalker(Callback, IsRootedAtHeader).TraverseDecl(&Root);
+  ASTWalker(Callback).TraverseDecl(&Root);
 }
 
 } // namespace clang::include_cleaner

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

 #include <unordered_map>
 #include <utility>
 #include <vector>
 
 namespace clang::include_cleaner {
 namespace {
 
 // Specifies a test of which symbols are referenced by a piece of code.
-// If `// c++-header` is present, treats referencing code as a header file.
 // Target should contain points annotated with the reference kind.
 // Example:
 //   Target:      int $explicit^foo();
 //   Referencing: int x = ^foo();
 // There must be exactly one referencing location marked.
 void testWalk(llvm::StringRef TargetCode, llvm::StringRef ReferencingCode) {
   llvm::Annotations Target(TargetCode);
   llvm::Annotations Referencing(ReferencingCode);
 
   TestInputs Inputs(Referencing.code());
   Inputs.ExtraFiles["target.h"] = Target.code().str();
   Inputs.ExtraArgs.push_back("-include");
   Inputs.ExtraArgs.push_back("target.h");
   Inputs.ExtraArgs.push_back("-std=c++17");
-  if (Referencing.code().contains("// c++-header\n"))
-    Inputs.ExtraArgs.push_back("-xc++-header");
   TestAST AST(Inputs);
   const auto &SM = AST.sourceManager();
 
   // We're only going to record references from the nominated point,
   // to the target file.
   FileID ReferencingFile = SM.getMainFileID();
   SourceLocation ReferencingLoc =
       SM.getComposedLoc(ReferencingFile, Referencing.point());
auto DiagnosePoint = [&](llvm::StringRef Message, unsigned Offset) {
     Diag.emitDiagnostic(
         FullSourceLoc(SM.getComposedLoc(TargetFile, Offset), SM),
         DiagnosticsEngine::Note, Message, {}, {});
   };
   for (auto RT : {RefType::Explicit, RefType::Implicit, RefType::Ambiguous}) {
     auto RTStr = llvm::to_string(RT);
     for (auto Expected : Target.points(RTStr))
       if (!llvm::is_contained(ReferencedOffsets[RT], Expected))
-        DiagnosePoint("location not marked used with type " + RTStr,
-                      Expected);
+        DiagnosePoint("location not marked used with type " + RTStr, Expected);
     for (auto Actual : ReferencedOffsets[RT])
       if (!llvm::is_contained(Target.points(RTStr), Actual))
-        DiagnosePoint("location unexpectedly used with type " + RTStr,
-                      Actual);
+        DiagnosePoint("location unexpectedly used with type " + RTStr, Actual);
   }
 
   // If there were any differences, we print the entire referencing code once.
   if (!DiagBuf.empty())
     ADD_FAILURE() << DiagBuf << "\nfrom code:\n" << ReferencingCode;
 }
 
 TEST(WalkAST, DeclRef) {
testWalk(R"cpp(
     using ns::$explicit^x;
   )cpp",
            "int y = ^x;");
   testWalk("using $explicit^foo = int;", "^foo x;");
   testWalk("struct S {}; using $explicit^foo = S;", "^foo x;");
 }
 
 TEST(WalkAST, Using) {
-  // Make sure we ignore unused overloads.
+  // We should report unused overloads as ambiguous.
   testWalk(R"cpp(
     namespace ns {
-      void $explicit^x(); void x(int); void x(char);
+      void $explicit^x(); void $ambiguous^x(int); void $ambiguous^x(char);
     })cpp",
            "using ns::^x; void foo() { x(); }");
-  // We should report unused overloads if main file is a header.
   testWalk(R"cpp(
     namespace ns {
       void $ambiguous^x(); void $ambiguous^x(int); void $ambiguous^x(char);
     })cpp",
-           "// c++-header\n using ns::^x;");
+           "using ns::^x;");
   testWalk("namespace ns { struct S; } using ns::$explicit^S;", "^S *s;");
+
+  testWalk(R"cpp(
+    namespace ns {
+      template<class T>
+      class $ambiguous^Y {};
+    })cpp",
+           "using ns::^Y;");
+  testWalk(R"cpp(
+    namespace ns {
+      template<class T>
+      class Y {};
+    }
+    using ns::$explicit^Y;)cpp",
+           "^Y<int> x;");
 }
 
 TEST(WalkAST, Namespaces) {
   testWalk("namespace ns { void x(); }", "using namespace ^ns;");
 }
 
 TEST(WalkAST, TemplateNames) {
   testWalk("template<typename> struct $explicit^S {};", "^S<int> s;");
   // FIXME: Template decl has the wrong primary location for type-alias template