<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 27, 2020 at 11:34 AM Stephen Kelly <<a href="mailto:steveire@gmail.com">steveire@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed 27 May 2020, 09:25 Sam McCall, <<a href="mailto:sammccall@google.com" rel="noreferrer noreferrer noreferrer noreferrer" target="_blank">sammccall@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Mon, May 25, 2020 at 11:56 PM Stephen Kelly via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" rel="noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 20/12/2019 21:01, Stephen Kelly via cfe-dev wrote:<br>
> <br>
> Hi,<br>
> <br>
> (Apologies if you receive this twice. GMail classified the first one as <br>
> spam)<br>
> <br>
> Aaron Ballman and I met by chance in Belfast and we discussed a way <br>
> forward towards making AST Matchers easier to use, particularly for C++ <br>
> developers who are not familiar with the details of the Clang AST.<br>
> <br>
> For those unaware, I expanded on this in the EuroLLVM conference this <br>
> year, and then expanded on it at ACCU:<br>
> <br>
>   <a href="https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">https://steveire.wordpress.com/2019/04/30/the-future-of-ast-matching</a><br>
> <br>
> One step in the process of getting there is changing the default <br>
> behavior of AST Matchers to ignore invisible nodes while matching using <br>
> the C++ API, and while matching and dumping AST nodes in clang-query.<br>
> <br>
> I think this is the most important change in the entire proposal as it <br>
> sets out the intention of making the AST Matchers easier to use for C++ <br>
> developers who are not already familiar with Clang APIs.<br>
> <br>
> To that end, I've written an AST to motivate the change:<br>
> <br>
> <br>
> <a href="https://docs.google.com/document/d/17Z6gAwwc3HoRXvsy0OdwU0X5MFQEuiGeSu3i6ICOB90" rel="noreferrer noreferrer noreferrer noreferrer noreferrer noreferrer" target="_blank">https://docs.google.com/document/d/17Z6gAwwc3HoRXvsy0OdwU0X5MFQEuiGeSu3i6ICOB90</a> <br>
> <br>
> <br>
> We're looking for feedback before pressing forward with the change. I <br>
> already have some patches written to port clang-tidy and unit tests to <br>
> account for the change of default.<br>
<br>
<br>
This change is now in master.<br></blockquote><div> </div><div>Thanks for simplifying the matchers! Unfortunately I missed the discussion on this.<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">That's a pity. This thread was bumped a few times since December. </div></div></blockquote><div>Yeah, this is a personal problem of mine: cfe-dev has enough traffic and covers too many topics that I find it too distracting to read continuously.</div><div>I try to check in occasionally, but missed it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Unfortunately this is causing widespread problems in downstream tools that are difficult to track:<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Sorry this is causing problems in downstream tools.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> - this is a prominent API that is used in many places<br> - 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<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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. </div><div dir="auto"><br></div><div dir="auto">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. </div><div dir="auto"><br></div><div dir="auto">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. </div></div></blockquote><div>Right, I should have addressed this.</div><div>The first problem is that we don't have a really solid way of identifying all the downstream tools that are affected. There are certainly hundreds of these.</div><div>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.</div><div><br></div><div>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 - if it's *just* clang-tidy then wrapping all of the matchers might be manageable (with the bug fixed). 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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> - 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<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">If that audit can be deferred, does the situation change?</div></div></blockquote><div>Yes - for a start it's now safe to upgrade LLVM again :-)</div><div>We usually integrate LLVM head daily, and when this process gets blocked it affects a large number of developers in unrelated areas (say, MLIR).</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> - because the default was flipped globally, all usage sites must be dealt with at once<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Does this become manageable if you flip the setting locally in your tool in your ParentMapContext?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> - the "revert-to-old-behaviour" is invasive at all usage sites (hurts readability a lot) and apparently has bugs (D80606)<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">If changing the TraversalKind in the  ParentMapContext in your tool is a potential solution, then I can look into the above issue this morning.</div><div dir="auto"><br></div><div dir="auto">Please let me know. </div></div></blockquote><div>+yitzhakm<br></div><div>Yitzhak was also looking into a fix here and may have ideas already.</div><div>  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>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.<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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 <span style="font-family:sans-serif">in your tool</span> (or just one place in the entire tool)  where ASTContexts are created, so "all" is either "1" or close to it?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>I expect this to bite people building tools against release versions to hit the same problems in a few months.<br></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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?</div></div></blockquote><div>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).</div><div>It certainly helps to have good documentation but subtly changing the meaning of existing code is still dangerous.</div><div>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.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="auto"><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>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).</div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Before doing that I'd like to find out more about whether the above solution works. </div><div dir="auto"><br></div><div dir="auto">Thanks,</div><div dir="auto"><br></div><div dir="auto">Stephen. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
</blockquote></div></div></div>
</blockquote></div></div>