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

Tue Jun 2 05:50:17 PDT 2020

On Wed, May 27, 2020 at 9:21 PM Stephen Kelly via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

>
> 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.
>

I'm sorry about your experience.

>
> 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
> :).
>
For what it's worth, I'd love to see your contributions go in. The problem
is that initially that will mean lots of round trips and rewriting, which
takes time. This usually gets better over time, as you get more fluid in
how code in clang is reviewed / written, and also when you start being an
owner for the areas you've contributed enough in to be the expert on them.

> 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
> _______________________________________________
> 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/20200602/17330650/attachment.html>