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

[Yitzhak Mandelbaum via cfe-dev]

My 2cents:

I mostly agree with Richard and Sam. My only concern w/ Richard's proposal
is that the hybrid model may be even more confusing to the beginner, since
it may be harder to predict how a given matcher will match against an AST.
The system will be more forgiving, yet it becomes harder to build a mental
model of how it works.  I think we'd need to be sure we can concisely
explain how this mode works, without having to reference any
code/algorithms. For example, is there a deterministic elaboration from any
unadorned matcher to a matcher with explicit `traverse` annotations? That
would allow us to give some examples that illuminate what's happening.
Otherwise, I'd prefer we just go back to the original default for the
library. I'm fine if beginner-oriented tools want to start with
"IgnoreUnless..." as their default. It sounds like Stephen is amenable to
this approach as well from his last reply.

re: context-sensitive suggestions.
Stephen, if you drop the need to suggest all possible matchers at that
point and only provide (beginner) helpful suggestions, does your concern go
away?  Why doesn't your concern already apply to the generic matchers like
"anyOf", "hasParent", etc.

Overall, I'm pessimistic about making the current matchers easier for
users. I think the AST is challenging for beginners and band aids will not
solve that and can often make it worse. If we want beginner-friendly
tooling (and I do!) we need to admit its cost and then invest in it. I have
opinions on alternatives (like Syntax Trees, touched on above, but also
other ideas), but this is not the place.  Even for your auto-suggestions
(which I really like), I think it would be more beneficial applied to such
alternatives.

Cheers,
yitzhak

On Mon, Jun 22, 2020 at 5:51 PM Stephen Kelly via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

>
> On 21/06/2020 19:59, Richard Smith wrote:
>
>> If I understand your suggestion, given
>>>
>>>
>>> ```
>>> 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.
>
> Thanks,
>
> Stephen
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200624/fc98918c/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3854 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200624/fc98918c/attachment-0001.bin>