Page MenuHomePhabricator

Make `hasTypeLoc` matcher support more node types.
ClosedPublic

Authored by SilensAngelusNex on Apr 29 2021, 2:33 PM.

Diff Detail

Event Timeline

SilensAngelusNex requested review of this revision.Apr 29 2021, 2:33 PM
SilensAngelusNex created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 2:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire added a subscriber: steveire.

According to

llvm-project/clang/include/clang/AST$ git grep "TypeSourceInfo.*const"

there are lots of other classes with TypeSourceInfo accessors which could be supported here. Should any others be supported? Is something specific motivating adding support for these two classes and not the others?

steveire edited reviewers, added: steveire; removed: stephenkelly.Apr 29 2021, 3:27 PM

Yes, the motivation for adding these is so I can use clang-transformer to refactor a bunch of constructor calls to call a static factory method instead.

// Before
ns::Foo x(1);
auto y = ns::Foo(1, 2);

// After
auto x = ns::Foo::Make(1);
auto y = ns::Foo::Make(1, 2);

That refactor only needs hasTypeInfo to work on these three node types, but for consistency's sake I'd be happy to add overloads for others as well. I looked at the types in the results of the grep you posted; I think it would make sense for hasTypeInfo to be able to handle the classes in the first and third groups; do you think that would be reasonable?

Pretty sure these should work with hasTypeInfo.

  • TypedefNameDecl (getTypeSourceInfo)
  • CXXBaseSpecifier (getTypeSourceInfo)
  • CXXCtorInitializer (getTypeSourceInfo)
  • ObjCPropertyDecl (getTypeSourceInfo)
  • ClassTemplateSpecializationDecl (getTypeAsWritten)
  • CompoundLiteralExpr (getTypeSourceInfo)
  • ExplicitCastExpr (getTypeInfoAsWritten)
  • CXXTemporaryObjectExpr (getTypeSourceInfo)
  • CXXUnresolvedConstructExpr (getTypeSourceInfo)
  • TemplateArgumentLoc (getTypeSourceInfo)

These could make sense, but no other matchers exist for nodes of these types:

  • VarTemplateSpecializationDecl (getTypeAsWritten)
  • OffsetOfExpr (getTypeSourceInfo)
  • ConvertVectorExpr (getTypeSourceInfo)
  • VAArgExpr (getWrittenTypeInfo)
  • CXXScalarValueInitExpr (getTypeSourceInfo)

These nodes have a method with a different name, but probably still make sense to use with hasTypeLoc.

  • BlockDecl (getSignatureAsWritten)
  • CXXNewExpr (getAllocatedTypeSourceInfo)

These all have a method that returns a TypeSourceInfo*, but seem like they're expressing something different and shouldn't work with hasTypeInfo.

  • EnumDecl (getIntegerTypeSourceInfo)
  • CXXRecordDecl (getLambdaTypeInfo) fails if not a lambda
  • FriendDecl (getFriendType)
  • ObjCMethodDecl (getReturnTypeSourceInfo)
  • ObjCInterfaceDecl (getSuperClassTInfo)
  • UnaryExprOrTypeTraitExpr (getArgumentTypeInfo) fails when arg is expr instead of type

Yes, the motivation for adding these is so I can use clang-transformer to refactor a bunch of constructor calls to call a static factory method instead.

// Before
ns::Foo x(1);
auto y = ns::Foo(1, 2);

// After
auto x = ns::Foo::Make(1);
auto y = ns::Foo::Make(1, 2);

That refactor only needs hasTypeInfo to work on these three node types, but for consistency's sake I'd be happy to add overloads for others as well. I looked at the types in the results of the grep you posted; I think it would make sense for hasTypeInfo to be able to handle the classes in the first and third groups; do you think that would be reasonable?

Pretty sure these should work with hasTypeInfo.

  • TypedefNameDecl (getTypeSourceInfo)
  • CXXBaseSpecifier (getTypeSourceInfo)
  • CXXCtorInitializer (getTypeSourceInfo)
  • ObjCPropertyDecl (getTypeSourceInfo)
  • ClassTemplateSpecializationDecl (getTypeAsWritten)
  • CompoundLiteralExpr (getTypeSourceInfo)
  • ExplicitCastExpr (getTypeInfoAsWritten)
  • CXXTemporaryObjectExpr (getTypeSourceInfo)
  • CXXUnresolvedConstructExpr (getTypeSourceInfo)
  • TemplateArgumentLoc (getTypeSourceInfo)

These could make sense, but no other matchers exist for nodes of these types:

  • VarTemplateSpecializationDecl (getTypeAsWritten)
  • OffsetOfExpr (getTypeSourceInfo)
  • ConvertVectorExpr (getTypeSourceInfo)
  • VAArgExpr (getWrittenTypeInfo)
  • CXXScalarValueInitExpr (getTypeSourceInfo)

These nodes have a method with a different name, but probably still make sense to use with hasTypeLoc.

  • BlockDecl (getSignatureAsWritten)
  • CXXNewExpr (getAllocatedTypeSourceInfo)

These all have a method that returns a TypeSourceInfo*, but seem like they're expressing something different and shouldn't work with hasTypeInfo.

  • EnumDecl (getIntegerTypeSourceInfo)
  • CXXRecordDecl (getLambdaTypeInfo) fails if not a lambda
  • FriendDecl (getFriendType)
  • ObjCMethodDecl (getReturnTypeSourceInfo)
  • ObjCInterfaceDecl (getSuperClassTInfo)
  • UnaryExprOrTypeTraitExpr (getArgumentTypeInfo) fails when arg is expr instead of type

Great work classifying those!

Yes, I think it makes sense to implement this for the 1st and 3rd groups. It might even make sense to make the method names on the classes consistent (in a prior patch) to make the matcher implementation simpler, but I'll leave it up to you to determine what's best there.

SilensAngelusNex retitled this revision from Make `hasTypeLoc` matcher support nodes of type `CXXFunctionalCastExpr` and `CXXTemporaryObjectExpr`. to Make `hasTypeLoc` matcher support more node types..

Added support for node types in groups #1 and #3.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h
169–181

Is there a better way to express this? I was originally using

template <typename... Expected> struct is_one_of {
  template <typename T>
  static constexpr bool value = (std::is_base_of_v<Expected, T> || ...);
};

but that didn't compile because is_base_of_v and fold expressions are C++17 features.

Maybe it would be better to just explicitly write out an overload of GetTypeSourceInfo for each type?

Please run llvm-project/clang/docs/tools/dump_ast_matchers.py to generate updated documentation for this (there is a urlopen(url) which slows things down, so I usually comment that out when running it).

Do you have this on a git clone somewhere? It's easier to take a patch and try it out from a git repo than phab.

clang/include/clang/ASTMatchers/ASTMatchers.h
3919

This should somehow document the types it works with. You don't have to add examples for all of the nodes though. hasDeclaration is another example of a matcher supporting many different nodes. It has Usable as: at the bottom of its docs which gets parsed by dump_ast_matchers.py. Please make a similar change here to verify that the script generates docs for each type.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h
169–181

I like your c++17 solution too, but given that we have TypeListContainsSuperOf in this file used for this kind of thing, I think it makes sense to use that. I think this will work:

template <typename T,
          std::enable_if_t<ast_matchers::internal::TypeListContainsSuperOf<
            ast_matchers::internal::TypeList<
              CXXBaseSpecifier, CXXCtorInitializer, CXXTemporaryObjectExpr,
              CXXUnresolvedConstructExpr, CompoundLiteralExpr, DeclaratorDecl,
              ObjCPropertyDecl, TemplateArgumentLoc, TypedefNameDecl>, T>::value>
              * = nullptr>
SilensAngelusNex marked 2 inline comments as done.

Regenerate docs and replace is_one_of with TypeListContainsSuperOf.

clang/include/clang/ASTMatchers/ASTMatchersInternal.h
169–181

Perfect, thanks!

steveire accepted this revision.May 6 2021, 3:51 PM

This looks good to me. I tried running it in clang-query, which crashes with a matcher like

m binaryOperator(hasEitherOperand(hasTypeLoc(loc(asString("int")))))

but it also crashes before your change, so it must be unrelated.

Do you have a commit account or will I commit this for you with the name/email in your github commit?

This revision is now accepted and ready to land.May 6 2021, 3:51 PM

I don't have commit access; you can go ahead and submit it.

Thanks for the review!

This revision was landed with ongoing or failed builds.May 7 2021, 4:36 PM
This revision was automatically updated to reflect the committed changes.