[ASTMatchers] New matcher hasReturnValue added

[baloghadamsoftware]

Summary

A checker (will be uploaded after this patch) needs to check implicit casts. Existing generic matcher "has" ignores implicit casts and parenthesized expressions and no specific matcher for matching return value expression preexisted. The patch adds such a matcher (hasReturnValue).

[sbenza]

The reason we haven't fixed hasAnyArgument is that it can potentially break its users.
I'd prefer if you separated the fix from the addition.
That way we can revert the fix if needed.

include/clang/ASTMatchers/ASTMatchers.h

4845

New matchers must be tested.
See ASTMatchersTest.cpp.

Also, they should be added to the dynamic registry.
See Dynamic/Registry.cpp

4849

I'm not sure this is needed.
returnStmt(hasReturnValue(...)) is equivalent to returnStmt(has(...))
There are many checks using the latter already.

[baloghadamsoftware]

In D17986#373134, @sbenza wrote:

The reason we haven't fixed hasAnyArgument is that it can potentially break its users.
I'd prefer if you separated the fix from the addition.
That way we can revert the fix if needed.

I will separate it, OK. In the Clang there is one use case that I fixed, although it did not break the tests. Neither of the other "has..." checkers (except the general ones) ignore implicit casts and parenthesized expressions so this one should not do it either because it makes checking implicit casts impossible.

Matcher "hasReturnValue" is needed because "has" ignores all implicit casts and parenthesized expressions and we need to the check implicit casts. I will add a test and add it to the dynamic registry.

[sbenza]

I will separate it, OK. In the Clang there is one use case that I fixed, although it did not break the tests. Neither of the other "has..." checkers (except the general ones) ignore implicit casts and parenthesized expressions so this one should not do it either because it makes checking implicit casts impossible.

I agree that we want it, just wanted to point out that it has to be done with care.

Matcher "hasReturnValue" is needed because "has" ignores all implicit casts and parenthesized expressions and we need to the check implicit casts. I will add a test and add it to the dynamic registry.

I see. has() suffers from the same problem. Then it makes sense.

[baloghadamsoftware]

Separation of new matcher and fix for existing matcher. This patch only contains the new matcher. Test added and matcher added to the registry.

[baloghadamsoftware]

Previous patch generation failed.

[sbenza]

include/clang/ASTMatchers/ASTMatchers.h

4843

a + b (with spaces)
A few in this comment, and one in the test.

4849

Wouldn't this just be:

AST_MATCHER_P(...) {
  return InnerMatcher.matches(*Node.getRetValue(), Finder, Builder);
}

[sbenza]

Can you rerun the doc script (dump_ast_matchers.py)?

[baloghadamsoftware]

I can rerun the script, however it seems it was not executed before the last commit on the master branch, thus if I rerun it then changes will appear in my diff which are not related to my work. What is the exect policy about running this scipt? Should it be rerun before the commit (then it was forgotten by the last committer) or it is executed only before releases?

[sbenza]

In D17986#379271, @baloghadamsoftware wrote:

I can rerun the script, however it seems it was not executed before the last commit on the master branch, thus if I rerun it then changes will appear in my diff which are not related to my work. What is the exect policy about running this scipt? Should it be rerun before the commit (then it was forgotten by the last committer) or it is executed only before releases?

I don't think there is a specific policy here. We try to keep it up to date, but some changes forget to do it.
We can do it in a separate patch if you feel the extra diff should not be on this one.

[baloghadamsoftware]

LibASTMatchersReference.html regenerated

include/clang/ASTMatchers/ASTMatchers.h

 /// Given
 /// \code
 ///   void x(int, int, int) { int y; x(1, y, 42); }
 /// \endcode
 /// callExpr(hasAnyArgument(declRefExpr()))
 ///   matches x(1, y, 42)
 /// with hasAnyArgument(...)
 ///   matching y
-///
-/// FIXME: Currently this will ignore parentheses and implicit casts on
-/// the argument before applying the inner matcher. We'll want to remove
-/// this to allow for greater control by the user once \c ignoreImplicit()
-/// has been implemented.
 AST_POLYMORPHIC_MATCHER_P(hasAnyArgument,
                           AST_POLYMORPHIC_SUPPORTED_TYPES(CallExpr,
                                                           CXXConstructExpr),
                           internal::Matcher<Expr>, InnerMatcher) {
   for (const Expr *Arg : Node.arguments()) {
     BoundNodesTreeBuilder Result(*Builder);
-    if (InnerMatcher.matches(*Arg->IgnoreParenImpCasts(), Finder, &Result)) {
+    if (InnerMatcher.matches(*Arg, Finder, &Result)) {
       *Builder = std::move(Result);
       return true;
     }
   }
   return false;
 }
 
 /// \brief Matches a constructor call expression which uses list initialization.

 AST_MATCHER_P(Decl, hasAttr, attr::Kind, AttrKind) {
   for (const auto *Attr : Node.attrs()) {
     if (Attr->getKind() == AttrKind)
       return true;
   }
   return false;
 }
 
+/// \brief Matches the return value expression of a return statement
+///
+/// Given
+/// \code
+///   return a+b;
+/// \endcode
+/// hasReturnValue(binaryOperator())
+///   matches 'return a+b'
+/// with binaryOperator()
+///   matching 'a+b'
+AST_MATCHER_P(ReturnStmt, hasReturnValue, internal::Matcher<Expr>, InnerMatcher) {
+  BoundNodesTreeBuilder Result(*Builder);
+  if(InnerMatcher.matches(*Node.getRetValue(), Finder, &Result)) {
+    *Builder = std::move(Result);
+    return true;
+  }
+  return false;
+}
+
+
 /// \brief Matches CUDA kernel call expression.
 ///
 /// Example matches,
 /// \code
 ///   kernel<<<i,j>>>();
 /// \endcode
 const internal::VariadicDynCastAllOfMatcher<
   Stmt,

lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(hasObjectExpression);
   REGISTER_MATCHER(hasOperatorName);
   REGISTER_MATCHER(hasOverloadedOperatorName);
   REGISTER_MATCHER(hasParameter);
   REGISTER_MATCHER(hasParent);
   REGISTER_MATCHER(hasQualifier);
   REGISTER_MATCHER(hasRangeInit);
   REGISTER_MATCHER(hasReceiverType);
+  REGISTER_MATCHER(hasReturnValue);
   REGISTER_MATCHER(hasRHS);
   REGISTER_MATCHER(hasSelector);
   REGISTER_MATCHER(hasSingleDecl);
   REGISTER_MATCHER(hasSize);
   REGISTER_MATCHER(hasSizeExpr);
   REGISTER_MATCHER(hasSourceExpression);
   REGISTER_MATCHER(hasStaticStorageDuration);
   REGISTER_MATCHER(hasTargetDecl);

unittests/ASTMatchers/ASTMatchersTest.cpp

EXPECT_TRUE(matches("#define NULL ((void *)0)\n"
                       "void *v1 = NULL;", expr(nullPointerConstant())));
   EXPECT_TRUE(matches("void *v2 = nullptr;", expr(nullPointerConstant())));
   EXPECT_TRUE(matches("void *v3 = __null;", expr(nullPointerConstant())));
   EXPECT_TRUE(matches("char *cp = (char *)0;", expr(nullPointerConstant())));
   EXPECT_TRUE(matches("int *ip = 0;", expr(nullPointerConstant())));
   EXPECT_TRUE(notMatches("int i = 0;", expr(nullPointerConstant())));
 }
 
+TEST(StatementMatcher, HasReturnValue) {
+  StatementMatcher RetVal = returnStmt(hasReturnValue(binaryOperator()));
+  EXPECT_TRUE(matches("int F() { return a+b; }", RetVal));
+  EXPECT_FALSE(matches("int F() { return a; }", RetVal));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang