Add an AST matcher for CastExpr kind

[etienneb]

Summary

This AST matcher will match a given CastExpr kind.
It's an narrowing matcher on CastExpr.

[aaron.ballman]

Is this required for some purpose?

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

Also, be sure to regenerate the documentation from dump_ast_matchers.py

[etienneb]

In D19871#419947, @aaron.ballman wrote:

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

Also, be sure to regenerate the documentation from dump_ast_matchers.py

Ah, this is the way to generate the doc. Thanks :)

[aaron.ballman]

In D19871#419954, @etienneb wrote:

In D19871#419947, @aaron.ballman wrote:

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

Also, be sure to regenerate the documentation from dump_ast_matchers.py

Ah, this is the way to generate the doc. Thanks :)

You're welcome!

[sbenza]

In D19871#419985, @aaron.ballman wrote:

In D19871#419954, @etienneb wrote:

In D19871#419947, @aaron.ballman wrote:

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

What we have done in the past with enum-arg matchers is to use string->enum conversion in the dynamic bindings.
See the specialization ArgTypeTraits<attr::Kind> in Marshallers.h.
We could add one for CastKind too.

[etienneb]

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

That was my original idea.

In D19871#419998, @sbenza wrote:

In D19871#419985, @aaron.ballman wrote:

In D19871#419954, @etienneb wrote:

In D19871#419947, @aaron.ballman wrote:

Is this required for some purpose?

It's used in clang-tidy checkers.

see http://reviews.llvm.org/D19841

It's good to have that context in a review for functionality that isn't part of the proposed patch. :-) Looking at the other patch, I would prefer to keep this matcher narrowed to just clang-tidy unless you can also solve how to expose it via the dynamic registry so that it can be used by tools like clang-query.

What we have done in the past with enum-arg matchers is to use string->enum conversion in the dynamic bindings.
See the specialization ArgTypeTraits<attr::Kind> in Marshallers.h.
We could add one for CastKind too.

I think I got the idea, I'll give a try to see the results. Thanks Samuel.

[alexfh]

In D19871#419947, @aaron.ballman wrote:

Is this required for some purpose?

I'm not keen on adding new AST matchers that we cannot expose via the dynamic API, so I would prefer to solve that problem if we want to add this matcher.

I agree that when possible, matchers should be available via the dynamic matchers API. It doesn't seem overly complicated to add this support here. As far as I understand, we just need to register the matcher in lib/ASTMatchers/Dynamic/Registry.cpp and add support for CastKind argument type (by adding a corresponding ArgTypeTraits instantiation). Etienne, can you try this?

[etienneb]

I agree that when possible, matchers should be available via the dynamic matchers API. It doesn't seem overly complicated to add this support here. As far as I understand, we just need to register the matcher in lib/ASTMatchers/Dynamic/Registry.cpp and add support for CastKind argument type (by adding a corresponding ArgTypeTraits instantiation). Etienne, can you try this?

1. The parameter is passed as a string (which is not the case for the hasCastKind matcher): hasAttr("attr::CUDADevice").

2. There is no easy way to list every cast kind. Which means we need to hardcode them (or iterate over the domain) [both solution sounds terrible to me].

static clang::CastKind getCastKind(llvm::StringRef AttrKind) {
  return llvm::StringSwitch<clang::CastKind>(AttrKind)
    .Case("CK_Dependent", CK_Dependent)
   [...]    <<-- list every cast kind here.
    .Default(clang::CastKind(-1));
}

So even if the above solution is working, we still need to call it that way (as a string):
clang-query> match castExpr(hasCastKind("CK_Dependent"))

[aaron.ballman]

In D19871#420095, @etienneb wrote:

I agree that when possible, matchers should be available via the dynamic matchers API. It doesn't seem overly complicated to add this support here. As far as I understand, we just need to register the matcher in lib/ASTMatchers/Dynamic/Registry.cpp and add support for CastKind argument type (by adding a corresponding ArgTypeTraits instantiation). Etienne, can you try this?

1. The parameter is passed as a string (which is not the case for the hasCastKind matcher): hasAttr("attr::CUDADevice").

Correct. The dynamic registry has no support for enumerations of values, and so it uses strings instead. I think that's fine here as well.

2. There is no easy way to list every cast kind. Which means we need to hardcode them (or iterate over the domain) [both solution sounds terrible to me].

static clang::CastKind getCastKind(llvm::StringRef AttrKind) {
  return llvm::StringSwitch<clang::CastKind>(AttrKind)
    .Case("CK_Dependent", CK_Dependent)
   [...]    <<-- list every cast kind here.
    .Default(clang::CastKind(-1));
}

This is unfortunate, but is likely the way we would move forward.

So even if the above solution is working, we still need to call it that way (as a string):
clang-query> match castExpr(hasCastKind("CK_Dependent"))

Correct, that's expected behavior for clang-query (though I would love if someday we could expose actual enumerations somehow instead of string literals).

[etienneb]

though I would love if someday we could expose actual enumerations somehow instead of string literals

I would like too. Ditto for "equals" which is missing :)

[etienneb]

add dynamic parsing

[etienneb]

nit

[sbenza]

So even if the above solution is working, we still need to call it that way (as a string):
clang-query> match castExpr(hasCastKind("CK_Dependent"))

Correct, that's expected behavior for clang-query (though I would love if someday we could expose actual enumerations somehow instead of string literals).

It is not hard to do, but it would require changing the parser, the registry, the type-erased value wrapper, etc.
The late conversion from "string" to enum was the easiest solution at the time.

[aaron.ballman]

lib/ASTMatchers/Dynamic/Marshallers.h

102

This might be an awful idea, but let's explore it.

What if we moved the CastKind enumerator names into a .def (or .inc) file and use macros to generate the enumeration as well as this monster switch statement? We do this in other places where it makes sense to do so, such as in Expr.h:

  enum AtomicOp {
#define BUILTIN(ID, TYPE, ATTRS)
#define ATOMIC_BUILTIN(ID, TYPE, ATTRS) AO ## ID,
#include "clang/Basic/Builtins.def"
    // Avoid trailing comma
    BI_First = 0
  };

[etienneb]

lib/ASTMatchers/Dynamic/Marshallers.h

102

Does the dynamic matching is used somewhere else than clang-query?
I wonder the impact of refactoring to support them if it's barely used.
It can't be worse than before as it wasn't supported at all (the matcher didn't exists).

I believe there is a larger cleanup to do to support correctly dynamic matcher like "equals".
And, this case is one among others.

I'm not a fan of this huge switch that may just get out-of-sync with the original enum.

I'm still in favor of adding this matcher to the unsupported list until we push the more complicated fix.
[which may fall in my plate anyway]

Any toughs?

[sbenza]

lib/ASTMatchers/Dynamic/Marshallers.h

102

I'm not a fan of this either.
If the enum has to be used in this way, it should be refactored to be generated with an .inc/.def file.

[etienneb]

revert to previous solution

[etienneb]

fix unittests

[etienneb]

Get back to this solution.
comments?

[sbenza]

lib/ASTMatchers/Dynamic/Registry.cpp

78

Is it registered or not?
You add this comment, but also add the matcher in the registry below.

[etienneb]

rebase over AST modifications

[etienneb]

This patch has been modified to support dynamic matchers.
It rely on : http://reviews.llvm.org/D20207

Comments?

lib/ASTMatchers/Dynamic/Marshallers.h

102

moved to that solution. (let try it)

lib/ASTMatchers/Dynamic/Registry.cpp

78

bad revert.
I'm making a patch to allow dynamic matcher, and I'll come back fixing this CL.

[etienneb]

regenerate doc

[jroelofs]

Drive-by thought: I think it would be useful to be able to match implicit casts separately from explicit casts when using this new matcher.

[etienneb]

add unittest

[etienneb]

In D19871#428997, @jroelofs wrote:

Drive-by thought: I think it would be useful to be able to match implicit casts separately from explicit casts when using this new matcher.

Jonathan, I think what you are asking for is already supported.
I added an unittest to confirm it.

EXPECT_TRUE(matches("char *p = 0;",
            implicitCastExpr(hasCastKind(CK_NullToPointer))));

If I misunderstood your comment, could you provide an example.

[etienneb]

nit: indentation

[jroelofs]

In D19871#429153, @etienneb wrote:

In D19871#428997, @jroelofs wrote:

Drive-by thought: I think it would be useful to be able to match implicit casts separately from explicit casts when using this new matcher.

Jonathan, I think what you are asking for is already supported.
I added an unittest to confirm it.

EXPECT_TRUE(matches("char *p = 0;",
            implicitCastExpr(hasCastKind(CK_NullToPointer))));

If I misunderstood your comment, could you provide an example.

Oh, right. That /is/ exactly what I meant. Excellent.

[aaron.ballman]

LGTM! Thank you for this!

[Prazek]

+1 I will probably also use this.

Does hasCastKind works for implicitCastExpr?

[etienneb]

In D19871#429466, @Prazek wrote:

+1 I will probably also use this.

Does hasCastKind works for implicitCastExpr?

Yes, there is a unittest with the code.

docs/LibASTMatchersReference.html

 a specific number of arguments (including absent default arguments).
 
 Example matches f(0, 0) (matcher = callExpr(argumentCountIs(2)))
   void f(int x, int y);
   f(0, 0);
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CastExpr.html">CastExpr</a>&gt;</td><td class="name" onclick="toggle('hasCastKind0')"><a name="hasCastKind0Anchor">hasCastKind</a></td><td>CastKind Kind</td></tr>
+<tr><td colspan="4" class="doc" id="hasCastKind0"><pre>Matches casts that has a given cast kind.
+
+Example: matches the implicit cast around 0
+(matcher = castExpr(hasCastKind(CK_NullToPointer)))
+  int *p = 0;
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CharacterLiteral.html">CharacterLiteral</a>&gt;</td><td class="name" onclick="toggle('equals3')"><a name="equals3Anchor">equals</a></td><td>ValueT  Value</td></tr>
 <tr><td colspan="4" class="doc" id="equals3"><pre>Matches literals that are equal to the given value.
 
 Example matches true (matcher = cxxBoolLiteral(equals(true)))
   true
 
 Usable as: Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1CharacterLiteral.html">CharacterLiteral</a>&gt;, Matcher&lt;CXXBoolLiteral&gt;,
            Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1FloatingLiteral.html">FloatingLiteral</a>&gt;, Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1IntegerLiteral.html">IntegerLiteral</a>&gt;

include/clang/ASTMatchers/ASTMatchers.h

AST_POLYMORPHIC_MATCHER_P(hasSourceExpression,
                                                           OpaqueValueExpr),
                           internal::Matcher<Expr>, InnerMatcher) {
   const Expr *const SubExpression =
       internal::GetSourceExpressionMatcher<NodeType>::get(Node);
   return (SubExpression != nullptr &&
           InnerMatcher.matches(*SubExpression, Finder, Builder));
 }
 
+/// \brief Matches casts that has a given cast kind.
+///
+/// Example: matches the implicit cast around \c 0
+/// (matcher = castExpr(hasCastKind(CK_NullToPointer)))
+/// \code
+///   int *p = 0;
+/// \endcode
+AST_MATCHER_P(CastExpr, hasCastKind, CastKind, Kind) {
+  return Node.getCastKind() == Kind;
+}
+
 /// \brief Matches casts whose destination type matches a given matcher.
 ///
 /// (Note: Clang's AST refers to other conversions as "casts" too, and calls
 /// actual casts "explicit" casts.)
 AST_MATCHER_P(ExplicitCastExpr, hasDestinationType,
               internal::Matcher<QualType>, InnerMatcher) {
   const QualType NodeType = Node.getTypeAsWritten();
   return InnerMatcher.matches(NodeType, Finder, Builder);

lib/ASTMatchers/Dynamic/Marshallers.h

public:
   static attr::Kind get(const VariantValue &Value) {
     return getAttrKind(Value.getString());
   }
   static ArgKind getKind() {
     return ArgKind(ArgKind::AK_String);
   }
 };
 
+template <> struct ArgTypeTraits<clang::CastKind> {
+private:
+  static clang::CastKind getCastKind(llvm::StringRef AttrKind) {
+    return llvm::StringSwitch<clang::CastKind>(AttrKind)
+#define CAST_OPERATION(Name) .Case( #Name, CK_##Name)
+#include "clang/AST/OperationKinds.def"
+        .Default(CK_Invalid);
+  }
+
+public:
+  static bool is(const VariantValue &Value) {
+    return Value.isString() &&  
+        getCastKind(Value.getString()) != CK_Invalid;
+  }
+  static clang::CastKind get(const VariantValue &Value) {
+    return getCastKind(Value.getString());
+  }
+  static ArgKind getKind() {
+    return ArgKind(ArgKind::AK_String);
+  }
+};
+
 /// \brief Matcher descriptor interface.
 ///
 /// Provides a \c create() method that constructs the matcher from the provided
 /// arguments, and various other methods for type introspection.
 class MatcherDescriptor {
 public:
   virtual ~MatcherDescriptor() {}
   virtual VariantMatcher create(SourceRange NameRange,

lib/ASTMatchers/Dynamic/Registry.cpp

registerMatcher(#name,                                                     \
                     new internal::OverloadedMatcherDescriptor(Callbacks));     \
   } while (0)
 
 /// \brief Generate a registry map with all the known matchers.
 RegistryMaps::RegistryMaps() {
   // TODO: Here is the list of the missing matchers, grouped by reason.
   //
   // Need Variant/Parser fixes:
   // ofKind
   //
   // Polymorphic + argument overload:
   // findAll
   //
   // Other:
   // equals
   // equalsNode
 
RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(hasArgument);
   REGISTER_MATCHER(hasArgumentOfType);
   REGISTER_MATCHER(hasAttr);
   REGISTER_MATCHER(hasAutomaticStorageDuration);
   REGISTER_MATCHER(hasBase);
   REGISTER_MATCHER(hasBody);
   REGISTER_MATCHER(hasCanonicalType);
   REGISTER_MATCHER(hasCaseConstant);
+  REGISTER_MATCHER(hasCastKind);
   REGISTER_MATCHER(hasCondition);
   REGISTER_MATCHER(hasConditionVariableStatement);
   REGISTER_MATCHER(hasDecayedType);
   REGISTER_MATCHER(hasDeclaration);
   REGISTER_MATCHER(hasDeclContext);
   REGISTER_MATCHER(hasDeducedType);
   REGISTER_MATCHER(hasDescendant);
   REGISTER_MATCHER(hasDestinationType);

unittests/ASTMatchers/ASTMatchersTest.cpp

 
 TEST(CastExpression, DoesNotMatchNonCasts) {
   EXPECT_TRUE(notMatches("char c = '0';", castExpr()));
   EXPECT_TRUE(notMatches("char c, &q = c;", castExpr()));
   EXPECT_TRUE(notMatches("int i = (0);", castExpr()));
   EXPECT_TRUE(notMatches("int i = 0;", castExpr()));
 }
 
+TEST(CastExpression, HasCastKind) {
+  EXPECT_TRUE(matches("char *p = 0;",
+              castExpr(hasCastKind(CK_NullToPointer))));
+  EXPECT_TRUE(notMatches("char *p = 0;",
+              castExpr(hasCastKind(CK_DerivedToBase))));
+  EXPECT_TRUE(matches("char *p = 0;",
+              implicitCastExpr(hasCastKind(CK_NullToPointer))));
+}
+
 TEST(ReinterpretCast, MatchesSimpleCase) {
   EXPECT_TRUE(matches("char* p = reinterpret_cast<char*>(&p);",
                       cxxReinterpretCastExpr()));
 }
 
 TEST(ReinterpretCast, DoesNotMatchOtherCasts) {
   EXPECT_TRUE(notMatches("char* p = (char*)(&p);", cxxReinterpretCastExpr()));
   EXPECT_TRUE(notMatches("char q, *p = const_cast<char*>(&q);",