Page MenuHomePhabricator

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

Authored by george.karpenkov on Mar 20 2018, 2:50 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

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.

@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?..)

george.karpenkov marked an inline comment as done.
aaron.ballman added inline comments.Mar 21 2018, 5:14 AM
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 marked 2 inline comments as done.
include/clang/ASTMatchers/ASTMatchers.h
3469 ↗(On Diff #139204)

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

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.

aaron.ballman added inline comments.Mar 21 2018, 12:40 PM
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 marked 2 inline comments as done.
george.karpenkov added inline comments.
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 accepted this revision.Mar 21 2018, 2:13 PM

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.

This revision is now accepted and ready to land.Mar 21 2018, 2:13 PM

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

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

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?

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 added inline comments.Mar 22 2018, 12:21 PM
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?

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 added inline comments.Mar 22 2018, 12:27 PM
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!

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:
...
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 added inline comments.Mar 22 2018, 12:38 PM
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?

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.

@alexfh This patch looks reasonable to you?

This revision was automatically updated to reflect the committed changes.
alexfh added a comment.Apr 6 2018, 5:15 AM

@alexfh This patch looks reasonable to you?

Yep, no concerns from me.