[PATCH] D60408: [LibTooling] Extend Transformer to support multiple simultaneous changes.

Mon Apr 15 09:12:51 PDT 2019

ymandel added inline comments.

================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87
+  TextGenerator Replacement;
+  TextGenerator Explanation;
+};
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > ymandel wrote:
> > > > ilya-biryukov wrote:
> > > > > I would've expected explanation to be the trait of the rewrite rule, since all changes have to be applied.
> > > > > What's the reasoning behind having it at the level of separate changes? How would this explanation be used? For debugging purposes or displaying that to the user?
> > > > I think that for most cases, one explanation sufficies for the whole transformation. However, there are some tidies that emit multiple diagnoses (for example if changing before a declaration and a definition).   Would it help if I clarify in the comments?
> > > Yeah, absolutely! Please document what it's used for and that would clear that up for me.
> > > I actually thing that explaining every part of the transformation is probably too complicated, so most of the time you would want to have an explanation for the `RewriteRule`, not for each individual change.
> > > 
> > > The other challenge that I see is show to display these explanations to the user, i.e. how should the clients combine these explanations in order to get the full one? Should the `RewriteRule` have an explanation of the full transformation too?
> > I've revised the comments, changed the name to "Note" and put "Explanation" back into RewriteRule.
> > 
> > As for how to display these, I imagine an interface like clang tidy's fixit hints.  The Explanation (if any) will be associated with the span of the entire match.  The Notes will be associated with the target node span of each annotated change.  WDYT?
> Do we really need the AST information to expose the edits to the users?
> IIUC, clang-tidy uses the information from textual replacements to render the changes produced by the fix-its today.
> 
> I guess it might be useful to add extra notes to clang-tidy warnings that have corresponding fix-its, but is the transformers library the right layer to produce those? 
> I haven't seen the proposed glue to clang-tidy yet, maybe that would make more sense when I see it.
> 
> One of the other reasons I ask this is that it seems that without `Note` we don't strictly `ASTEditBuilder`, we could replace
> ```
> change("ref").to("something"); // without nodepart
> change("ref", NodePart::Member).to("something");
> ```
> with
> ```
> change("ref", "something")
> change("ref", NodePart::Member, "something");
> ```
> That would remove the boilerplate of the builder, simplifying the code a bit.
> 
> That trick would be hard to pull if we have a `Note` field inside, we'll need more overloads and having note and replacement after each other might be confusing (they have the same type, might be hard to read without the guiding `.to()` and `.because()`)
Breaking this explicitly into two questions:
1. Do Notes belong here?
2. Can we drop the builder?

1. Do Notes belong here?
I think so.  When users provide a diagnostic, they are specifying its location. So, we don't quite need the AST but we do need location info.  The diagnostic generator does take information from the replacements themselves, but that's not alone. For example: clang-tidy/readability/ConstReturnTypeCheck.cpp:104.  That demos both the diagnostic construction and multiple diagnostics in a single tidy result.

Given that, i think that RewriteRules are the appropriate place. The goal is that they be self contained, so I think the explanations should be bundled with the description of that actual change.  An example approach to merging with clang tidy is here: https://github.com/ymand/llvm-project/blob/transformer/clang-tools-extra/clang-tidy/utils/TransformerTidy.cpp (although that's based on a somewhat different version of the Transformer library; it should make clear what I have in mind).

2. Do we need the builder?
No, despite my opinion that notes belong.  Given the explanation on RewriteRule, I think notes will be used only rarely (like in the example clang-tidy above; but that's not standard practice).  So, we can provide the two overloads of `change` that you mention and then let users assign the note directly in those rare cases they need it. then, we can drop the builder.

So, I propose keeping the note but dropping the builder. WDYT?

================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:204
+applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result,
+                 llvm::ArrayRef<TextChange> Changes);

----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Why would we change the interface here? Rewrite rule seemed like a perfect input to this function
> > Good point.  For the name, you're right -- but I think the name was misleading. The function doesn't really "apply" anything and what it does use is only the _change_ part of the rule -- it doesn't handle (and never even looks at) the matcher part, because it's already given the match.
> > 
> > Rather, this just translates change descriptions that are in terms of the AST into ones in terms of the source code. I've renamed it to `translateChanges` and updates the comments describing it. Let me know what you think.
> The new name looks ok. Although if the `RewriteRule` is the central concept of this file (and it looks like it is), I think it'd make sense to make this a narrower interface, accepting a `RewriteRule` as a parameter.
> 
> But up to you, as long as we have this spelled out in the docs, I don't think this should cause problems to the users.
SG.  To be clear -- this is intended only as an "advanced" part of the API, to factor out code that will be common to:
1. Transformer (here)
2. TransformerTidy -- the clang-tidy version of this code that creates a ClangTidy from a RewriteRule
3. applyRewriteRule(ASTContext) -- an upcoming function that will do what this function looked like it was doing. It will apply a rewriterule (including the matcher) to a node or an ASTContext.

However, I don't expect that any standard users should need this. I'd originally placed it in an "internal" namespace, but it's not quite internal because TransformerTidy will be living somewhere else. So, it should be exposed, but its really only for other implementors rather than standard users.

================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
+      return SmallVector<Transformation, 0>();
+    Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+  }
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > What if the changes intersect? I'd expect this function to handle this somehow, handling this is complicated and I feel we shouldn't rely on the clients to make it fast.
> > Right now, we rely on clients (as you mention).  The clients will receive `AtomicChange`s that overlap and will have to handle that.  `clang::tooling::applyAtomicChanges` will fail on this, although one could imagine a client that could be smarter.
> > 
> > That said, I'm fine with us handling this here, instead. My only argument against is that the same issue applies for matchers in general, so catching it here won't prevent the case where separate firings of the same rule (or of different rules if more than one is registered) try to change the source.  Moreover, since we already have the logic elsewhere (clang::tooling::Replacements) we would be duplicating that here.
> > 
> > I've thought of using AtomicChange here directly, but there's no way to get the changes back in terms of SourceLocations, which we need for this function to be useable inside the ClangTidy adapter (FixItHint uses SourceLocations, not raw filename and offset).
> > 
> > What would you say to a FIXME to find a better means of failing fast?
> > 
> > On a related note, I see that this struct is very similar to clang::FixItHint.  Do you think its worth just using that directly? Or, is this a sufficiently different situation to merit a different type definition?
> I can hardly imagine clients doing anything smarter than failing on intersecting changes (at least for the general case).
> 
> That said, I do see the reasons to keep this error-handling out of this function for the matter of simplicity. And totally agree that in order to deal with it properly we'd want to return something like `tooling::Replacements` to make sure we can reuse facilities that handle the overlapping cases properly.
> 
> Could we add a comment to the declaration, explaining that returned ranges might overlap and clients are expected to deal with that (and maybe mentioning `AtomicChanges` and `tooling::Replacements` as potential options to do so).
> 
> I'd avoid using `clang::FixItHint` while we keep experimenting with the interface of the library, having our own type gives us more freedom to change it.  As soon as the interface is stable, we can take revisit this (and FWIW, `FixItHint` does not look like a good name for this).
Agreed. I've updated the comments as per your suggestion.  Also added a note to the Transformer constructor below along the same lines.  

For the future: we should think about adding an option to have Transformer handle conflicts instead of leaving it up to the user.  It would probably require a somewhat different API -- e.g. creating one large AtomicChange and handing that off at destruction time, rather then calling `Consumer` with independent changes for each match. 

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60408/new/

https://reviews.llvm.org/D60408