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

[Stephen Kelly via cfe-dev]

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
library?

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

Yes.


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



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


Thanks,

Stephen.


>
>> 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.
>>
>> Thanks,
>>
>> Stephen.
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20200527/464d8fcd/attachment.html>