Add forEachArgumentWithParam AST matcher

[flx]

Summary

The new matcher allows users to provide a matcher for both the argument of a CallExpr/CxxConstructExpr a well as the ParmVarDecl of the argument.

[klimek]

include/clang/ASTMatchers/ASTMatchers.h

2825–2851 ↗

(On Diff #37680)

I don't think the handling of BoundNodesTreeBuilder is correct (writing tests to catch that will be a nice exercise ;)

The trick is that the resulting BoundNodesTreeBuilder must contain the full results of all matches.

If you look at the other forEach matchers, you can see a pattern:
a) create a new empty BoundNodesTreeBuilder 'Result'
b) for each match you try, create a new BoundNodesTreeBuilder with a copy of the current one
c) call match with that builder
d) for the result that matches, call addMatch

If you don't pass the builder in that is initialized with the current result, any subexpressions that uses equalsBoundNode will be incorrect.

Please add tests that test equalsBoundNode behavior in all cases, especially if you have multiple ones where everything matches, but then the equalsBoundNode fails.

[flx]

I think I'm not yet fully clean on the semantics of matching bound nodes. Do we want to allow that also between the argument matcher and the param matcher by copying the matches between the two matchers or do we only support matches to bound nodes that are already matched before forEachArgumetWithParam is invoked?

I'm also not quite sure how write these tests. Looking at some of the other forEach matchers (SwitchCase, ContructorInitializer) I don't see examples I could pattern mine after.

I updated the change to copy the Builder.

[klimek]

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

[sbenza]

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

include/clang/ASTMatchers/ASTMatchers.h

2837 ↗

(On Diff #37872)

use isa<> instead of dyn_cast<> if you don't care about the pointer itself.

Also, you could fold that check into the matcher itself using expr(anyOf(cxxConstructExpr(...), callExpr(...)))

[alexfh]

What's the state here?

[flx]

In D13845#271102, @klimek wrote:

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

Thanks. I've added that test case and am counting 4 matches, two where 'a' is used as argument and two for 'b'.

Let me know if that is sufficient whether you can think of more test cases.

In D13845#271102, @klimek wrote:

The tests to look at are the equalsBoundNode matchers (because it mainly makes a difference there). Especially take a look at FiltersMatchedCombinations.

The question is what we want to happen for things like:
void g(int x, int y);

void f() {
  int a;
  int b;
  int c;
  g(a, 0);
  g(a, b);
  b(0, b);
}

functionDecl(
  forEachDescendant(varDecl().bind("v")),
  forEachDescendant(callExpr(forEachArgumentWithParam(declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl()))))

The trick is that we currently want all combinations to match (and no match with 'c' bound to 'v').
Does that make sense?

Thanks for the example. I created a test case based on it. Let me know if any other cases are still missing.

include/clang/ASTMatchers/ASTMatchers.h

2837 ↗

(On Diff #37872)

Thanks, done!

[alexfh]

In D13845#271378, @sbenza wrote:

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

[alexfh]

Sam, Manuel, any other concerns?

[flx]

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the first 0-th argument is actually the implicit object argument and the corresponding matcher should not be applied against parameter 0 which belongs to argument 0.

To handle this I'm incrementing the argument index by one in this case.

[flx]

Sorry, I uploaded the wrong patch. This is fixed now.

[flx]

In D13845#315523, @alexfh wrote:

In D13845#271378, @sbenza wrote:

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

Updated patch with this.

In D13845#315523, @alexfh wrote:

In D13845#271378, @sbenza wrote:

Add the new matcher to ASTMatchers/Dynamic/Registry.cpp to make it accessible to clang-query.

This is not done yet, I guess?

In D13845#318127, @flx wrote:

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the first 0-th argument is actually the implicit object argument and the corresponding matcher should not be applied against parameter 0 which belongs to argument 0.

To handle this I'm incrementing the argument index by one in this case.

Sorry, the above comment was confusing (a lot of off-by one errors). Corrected version:

I had to update the semantics of forEachArgumentWithParam further to correctly handle CXXOperatorCallExpr (a sub-class of CallExpr) for member operator calls. In this case the 0-th argument is actually the implicit object argument and Argument matcher should not be applied it. Instead argument 1 and parameter 0 should be paired.

To handle this I'm incrementing the argument index by one in this case.

[flx]

Manuel, could you take another look?

[klimek]

LG.

unittests/ASTMatchers/ASTMatchersTest.cpp

1689–1692 ↗

(On Diff #43797)

Nice!

[flx]

Manuel, could you submit the change? I'm still working on becoming a submitter.

[klimek]

include/clang/ASTMatchers/ASTMatchers.h

2899 ↗

(On Diff #43797)

Changing to unsigned due to -Wsign-compare trigger.

cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h

 AST_MATCHER_P2(FunctionDecl, hasParameter,
                unsigned, N, internal::Matcher<ParmVarDecl>,
                InnerMatcher) {
   return (N < Node.getNumParams() &&
           InnerMatcher.matches(
               *Node.getParamDecl(N), Finder, Builder));
 }
 
+/// \brief Matches all arguments and their respective ParmVarDecl.
+///
+/// Given
+/// \code
+///   void f(int i);
+///   int y;
+///   f(y);
+/// \endcode
+/// callExpr(declRefExpr(to(varDecl(hasName("y")))),
+/// parmVarDecl(hasType(isInteger())))
+///   matches f(y);
+/// with declRefExpr(...)
+///   matching int y
+/// and parmVarDecl(...)
+///   matching int i
+AST_POLYMORPHIC_MATCHER_P2(forEachArgumentWithParam,
+                           AST_POLYMORPHIC_SUPPORTED_TYPES(CallExpr,
+                                                           CXXConstructExpr),
+                           internal::Matcher<Expr>, ArgMatcher,
+                           internal::Matcher<ParmVarDecl>, ParamMatcher) {
+  BoundNodesTreeBuilder Result;
+  // The first argument of an overloaded member operator is the implicit object
+  // argument of the method which should not be matched against a parameter, so
+  // we skip over it here.
+  BoundNodesTreeBuilder Matches;
+  unsigned ArgIndex = cxxOperatorCallExpr(callee(cxxMethodDecl()))
+                              .matches(Node, Finder, &Matches)
+                          ? 1
+                          : 0;
+  int ParamIndex = 0;
+  bool Matched = false;
+  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex) {
+    BoundNodesTreeBuilder ArgMatches(*Builder);
+    if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()),
+                           Finder, &ArgMatches)) {
+      BoundNodesTreeBuilder ParamMatches(ArgMatches);
+      if (expr(anyOf(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
+                         hasParameter(ParamIndex, ParamMatcher)))),
+                     callExpr(callee(functionDecl(
+                         hasParameter(ParamIndex, ParamMatcher))))))
+              .matches(Node, Finder, &ParamMatches)) {
+        Result.addMatch(ParamMatches);
+        Matched = true;
+      }
+    }
+    ++ParamIndex;
+  }
+  *Builder = std::move(Result);
+  return Matched;
+}
+
 /// \brief Matches any parameter of a function declaration.
 ///
 /// Does not match the 'this' parameter of a method.
 ///
 /// Given
 /// \code
 ///   class X { void f(int x, int y, int z) {} };
 /// \endcode

cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp

RegistryMaps::RegistryMaps() {
   REGISTER_MATCHER(equalsBoundNode);
   REGISTER_MATCHER(equalsIntegralValue);
   REGISTER_MATCHER(explicitCastExpr);
   REGISTER_MATCHER(expr);
   REGISTER_MATCHER(exprWithCleanups);
   REGISTER_MATCHER(fieldDecl);
   REGISTER_MATCHER(floatLiteral);
   REGISTER_MATCHER(forEach);
+  REGISTER_MATCHER(forEachArgumentWithParam);
   REGISTER_MATCHER(forEachConstructorInitializer);
   REGISTER_MATCHER(forEachDescendant);
   REGISTER_MATCHER(forEachSwitchCase);
   REGISTER_MATCHER(forField);
   REGISTER_MATCHER(forStmt);
   REGISTER_MATCHER(friendDecl);
   REGISTER_MATCHER(functionDecl);
   REGISTER_MATCHER(functionTemplateDecl);

cfe/trunk/unittests/ASTMatchers/ASTMatchersTest.cpp

 TEST(Matcher, AnyArgument) {
   StatementMatcher CallArgumentY = callExpr(
       hasAnyArgument(declRefExpr(to(varDecl(hasName("y"))))));
   EXPECT_TRUE(matches("void x(int, int) { int y; x(1, y); }", CallArgumentY));
   EXPECT_TRUE(matches("void x(int, int) { int y; x(y, 42); }", CallArgumentY));
   EXPECT_TRUE(notMatches("void x(int, int) { x(1, 2); }", CallArgumentY));
 }
 
+TEST(ForEachArgumentWithParam, ReportsNoFalsePositives) {
+  StatementMatcher ArgumentY =
+      declRefExpr(to(varDecl(hasName("y")))).bind("arg");
+  DeclarationMatcher IntParam = parmVarDecl(hasType(isInteger())).bind("param");
+  StatementMatcher CallExpr =
+      callExpr(forEachArgumentWithParam(ArgumentY, IntParam));
+
+  // IntParam does not match.
+  EXPECT_FALSE(matches("void f(int* i) { int* y; f(y); }", CallExpr));
+  // ArgumentY does not match.
+  EXPECT_FALSE(matches("void f(int i) { int x; f(x); }", CallExpr));
+}
+
+TEST(ForEachArgumentWithParam, MatchesCXXMemberCallExpr) {
+  StatementMatcher ArgumentY =
+      declRefExpr(to(varDecl(hasName("y")))).bind("arg");
+  DeclarationMatcher IntParam = parmVarDecl(hasType(isInteger())).bind("param");
+  StatementMatcher CallExpr =
+      callExpr(forEachArgumentWithParam(ArgumentY, IntParam));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "struct S {"
+      "  const S& operator[](int i) { return *this; }"
+      "};"
+      "void f(S S1) {"
+      "  int y = 1;"
+      "  S1[y];"
+      "}",
+      CallExpr, new VerifyIdIsBoundTo<ParmVarDecl>("param", 1)));
+}
+
+TEST(ForEachArgumentWithParam, MatchesCallExpr) {
+  StatementMatcher ArgumentY =
+      declRefExpr(to(varDecl(hasName("y")))).bind("arg");
+  DeclarationMatcher IntParam = parmVarDecl(hasType(isInteger())).bind("param");
+  StatementMatcher CallExpr =
+      callExpr(forEachArgumentWithParam(ArgumentY, IntParam));
+
+  EXPECT_TRUE(
+      matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+                               new VerifyIdIsBoundTo<ParmVarDecl>("param")));
+  EXPECT_TRUE(
+      matchAndVerifyResultTrue("void f(int i) { int y; f(y); }", CallExpr,
+                               new VerifyIdIsBoundTo<DeclRefExpr>("arg")));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "void f(int i, int j) { int y; f(y, y); }", CallExpr,
+      new VerifyIdIsBoundTo<ParmVarDecl>("param", 2)));
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "void f(int i, int j) { int y; f(y, y); }", CallExpr,
+      new VerifyIdIsBoundTo<DeclRefExpr>("arg", 2)));
+}
+
+TEST(ForEachArgumentWithParam, MatchesConstructExpr) {
+  StatementMatcher ArgumentY =
+      declRefExpr(to(varDecl(hasName("y")))).bind("arg");
+  DeclarationMatcher IntParam = parmVarDecl(hasType(isInteger())).bind("param");
+  StatementMatcher ConstructExpr =
+      cxxConstructExpr(forEachArgumentWithParam(ArgumentY, IntParam));
+
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "struct C {"
+      "  C(int i) {}"
+      "};"
+      "int y = 0;"
+      "C Obj(y);",
+      ConstructExpr, new VerifyIdIsBoundTo<ParmVarDecl>("param")));
+}
+
+TEST(ForEachArgumentWithParam, HandlesBoundNodesForNonMatches) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "void g(int i, int j) {"
+      "  int a;"
+      "  int b;"
+      "  int c;"
+      "  g(a, 0);"
+      "  g(a, b);"
+      "  g(0, b);"
+      "}",
+      functionDecl(
+          forEachDescendant(varDecl().bind("v")),
+          forEachDescendant(callExpr(forEachArgumentWithParam(
+              declRefExpr(to(decl(equalsBoundNode("v")))), parmVarDecl())))),
+      new VerifyIdIsBoundTo<VarDecl>("v", 4)));
+}
+
 TEST(Matcher, ArgumentCount) {
   StatementMatcher Call1Arg = callExpr(argumentCountIs(1));
 
   EXPECT_TRUE(matches("void x(int) { x(0); }", Call1Arg));
   EXPECT_TRUE(matches("class X { void x(int) { x(0); } };", Call1Arg));
   EXPECT_TRUE(notMatches("void x(int, int) { x(0, 0); }", Call1Arg));
 }