Unify and simplify the behavior of the hasDeclaration matcher.

[klimek]

Summary

Originally, we weren't able to match on Type nodes themselves (only QualType),
so the hasDeclaration matcher was initially written to give what we thought are
reasonable results for QualType matches.

When we chagned the matchers to allow matching on Type nodes, it turned out
that the hasDeclaration matcher was by chance written templated enough to now
allow hasDeclaration to also match on (some) Type nodes.

This patch change the hasDeclaration matcher to:
a) work the same on Type and QualType nodes,
b) be completely explicit about what nodes we can match instead of just allowing

anything with a getDecl() to match,

c) explicitly control desugaring only one level in very specific instances.

[lukasza]

Forcing shallow matching means that unit test below will stop passing after this CL.

TEST(HasDeclaration, DeepTagType) {
  std::string input =
      "class Foo {};\n"
      "using Bar = Foo;\n"
      "void Function(Bar param) {}\n";

  // Matcher for declaration of the Foo class.
  auto param_type_decl_matcher = cxxRecordDecl(hasName("Foo"));

  // hasDeclaration / qualType-flavour.
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(hasDeclaration(decl(param_type_decl_matcher)))))));
}

I am working on a tool that renames methods in a specific namespace - the tool depends on hasDeclaration doing deep-matching to tell whether an UnresolvedUsingValueDecl and/or CXXDependentScopeMemberExpr end up pointing at a record or template from the namespace of interest.

Q1: I assume that the breaking aspect of this CL is understood and accepted?

Q2: Can you please provide an example code (here or in the CL description) for how to achieve deep matching (since AFAIU hasDeclaration would no longer do deep matching after the CL under review)? Can deep matching be achieved by combining existing matchers with the now shallow hasDeclaration? Or does deep matching require authoring a custom matcher? How would the custom matcher look like (i.e. would it have to consider all possible types that can be desugared - e.g. requiring separate code for each possible type node - i.e. having separate code for hopping over a type alias and over other type nodes).

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

250

Could you please add a unit test that covers a scenario made possible by your CL and which involves an elaborated type?

TEST(HasDeclaration, ElaboratedTypeAndTemplateSpecializationType) {
  std::string input =
      "namespace Namespace {\n"
      "template<typename T>\n"
      "class Template {\n"
      " public:\n"
      "  void Method() {}\n"
      "};\n"
      "}  // namespace Namespace\n"
      "template <typename U>\n"
      "void Function(Namespace::Template<U> param) {\n"
      "  param.Method();\n"
      "};\n";

  // Matcher for ::Namespace::Template<T> template decl.
  auto param_type_decl_matcher = classTemplateDecl(
      hasName("Template"),
      hasParent(namespaceDecl(hasName("Namespace"))),
      has(templateTypeParmDecl(hasName("T"))));

  // hasDeclaration / qualType-flavour.
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(hasDeclaration(decl(param_type_decl_matcher)))))));
}

I was a little bit surprised that ElaboratedType is handled when going via qualType, but not when matching the type directly - the test code below (an extension of the unit test proposed above) fails to compile. Is this expected (for this CL? for long-term?)?

// hasDeclaration / elaboratedType-flavour.
EXPECT_TRUE(matches(input, parmVarDecl(
    hasName("param"),
    hasType(elaboratedType(hasDeclaration(decl(param_type_decl_matcher)))))));

[klimek]

Add tests, update hasDeclaration to work for ElaboratedType.

[klimek]

In D27104#607161, @lukasza wrote:

Forcing shallow matching means that unit test below will stop passing after this CL.

TEST(HasDeclaration, DeepTagType) {
  std::string input =
      "class Foo {};\n"
      "using Bar = Foo;\n"
      "void Function(Bar param) {}\n";

  // Matcher for declaration of the Foo class.
  auto param_type_decl_matcher = cxxRecordDecl(hasName("Foo"));

  // hasDeclaration / qualType-flavour.
  EXPECT_TRUE(matches(input, parmVarDecl(
      hasName("param"),
      hasType(qualType(hasDeclaration(decl(param_type_decl_matcher)))))));
}

I am working on a tool that renames methods in a specific namespace - the tool depends on hasDeclaration doing deep-matching to tell whether an UnresolvedUsingValueDecl and/or CXXDependentScopeMemberExpr end up pointing at a record or template from the namespace of interest.

Q1: I assume that the breaking aspect of this CL is understood and accepted?

Yes. That this worked was by chance.

Q2: Can you please provide an example code (here or in the CL description) for how to achieve deep matching (since AFAIU hasDeclaration would no longer do deep matching after the CL under review)? Can deep matching be achieved by combining existing matchers with the now shallow hasDeclaration? Or does deep matching require authoring a custom matcher? How would the custom matcher look like (i.e. would it have to consider all possible types that can be desugared - e.g. requiring separate code for each possible type node - i.e. having separate code for hopping over a type alias and over other type nodes).

I think we should either:
a) add a hasAnyDeclaration matcher, or
b) add a desugarsTo() matcher that just triest to run getAs<> on the node.

I'm going to get you an implementation in a different CL.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

250

Added unit test and added ElaboratedType to the list of allowed types for hasDeclaration.

[lukasza]

Do we also need to update the documentation (e.g. to say that ElaboratedType is covered by hasDeclaration)?

Other than that, I think this CL LGTM, although I would like to have a specific, working equivalent of the old, deep-maching hasDeclaration (at least when the inner, deep match is for tag types). Let's continue this discussion in https://reviews.llvm.org/D27207

[klimek]

Update to handle hasDeclaration for type alias template decls.

[klimek]

Adding Ben as reviewer.

[bkramer]

lg under the precondition that clang-tidy tests still work.

include/clang/ASTMatchers/ASTMatchersInternal.h

explicit HasDeclarationMatcher(const Matcher<Decl> &InnerMatcher)
       : HasDeclarationMatcher::WrapperMatcherInterface(InnerMatcher) {}
 
   bool matches(const T &Node, ASTMatchFinder *Finder,
                BoundNodesTreeBuilder *Builder) const override {
     return matchesSpecialized(Node, Finder, Builder);
   }
 
 private:
-  /// \brief If getDecl exists as a member of U, returns whether the inner
-  /// matcher matches Node.getDecl().
-  template <typename U>
-  bool matchesSpecialized(
-      const U &Node, ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder,
-      typename std::enable_if<has_getDecl<U>::value, int>::type = 0) const {
-    return matchesDecl(Node.getDecl(), Finder, Builder);
-  }
-
-  /// \brief Extracts the TagDecl of a QualType and returns whether the inner
-  /// matcher matches on it.
+  /// \brief Forwards to matching on the underlying type of the QualType.
   bool matchesSpecialized(const QualType &Node, ASTMatchFinder *Finder,
                           BoundNodesTreeBuilder *Builder) const {
     if (Node.isNull())
       return false;
 
-    if (auto *TD = Node->getAsTagDecl())
-      return matchesDecl(TD, Finder, Builder);
-    else if (auto *TT = Node->getAs<TypedefType>())
-      return matchesDecl(TT->getDecl(), Finder, Builder);
-    // Do not use getAs<TemplateTypeParmType> instead of the direct dyn_cast.
-    // Calling getAs will return the canonical type, but that type does not
-    // store a TemplateTypeParmDecl. We *need* the uncanonical type, if it is
-    // available, and using dyn_cast ensures that.
-    else if (auto *TTP = dyn_cast<TemplateTypeParmType>(Node.getTypePtr()))
-      return matchesDecl(TTP->getDecl(), Finder, Builder);
-    else if (auto *OCIT = Node->getAs<ObjCInterfaceType>())
-      return matchesDecl(OCIT->getDecl(), Finder, Builder);
-    else if (auto *UUT = Node->getAs<UnresolvedUsingType>())
-      return matchesDecl(UUT->getDecl(), Finder, Builder);
-    else if (auto *ICNT = Node->getAs<InjectedClassNameType>())
-      return matchesDecl(ICNT->getDecl(), Finder, Builder);
+    return matchesSpecialized(*Node, Finder, Builder);
+  }
+
+  /// \brief Finds the best declaration for a type and returns whether the inner
+  /// matcher matches on it.
+  bool matchesSpecialized(const Type &Node, ASTMatchFinder *Finder,
+                          BoundNodesTreeBuilder *Builder) const {
+    // First, for any types that have a declaration, extract the declaration and
+    // match on it.
+    if (const auto *S = dyn_cast<TagType>(&Node)) {
+      return matchesDecl(S->getDecl(), Finder, Builder);
+    }
+    if (const auto *S = dyn_cast<InjectedClassNameType>(&Node)) {
+      return matchesDecl(S->getDecl(), Finder, Builder);
+    }
+    if (const auto *S = dyn_cast<TemplateTypeParmType>(&Node)) {
+      return matchesDecl(S->getDecl(), Finder, Builder);
+    }
+    if (const auto *S = dyn_cast<TypedefType>(&Node)) {
+      return matchesDecl(S->getDecl(), Finder, Builder);
+    }
+    if (const auto *S = dyn_cast<UnresolvedUsingType>(&Node)) {
+      return matchesDecl(S->getDecl(), Finder, Builder);
+    }
+    if (const auto *S = dyn_cast<ObjCObjectType>(&Node)) {
+      return matchesDecl(S->getInterface(), Finder, Builder);
+    }
+
+    // A SubstTemplateTypeParmType exists solely to mark a type substitution
+    // on the instantiated template. As users usually want to match the
+    // template parameter on the uninitialized template, we can always desugar
+    // one level without loss of expressivness.
+    // For example, given:
+    //   template<typename T> struct X { T t; } class A {}; X<A> a;
+    // The following matcher will match, which otherwise would not:
+    //   fieldDecl(hasType(pointerType())).
+    if (const auto *S = dyn_cast<SubstTemplateTypeParmType>(&Node)) {
+      return matchesSpecialized(S->getReplacementType(), Finder, Builder);
+    }
+
+    // For template specialization types, we want to match the template
+    // declaration, as long as the type is still dependent, and otherwise the
+    // declaration of the instantiated tag type.
+    if (const auto *S = dyn_cast<TemplateSpecializationType>(&Node)) {
+      if (S->isSugared()) {
+        // If the template is non-dependent, we want to match the instantiated
+        // tag type.
+        // For example, given:
+        //   template<typename T> struct X {}; X<int> a;
+        // The following matcher will match, which otherwise would not:
+        //   templateSpecializationType(hasDeclaration(cxxRecordDecl())).
+        return matchesSpecialized(*S->desugar(), Finder, Builder);
+      }
+      // If the template is dependent, match the template declaration.
+      return matchesDecl(S->getTemplateName().getAsTemplateDecl(), Finder,
+                         Builder);
+    }
+
+    // FIXME: We desugar elaborated types. This makes the assumption that users
+    // do never want to match on whether a type is elaborated - there are
+    // arguments for both sides; for now, continue desugaring.
+    if (const auto *S = dyn_cast<ElaboratedType>(&Node)) {
+      return matchesSpecialized(S->desugar(), Finder, Builder);
+    }
     return false;
   }
 
-  /// \brief Gets the TemplateDecl from a TemplateSpecializationType
-  /// and returns whether the inner matches on it.
-  bool matchesSpecialized(const TemplateSpecializationType &Node,
-                          ASTMatchFinder *Finder,
+  /// \brief Extracts the Decl the DeclRefExpr references and returns whether
+  /// the inner matcher matches on it.
+  bool matchesSpecialized(const DeclRefExpr &Node, ASTMatchFinder *Finder,
                           BoundNodesTreeBuilder *Builder) const {
-    return matchesDecl(Node.getTemplateName().getAsTemplateDecl(),
-                       Finder, Builder);
+    return matchesDecl(Node.getDecl(), Finder, Builder);
   }
 
   /// \brief Extracts the Decl of the callee of a CallExpr and returns whether
   /// the inner matcher matches on it.
   bool matchesSpecialized(const CallExpr &Node, ASTMatchFinder *Finder,
                           BoundNodesTreeBuilder *Builder) const {
     return matchesDecl(Node.getCalleeDecl(), Finder, Builder);
   }
private:
   /// \brief Extracts the \c LabelDecl a \c AddrLabelExpr refers to and returns
   /// whether the inner matcher matches on it.
   bool matchesSpecialized(const AddrLabelExpr &Node,
                           ASTMatchFinder *Finder,
                           BoundNodesTreeBuilder *Builder) const {
     return matchesDecl(Node.getLabel(), Finder, Builder);
   }
 
+  /// \brief Extracts the declaration of a LabelStmt and returns whether the
+  /// inner matcher matches on it.
+  bool matchesSpecialized(const LabelStmt &Node, ASTMatchFinder *Finder,
+                          BoundNodesTreeBuilder *Builder) const {
+    return matchesDecl(Node.getDecl(), Finder, Builder);
+  }
+
   /// \brief Returns whether the inner matcher \c Node. Returns false if \c Node
   /// is \c NULL.
   bool matchesDecl(const Decl *Node, ASTMatchFinder *Finder,
                    BoundNodesTreeBuilder *Builder) const {
     return Node != nullptr &&
            this->InnerMatcher.matches(
                ast_type_traits::DynTypedNode::create(*Node), Finder, Builder);
   }

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

TEST(HasDeclaration, HasDeclarationOfTypeWithDecl) {
 
   // FIXME: Add tests for other types with getDecl() (e.g. RecordType)
 }
 
 TEST(HasDeclaration, HasDeclarationOfTemplateSpecializationType) {
   EXPECT_TRUE(matches("template <typename T> class A {}; A<int> a;",
                       varDecl(hasType(templateSpecializationType(
                         hasDeclaration(namedDecl(hasName("A"))))))));
+  EXPECT_TRUE(matches("template <typename T> class A {};"
+                      "template <typename T> class B { A<T> a; };",
+                      fieldDecl(hasType(templateSpecializationType(
+                        hasDeclaration(namedDecl(hasName("A"))))))));
+  EXPECT_TRUE(matches("template <typename T> class A {}; A<int> a;",
+                      varDecl(hasType(templateSpecializationType(
+                          hasDeclaration(cxxRecordDecl()))))));
 }
 
 TEST(HasDeclaration, HasDeclarationOfCXXNewExpr) {
   EXPECT_TRUE(
       matches("int *A = new int();",
               cxxNewExpr(hasDeclaration(functionDecl(parameterCountIs(1))))));
 }
 
 TEST(HasUnderlyingDecl, Matches) {
   EXPECT_TRUE(matches("namespace N { template <class T> void f(T t); }"