hasDeclaration(qualType(...)) matcher should unwrap ElaboratedType and TemplateSpecializationType

[lukasza]

Summary

The new unit tests demonstrates that without unwrapping ElaboratedType
and TemplateSpecializationType we end up with a somewhat absurd
situation where a ParmVarDecl's type doesn't have *any* associated
declaration.

Fixes PR30331.

[lukasza]

I am assuming that the expectation I expressed in the unit test is a valid/correct expectation. I guess this is the first thing to validate/ack in the review :-)

include/clang/ASTMatchers/ASTMatchersInternal.h

752

I am a little bit confused and concerned whether the order of the if statements here matters. In particular - I am not sure if the ordering of dyn_cast below and getAs calls here matters (and whether things would be safer / less fragile if the dyn_cast was moved all the way to the top?).

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2130

I was a little bit torn between using |anything()| above (i.e. testing that a parameter type in this test should always have _a_ corresponding declaration) VS using |templateDecl()| instead (i.e. having a more specific verification / testing that a parameter type here has _the_ right corresponding declaration).

WDYT?

[lukasza]

Richard, could you please take a look?

include/clang/ASTMatchers/ASTMatchersInternal.h

750

There is also a question whether I should not only start supporting not only qualType(hasDeclaration(...)) when qualType wraps an ElaboratedType node, but also start supporting elaboratedType(hasDeclaration(...)).

Snippets of a patch that could achieve this (but then again - I am not sure if this is desirable and/or necessary):

+++ include/clang/ASTMatchers/ASTMatchersInternal.h	(working copy)
...
+  /// \brief Gets the QualType from ElaboratedType
+  /// and returns whether the inner matches on it.
+  bool matchesSpecialized(const ElaboratedType &Node,
+                          ASTMatchFinder *Finder,
+                          BoundNodesTreeBuilder *Builder) const {
+    return matchesSpecialized(Node.getNamedType(), Finder, Builder);
+  }
+
...
 /// \brief All types that are supported by HasDeclarationMatcher above.
-typedef TypeList<CallExpr, CXXConstructExpr, DeclRefExpr, EnumType,
-                 InjectedClassNameType, LabelStmt, AddrLabelExpr, MemberExpr,
-                 QualType, RecordType, TagType, TemplateSpecializationType,
-                 TemplateTypeParmType, TypedefType,
+typedef TypeList<AutoType, CallExpr, CXXConstructExpr, DeclRefExpr, ElaboratedType,
+                 EnumType, InjectedClassNameType, LabelStmt, AddrLabelExpr,
+                 MemberExpr, QualType, RecordType, TagType,
+                 TemplateSpecializationType, TemplateTypeParmType, TypedefType,
                  UnresolvedUsingType> HasDeclarationSupportedTypes;

753

I do notice that there are quite a few similar Node->getAs<T> if statements here. They could be replaced with something "clever" (and probably unreadable):

+++ include/clang/ASTMatchers/ASTMatchersInternal.h	(working copy)
...
+  /// \brief Terminating case for recursion over template parameters
+  /// performed by matchesQualTypeAsAnyOf template.
+  template <typename U>
+  bool matchesQualTypeAsAnyOf(const QualType &Node, ASTMatchFinder *Finder,
+                              BoundNodesTreeBuilder *Builder) const {
+    return false;
+  }
+
+  /// \brief Returns true if Node->getAs<U> is not null and matches inner
+  /// matcher for any type U listed in template parameters.
+  template <typename U, typename... UTail,
+           typename std::enable_if<!std::is_void<U>::value, int>::type = 0>
+  bool matchesQualTypeAsAnyOf(const QualType &Node, ASTMatchFinder *Finder,
+                              BoundNodesTreeBuilder *Builder) const {
+    if (const U *u = Node->getAs<U>())
+      return matchesSpecialized(*u, Finder, Builder);
+
+    return matchesQualTypeAsAnyOf<UTail...>(Node, 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 false;
+    else if (auto *TD = Node->getAsTagDecl())
+      return matchesDecl(TD, Finder, Builder);
+
+    return matchesQualTypeAsAnyOf<
+        ArrayType, InjectedClassNameType, ObjCInterfaceType, TemplateSpecializationType,
+        TypedefType, UnresolvedUsingType, void>(
+            Node, Finder, Builder);

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2124

FWIW, I couldn't repro a similar issue (i.e. VarDecl or ParmVarDecl of a type with user-provided declaration, but where hasDeclaration doesn't match *anything*) for other types (i.e. AutoType or ArrayType) - only for ElaboratedType and TemplateSpecializationType.

[rsmith]

include/clang/ASTMatchers/ASTMatchersInternal.h

752

I don't think the position of the TemplateTypeParmType case matters; a non-canonical TTPT always desugars to a canonical TTPT, so none of the other cases can match.

The relative order of the getAs cases can matter, though, and neither ordering of TypedefType and TemplateSpecializationType will actually do the right thing in general (you can have a typedef that desugars to a TST and vice versa -- the TST would need to represent an alias template specialization in the latter case). If you want to get this "right" you'll need to do single-step desugaring until you hit a type you like the look of. (See getAsSugar in lib/AST/Type.cpp for an example.)

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2119

@klimek, would you expect hasDeclaration to match the type of param here, and if so, what should it produce? There seem to be three possible answers:

1. There is a declaration for Namespace::Template, but not for the type Namespace::Template<parameter 0 of Function>, so no declaration is matched.

2. This matches and produces the declaration of the template Namespace::Template

3. This matches and produces the declaration of the class *within* the declaration of that template.

(The difference between 2 and 3 is whether you produce a CXXRecordDecl or a ClassTemplateDecl.) WDYT? The existing behavior for matchesSpecialized on a TemplateSpecializationType makes me think that (2) is expected (but I'd have personally expected (3)).

2130

I think we should be testing that we get the right declaration here, especially since (as noted above) there are actually two reasonable possibilities in this case.

[lukasza]

• Added test where both TemplateSpecializationType and TypedefType are present and both should match regardless of code order inside HasDeclarationMatcher::matchesSpecialized(const QualType &Node...). Removed significance of order inside this matchesSpecialized method.

• Tweaked test proposed in the previous patchset, so that it matches specific decls (rather than any decls)

• Added support for matching hasDeclaration(elaboratedType(...)) + test (in the test from the previous patchset).

include/clang/ASTMatchers/ASTMatchersInternal.h

750

FWIW, I've added this support in the latest patchset.

752

Thank you for the explanation and the hint to look at getAsSugar.

I've added a test where both TypeSpecializationType and TypedefType could be used. One of the asserts (the one trying to match typeAliasDecl) was initially failing as expected / as explained by your comment above (i.e. HasDeclarationMatcher::matchesSpecialized(QualType...) was first looking at TypeSpecializationType and when that succeeded it would not consider TypedefType later on). The test covers alias-desugars-to-template-specialization-type case.

Unfortunately, I was not able to come up with desugaring going in the other direction. I started with something like what I have below, but there is no typeAliasDecl here (I think) and I don't know how to insert one:

template<typename T>
class Foo {};
template <typename T>
using Bar = Foo<T>;
Bar<int> variable;

I am not sure if I really need to do single-step desugaring (and consult hasDeclaration's innerMatcher for each step). Wouldn't it be sufficient that desugaring to TemplateSpecializationType does not latch onto TST code path, but that we will also consider TypedefType desugaring later? Code is probably better at explaining at what I mean here - please look at the latest patchset. The new test passes, so I think my changes really made ordering insignificant as desired (at least insignificant when it comes to ordering between the only types (TemplateSpecializationType and TypedefType) that are 1) used in HasDeclarationMatcher / QualType overload + 2) covered via getAsSugar helper).

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2119

While waiting for klimek@'s reply, let me share my perspective.

hasDeclaration matching parmVarDecl has come up for me when working on a
tool that needs to rename all methods in a given namespace from
camelCase to PascalCase. If the tool can't see that a parmVarDecl's
declaration is from the targeted namespace, then it will fail to rename
methods invoked on that parameter.

Unfortunately, I lost track of a real repro in the wild (when trying to
run the renaming tool on Chromium), so I can only cook-up an artificial
example below. Hopefully the example is useful for illustrating what I
am talking about above:

namespace NamespaceWhereMethodsShouldBeRenamed {
template <typename T>
class Template {
 public:
  // All methods in NamespaceWhereMethodsShouldBeRenamed
  // will be renamed from camelCase to PascalCase - i.e.
  // |fooBar| below will be renamed to |FooBar|.
  void fooBar() {}
};
}

namespace SomeOtherNamespace {
class OtherClass {
 public:
  void barBaz() {}
}
}

template <typename U>
void Function(::NamespaceWhereMethodsShouldBeRenamed::Template<U>* p) {
  // |fooBar| should be rewritten to |FooBar|, but this will only
  // happen if we can see that |p|'s declaration comes from
  // NamespaceWhereMethodsShouldBeRenamed - we won't be able to see
  // that if we claim that |p| has no declaration.
  p->fooBar();

  // Obviously |barBaz| should not be rewritten below, because |x|
  // doesn't come from NamespaceWhereMethodsShouldBeRenamed
  // namespace.
  ::SomeOtherNamespace::OtherClass x;
  x.barBaz():
}

So - answer #1 is undesirable for me (because it would make my tool miss
the method renaming - i.e. it wouldn't rename |fooBar| to |FooBar| in
the example above). I don't really care whether the answer is #2
(classTemplateDecl) vs #3 (cxxRecordDecl) since both are in the same
namespace. FWIW, my naive / simple fix results in answer #2 (which I've
reflected in the unit tests in the latest patch proposal).

BTW: After reading the earlier comment about *step-by-step* desugaring,
I wonder whether the answer should be that *both* #2 and #3 should match.
I also wonder if this change should be done in a separate CL (because it
doesn't seem related to ElaboratedType and TemplateSpecializationType
cases I've added in the patches proposed here + I am a bit uncertain to
what cases exactly such step-by-step decomposing should be added).

2130

Done.

[klimek]

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2119

Given your use case: why do we need hasDeclaration here at all?
I'd have expected this working with just matching on the nested name specifier of the type instead of saying hasDeclaration on the template type.
Btw, if you add a type alias for a class not in the namespace into the namespace (typedef / using), do you wan that to rename or not? :)

I'd personally probably have expected (2), but I'm never sure in these cases without playing around with more test cases...

[lukasza]

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2119

Given your use case: why do we need hasDeclaration here at all?
I'd have expected this working with just matching on the nested name specifier of the type instead of saying hasDeclaration on the template type.

Because I want "namespace-of-user-provided-declaration" matching to work both for ElaboratedType nodes (with explicit nested name specifier) and for other kinds of nodes (where there might be no nested name specifier). I was hoping that I could do this with a single hasDeclaration matcher, rather than listing all possible type nodes myself (when building my own matcher) like I sort of do in a workaround. In particular, after this CL a single, simple hasDeclaration-based matcher can be used in

//    auto blink_qual_type_base_matcher =
//        qualType(hasDeclaration(in_blink_namespace));

inside https://codereview.chromium.org/2256913002/patch/180001/190001.

Btw, if you add a type alias for a class not in the namespace into the namespace (typedef / using), do you wan that to rename or not? :)

Good question. I want a rename to happen if I have ::SomeOtherNamespace::Typedef resolving to ::NamespaceWithRenamedMethods::Class, but I do not want rename to happen if I have ::NamespaceWithRenamtedMethods::Typedef resolving to ::SomeOtherNamespace::Class. I guess my current hasDeclaration-based matcher will match both cases :-( One way to fix this would be to exclude typedefs in |decl_under_blink_namespace| at https://chromium.googlesource.com/chromium/src/+/14d095b4df6754fa4e6959220b2b332db0b4f504/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp#646

But... this question+answer should have no impact on the CL under review, right?

I'd personally probably have expected (2), but I'm never sure in these cases without playing around with more test cases...

Ok. This (#2) is what the current patch results in.

[lukasza]

Richard - what are the next steps for this patch?

[klimek]

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

2119

You're right that regardless of what the right solution for your tool is, we should close this hole :)

Richard, can you elaborate on why you would have expected (3) to happen? I'm reluctant to put something into the matchers that you think is unexpected...

[lukasza]

Reverted changes in the patch that are not related to the issue of hasDeclaration not matching *anything* in some cases.

include/clang/ASTMatchers/ASTMatchers.h

 /// function. e.g. various subtypes of clang::Type and various expressions.
 ///
 /// Usable as: Matcher<CallExpr>, Matcher<CXXConstructExpr>,
-///   Matcher<DeclRefExpr>, Matcher<EnumType>, Matcher<InjectedClassNameType>,
+///   Matcher<DeclRefExpr>, Matcher<ElaboratedType>, Matcher<EnumType>,
+///   Matcher<InjectedClassNameType>,
 ///   Matcher<LabelStmt>, Matcher<AddrLabelExpr>, Matcher<MemberExpr>,
 ///   Matcher<QualType>, Matcher<RecordType>, Matcher<TagType>,
 ///   Matcher<TemplateSpecializationType>, Matcher<TemplateTypeParmType>,

include/clang/ASTMatchers/ASTMatchersInternal.h

       return matchesDecl(TD, Finder, Builder);
     else if (auto *TT = Node->getAs<TypedefType>())
       return matchesDecl(TT->getDecl(), Finder, Builder);
+    else if (auto *ET = Node->getAs<ElaboratedType>())
+      return matchesSpecialized(ET->getNamedType(), Finder, Builder);
+    else if (auto *TST = Node->getAs<TemplateSpecializationType>())
+      return matchesSpecialized(*TST, 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

     return false;
   }
 
+  /// \brief Gets the QualType from ElaboratedType
+  /// and returns whether the inner matches on it.
+  bool matchesSpecialized(const ElaboratedType &Node,
+                          ASTMatchFinder *Finder,
+                          BoundNodesTreeBuilder *Builder) const {
+    return matchesSpecialized(Node.getNamedType(), Finder, Builder);
+  }
+
   /// \brief Gets the TemplateDecl from a TemplateSpecializationType
   /// and returns whether the inner matches on it.
   bool matchesSpecialized(const TemplateSpecializationType &Node,

                  TypeLoc, QualType> AdaptativeDefaultToTypes;
 
 /// \brief All types that are supported by HasDeclarationMatcher above.
-typedef TypeList<CallExpr, CXXConstructExpr, DeclRefExpr, EnumType,
-                 InjectedClassNameType, LabelStmt, AddrLabelExpr, MemberExpr,
-                 QualType, RecordType, TagType, TemplateSpecializationType,
-                 TemplateTypeParmType, TypedefType,
+typedef TypeList<CallExpr, CXXConstructExpr, DeclRefExpr, ElaboratedType,
+                 EnumType, InjectedClassNameType, LabelStmt, AddrLabelExpr,
+                 MemberExpr, QualType, RecordType, TagType,
+                 TemplateSpecializationType, TemplateTypeParmType, TypedefType,
                  UnresolvedUsingType> HasDeclarationSupportedTypes;
 
 /// \brief Converts a \c Matcher<T> to a matcher of desired type \c To by

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

                                        functionDecl(hasName("bar"))))));
 }
 
+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)))))));
+
+  // hasDeclaration / elaboratedType-flavour.
+  EXPECT_TRUE(matches(input, parmVarDecl(
+      hasName("param"),
+      hasType(elaboratedType(hasDeclaration(decl(param_type_decl_matcher)))))));
+}
+
 } // namespace ast_matchers
 } // namespace clang