[cfe-dev] RFC: Easier AST Matching by Default
Stephen Kelly via cfe-dev
cfe-dev at lists.llvm.org
Wed May 27 03:42:37 PDT 2020
On Wed 27 May 2020, 11:01 Sam McCall, <sammccall at google.com> wrote:
> Unfortunately this is causing widespread problems in downstream tools that
>>> are difficult to track:
>> Sorry this is causing problems in downstream tools.
>> - 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
>> Is it trivial to set the TraversalKind presumptively on the
>> ParentMapContext early in the runtime of the downstream tool? The
>> ParentMapContext is available via your ASTContext.
>> I ask because reading your email it doesn't look like you considered this
>> option? If you did consider it and rejected it, please say more about why
>> it is not an option.
>> That way you don't have to try to audit any matchers yet. You would just
>> set the TraversalKind to AsIs in the entire tool.
> Right, I should have addressed this.
> The first problem is that we don't have a really solid way of identifying
> all the downstream tools that are affected.
Can you just change the setting for all of your downstream tools?
(Excepting that clang-tidy might be a different beast in which you have to
set it for each downstream check)
There are certainly hundreds of these.
Are all of these hundreds of tools independent? Or do they share some
abstraction or common setup code or initialization code which lives in a
I'm trying to find out whether the number of places in your code which need
to be adjusted can be reduced from hundreds to a handful. Any early-enough
place in the code which you own and which affects all or most of your tools
would be a good start.
Another problem is tools that mix matchers defined upstream with those
> defined downstream, such as clang-tidy, where there are large numbers of
> checks defined with various owners.
> For this reason I'd been assuming the right workaround was to wrap the
> matchers in traverse() everywhere. Maybe we should consider clang-tidy and
> other tools as separate problems -
I also suspect that would be helpful.
if it's *just* clang-tidy then wrapping all of the matchers might be
> manageable (with the bug fixed).
I have a proof of concept patch which adds a virtual method to
ClangTidyCheck which returns an overriding optional<TraversalKind> to use
for the check. If you reimplement that method in your downstream checks to
return AsIs then you don't have to change individual matchers yet.
What do you think?
I have the patch on a computer I will have access to in about 2 hours.
And for other tools I think we can probably flip the mode, if we can find
> all the tools and find an appropriate place in the code to do so.
>> - 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
>> If that audit can be deferred, does the situation change?
> Yes - for a start it's now safe to upgrade LLVM again :-)
> We usually integrate LLVM head daily, and when this process gets blocked
> it affects a large number of developers in unrelated areas (say, MLIR).
Sorry for the disruption. At least the issue is getting visibility here and
feedback already indicates that the release notes could be more
> - because the default was flipped globally, all usage sites must be dealt
>>> with at once
>> Does this become manageable if you flip the setting locally in your tool
>> in your ParentMapContext?
>> - the "revert-to-old-behaviour" is invasive at all usage sites (hurts
>>> readability a lot) and apparently has bugs (D80606)
>> If changing the TraversalKind in the ParentMapContext in your tool is a
>> potential solution, then I can look into the above issue this morning.
>> Please let me know.
> Yitzhak was also looking into a fix here and may have ideas already.
>> 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.
>> Hmm, the location to call setTraversalKind on the ParentMapContext in
>> each external tool should be easy to find? Should be easy to analyze too.
>> There is presumably fewer places in your tool (or just one place in the
>> entire tool) where ASTContexts are created, so "all" is either "1" or
>> close to it?
>> I expect this to bite people building tools against release versions to
>>> hit the same problems in a few months.
>> Perhaps the solution of changing the setting in the ParentMapContext
>> needs to be spelled out better in the release notes. It would also be good
>> to know whether doing so makes the impact on your tools manageable. Can you
>> look into that?
> I think putting the solution in the release notes will hurt people not
> paying attention in the same way I didn't spot this early enough on cfe-dev
> (well, Dmitri did).
I'd think that at least if someone upgrades to a new release and their code
breaks, the they would check the release notes. I realize that doesn't
scale so we'll for those that "live at HEAD".
It certainly helps to have good documentation but subtly changing the
> meaning of existing code is still dangerous.
> I don't know the matcher API details well enough yet to have a concrete
> suggestion, but I'm trying to get a better grasp on this.
I agree that compile failures are better than runtime failures. We couldn't
come up with a better solution.
At least this runtime failure is likely to be "loud" where it occurs.
>> 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).
>> Before doing that I'd like to find out more about whether the above
>> solution works.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-dev