[ASTMatchers] Extend hasParameter and hasAnyParameter matches to handle Objective-C methods

[aaron.ballman]

You should also regenerate the AST matcher documentation by running docs/tools/dump_ast_matchers.py

include/clang/ASTMatchers/ASTMatchers.h

3469 ↗

(On Diff #139204)

Please add an ObjC example below.

3543 ↗

(On Diff #139204)

Same here.

[george.karpenkov]

@aaron.ballman OK.

BTW do you know how to fix the following:

clang-query> match anyOf( objcMethodDecl(), functionDecl() )
Input value has unresolved overloaded type: Matcher<Decl>&Matcher<Decl>

The underlying problem is that the only base class they have in common is Decl (though I'm not sure why is it a problem?..)

[aaron.ballman]

include/clang/ASTMatchers/ASTMatchers.h

3486 ↗

(On Diff #139239)

This statement isn't quite correct -- it matches the objcMethodDecl() with hasAnyArgument(...) matching int y.

However, why is this calling hasAnyArgument instead of hasParameter?

3543 ↗

(On Diff #139204)

Should add a code example for hasAnyParameter.

unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

572 ↗

(On Diff #139239)

You should also have a test for hasParameter().

[george.karpenkov]

include/clang/ASTMatchers/ASTMatchers.h

3496 ↗

(On Diff #139341)

Yes, this is ugly, and I do plan to eventually introduce a common superclass, so we could enforce same name for same operations.

3469 ↗

(On Diff #139204)

Decided to revert this block, getting it right is a bit tricky, and I do not need that right now.

[aaron.ballman]

include/clang/ASTMatchers/ASTMatchers.h

3496 ↗

(On Diff #139341)

Both classes have a parameters() member function that returns an ArrayRef<ParmVarDecl *>, so I think this could be factored to use that function in both cases and have common count checking.

lib/Analysis/BodyFarm.cpp

317 ↗

(On Diff #139343)

This change looks unrelated to the patch.

[george.karpenkov]

include/clang/ASTMatchers/ASTMatchers.h

3504 ↗

(On Diff #139343)

For now don't know how to avoid this ugliness without a huge refactoring...

3496 ↗

(On Diff #139341)

👏 Thanks! Somehow completely missed that.

[aaron.ballman]

LGTM with a minor doc nit.

docs/LibASTMatchersReference.html

5336–5337 ↗

(On Diff #139367)

The doxygen use here seems to be causing fits for the generated docs. You can drop its use in these cases.

[george.karpenkov]

@aaron.ballman turns out larger changes for dynamic tests were required, could you take a brief look?

[george.karpenkov]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

the problem here is that it's not a dyn_typed_matcher anymore.
There's a seemingly related code in Marshallers.h constructing VariantMatcher::PolymorphicMatcher out of what seems to be a polymorphic matcher, but it's not accessible from here.

336 ↗

(On Diff #139383)

seems that the order is not actually fixed

[aaron.ballman]

Adding some folks to review the dynamic matcher bits.

include/clang/StaticAnalyzer/Core/PathSensitive/WorkList.h

90 ↗

(On Diff #139383)

This change looks unrelated (but would be fine to commit on its own without waiting for review).

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

I'm not as familiar with the dynamic matcher infrastructure as I am other parts of the matchers; can you give a bit more background on what's happening here?

[george.karpenkov]

include/clang/StaticAnalyzer/Core/PathSensitive/WorkList.h

90 ↗

(On Diff #139383)

yeah somehow i always screw up the exact thing I am meant to sent to the diff, thanks!

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

@aaron.ballman OK.
Dynamic matcher kicks in clang-query where the query is typed as a string, and then libASTMatcher has to parse it itself, instead of relying on crazy C++ templates.
In this case, hasParameter is no longer a dyn_typed_matcher, so SingleMatcher can not be constructed out of it.
The construction is used to store matchers in a kind of "registry" which is then tested (for subsequent queries and autocompletion).
The change itself is pretty trivial.

[aaron.ballman]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

Hmm, so will this break behavior for clang-query users where their use of hasParameter() is now ambiguous? Or are these changes only needed because we're not really being fully dynamic in the unit tests like we are with clang-query directly?

[george.karpenkov]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

From what I have tried, hasParameter works in clang-query as before (just as hasArgument does).
I think the problem is in unit tests, as they try to save this object to a type which does not (directly) support that.

[aaron.ballman]

include/clang/StaticAnalyzer/Core/PathSensitive/WorkList.h

90 ↗

(On Diff #139383)

It happens to all of us. :-)

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

Okay, that makes sense to me. Then this continues to LGTM, thank you for double-checking!

[george.karpenkov]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

From what I have tried, hasParameter works in clang-query as before (just as hasArgument does).

@aaron.ballman
Actually sorry, this was incorrect. Yes, it is breaking:

clang-query> match hasParameter(0, parmVarDecl())
Input value has unresolved overloaded type: Matcher<FunctionDecl|ObjCMethodDecl>

The previous behavior can easily obtained though:

clang-query> match functionDecl(hasParameter(0, parmVarDecl()))

Match #1:
...

[george.karpenkov]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

The lack of a reasonable common supertype for functionDecl and objcMethodDecl is very annoying.

Strangely, this does not work:

clang-query> match decl(hasParameter(0, parmVarlDec))
1:2: Error building matcher decl.
1:7: Incorrect type for arg 1. (Expected = Matcher<Decl>) != (Actual = Matcher<FunctionDecl|ObjCMethodDecl>)

So we have to go through the hoops:

clang-query> match decl(anyOf(objcMethodDecl(hasParameter(0, parmVarDecl())), functionDecl(hasParameter(0, parmVarDecl()))))

[aaron.ballman]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

Ah, that's what I was afraid of. However, because this is a traversal matcher, I have a hard time imagining that anyone uses the matcher in this way because there would be no current node to traverse *from* in a top-level hasParameter() match.

I think the behavior change will be reasonable. What do you think, @sbenza and @klimek?

[george.karpenkov]

unittests/ASTMatchers/Dynamic/ParserTest.cpp

189 ↗

(On Diff #139383)

I'm also sort of puzzled why the typechecker can not figure out a common superclass. Maybe just in order to be consistent with static behavior.

[george.karpenkov]

@alexfh This patch looks reasonable to you?

[alexfh]

In D44707#1048629, @george.karpenkov wrote:

@alexfh This patch looks reasonable to you?

Yep, no concerns from me.

cfe/trunk/docs/LibASTMatchersReference.html

 Example:
     forStmt(hasLoopInit(declStmt()))
 matches 'int x = 0' in
     for (int x = 0; x < N; ++x) { }
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('hasAnyParameter0')"><a name="hasAnyParameter0Anchor">hasAnyParameter</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1ParmVarDecl.html">ParmVarDecl</a>&gt; InnerMatcher</td></tr>
-<tr><td colspan="4" class="doc" id="hasAnyParameter0"><pre>Matches any parameter of a function declaration.
+<tr><td colspan="4" class="doc" id="hasAnyParameter0"><pre>Matches any parameter of a function or ObjC method declaration.
 
 Does not match the 'this' parameter of a method.
 
 Given
   class X { void f(int x, int y, int z) {} };
 cxxMethodDecl(hasAnyParameter(hasName("y")))
   matches f(int x, int y, int z) {}
 with hasAnyParameter(...)
   matching int y
+
+For ObjectiveC, given
+  @interface I - (void) f:(int) y; @end
+
+the matcher objcMethodDecl(hasAnyParameter(hasName("y")))
+matches the declaration of method f with hasParameter
+matching y.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('hasAnyTemplateArgument2')"><a name="hasAnyTemplateArgument2Anchor">hasAnyTemplateArgument</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1TemplateArgument.html">TemplateArgument</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="hasAnyTemplateArgument2"><pre>Matches classTemplateSpecializations, templateSpecializationType and
 functionDecl that have at least one TemplateArgument matching the given
 InnerMatcher.
 

 hasBody(compoundStmt())
   matches 'for (;;) {}'
 with compoundStmt()
   matching '{}'
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('hasParameter0')"><a name="hasParameter0Anchor">hasParameter</a></td><td>unsigned N, Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1ParmVarDecl.html">ParmVarDecl</a>&gt; InnerMatcher</td></tr>
-<tr><td colspan="4" class="doc" id="hasParameter0"><pre>Matches the n'th parameter of a function declaration.
+<tr><td colspan="4" class="doc" id="hasParameter0"><pre>Matches the n'th parameter of a function or an ObjC method
+declaration.
 
 Given
   class X { void f(int x) {} };
 cxxMethodDecl(hasParameter(0, hasType(varDecl())))
   matches f(int x) {}
 with hasParameter(...)
   matching int x
+
+For ObjectiveC, given
+  @interface I - (void) f:(int) y; @end
+
+the matcher objcMethodDecl(hasParameter(0, hasName("y")))
+matches the declaration of method f with hasParameter
+matching y.
 </pre></td></tr>
 
 
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('hasTemplateArgument2')"><a name="hasTemplateArgument2Anchor">hasTemplateArgument</a></td><td>unsigned N, Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1TemplateArgument.html">TemplateArgument</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="hasTemplateArgument2"><pre>Matches classTemplateSpecializations, templateSpecializationType and
 functionDecl where the n'th TemplateArgument matches the given InnerMatcher.
 
 Given

 matcher = objCMessageExpr(hasReceiverType(asString("UIWebView *")));
 matches the [webView ...] message invocation.
   NSString *webViewJavaScript = ...
   UIWebView *webView = ...
   [webView stringByEvaluatingJavaScriptFromString:webViewJavascript];
 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html">ObjCMethodDecl</a>&gt;</td><td class="name" onclick="toggle('hasAnyParameter1')"><a name="hasAnyParameter1Anchor">hasAnyParameter</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1ParmVarDecl.html">ParmVarDecl</a>&gt; InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="hasAnyParameter1"><pre>Matches any parameter of a function or ObjC method declaration.
+
+Does not match the 'this' parameter of a method.
+
+Given
+  class X { void f(int x, int y, int z) {} };
+cxxMethodDecl(hasAnyParameter(hasName("y")))
+  matches f(int x, int y, int z) {}
+with hasAnyParameter(...)
+  matching int y
+
+For ObjectiveC, given
+  @interface I - (void) f:(int) y; @end
+
+the matcher objcMethodDecl(hasAnyParameter(hasName("y")))
+matches the declaration of method f with hasParameter
+matching y.
+</pre></td></tr>
+
+
+<tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html">ObjCMethodDecl</a>&gt;</td><td class="name" onclick="toggle('hasParameter1')"><a name="hasParameter1Anchor">hasParameter</a></td><td>unsigned N, Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1ParmVarDecl.html">ParmVarDecl</a>&gt; InnerMatcher</td></tr>
+<tr><td colspan="4" class="doc" id="hasParameter1"><pre>Matches the n'th parameter of a function or an ObjC method
+declaration.
+
+Given
+  class X { void f(int x) {} };
+cxxMethodDecl(hasParameter(0, hasType(varDecl())))
+  matches f(int x) {}
+with hasParameter(...)
+  matching int x
+
+For ObjectiveC, given
+  @interface I - (void) f:(int) y; @end
+
+the matcher objcMethodDecl(hasParameter(0, hasName("y")))
+matches the declaration of method f with hasParameter
+matching y.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1OpaqueValueExpr.html">OpaqueValueExpr</a>&gt;</td><td class="name" onclick="toggle('hasSourceExpression1')"><a name="hasSourceExpression1Anchor">hasSourceExpression</a></td><td>Matcher&lt;<a href="http://clang.llvm.org/doxygen/classclang_1_1Expr.html">Expr</a>&gt; InnerMatcher</td></tr>
 <tr><td colspan="4" class="doc" id="hasSourceExpression1"><pre>Matches if the cast's source expression
 or opaque value's source expression matches the given matcher.
 
 Example 1: matches "a string"
 (matcher = castExpr(hasSourceExpression(cxxConstructExpr())))
 class URL { URL(string); };
 URL url = "a string";

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

 /// }
 /// \endcode
 /// initListExpr(has(cxxConstructExpr(requiresZeroInitialization()))
 /// will match the implicit array filler for pt[1].
 AST_MATCHER(CXXConstructExpr, requiresZeroInitialization) {
   return Node.requiresZeroInitialization();
 }
 
-/// \brief Matches the n'th parameter of a function declaration.
+/// \brief Matches the n'th parameter of a function or an ObjC method
+/// declaration.
 ///
 /// Given
 /// \code
 ///   class X { void f(int x) {} };
 /// \endcode
 /// cxxMethodDecl(hasParameter(0, hasType(varDecl())))
 ///   matches f(int x) {}
 /// with hasParameter(...)
 ///   matching int x
-AST_MATCHER_P2(FunctionDecl, hasParameter,
+///
+/// For ObjectiveC, given
+/// \code
+///   @interface I - (void) f:(int) y; @end
+/// \endcode
+//
+/// the matcher objcMethodDecl(hasParameter(0, hasName("y")))
+/// matches the declaration of method f with hasParameter
+/// matching y.
+AST_POLYMORPHIC_MATCHER_P2(hasParameter,
+                           AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+                                                           ObjCMethodDecl),
                            unsigned, N, internal::Matcher<ParmVarDecl>,
                            InnerMatcher) {
-  return (N < Node.getNumParams() &&
-          InnerMatcher.matches(
-              *Node.getParamDecl(N), Finder, Builder));
+  return (N < Node.parameters().size()
+          && InnerMatcher.matches(*Node.parameters()[N], Finder, Builder));
 }
 
 /// \brief Matches all arguments and their respective ParmVarDecl.
 ///
 /// Given
 /// \code
 ///   void f(int i);
 ///   int y;
if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()),
       }
     }
     ++ParamIndex;
   }
   *Builder = std::move(Result);
   return Matched;
 }
 
-/// \brief Matches any parameter of a function declaration.
+/// \brief Matches any parameter of a function or ObjC method declaration.
 ///
 /// Does not match the 'this' parameter of a method.
 ///
 /// Given
 /// \code
 ///   class X { void f(int x, int y, int z) {} };
 /// \endcode
 /// cxxMethodDecl(hasAnyParameter(hasName("y")))
 ///   matches f(int x, int y, int z) {}
 /// with hasAnyParameter(...)
 ///   matching int y
-AST_MATCHER_P(FunctionDecl, hasAnyParameter,
-              internal::Matcher<ParmVarDecl>, InnerMatcher) {
+///
+/// For ObjectiveC, given
+/// \code
+///   @interface I - (void) f:(int) y; @end
+/// \endcode
+//
+/// the matcher objcMethodDecl(hasAnyParameter(hasName("y")))
+/// matches the declaration of method f with hasParameter
+/// matching y.
+AST_POLYMORPHIC_MATCHER_P(hasAnyParameter,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+                                                          ObjCMethodDecl),
+                          internal::Matcher<ParmVarDecl>,
+                          InnerMatcher) {
   return matchesFirstInPointerRange(InnerMatcher, Node.param_begin(),
                                     Node.param_end(), Finder, Builder);
 }
 
 /// \brief Matches \c FunctionDecls and \c FunctionProtoTypes that have a
 /// specific parameter count.
 ///
 /// Given

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/WorkList.h

public:
 
   static std::unique_ptr<WorkList> makeDFS();
   static std::unique_ptr<WorkList> makeBFS();
   static std::unique_ptr<WorkList> makeBFSBlockDFSContents();
   static std::unique_ptr<WorkList> makeUnexploredFirst();
   static std::unique_ptr<WorkList> makeUnexploredFirstPriorityQueue();
 };
 
-} // end GR namespace
+} // end ento namespace
 
 } // end clang namespace
 
 #endif

cfe/trunk/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

matches("typedef int &int_ref;"
             varDecl(hasType(qualType(hasCanonicalType(referenceType()))))));
 }
 
 TEST(HasParameter, CallsInnerMatcher) {
   EXPECT_TRUE(matches("class X { void x(int) {} };",
                       cxxMethodDecl(hasParameter(0, varDecl()))));
   EXPECT_TRUE(notMatches("class X { void x(int) {} };",
                          cxxMethodDecl(hasParameter(0, hasName("x")))));
+  EXPECT_TRUE(matchesObjC("@interface I -(void)f:(int) x; @end",
+                          objcMethodDecl(hasParameter(0, hasName("x")))));
 }
 
 TEST(HasParameter, DoesNotMatchIfIndexOutOfBounds) {
   EXPECT_TRUE(notMatches("class X { void x(int) {} };",
                          cxxMethodDecl(hasParameter(42, varDecl()))));
 }
 
 TEST(HasType, MatchesParameterVariableTypesStrictly) {

 
 TEST(HasAnyParameter, MatchesIndependentlyOfPosition) {
   EXPECT_TRUE(matches(
     "class Y {}; class X { void x(X x, Y y) {} };",
     cxxMethodDecl(hasAnyParameter(hasType(recordDecl(hasName("X")))))));
   EXPECT_TRUE(matches(
     "class Y {}; class X { void x(Y y, X x) {} };",
     cxxMethodDecl(hasAnyParameter(hasType(recordDecl(hasName("X")))))));
+  EXPECT_TRUE(matchesObjC("@interface I -(void)f:(int) x; @end",
+                          objcMethodDecl(hasAnyParameter(hasName("x")))));
 }
 
 TEST(Returns, MatchesReturnTypes) {
   EXPECT_TRUE(matches("class Y { int f() { return 1; } };",
                       functionDecl(returns(asString("int")))));
   EXPECT_TRUE(notMatches("class Y { int f() { return 1; } };",
                          functionDecl(returns(asString("float")))));
   EXPECT_TRUE(matches("class Y { Y getMe() { return *this; } };",

cfe/trunk/unittests/ASTMatchers/Dynamic/ParserTest.cpp

TEST(ParserTest, ParseMatcher) {
   EXPECT_EQ("Yo!", Foo.BoundID);
 }
 
 using ast_matchers::internal::Matcher;
 
 Parser::NamedValueMap getTestNamedValues() {
   Parser::NamedValueMap Values;
   Values["nameX"] = llvm::StringRef("x");
-  Values["hasParamA"] =
-      VariantMatcher::SingleMatcher(hasParameter(0, hasName("a")));
+  Values["hasParamA"] = VariantMatcher::SingleMatcher(
+      functionDecl(hasParameter(0, hasName("a"))));
   return Values;
 }
 
 TEST(ParserTest, FullParserTest) {
   Diagnostics Error;
   llvm::Optional<DynTypedMatcher> VarDecl(Parser::parseMatcherExpression(
       "varDecl(hasInitializer(binaryOperator(hasLHS(integerLiteral()),"
       "                                      hasOperatorName(\"+\"))))",
TEST(ParserTest, CompletionNamedValues) {
   EXPECT_EQ("String nameX", Comps[0].MatcherDecl);
 
   // Can complete if there are names in the expression.
   Code = "cxxMethodDecl(hasName(nameX), ";
   Comps = Parser::completeExpression(Code, Code.size(), nullptr, &NamedValues);
   EXPECT_LT(0u, Comps.size());
 
   // Can complete names and registry together.
-  Code = "cxxMethodDecl(hasP";
+  Code = "functionDecl(hasP";
   Comps = Parser::completeExpression(Code, Code.size(), nullptr, &NamedValues);
   ASSERT_EQ(3u, Comps.size());
-  EXPECT_EQ("aramA", Comps[0].TypedText);
-  EXPECT_EQ("Matcher<FunctionDecl> hasParamA", Comps[0].MatcherDecl);
 
-  EXPECT_EQ("arameter(", Comps[1].TypedText);
+  EXPECT_EQ("arameter(", Comps[0].TypedText);
   EXPECT_EQ(
       "Matcher<FunctionDecl> hasParameter(unsigned, Matcher<ParmVarDecl>)",
-      Comps[1].MatcherDecl);
+      Comps[0].MatcherDecl);
+
+  EXPECT_EQ("aramA", Comps[1].TypedText);
+  EXPECT_EQ("Matcher<Decl> hasParamA", Comps[1].MatcherDecl);
 
   EXPECT_EQ("arent(", Comps[2].TypedText);
   EXPECT_EQ(
       "Matcher<Decl> "
       "hasParent(Matcher<NestedNameSpecifierLoc|TypeLoc|Decl|...>)",
       Comps[2].MatcherDecl);
 }
 
 }  // end anonymous namespace
 }  // end namespace dynamic
 }  // end namespace ast_matchers
 }  // end namespace clang