[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

86

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

130

nit: update the comment.

140–145

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

148

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

86

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

136

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

149

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

86

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

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

136

sure

140–145

Ok, removed the header logic from testWalk, too.

149

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

86

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

86

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

[hokein]

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

130

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

[VitaNuo]

Change/remove stale comments.

[VitaNuo]

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

130

oops, sorry.

149

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

152

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

160

nit: fix the alignment

161

nit: and here as well.

[VitaNuo]

Formatting fixes.

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

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);

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

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;");
+  testWalk(R"cpp(
+    namespace ns {
+      void $ambiguous^x(); void $ambiguous^x(int); void $ambiguous^x(char);
+    })cpp",
+           "using ns::^x;");
   testWalk("namespace ns { struct S; } using ns::$explicit^S;", "^S *s;");
+
+  // We should report references to templates as ambiguous.
+  testWalk(R"cpp(
+    namespace ns {
+      template<class T>
+      class $ambiguous^Y {};
+    }
+    )cpp",
+           "using ns::^Y; Y<int> x;");
+  testWalk(R"cpp(
+    namespace ns {
+      template<class T>
+      class $ambiguous^Y {};
+    }
+    )cpp",
+           "using ns::^Y;");
 }
 
 TEST(WalkAST, Namespaces) {
   testWalk("namespace ns { void x(); }", "using namespace ^ns;");
 }
 
 TEST(WalkAST, TemplateNames) {
   testWalk("template<typename> struct $explicit^S {};", "^S<int> s;");