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

[Stephen Kelly via cfe-dev]

On 27/05/2020 15:24, Sam McCall wrote:
> Hi Stephen,
>
> First off: I think we've mitigated this enough on our end that we're 
> not in immediate need of changes beyond fixing the problems found in 
> Transformers.
> Thanks for being so engaged on this, it helped a lot clarifying our 
> options and I think we can help make this smoother for adopters when 
> this gets released.


Great!


> I've also had a chance to sync internally with Dmitri, Haojian, 
> Yitzhak and Manuel and get their perspectives too.
>
> Some brief notes on our local fixes:
> Haojian wrote a (matcher/transformer-based!) tool to identify the 
> "roots" where matchers were passed into consuming clang APIs and 
> mechanically wrapped them with traverse() at that point.


Sounds good. There's something MC-Escher-like pleasing about that.


> This was applied across the codebase, whether needed to make tests 
> pass or not. We have no plans to systematically remove AsIs from these 
> - tool owners can do this (or not) as part of maintenance.
> We chose this approach because:
>  - there was preference for expressing this subtle difference next to 
> the matcher logic rather than "burying" it in tool framework stuff. 
> Otherwise there's little prospect of ever using the new mode, and 
> copy/pasting matchers would give mysteriously different results.


Yes, I listed the "burying" method as something small and quick that 
would be quick to unblock others, but not a long-term solution.


>  - it was amenable to automated refactoring and thus scaling (more-so 
> than modifying tool entry points) because there were a few 
> identifiable points to instrument.
>  - applying it to everything meant not worrying about whether tests 
> were adequate
>  - updating all tools to use the new traversal mode probably isn't a 
> good use of time due to the long tail.
>
> There were ~40 direct uses of ASTContext::getParents(), whose 
> behaviour was also changed, and these were significantly more work to 
> adapt. (Apparently. I wasn't the one doing the work :-))
> I suppose the trivial/safe fix is to set and restore the mode before 
> each call, but this seems pretty gross - this doesn't seem like 
> something that should require mutating global state.


Yes, I agree it has a bit of a smell. There is a TraversalKindScope RAII 
type for use in a few checks as it was only needed in `::check` 
implementations.


>
> How do you feel about making the old behaviour and the 
> traversal-mode-respecting version of this function both reasonably 
> available, with matchers calling the new one?
> I don't have a strong opinion on naming/structure, but would prefer 
> existing getParents() calls to either get the existing behaviour or a 
> build break forcing a choice.


That sounds fine to me.


>
> We had some discussion of the direction of the change overall (I know 
> it's late for this). Making matchers easier is great. Some concern 
> that you really have to know the AST to use matchers effectively

[Side note:

This is something of a side-note to this thread, but I don't think you 
do need to be familiar with AST structure in order to use AST Matchers. 
At least not in the future.

Consider

  https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590

versus

  https://steveire.files.wordpress.com/2018/11/allenabled.png?w=900&h=590

The latter is what we have today. The former does not require users to 
be familiar with the AST because clang-query can tell you what matchers 
and source locations you can use with binding results:

  http://ce.steveire.com/z/Kbk8Yj

I demo'd these clang-query features in November 2018 but I haven't been 
able to get the patches upstream and I haven't seen interest or drive 
from the community in getting the patches actually approved unfortunately.

  https://steveire.wordpress.com/2019/01/02/refactor-with-clang-tooling-at-codedive-2018/

If you have a declarative debugger for your matchers you don't need to 
debug via an trip through the AST either:

  http://ce.steveire.com/z/Miznm_

I demo'd that one at EuroLLVM 2019 along with a Proof of Concept Qt UI 
for this stuff: 
https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching-refactoring-tools-eurollvm-and-accu/

It is definitely not necessary for novices to see an AST dump or to be 
very familiar with it before starting. It might be the current state in 
LLVM upstream, but it hasn't needed to be true for almost 1.5 years, 
depending on how you see it :).

There is a huge amount more that we could do to improve mechanical 
refactoring tools for novices and others. But I haven't been able to 
drive that forward because I can't get my changes reviewed upstream in a 
timeframe that reflects a drive from upstream to get them in. That means 
I can't stay engaged and I have to disappear for months. Better is 
possible :).

If you or someone you know have interest in any of this and have drive 
to actually get them upstream (this is the critical part - the desire to 
approve the patches) please let me know.

End Side Note]

> , and this might give the illusion you only need to know syntax. But 
> some pushback against this idea: ignoreImplicit.... is a hassle even 
> for experienced people.

Yes.

Thanks,

Stephen

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200527/bc224f70/attachment.html>