[include-cleaner] Mark RecordDecls referenced in UsingDecls as explicit

[kadircet]

We were reporting ambigious references from using declarations as user
can be depending on different overloads of a function just because they
are visible in the TU.
This doesn't apply to records, or primary templates as declaration being
referenced in such cases is unambigious, the ambiguity applies to
specializations though.

Hence this patch returns an explicit reference to record decls and
primary templates of those.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: kadir çetinkaya (kadircet)

Changes

We were reporting ambigious references from using declarations as user
can be depending on different overloads of a function just because they
are visible in the TU.
This doesn't apply to records, or primary templates as declaration being
referenced in such cases is unambigious, the ambiguity applies to
specializations though.

Hence this patch returns an explicit reference to record decls and
primary templates of those.

---

Full diff: https://github.com/llvm/llvm-project/pull/106430.diff

2 Files Affected:

• (modified) clang-tools-extra/include-cleaner/lib/WalkAST.cpp (+1-1)
• (modified) clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp (+8-3)

diff --git a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
index 598484d09712e5..5e6b862caf0d6c 100644
--- a/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
+++ b/clang-tools-extra/include-cleaner/lib/WalkAST.cpp
@@ -203,7 +203,7 @@ class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
   bool VisitUsingDecl(UsingDecl *UD) {
     for (const auto *Shadow : UD->shadows()) {
       auto *TD = Shadow->getTargetDecl();
-      auto IsUsed = TD->isUsed() || TD->isReferenced();
+      auto IsUsed = TD->isUsed() || TD->isReferenced() || !TD->getAsFunction();
       report(UD->getLocation(), TD,
              IsUsed ? RefType::Explicit : RefType::Ambiguous);
 
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 6c8eacbff1cea3..9286758cab081c 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -255,7 +255,7 @@ TEST(WalkAST, TemplateSpecializationsFromUsingDecl) {
   // Class templates
   testWalk(R"cpp(
 namespace ns {
-template<class T> class $ambiguous^Z {};      // primary template
+template<class T> class $explicit^Z {};      // primary template
 template<class T> class $ambiguous^Z<T*> {};  // partial specialization
 template<> class $ambiguous^Z<int> {};        // full specialization
 }
@@ -265,7 +265,7 @@ template<> class $ambiguous^Z<int> {};        // full specialization
   // Var templates
   testWalk(R"cpp(
 namespace ns {
-template<class T> T $ambiguous^foo;      // primary template
+template<class T> T $explicit^foo;      // primary template
 template<class T> T $ambiguous^foo<T*>;  // partial specialization
 template<> int* $ambiguous^foo<int>;     // full specialization
 }
@@ -335,7 +335,12 @@ TEST(WalkAST, Using) {
   testWalk(R"cpp(
     namespace ns {
       template<class T>
-      class $ambiguous^Y {};
+      class $explicit^Y {};
+    })cpp",
+           "using ns::^Y;");
+  testWalk(R"cpp(
+    namespace ns {
+      class $explicit^Y {};
     })cpp",
            "using ns::^Y;");
   testWalk(R"cpp(

[hokein]

nit: the name IsUsed no longer reflects its purpose. Consider renaming it to something more suitable or inlining it.

Additionally, it would be helpful to add comments to document the intention behind this code.

[kadircet]

Well I was actually thinking that usedness is still the right concept here, spelling of the type name in the using declaration is enough to trigger that use for records.

I added comments also along those lines, LMK if it still doesn't resonate with you.

[hokein]

The connection between usedness and !TD->getAsFunction() wasn’t immediately obvious to me. With the newly-added comment, it's much clearer now, everything looks good, thanks.

[hokein]

The connection between usedness and !TD->getAsFunction() wasn’t immediately obvious to me. With the newly-added comment, it's much clearer now, everything looks good, thanks.