[clang-tidy] Fix a false positive in unused-using-decl check.

[hokein]

Summary

The previous matcher "hasAnyTemplateArgument(templateArgument())" only
matches the first template argument, but the check wants to iterate all
template arguments. This patch fixes this.

Also some refactorings in this patch (to make the code reusable).

[gribozavr]

clang-tools-extra/test/clang-tidy/misc-unused-using-decls.cpp

212

It is very unclear what this test does, if I didn't know about this bug. Could you add a comment?

[hokein]

Use a matcher to catch all template arguments

[hokein]

minor fix

[gribozavr]

clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp

104–105

Please move the comment below the lambda. The lambda is not doing what this comment says.

137

Could you add a test for this case?

[hokein]

Update the comment.

clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp

104–105

I think the comment is pretty old (it was added in the first patch, but the check has been evolved a lot). I updated the comment here.

137

This is already covered by the existing test case a.func2<int, ff>(t3); -- in this patch, we change the matcher of callExpr on Line 63, and the callback of it now runs into this codepath (previously it falls into the if (const auto *Used = Result.Nodes.getNodeAs<NamedDecl>("used") branch above).

[hokein]

committed in rL370760

clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
+namespace {
+// FIXME: Move ASTMatcher library.
+AST_POLYMORPHIC_MATCHER_P(
+    forEachTemplateArgument,
+    AST_POLYMORPHIC_SUPPORTED_TYPES(ClassTemplateSpecializationDecl,
+                                    TemplateSpecializationType, FunctionDecl),
+    clang::ast_matchers::internal::Matcher<TemplateArgument>, InnerMatcher) {
+  ArrayRef<TemplateArgument> TemplateArgs =
+      clang::ast_matchers::internal::getTemplateSpecializationArgs(Node);
+  clang::ast_matchers::internal::BoundNodesTreeBuilder Result;
+  bool Matched = false;
+  for (const auto &Arg : TemplateArgs) {
+    clang::ast_matchers::internal::BoundNodesTreeBuilder ArgBuilder(*Builder);
+    if (InnerMatcher.matches(Arg, Finder, &ArgBuilder)) {
+      Matched = true;
+      Result.addMatch(ArgBuilder);
+    }
+  }
+  *Builder = std::move(Result);
+  return Matched;
+}
+} // namespace
+
 // A function that helps to tell whether a TargetDecl in a UsingDecl will be
 // checked. Only variable, function, function template, class template, class,
 // enum declaration and enum constant declaration are considered.
 static bool ShouldCheckDecl(const Decl *TargetDecl) {
   return isa<RecordDecl>(TargetDecl) || isa<ClassTemplateDecl>(TargetDecl) ||
          isa<FunctionDecl>(TargetDecl) || isa<VarDecl>(TargetDecl) ||
          isa<FunctionTemplateDecl>(TargetDecl) || isa<EnumDecl>(TargetDecl) ||
          isa<EnumConstantDecl>(TargetDecl);
 }
 
 void UnusedUsingDeclsCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(usingDecl(isExpansionInMainFile()).bind("using"), this);
   auto DeclMatcher = hasDeclaration(namedDecl().bind("used"));
   Finder->addMatcher(loc(enumType(DeclMatcher)), this);
   Finder->addMatcher(loc(recordType(DeclMatcher)), this);
   Finder->addMatcher(loc(templateSpecializationType(DeclMatcher)), this);
   Finder->addMatcher(declRefExpr().bind("used"), this);
   Finder->addMatcher(callExpr(callee(unresolvedLookupExpr().bind("used"))),
                      this);
   Finder->addMatcher(
-      callExpr(hasDeclaration(functionDecl(hasAnyTemplateArgument(
-          anyOf(refersToTemplate(templateName().bind("used")),
-                refersToDeclaration(functionDecl().bind("used"))))))),
+      callExpr(hasDeclaration(functionDecl(
+          forEachTemplateArgument(templateArgument().bind("used"))))),
       this);
-  Finder->addMatcher(loc(templateSpecializationType(hasAnyTemplateArgument(
+  Finder->addMatcher(loc(templateSpecializationType(forEachTemplateArgument(
                          templateArgument().bind("used")))),
                      this);
 }
 
 void UnusedUsingDeclsCheck::check(const MatchFinder::MatchResult &Result) {
   if (Result.Context->getDiagnostics().hasUncompilableErrorOccurred())
     return;
 
if (const auto *Using = Result.Nodes.getNodeAs<UsingDecl>("using")) {
     for (const auto *UsingShadow : Using->shadows()) {
       const auto *TargetDecl = UsingShadow->getTargetDecl()->getCanonicalDecl();
       if (ShouldCheckDecl(TargetDecl))
         Context.UsingTargetDecls.insert(TargetDecl);
     }
     if (!Context.UsingTargetDecls.empty())
       Contexts.push_back(Context);
     return;
   }
-  // Mark using declarations as used by setting FoundDecls' value to zero. As
-  // the AST is walked in order, usages are only marked after a the
-  // corresponding using declaration has been found.
-  // FIXME: This currently doesn't look at whether the type reference is
-  // actually found with the help of the using declaration.
-  if (const auto *Used = Result.Nodes.getNodeAs<NamedDecl>("used")) {
+
+  // Mark a corresponding using declaration as used.
+  auto RemoveNamedDecl = [&](const NamedDecl *Used) {
+    removeFromFoundDecls(Used);
+    // Also remove variants of Used.
     if (const auto *FD = dyn_cast<FunctionDecl>(Used)) {
       removeFromFoundDecls(FD->getPrimaryTemplate());
     } else if (const auto *Specialization =
                    dyn_cast<ClassTemplateSpecializationDecl>(Used)) {
-      Used = Specialization->getSpecializedTemplate();
+      removeFromFoundDecls(Specialization->getSpecializedTemplate());
+    } else if (const auto *FD = dyn_cast<FunctionDecl>(Used)) {
+      if (const auto *FDT = FD->getPrimaryTemplate())
+        removeFromFoundDecls(FDT);
+    } else if (const auto *ECD = dyn_cast<EnumConstantDecl>(Used)) {
+      if (const auto *ET = ECD->getType()->getAs<EnumType>())
+        removeFromFoundDecls(ET->getDecl());
     }
-    removeFromFoundDecls(Used);
+  };
+  // We rely on the fact that the clang AST is walked in order, usages are only
+  // marked after a corresponding using decl has been found.
+  if (const auto *Used = Result.Nodes.getNodeAs<NamedDecl>("used")) {
+    RemoveNamedDecl(Used);
     return;
   }
 
   if (const auto *Used = Result.Nodes.getNodeAs<TemplateArgument>("used")) {
-    // FIXME: Support non-type template parameters.
     if (Used->getKind() == TemplateArgument::Template) {
       if (const auto *TD = Used->getAsTemplate().getAsTemplateDecl())
         removeFromFoundDecls(TD);
     } else if (Used->getKind() == TemplateArgument::Type) {
       if (auto *RD = Used->getAsType()->getAsCXXRecordDecl())
         removeFromFoundDecls(RD);
+    } else if (Used->getKind() == TemplateArgument::Declaration) {
+      RemoveNamedDecl(Used->getAsDecl());
     }
     return;
   }
 
-  if (const auto *Used = Result.Nodes.getNodeAs<TemplateName>("used")) {
-    removeFromFoundDecls(Used->getAsTemplateDecl());
-    return;
-  }
-
   if (const auto *DRE = Result.Nodes.getNodeAs<DeclRefExpr>("used")) {
-    if (const auto *FD = dyn_cast<FunctionDecl>(DRE->getDecl())) {
-      if (const auto *FDT = FD->getPrimaryTemplate())
-        removeFromFoundDecls(FDT);
-      else
-        removeFromFoundDecls(FD);
-    } else if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
-      removeFromFoundDecls(VD);
-    } else if (const auto *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl())) {
-      removeFromFoundDecls(ECD);
-      if (const auto *ET = ECD->getType()->getAs<EnumType>())
-        removeFromFoundDecls(ET->getDecl());
-    }
+    RemoveNamedDecl(DRE->getDecl());
+    return;
   }
   // Check the uninstantiated template function usage.
   if (const auto *ULE = Result.Nodes.getNodeAs<UnresolvedLookupExpr>("used")) {
     for (const NamedDecl *ND : ULE->decls()) {
       if (const auto *USD = dyn_cast<UsingShadowDecl>(ND))
         removeFromFoundDecls(USD->getTargetDecl()->getCanonicalDecl());
     }
   }

clang-tools-extra/test/clang-tidy/misc-unused-using-decls.cpp

 class L {};
 
 template <typename T> class M {};
 class N {};
 
 template <int T> class P {};
 const int Constant = 0;
 
+template <typename T> class Q {};
+
 class Base {
  public:
   void f();
 };
 
 D UsedInstance;
 D UnusedInstance;
 

 using n::K;
 
 using n::N;
 
 // FIXME: Currently non-type template arguments are not supported.
 using n::Constant;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: using decl 'Constant' is unused
 
+using n::Q;
+
 // ----- Usages -----
 void f(B b);
 void g() {
   vector<C> data;
   D::i = 1;
   F<int> f;
   void (*func)() = &G::func;
   int *i = &H::i;

 void h(n::M<T>* t) {}
 // n::N is used the explicit template instantiation.
 template void h(n::M<N>* t);
 
 // Test on Non-type template arguments.
 template <int T>
 void i(n::P<T>* t) {}
 template void i(n::P<Constant>* t);
+
+template <typename T, template <typename> class U> class Bar {};
+// We used to report Q unsued, because we only checked the first template
+// argument.
+Bar<int, Q> *bar;