Traversing template paramter lists of DeclaratorDecls and/or TagDecls.

[lukasza]

Summary

The unit tests in this patch demonstrate the need to traverse template
parameter lists of DeclaratorDecls (e.g. VarDecls, CXXMethodDecls) and
TagDecls (e.g. EnumDecls, RecordDecls).

Fixes PR29042.

[lukasza]

include/clang/AST/RecursiveASTVisitor.h

1728

I think that tweaks of EnumDecl traversal together with tweaks of TraverseRecordHelper tweaks, take care of all the relevant node types derived from TagDecl:

• EnumDecl - traversal now calls TraverseDeclTemplateParameterLists (+EnumDecl is covered by the new unit tests)
• RecordDecl - TraverseRecordHelper now calls TraverseDeclTemplateParameterLists (+RecordDecl is covered by the new unit tests)
• CXXRecordDecl - traversal includes a call to TraverseRecordHelper

I believe that nodes derived further from CXXRecordDecl cannot have a template parameter list (?):

• ClassTemplateSpecializationDecl
• ClassTemplatePartialSpecializationDecl

If this is not correct, then can you please help me see the shape of unit tests that would cover these nodes?

1823

I think that TraverseDeclaratorHelper together with TraverseFunctionHelper tweaks, this takes care of all node types derived from DeclaratorDecl:

• FieldDecl - traversal calls TraverseDeclaratorHelper
• FunctionDecl - traversal calls TraverseFunctionHelper
• CXXMethodDecl - traversal calls TraverseFunctionHelper (+CXXMethodDecl is covered by the new unit tests)
• CXXConstructorDecl - traversal calls TraverseFunctionHelper
• CXXConversionDecl - traversal calls TraverseFunctionHelper
• CXXDestructorDecl - traversal calls TraverseFunctionHelper
• VarDecl - TraverseVarHelper calls TraverseDeclaratorHelper (+VarDecl is covered by the new unit tests)

I believe that nodes derived further from VarDecl cannot have a template parameter list (?), but most of them should still be safe (because their traversal goes through TraverseVarHelper):

• DecompositionDecl - traversal calls TraverseVarHelper
• ImplicitParamDecl - traversal calls TraverseVarHelper
• OMPCapturedExprDecl - traversal calls TraverseVarHelper
• ParmVarDecl - traversal calls TraverseVarHelper
• VarTemplateSpecializationDecl
• VarTemplatePartialSpecializationDecl

Please let me know if I am wrong above and it is indeed possible to have template parameter lists next to VarTemplateSpecializationDecl and/or VarTemplatePartialSpecializationDecl (and I would also appreciate if you could help me see the shape of unit tests that would cover these nodes).

1870

Richard, in https://llvm.org/bugs/show_bug.cgi?id=29042#c6 you've suggested reusing TraverseDeclaratorHelper here. I agree that there are refactoring and code reusing opportunities here (e.g. TraverseNestedNameSpecifier[Loc] are getting called from both TraverseDeclaratorHelper and from TraverseFunctionHelper), but I didn't pursue them in this patch, because I was worried about little details:

1. traversal order (i.e. TraverseDeclaratorHelper traverses NestedNameSpecifier immediately before traversing TypeSourceInfo; this is different than TraverseFunctionHelper which traverses DeclarationNameInfo in-between traversing NestedNameSpecifier and TypeSourceInfo)

2. traversal in absence of TypeSourceInfo (i.e. when TypeSourceInfo is missing, then TraverseDeclaratorHelper traverses Type; this is different than TraverseFunctionHelper which in this case 1) checks shouldVisitImplicitCode condition and only then 2) traverses function parameters).

Because of the above, I think that for now it is better to call TraverseDeclTemplateParameterLists directly from TraverseFunctionHelper, rather than trying to reuse the whole (or bigger part of) TraverseDeclaratorHelper from TraverseFunctionHelper.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

1730

I've needed this custom AST matcher in https://codereview.chromium.org/2256913002 - let me try to sneak in a question related to this other CL: Can I somehow avoid introducing a new, custom AST matcher? (i.e. is it possible to express hasUnderlyingType in terms of built-in matchers?)

[rsmith]

include/clang/AST/RecursiveASTVisitor.h

487–489

What's the purpose of the enable_if here?

1728

I don't think ClassTemplateSpecializationDecl can have a template parameter list for its qualifier. A ClassTemplatePartialSpecializationDecl can, though:

template<typename T> struct A {
  template<typename U> struct B; 
};
template<typename T> template<typename U> struct A<T>::B<U*> {};

1823

VarTemplatePartialSpecializationDecls can have template parameter lists:

template<typename T> struct A { template<typename U> static int n; };
template<typename T> template<typename U> int A<T>::n<U*>;

I don't think other VarTemplateSpecializationDecls can.

1870

I'm happy to leave the refactoring (and its changes to traversal order / fixing missing traversal of type) to another day.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

1730

I think this is poorly named -- "underlying type" means something specific to do with enumerations, which is not what you're matching here. I'm also not sure why you need this at all; why can't you just match a templateTypeParmType directly?

1741

This seems like a non-obvious way to check that you matched the right TemplateTypeParmDecl. Can you check the source location of the match instead, perhaps?

[lukasza]

Addressing CR feedbackfrom rsmith@.

include/clang/AST/RecursiveASTVisitor.h

487–489

enable_if is here to document via code that this method works for arguments that are either a TagDecl or a DeclaratorDecl. enable_if is not really necessary here - I could take it out and just make the TraverseDeclTemplateParameterLists<T> method compile as long as T contains the right getNumTemplateParameterLists and getTemplateParameterList methods.

FWIW, I got rid of std::enable_if above in the latest patch revision. Does the code look better and less surprising now?

1728

Thanks for the example. I've added a unit test covering this case (and verified that it fails before the fix and passes afterwards). A bit surprisingly, the extra test passed without further tweaks to the product code (surprisingly, because I am not sure how TraverseDeclTemplateParameterLists ends up getting called for ClassTemplatePartialSpecializationDecl). I don't think I want to keep digging to understand why things work without further tweaks, but the surprise factor makes me think the extra test is worth keeping (even though it apparently doesn't really increase test coverage).

1823

Thanks for the example. I've added a unit test covering this case (and verified that it fails before the fix and passes afterwards). A bit surprisingly, the extra test passed without further tweaks to the product code (surprisingly, because I am not sure how TraverseDeclTemplateParameterLists ends up getting called for VarTemplatePartialSpecializationDecl). I don't think I want to keep digging to understand why things work without further tweaks, but the surprise factor makes me think the extra test is worth keeping (even though it apparently doesn't really increase test coverage).

Also, I've found out that in my little write-up above I missed the following types of nodes derived from DeclaratorDecl (although looking at the code everything seems okay):

• ObjCAtDefsFieldDecl - traversal calls TraverseDeclaratorHelper
• ObjCIvarDecl - traversal calls TraverseDeclaratorHelper
• MSPropertyDecl - traversal calls TraverseDeclaratorHelper
• NonTypeTemplateParmDecl - traversal calls TraverseDeclaratorHelper

1870

Ack.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

1730

Good point on the name - does hasBaseType sound better? (not important for this patch, because as you point out I don't need this custom matcher, but possibly still important for the non-LLVM patch at https://codereview.chromium.org/2256913002).

For the unit tests, I can indeed just use templateTypeParmType directly - done in the next patch revision.

For https://codereview.chromium.org/2256913002, I think that I still need hasBaseType to only match DependentScopeDeclRefExpr if |T| in |T::foo| comes from a namespace I am interested in (e.g. T has to come from a namespace where my tool has to do massive renaming). The code I used (and am not very happy about) in https://codereview.chromium.org/2256913002/patch/140001/150001 is:

auto in_blink_namespace = decl(
    anyOf(decl_under_blink_namespace, decl_has_qualifier_to_blink_namespace,
          hasAncestor(decl_has_qualifier_to_blink_namespace)),
    unless(isExpansionInFileMatching(kGeneratedFileRegex)));

...
+ auto blink_qual_type_matcher = qualType(
+ anyOf(pointsTo(in_blink_namespace), references(in_blink_namespace),
+ hasUnderlyingType(anyOf(
+ enumType(hasDeclaration(in_blink_namespace)),
+ recordType(hasDeclaration(in_blink_namespace)),
+ templateSpecializationType(hasDeclaration(in_blink_namespace)),
+ templateTypeParmType(hasDeclaration(in_blink_namespace)),
+ typedefType(hasDeclaration(in_blink_namespace))))));

1741

I don't understand this feedback :-(

If you are referring to the usage of |hasAncestor|, then this is in here to trigger the assert that fails the tests before the fix - assert(!Parents.empty() && "Found node that is not in the parent map.");

I guess something like this would be an alternative unit test - before the fix this would fail, but instead of hitting the assert, it would fail to find a match. In other words - this text expectation ensures the right match:

EXPECT_TRUE(matches(
    "...",
    templateTypeParmDecl(hasName("U"))));

And this test expectation (more-or-less what I had in the originally proposed patch) catches the assert failure:

EXPECT_TRUE(matches(
    "...",
    templateTypeParmType(hasDeclaration(decl(hasAncestor(decl()))))));

Are you saying that I should use only the second test expectation (the one with hasName)? Both (i.e. the one with hasAncestor and the one with hasName)? Something else? FWIW, I switched to the |hasName| expectation in the most recent patch revision.

PS. If we were to remove |hasAncestor| from the matcher used in the test cases, then I guess the test(suite) name wouldn't quite fit - this makes me think that I should rename the testcases from TEST(HasAncestor, TemplateTypeParmDeclUnder...) to TEST(TemplateTypeParmDecl, Under...).

[rsmith]

This patch looks great, thank you! Do you have an SVN account or do you need someone to commit this for you?

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

729

I'd view the hasAncestor failure as an indirect symptom of the root problem, which is that we were failing to traverse some nodes. I prefer the new hasName test, since it more directly checks for that immediate problem.

[lukasza]

In D24268#535463, @rsmith wrote:

This patch looks great, thank you! Do you have an SVN account or do you need someone to commit this for you?

I don't have an SVN account. Could you please commit the patch for me?

[thakis]

landed in r281345.

include/clang/AST/RecursiveASTVisitor.h

 private:
   // These are helper methods used by more than one Traverse* method.
   bool TraverseTemplateParameterListHelper(TemplateParameterList *TPL);
+
+  // Traverses template parameter lists of either a DeclaratorDecl or TagDecl.
+  template <typename T, typename Enabled = typename std::enable_if<
+      std::is_base_of<DeclaratorDecl, T>::value ||
+      std::is_base_of<TagDecl, T>::value>::type>
+  bool TraverseDeclTemplateParameterLists(T *D);
+
 #define DEF_TRAVERSE_TMPL_INST(TMPLDECLKIND)                                   \
   bool TraverseTemplateInstantiations(TMPLDECLKIND##TemplateDecl *D);
   DEF_TRAVERSE_TMPL_INST(Class)

 }
 
 template <typename Derived>
+template <typename T, typename Enabled>
+bool RecursiveASTVisitor<Derived>::TraverseDeclTemplateParameterLists(T *D) {
+  for (unsigned i = 0; i < D->getNumTemplateParameterLists(); i++) {
+    TemplateParameterList *TPL = D->getTemplateParameterList(i);
+    TraverseTemplateParameterListHelper(TPL);
+  }
+  return true;
+}
+
+template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseTemplateInstantiations(
     ClassTemplateDecl *D) {
   for (auto *SD : D->specializations()) {

 })
 
 DEF_TRAVERSE_DECL(EnumDecl, {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
+
   if (D->getTypeForDecl())
     TRY_TO(TraverseType(QualType(D->getTypeForDecl(), 0)));
 

   // We shouldn't traverse D->getTypeForDecl(); it's a result of
   // declaring the type, not something that was written in the source.
 
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
   return true;
 }

 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseDeclaratorHelper(DeclaratorDecl *D) {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
   if (D->getTypeSourceInfo())
     TRY_TO(TraverseTypeLoc(D->getTypeSourceInfo()->getTypeLoc()));

 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseFunctionHelper(FunctionDecl *D) {
+  TRY_TO(TraverseDeclTemplateParameterLists(D));
   TRY_TO(TraverseNestedNameSpecifierLoc(D->getQualifierLoc()));
   TRY_TO(TraverseDeclarationNameInfo(D->getNameInfo()));
 

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

                         "}\n",
                       declRefExpr(to(decl(hasAncestor(decl()))))));
 }
+
+AST_MATCHER_P(QualType,
+              hasUnderlyingType,
+              internal::Matcher<Type>,
+              InnerMatcher) {
+  const Type* type = Node.getTypePtrOrNull();
+  return type && InnerMatcher.matches(*type, Finder, Builder);
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderCXXMethodDecl) {
+  EXPECT_TRUE(matches(
+      "template<typename T>\n"
+      "class Class {\n"
+      "  void method();\n"
+      "};\n"
+      "template<typename U>\n"
+      "void Class<U>::method() {}\n",
+      qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+          hasAncestor(decl()))))))));
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderVarDecl) {
+  EXPECT_TRUE(matches(
+      "template<typename T>\n"
+      "class Class {\n"
+      "  static T pi;\n"
+      "};\n"
+      "template<typename U>\n"
+      "U Class<U>::pi = U(3.1415926535897932385);\n",
+      qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+          hasAncestor(decl()))))))));
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderEnumDecl) {
+  EXPECT_TRUE(matches(
+      "template<typename T>\n"
+      "struct Struct {\n"
+      "  enum class Enum : T;\n"
+      "};\n"
+      "template<typename U>\n"
+      "enum class Struct<U>::Enum : U {\n"
+      "  e1,\n"
+      "  e2\n"
+      "};\n",
+      qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+          hasAncestor(decl()))))))));
+}
+
+TEST(HasAncestor, TemplateTypeParmDeclUnderStructDecl) {
+  EXPECT_TRUE(matches(
+      "template<typename T>\n"
+      "class Class {\n"
+      "  struct Struct;\n"
+      "};\n"
+      "template<typename U>\n"
+      "struct Class<U>::Struct {\n"
+      "  U field;\n"
+      "};\n",
+      qualType(hasUnderlyingType(templateTypeParmType(hasDeclaration(decl(
+          hasAncestor(decl()))))))));
+}
+
 TEST(HasAncestor, NonParmDependentTemplateParmVarDeclRefExpr) {
   EXPECT_TRUE(matches("struct PartitionAllocator {\n"
                         "  template<typename T>\n"