[cfe-dev] RFC: Easier AST Matching by Default

[David Rector via cfe-dev]

>>       ```
>>        struct B
>>        {
>>          B(int);
>>        };
>> 
>>        B func1() {
>>          return 42;
>>        }
>>        ```
>> 
>>        a matcher like `returnStmt(hasReturnValue(fooExpr()))`
>> 
>>        would match for `fooExpr` as any of
>> 
>> 
>>        * exprWithCleanups()
>>        * cxxConstructExpr() (twice)
>>        * materializeTemporaryExpr()
>>        * implicitCastExpr()
>>        * integerLiteral()
>> 
>>        right?
>> 
>>    Yes.
>> 
>>        and a matcher like
>>        `returnStmt(hasReturnValue(expr().bind("theReturnVal")))`
>> 
>>        would bind "theReturnVal" to the `exprWithCleanups()` right?
>> 
>>        Your suggestion may be that it should bind to the
>>        integerLiteral() in that case. That may have more scope for
>>        confusion though?
>> 
>>    I think that's an interesting question. My inclination would be to
>>    bind to the ExprWithCleanups.
> 
> 
> The main reason I'm not supportive of this idea is that I want to make 
> clang-query tell the user what matchers they can use with the nodes in 
> the binding list.
> 
> For example, if I write
> 
>  returnStmt()
> 
> in clang-query it should tell me that I can write
> 
>  hasReturnValue(integerLiteral())
> 
> inside the expr():
> 
>  http://ce.steveire.com/z/9EF8x0
> 
> That's how I as a newcomer quickly discover hasReturnValue() and 
> integerLiteral().
> 
> You have no idea how overwhelming
> 
>  http://clang.llvm.org/docs/LibASTMatchersReference.html
> 
> is to someone who has zero familiarity with the clang AST.
> 
> But the above link is interactive and it aids *discovery* which is what 
> my talk was about and which is what I'm trying to make easier with AST 
> Matchers. The above suggested clang-query feature (not upstream yet) 
> tells me at every step which matchers I can use in the context of what I 
> am trying to achieve.
> 
> However, if all of the implicit nodes should also appear in that output, 
> and if in
> 
> `returnStmt(hasReturnValue(expr().bind("theReturnVal")))`
> 
> the expr() should match the exprWithCleanups(), then I don't know how to 
> make that feature work. If the expr() is the exprWithCleanups() then the 
> tool definitely shouldn't tell the user they can write integerLiteral() 
> there. The output for implicit nodes would add lots of noise and would 
> have to be neutral in terms of what it recommends.
> 
> The entire point of what I'm trying to do is not present the user with 
> exprWithCleanups() and let them achieve their goals without it. I don't 
> know if that's possible with the ideas floating around at the moment 
> about making AST Matchers present all of the implicit nodes.
> 
> But, if making IgnoreUnlessSpelledInSource non-default means that I can 
> continue work toward that goal (and eg ignore template instantiations 
> and other implicit nodes which are not skipped yet), then maybe that's a 
> viable way forward. So, that's my preference now I think.
> 
> That way, people who want expert mode get it by default, and newcomers 
> (and anyone else who doesn't want to write ignoringImplicit() 
> everywhere) can relatively easily get the easier mode, and can use the 
> expert mode when wanting to match implicit nodes.
> 
> That might be the best compromise.
> 


struct B { B(int); }
B func1() { return 42; }

Perhaps fatigue is setting in, but I think you’ve raised a good point and it would be nice to solve it eventually: users really should be able to treat the 42 as an integer literal without fuss.  On the other hand experts should also be able to treat it as an ExprWCleanups without fuss.  

It sounds like a problem that would be resolved by inheritance: 42 is an integer literal, and it also is an ExprWCleanups — no conflict.

That’s not an option here, but I still think we might solve it by mimicing inheritance via a getAs<T>() method similar to that in Type: e.g. TemplateSpecializationType does not inherit from RecordType, but seems to “be" one, and can become it via TST->getAs<RecordType>().

To implement it in Stmt, I think this what would need to be done:

1) separate out Implicit versions of any Stmt nodes which can be implicit, so that every Stmt leaf node is either implicit or not
	- e.g. you’d have WrittenCXXConstructExpr : CXXConstructExpr and ImplicitCXXConstructExpr : CXXConstructExpr.
	  `-Code depending only on CXXConstructExpr would still work without change, so minimal “churn” in the interface

2) Add a getAs<T>() method in Stmt.  It would be very simple since all Stmts already have children: 

class Stmt {
…
	template<typename T>
	T *getAs() {
		if (isa<T>(this))
			return cast<T>(this);
		if (isImplicitStmtKind(getStmtClass())) {
			assert(children().size() == 1 && “Expected implicit nodes to have exactly one child”);
			return (*children.begin())->getAs<T>();
		}
		return nullptr;
	}
};

3) Then, in ASTMatchers, use ...->getAs<T>() in getNodeAs<T>() and wherever the user requests to do something if a node “is a” T.  The user who wants to getNodeAs<IntegerLiteral>() when it’s really an ExprWCleanups will still get what they want.  The expert who wants the ExprWCleanups could get that too.  You could also getAs<CXXConstructExpr>(), and the result might be implicit or written — or you could specify getAs<WrittenCXXConstructExpr>().  No expert vs. beginner / syntax vs. semantic options to juggle.

I am probably missing something though.

However this is resolved I admire and appreciate your efforts on behalf of newcomers, Stephen.  Richard also has offered very clear and educational commentary. A good back and forth.  This has finally exposed me to ASTMatchers — I’ll have to try to include it in the next iteration of my clang reflection fork.  Wouldn’t it be nice to run these matchers within consteval functions acting on reflected meta::clang::Stmt*s etc. at compile time, write them into templates so they run on each instantiation, no need to write external tools.  For another time.

Dave