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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:80
+// \endcode
+struct TextChange {
+  // The (bound) id of the node whose source will be replaced.  This id should
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > ymandel wrote:
> > > ilya-biryukov wrote:
> > > > `MatchChange` or something similar might be a better name.
> > > > This actually tries to change the matched AST node to a textual replacement.
> > > I chose this to contrast with an AST change.  The idea being that we're specifying a replacement relative to source code locations (informed by the ast). If we later, say, integrate with your library I could imagine specifying changes to AST nodes.  But, maybe I'm overthinking... If we're going to drop "text", what about "source?" be clearer than "text"? E.g, `SourceChange` or (my preference) `SourceEdit`?
> > The reasons I find `TextChange` confusing is because I immediately think of something super-simple (a range of text + the replaced text), and I definitely don't think of the AST.
> > 
> > `SourceChange` and `SourceEdit` does not cause this confusion for me personally, so both look ok. Although they do look pretty similar.
> > Also, it's not actually a final edit, rather a description of it. So something like `EditDescription` could work too.
> Right, I'm now tending way from the whole focus on text/source/etc. I agree with your point that this is operating at the AST level, especially in light of the discussion below on applyRewriteRule.  Given that these are all anchored by an AST node, let's go with `ASTEdit`, unless that will conflict with the library that you're developing?
> 
> I agree that "Description" is more apt, but I feel like this is (somewhat) implicit in the fact that its a struct versus a function (which would actually be carrying out the action). I'm also afraid that "EditDescription" will read like an action, which may be a bit confusing (although the casing should help distinguish).
`ASTEdit` seems like a proper name here, LG!


================
Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:87
+  TextGenerator Replacement;
+  TextGenerator Explanation;
+};
----------------
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()`)


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


================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
+      return SmallVector<Transformation, 0>();
+    Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+  }
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60408