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

[Sam McCall via cfe-dev]

On Mon, May 25, 2020 at 11:56 PM Stephen Kelly via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> On 20/12/2019 21:01, Stephen Kelly via cfe-dev wrote:
> >
> > Hi,
> >
> > (Apologies if you receive this twice. GMail classified the first one as
> > spam)
> >
> > Aaron Ballman and I met by chance in Belfast and we discussed a way
> > forward towards making AST Matchers easier to use, particularly for C++
> > developers who are not familiar with the details of the Clang AST.
> >
> > For those unaware, I expanded on this in the EuroLLVM conference this
> > year, and then expanded on it at ACCU:
> >
> >   https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching
> >
> > One step in the process of getting there is changing the default
> > behavior of AST Matchers to ignore invisible nodes while matching using
> > the C++ API, and while matching and dumping AST nodes in clang-query.
> >
> > I think this is the most important change in the entire proposal as it
> > sets out the intention of making the AST Matchers easier to use for C++
> > developers who are not already familiar with Clang APIs.
> >
> > To that end, I've written an AST to motivate the change:
> >
> >
> >
> https://docs.google.com/document/d/17Z6gAwwc3HoRXvsy0OdwU0X5MFQEuiGeSu3i6ICOB90
> >
> >
> > We're looking for feedback before pressing forward with the change. I
> > already have some patches written to port clang-tidy and unit tests to
> > account for the change of default.
>
>
> This change is now in master.
>

Thanks for simplifying the matchers! Unfortunately I missed the discussion
on this.

Unfortunately this is causing widespread problems in downstream tools that
are difficult to track:
 - this is a prominent API that is used in many places
 - the semantics have changed subtly but pervasively, so all usage sites
need to be audited, but because there was no API syntax change (e.g. build
break), it's not trivial to find all the affected locations to audit
 - the differences between old and new only occur when running against
certain code patterns that often aren't covered by tests, therefore each
audit is a lot of work
 - because the default was flipped globally, all usage sites must be dealt
with at once
 - the "revert-to-old-behaviour" is invasive at all usage sites (hurts
readability a lot) and apparently has bugs (D80606)

In summary: it requires changes in consuming tools that are numerous, hard
to find, hard to analyze, must all be done at once, and the changes aren't
mechanically reliable.
I expect this to bite people building tools against release versions to hit
the same problems in a few months.

We should consider reverting this and finding a less-silent way to roll out
the change (like adding it with different syntax, or having no default for
a transition period covering a stable release).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200527/3ef1e786/attachment.html>