<div dir="ltr"><div dir="ltr">On Sun, Jan 26, 2020 at 8:54 PM Stephen Kelly <<a href="mailto:steveire@gmail.com">steveire@gmail.com</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">
<div bgcolor="#FFFFFF">
<p><br>
</p>
<div>On 17/01/2020 12:54, Dmitri Gribenko
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">On Fri, Jan 17, 2020 at 12:41 PM Stephen Kelly
<<a href="mailto:steveire@gmail.com" target="_blank">steveire@gmail.com</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">
<div bgcolor="#FFFFFF">
<p><br>
</p>
<div>On 13/01/2020 16:39, Dmitri Gribenko wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">On Fri, Jan 10, 2020 at 4:34 PM Aaron
Ballman via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" 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">I've not seen
much to suggest the community is not in favor of
this<br>
direction, so I think you're all set to post
patches for review. If<br>
there are community concerns, we can address them
during the review<br>
process. Thank you for working on this!<br>
</blockquote>
<div><br>
</div>
<div>Sorry, I just came back from vacation. I'm not
sure if it is a good idea, in particular, for the
following reasons:</div>
<div><br>
</div>
<div>- The document specifies the goals in terms of
a solution: "Goal: Users should not have to
account for implicit AST nodes when writing AST
Matchers". <br>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>The goal is to make AST Matchers more accessible/easy
to use for newcomers/non-experts in the clang AST by
no-longer confronting them with 100% of the complexity
on first use.<br>
</p>
<p>I think it's important to bridge that gap. See my
EuroLLVM talk for more.<br>
</p>
</div>
</blockquote>
<div>I totally agree with the goal of making AST Matchers,
refactoring, and tooling more accessible (which is why I'm
helping with libraries like Stencil and Syntax trees). </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>This change affects AST Matchers, but as far as I can see, it
does not affect any other effort. <br>
</p>
<p>I don't see why you mention those initiatives in this and your
other email. We're only talking about AST Matchers. Unless AST
Matchers are to be removed, we should improve them, regardless of
Syntax Tree, Stencil and Transformer.<br></p></div></blockquote><div><br></div><div>I mention all of these libraries because people don't use AST matchers just for the sake of it. People use AST matchers to achieve a high-level goal. Therefore, we should be thinking of the whole user journey, not just the individual piece. My point is that, we are working on better solutions for the whole user journey. Those better solutions will allow to achieve the same high-level goals more easily than today's tools, and (I think) even more easily than *any* modification restricted to just AST matchers would have allowed us to do.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>However the goal that I quoted from the proposal on this
thread ("Goal: Users should not have to account for implicit
AST nodes when writing AST Matchers") constrains the
solution to exactly what is being proposed.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Perhaps there's a misunderstanding. That's the goal of changing
the default.<br></p></div></blockquote><div>The goal should be something high-level that the user cares about, for example, "more easily write refactoring tools". Individual technical details are not meaningful as goals, because they don't have intrinsic value from the perspective of end users, maintainers of Clang, maintainers of libraries etc.</div><div><br></div><div>How do we evaluate whether the proposal to change the default traversal is good or bad, if changing the default *is* the goal? It becomes a circular argument: we want to change it because we want it so. The goal should be stated in terms of a problem that the user is trying to solve.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<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 bgcolor="#FFFFFF">
<p> </p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>- The complexity for implementers (although it
also applies to <a href="https://reviews.llvm.org/D61837" target="_blank">https://reviews.llvm.org/D61837</a>,
which landed a while ago, and I didn't see it).
Allowing to run the same matcher in multiple modes
increases our support and testing matrix.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>I don't think it increases the test matrix more than
any other feature. Can you expand on this?<br>
</p>
</div>
</blockquote>
<div><a href="https://reviews.llvm.org/D61837" target="_blank">https://reviews.llvm.org/D61837</a>
makes it possible to run every matcher with different
traversal semantics. Our current tests only test one mode
for each matcher, the default mode. There are two ways to
define custom matchers: the advanced way (through the
AST_MATCHER macro) that is needed when you want to traverse
the AST yourself, and a simpler way -- by combining existing
matchers.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>AFAICT, defining a matcher using the macro, or defining it as a
composition function doesn't change much?<br></p></div></blockquote><div>When you define a matcher using a macro, you usually traverse the AST yourself, by directly using Clang AST APIs. However, when you define a matcher as a composition of other matchers, the user of your matcher can affect the traversal mode *within your matcher* by calling your matcher within traverse(). In other words, there is no abstraction barrier between the matcher and the user.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>Consider the `makeIteratorLoopMatcher` defined in
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp.
The caller can run the newly made matcher in any traversal
mode, even though the matcher was probably written with the
default mode in mind.</div>
<div><br>
</div>
<div>Clang codebase does not have many matchers that are
combinations of existing matchers</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>It does actually - callee() for example. The fact that is uses a
macro doesn't change anything AFAICT. Also alignOfExpr() which
does not use a macro, but I don't see how that changes anything. <br>
</p>
<p>Am I missing something?<br></p></div></blockquote><div>I didn't say such matchers *don't exist* in Clang, I said there are not many of them. And yes, these are the types of matchers I am concerned about being affected by traverse().</div><div><br></div><div>callee and alignOfExpr() are not affected by traverse() because they only match one level of AST nodes -- the call expression, or the alignof expression, and pass down the inner matcher. Had they been traversing multiple levels of AST nodes, they would have been affected by traverse(), if the user called these matchers within traverse().</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>, but code that uses Clang as a library (and often
written by engineers who are not compiler experts and just
need to get some refactoring done) does that very often in
my experience working at Google.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>I don't think the traverse() matcher changes so much in this
area. "Just need to get some refactoring done" sounds like writing
a composition matcher within a check, not writing it for some
library code.<br></p></div></blockquote><div>We have lots of engineers who are not Clang experts and write reusable refactoring code.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><blockquote type="cite">
<div dir="ltr">
<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 bgcolor="#FFFFFF">
<p> </p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>- The complexity cliff for users. Many
non-trivial matchers need to explicitly manage
implicit AST nodes.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>You just described the complexity cliff which is (after
my change) not the first thing a newcomer hits.</p>
<p>It's good to move that cliff back so that the newcomer
is not confronted with it immediately.<br>
</p>
</div>
</blockquote>
<div>As soon as you pretty-print the AST (which I think most
people should do before writing a matcher), you see implicit
nodes.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>If using clang-query, you can change the mode of how clang-query
matches and dumps ASTs. But, in the future you won't need to dump
the AST anyway, because clang-query will just tell you the
available matchers and what they match. <br>
</p>
<p>See my EuroLLVM talk for more, in particular the workflow diagram
which does not include inspecting the AST:
<a href="https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590" target="_blank">https://steveire.files.wordpress.com/2018/11/all-no-ast.png?w=900&h=590</a><br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<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 bgcolor="#FFFFFF">
<p> </p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>- The cost of ClangTidy. ClangTidy runs all
matchers simultaneously in one AST traversal. If
we implement implicit/no-implicit modes as
separate AST traversals, ClangTidy would need to
run two traversals, one for "easier" matchers
(skipping implicit code), and one for "expert"
matchers (traversing implicit code). We would also
need to build two parent maps.</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
<p>As of <a href="https://reviews.llvm.org/D73388" target="_blank">https://reviews.llvm.org/D73388</a> we no longer create a
parent map for each traversal kind used at runtime.<br>
</p>
<p>This means I can no longer measure a runtime cost of introducing
use of the traverse() matcher in clang-tidy.</p></div></blockquote><div>Thank you, that's a great improvement! However, I'm not sure about the absence of runtime cost -- I think it would be more fair to say that there is no cost *right now*. This change adds extra logic (AscendIgnoreUnlessSpelledInSource) that is only called in the case of TK_IgnoreUnlessSpelledInSource traversal. Since most checkers in ClangTidy don't use that mode, we see no increase in runtime cost today. However, once the default is flipped, or once we start using TK_IgnoreUnlessSpelledInSource more, we could see an increased cost of AST matchers compared to the TK_AsIs traversal.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<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 bgcolor="#FFFFFF">
<p> </p>
<p>As long as the current AST Matchers APIs exist, they
should be improved. Perhaps they will be less important
in the future when we have Syntax Trees for these
use-cases instead. However, for now, I don't think the
ease of use improvement in my patches should be
prevented based existence of a future alternative
system.<br>
</p>
</div>
</blockquote>
<div>I agree that we should not block improvements on
existence of a future system. However, each specific change
should be evaluated based on its merits and costs. I think
there are non-trivial downsides to offering a different mode
in existing AST matchers, which create maintenance and
testing costs that we will be paying perpetually. </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>We may not achieve your agreement on this change, but I think we
have consensus already that this change should proceed.<br></p></div></blockquote><div>I'm not sure how you measured the degree of consensus -- could you clarify?</div><div><br></div><div>Here's what I see: so far, there have been few people participating on this thread, and outside of the authors of the proposal (you and Aaron), only I have provided direct feedback about supporting/opposing the proposal, other people (Artem, Gabor, and Ilya) only asked questions, provided clarifications, and provided feedback on the syntax trees developments. If you count me as objecting to the traverse() API in general, and not objecting to this change, then there has been no feedback from the community on this proposal at all. If you count me as objecting to this proposal, then there's one person objecting. Based on these observations, I don't see consensus yet, even if you exclude my feedback.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF"><p>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>It instantly creates a huge testing gap for existing AST
matchers (which we currently test only in one mode). </div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>The traverse() matcher does not introduce a need to test each AST
matcher in each mode.<br></p></div></blockquote><div>Why do you say so? Matchers composed out of other matchers are affected by the traversal mode that the user invokes them in.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>It also creates traps for existing code that will be able
to start accidentally calling existing matchers in a
traversal mode that they were not designed for.</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Yes, true. It is possible to write a matcher which works as
someone expects in one mode but not the other, such as a matcher
which specifically expects to match a implicitCastExpr().</p></div></blockquote><div>Exactly.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div bgcolor="#FFFFFF">
<p>This can matter for matchers which are in "libraries", but not
for matchers which are implementation details inside checks.
Unless a particular check exposes the traversal kind as an option,
those matchers can not be affected. Any check which does expose
that as an option has to do so consciously and explicitly and has
to test it. Nothing new there.<br></p></div></blockquote><div>I'm sorry if I have been unclear about this point, but during this whole conversation I have been talking not about specific ClangTidy checkers, but about libraries that provide matchers. Users of Clang (for example, internal users at Google) have such libraries, and use them broadly outside of ClangTidy. Previously in this thread I pointed at a matcher in ClangTidy as an example because that's the only piece of open source code I could easily find, that has similar complexity to libraries that we have at Google. Introduction of traverse() has increased the testing matrix for these matchers, so it is a new increase in complexity, and increase in technical debt (missing tests).</div><div><br></div><div>Let me reiterate: I understand that Clang or Clang Tooling does not provide a stable API, so making breaking changes here is totally within the policy.</div><div><br></div><div>However, the specific aspects of this change (silently breaking existing matchers, creating new traps for people who write reusable AST matchers, increasing the testing matrix) have consequences that are very different from a typical API change in Clang AST, where the user of that API will see a build break, and can easily locally fix code. The cost of this change is much higher for users of the API being changed.</div><div><br></div><div>Let me point you at similar change that was done previously: the change in elidable constructor representation in Clang AST between C++14 and C++17. (TL;DR: AST built by Clang in C++11 mode has AST nodes for elidable constructor calls, but in C++17 mode they are always elided.) Lots of ClangTidy checkers were broken by this change, but nobody noticed, because ClangTidy was not being tested in all language modes. I closed this testing gap (<a href="https://reviews.llvm.org/D62125">https://reviews.llvm.org/D62125</a>), and our team has fixed a bunch of ClangTidy checkers, but other checkers still remain broken until this day (run `git grep 'FIXME: Fix the checker to work in C++17 mode.'`) The worst part of this story is that the breakage was noticed a long, long time after the AST change was implemented, and that the cost of the change (expanding testing, fixing checkers) has fallen onto people other than the ones who changed the AST.</div><div><br></div><div>The proposal has been implying (correct me if I'm wrong) that the maintenance cost and complexity that is being put onto experts with a codebase that uses AST matchers that they maintain long-term is worth the added convenience for novice users who write one-off tools that don't need to be maintained. If it is indeed the case, I would prefer this argument to be made explicitly (as in, the proposal should describe the downsides of making this change; right now it does not describe them).</div><div><br></div><div>Dmitri</div></div><div><br></div>-- <br><div dir="ltr" class="gmail_signature">main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>(j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com" target="_blank">gribozavr@gmail.com</a>>*/</div></div>