[LibTooling] Extend Transformer to support multiple simultaneous changes.

[ymandel]

Summary

This revision allows users to specify independent changes to multiple (related) sections of the input. Previously, only a single section of input could be selected for replacement.

[ymandel]

gentle ping

[ilya-biryukov]

Sorry for the delay.
There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal?

clang/include/clang/Tooling/Refactoring/Transformer.h

80

MatchChange or something similar might be a better name.
This actually tries to change the matched AST node to a textual replacement.

87

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?

[ymandel]

Restore code ordering for unrelated code.

[ymandel]

More code movement (putting things back).

[ymandel]

Final code shifting.

[ymandel]

In D60408#1463909, @ilya-biryukov wrote:

Sorry for the delay.
There seem to be a few changes that are unrelated to the actual patch. Could we separate various non-functional changes (moving code around, etc.) into a separate change to keep the diff for this one minimal?

I've done that as far as I can tell. Please let me know if I've missed anything.

clang/include/clang/Tooling/Refactoring/Transformer.h

80

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?

87

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?

[ilya-biryukov]

clang/include/clang/Tooling/Refactoring/Transformer.h

80

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.

87

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?

187

NIT: I'd suggest just paying a few extra lines for initializing the struct instead of using the ctor.

Transformation T;
T.Range = ...;
T.Replacement = ...;

v.push_back(std::move(T));

or

v.emplace_back();
v.back().Range = ...;
v.back().Replacement = ...;

But I can see why you might want a ctor instead. If you decide to leave it, consider re-adding the default ctor that got implicitly deleted as you defined this other one.

192

Why would we change the interface here? Rewrite rule seemed like a perfect input to this function

clang/lib/Tooling/Refactoring/Transformer.cpp

164

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.

[ilya-biryukov]

In D60408#1464364, @ymandel wrote:

I've done that as far as I can tell. Please let me know if I've missed anything.

Many thanks!

[ymandel]

Responses to comments, including renaming TextChange and applyRewriteRule.

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

80

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

87

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?

187

Agreed. I only use it one place, hardly worth the cost. thanks

192

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.

clang/lib/Tooling/Refactoring/Transformer.cpp

164

Right now, we rely on clients (as you mention). The clients will receive AtomicChanges 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?

[ilya-biryukov]

clang/include/clang/Tooling/Refactoring/Transformer.h

80

ASTEdit seems like a proper name here, LG!

87

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())

192

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.

clang/lib/Tooling/Refactoring/Transformer.cpp

164

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

[ymandel]

Clarified comments for translateEdits function.

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

87

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?

192

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.

clang/lib/Tooling/Refactoring/Transformer.cpp

164

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.

[ilya-biryukov]

A few quick responses, will take a closer look again tomorrow.

clang/include/clang/Tooling/Refactoring/Transformer.h

87

Thanks for pointing out how this usage in clang-tidy. Seems to be the only way to put it into clang-tidy (and there are good reasons for that, e.g. having the full context of the change).

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

LGTM!

192

The tidy use-case makes sense now, thanks! The fact that fix-it hints is the only way to report changes there kinda forces us to have a function that works on a level of transformations.

clang/lib/Tooling/Refactoring/Transformer.cpp

164

Thanks, LG!
Agree that having a simpler interface would be nice, still not sure how to properly wire it up with matchers, especially in the clang-tidy use-case.
But if we can run the library function would run the matchers on its own, a simpler interface seems doable.

[ymandel]

Deleted ASTEdit builder; added more change overloads.

[ymandel]

Fix include ordering (bad clang-format config).

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

87

Removed the builder, added more change overloads. Also renamed changeRoot to changeMatched because I think it's a better description.

The only part I don't love (here and elsewhere) is the duplicate overloads one each for std::string and TextGenerator.

[ilya-biryukov]

Thanks for the update, looks good! Just a few nits from my side.

The only part I don't love (here and elsewhere) is the duplicate overloads one each for std::string and TextGenerator.

Totally agree, that's not ideal. Would be nice to find a way to workaround this.

clang/include/clang/Tooling/Refactoring/Transformer.h

76

Comment and the code example still mention ASTBuilder.

129–130

Are addition/removal of headers something that is going to land in the future?

Maybe leave out this comment until it actually lands?

132

Any reason to use a different name? Having an overload for change without Target does not look confusing.

change<clang::Expr>("replacement(1,2,3)");
vs 
changeMatched<clang::Expr>("replacement(1,2,3)");

clang/lib/Tooling/Refactoring/Transformer.cpp

193–206

Could we add a test for this case?
(Sorry for spam if there's one already and I missed it)

221

NIT: maybe add braces?
I believe they're not necessary in LLVM Style, but IMO add readability in case of nested statements with their own braces, like if in this code.

clang/unittests/Tooling/TransformerTest.cpp

360

Could we add a test with intersecting ranges?
To have a regression test in case we'll change behavior in this corner case later.

[ymandel]

Cleaned up header comments; added tests.

[ymandel]

Cleaned up header comments; added tests.

[ymandel]

clang/include/clang/Tooling/Refactoring/Transformer.h

76

Thanks. I noticed I was also using an older version that integrated with Stencil. I've updated to make that explicit. I expect Stencil to land really soon (and can wait to commit until then), but I can remove the reference if you prefer. Without Stencils, though, the examples are pretty lame.

I also noticed other spurious mentions of "builder" below and deleted them as well.

129–130

Good catch

132

Good idea. changed as you suggested.

clang/lib/Tooling/Refactoring/Transformer.cpp

193–206

Nope -- I hadn't tested this yet. Thanks!

221

Done. I personally always prefer braces on for loops, so always happy to add them. ;)

clang/unittests/Tooling/TransformerTest.cpp

360

Added two tests: one for conflict in single match, one for conflict between matches.

[ilya-biryukov]

LGTM!

[ilya-biryukov]

Not sure if you want to land this before or after stencil. This seems useful even without the latter.

[liutianle]

hi ymandel,

When I run "check-all", there are some warning/error in TransformerTest.cpp as follow. My version is llvm:0ee120077 and clang:d87ee8e678. Could you please have a fix or guild me how to fix it?

In file included from myLLVM/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:47:0,

from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:46,
from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58,
from myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:13:

myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]':
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:23: required from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; bool lhs_is_null_literal = false]'
myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:372:3: required from here
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

if (lhs == rhs) {
        ^

---

[ymandel]

In D60408#1473370, @liutianle wrote:

hi ymandel,

When I run "check-all", there are some warning/error in TransformerTest.cpp as follow. My version is llvm:0ee120077 and clang:d87ee8e678. Could you please have a fix or guild me how to fix it?

In file included from myLLVM/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:47:0,

from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:46,
from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58,
from myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:13:

myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]':
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:23: required from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; bool lhs_is_null_literal = false]'
myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:372:3: required from here
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

if (lhs == rhs) {
        ^

---

Sorry about that. Have you sync'd to head? I thought that was fixed with r358745 (from Bjorn Pettersson).

[liutianle]

In D60408#1474100, @ymandel wrote:

In D60408#1473370, @liutianle wrote:

hi ymandel,

When I run "check-all", there are some warning/error in TransformerTest.cpp as follow. My version is llvm:0ee120077 and clang:d87ee8e678. Could you please have a fix or guild me how to fix it?

In file included from myLLVM/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:47:0,

from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:46,
from myLLVM/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58,
from myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:13:

myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]':
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1421:23: required from 'static testing::AssertionResult testing::internal::EqHelper<lhs_is_null_literal>::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; bool lhs_is_null_literal = false]'
myLLVM/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:372:3: required from here
myLLVM/llvm/utils/unittest/googletest/include/gtest/gtest.h:1392:11: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

if (lhs == rhs) {
        ^

---

Sorry about that. Have you sync'd to head? I thought that was fixed with r358745 (from Bjorn Pettersson).

I see. Thank you very much!

clang/include/clang/Tooling/Refactoring/Transformer.h

 #ifndef LLVM_CLANG_TOOLING_REFACTOR_TRANSFORMER_H_
 #define LLVM_CLANG_TOOLING_REFACTOR_TRANSFORMER_H_
 
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "clang/Tooling/Refactoring/AtomicChange.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
 #include <deque>
 #include <functional>
 #include <string>
 #include <type_traits>
 #include <utility>
-#include <vector>
 
 namespace clang {
 namespace tooling {
 /// Determines the part of the AST node to replace.  We support this to work
 /// around the fact that the AST does not differentiate various syntactic
 /// elements into their own nodes, so users can specify them relative to a node,
 /// instead.
 enum class NodePart {
   /// The node itself.
   Node,
   /// Given a \c MemberExpr, selects the member's token.
   Member,
   /// Given a \c NamedDecl or \c CxxCtorInitializer, selects that token of the
   /// relevant name, not including qualifiers.
   Name,
 };
 
 using TextGenerator =
     std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
 
+// Description of a textual change, based on an AST node. Includes: an ID for
+// the (bound) node, a selector for text in relation to the node, a replacement
+// and an explanation for the change.
+//
+// * Target: the source code impacted by the rule. This identifies an AST node,
+//   or part thereof (\c Part), whose source range indicates the extent of the
+//   replacement applied by the replacement term.  By default, the extent is the
+//   node matched by the pattern term (\c NodePart::Node). Target's are typed
+//   (\c Kind), which guides the determination of the node extent.
+//
+// * Replacement: a function that produces a replacement string for the target,
+//   based on the match result.
+//
+// * Explanation: explanation of the rewrite (also based on the match result).
+//   This will be displayed to the user, where possible; for example, in
+//   clang-tidy fix descriptions.
+//
+// TextChange should be built using the corresponding builder class. For
+// example,
+// \code
+//   changeTextOf<Expr>(call, NodePart::Args).to(x, ",", y)
+//     .because("Argument order has changed.")
+//   changeTextOf<Stmt>(thenNode).to("{", thenNode, "}")
+//   changeTextOf<FunctionDecl>(fun, NodePart::Name).to("Frodo")
+// \endcode
+// Or, if you are changing the node corresponding to the rule's matcher, you can
+// use the shorthand \c changeRoot:
+// \code
+//   changeRoot<Expr>().to(x, "+", y)
+// \endcode
+struct TextChange {
+  // The (bound) id of the node whose source will be replaced.  This id should
+  // never be the empty string.
+  std::string Target;
+  ast_type_traits::ASTNodeKind Kind;
+  NodePart Part;
+  TextGenerator Replacement;
+  TextGenerator Explanation;
+};
+
+/// A fluent builder class for \c TextChange.  See comments on \c TextChange for
+/// usage.
+class TextChangeBuilder {
+  TextChange Change;
+
+  // Wraps a string as a TextGenerator.
+  static TextGenerator text(std::string M) {
+    return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+   }
+
+public:
+  TextChangeBuilder(StringRef Target, ast_type_traits::ASTNodeKind Kind,
+                    NodePart Part) {
+    Change.Target = Target;
+    Change.Kind = Kind;
+    Change.Part = Part;
+  }
+
+  /// (Implicit) "build" operator to build a TextChange from this builder.
+  operator TextChange() && { return std::move(Change); }
+
+  TextChangeBuilder to(TextGenerator Replacement);
+  TextChangeBuilder to(std::string Replacement) {
+    return to(text(std::move(Replacement)));
+  }
+
+  TextChangeBuilder because(TextGenerator Explanation);
+  TextChangeBuilder because(std::string Explanation) {
+    return because(text(std::move(Explanation)));
+  }
+};
+
 /// Description of a source-code transformation.
 //
 // A *rewrite rule* describes a transformation of source code. It has the
 // following components:
 //
 // * Matcher: the pattern term, expressed as clang matchers (with Transformer
 //   extensions).
 //
-// * Target: the source code impacted by the rule. This identifies an AST node,
-//   or part thereof (\c TargetPart), whose source range indicates the extent of
-//   the replacement applied by the replacement term.  By default, the extent is
-//   the node matched by the pattern term (\c NodePart::Node). Target's are
-//   typed (\c TargetKind), which guides the determination of the node extent
-//   and might, in the future, statically constrain the set of eligible
-//   NodeParts for a given node.
-//
-// * Replacement: a function that produces a replacement string for the target,
-//   based on the match result.
-//
-// * Explanation: explanation of the rewrite.  This will be displayed to the
-//   user, where possible (for example, in clang-tidy fix descriptions).
+// * Changes: a set of changes to the code text, including addition/removal of
+//   headers and textual replacements.
 //
 // Rules have an additional, implicit, component: the parameters. These are
 // portions of the pattern which are left unspecified, yet named so that we can
 // reference them in the replacement term.  The structure of parameters can be
 // partially or even fully specified, in which case they serve just to identify
 // matched nodes for later reference rather than abstract over portions of the
 // AST.  However, in all cases, we refer to named portions of the pattern as
 // parameters.
 //
-// RewriteRule is constructed in a "fluent" style, by creating a builder and
-// chaining setters of individual components.
-// \code
-//   RewriteRule MyRule = buildRule(functionDecl(...)).replaceWith(...);
-// \endcode
-//
-// The \c Transformer class should then be used to apply the rewrite rule and
-// obtain the corresponding replacements.
+// The \c Transformer class should be used to apply the rewrite rule and obtain
+// the corresponding replacements.
 struct RewriteRule {
   // `Matcher` describes the context of this rule. It should always be bound to
   // at least `RootId`.  The builder class below takes care of this
   // binding. Here, we bind it to a trivial Matcher to enable the default
   // constructor, since DynTypedMatcher has no default constructor.
-  ast_matchers::internal::DynTypedMatcher Matcher = ast_matchers::stmt();
-  // The (bound) id of the node whose source will be replaced.  This id should
-  // never be the empty string.
-  std::string Target;
-  ast_type_traits::ASTNodeKind TargetKind;
-  NodePart TargetPart;
-  TextGenerator Replacement;
-  TextGenerator Explanation;
+  // ast_matchers::internal::DynTypedMatcher Matcher = ast_matchers::stmt();
+  ast_matchers::internal::DynTypedMatcher Matcher;
+  SmallVector<TextChange, 1> Changes;
 
   // Id used as the default target of each match. The node described by the
   // matcher is guaranteed to be bound to this id, for all rewrite rules
   // constructed with the builder class.
   static constexpr llvm::StringLiteral RootId = "___root___";
 };
 
-/// A fluent builder class for \c RewriteRule.  See comments on \c RewriteRule.
-class RewriteRuleBuilder {
-  RewriteRule Rule;
-
-public:
-  RewriteRuleBuilder(ast_matchers::internal::DynTypedMatcher M) {
-    M.setAllowBind(true);
-    // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
-    Rule.Matcher = *M.tryBind(RewriteRule::RootId);
-    Rule.Target = RewriteRule::RootId;
-    Rule.TargetKind = M.getSupportedKind();
-    Rule.TargetPart = NodePart::Node;
-  }
+/// Convenience function for constructing a \c RewriteRule. Takes care of
+/// binding the matcher to RootId.
+RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                     SmallVector<TextChange, 1> Changes);
+
+/// Convenience overload of \c makeRule for common case of only one change.
+inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                            TextChange Change) {
+  SmallVector<TextChange, 1> Changes;
+  Changes.emplace_back(std::move(Change));
+  return makeRule(std::move(M), std::move(Changes));
 
-  /// (Implicit) "build" operator to build a RewriteRule from this builder.
-  operator RewriteRule() && { return std::move(Rule); }
-
-  // Sets the target kind based on a clang AST node type.
-  template <typename T> RewriteRuleBuilder as();
-
-  template <typename T>
-  RewriteRuleBuilder change(llvm::StringRef Target,
-                            NodePart Part = NodePart::Node);
-
-  RewriteRuleBuilder replaceWith(TextGenerator Replacement);
-  RewriteRuleBuilder replaceWith(std::string Replacement) {
-    return replaceWith(text(std::move(Replacement)));
-  }
-
-  RewriteRuleBuilder because(TextGenerator Explanation);
-  RewriteRuleBuilder because(std::string Explanation) {
-    return because(text(std::move(Explanation)));
-  }
-
-private:
-  // Wraps a string as a TextGenerator.
-  static TextGenerator text(std::string M) {
-    return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
-   }
-};
-
-/// Convenience factory functions for starting construction of a \c RewriteRule.
-inline RewriteRuleBuilder buildRule(ast_matchers::internal::DynTypedMatcher M) {
-  return RewriteRuleBuilder(std::move(M));
-}
-
-template <typename T> RewriteRuleBuilder RewriteRuleBuilder::as() {
-  Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
-  return *this;
+/// Convenience function for changing the text of the given `Target` node. Must
+/// be explicitly instantiated with the desired AST type.
+//
+// FIXME: constrain accepted values of \c Part based on type \c T.
+TextChangeBuilder changeTextOf(StringRef Target,
+                               NodePart Part = NodePart::Node) {
+  return TextChangeBuilder(
+      Target, ast_type_traits::ASTNodeKind::getFromNodeKind<T>(), Part);
 }
 
 template <typename T>
-RewriteRuleBuilder RewriteRuleBuilder::change(llvm::StringRef TargetId,
-                                              NodePart Part) {
-  Rule.Target = TargetId;
-  Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
-  Rule.TargetPart = Part;
-  return *this;
+TextChangeBuilder changeRoot() {
+  return changeTextOf<T>(RewriteRule::RootId);
 }
 
-/// A source "transformation," represented by a character range in the source to
-/// be replaced and a corresponding replacement string.
-struct Transformation {
-  CharSourceRange Range;
-  std::string Replacement;
-};
-
-/// Attempts to apply a rule to a match.  Returns an empty transformation if the
-/// match is not eligible for rewriting (certain interactions with macros, for
-/// example).  Fails if any invariants are violated relating to bound nodes in
-/// the match.
-Expected<Transformation>
-applyRewriteRule(const RewriteRule &Rule,
-                 const ast_matchers::MatchFinder::MatchResult &Match);
-
 /// Handles the matcher and callback registration for a single rewrite rule, as
 /// defined by the arguments of the constructor.
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
   using ChangeConsumer =
       std::function<void(const clang::tooling::AtomicChange &Change)>;
 
+private:
+  RewriteRule Rule;
+  /// Receives each successful rewrites as an \c AtomicChange.
+  ChangeConsumer Consumer;
+
+public:
   /// \param Consumer Receives each successful rewrites as an \c AtomicChange.
   Transformer(RewriteRule Rule, ChangeConsumer Consumer)
       : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
 
   /// N.B. Passes `this` pointer to `MatchFinder`.  So, this object should not
   /// be moved after this call.
   void registerMatchers(ast_matchers::MatchFinder *MatchFinder);
 
   /// Not called directly by users -- called by the framework, via base class
   /// pointer.
   void run(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
 
-private:
-  RewriteRule Rule;
-  /// Receives each successful rewrites as an \c AtomicChange.
-  ChangeConsumer Consumer;
+/// A source "transformation," represented by a character range in the source to
+/// be replaced and a corresponding replacement string.
+struct Transformation {
+  // Trivial constructor to enable `emplace_back()` and the like.
+  Transformation(CharSourceRange Range, std::string Replacement)
+      : Range(Range), Replacement(std::move(Replacement)) {}
+
+  CharSourceRange Range;
+  std::string Replacement;
 };
+
+/// Attempts to apply a set of changes to a given a match.  Returns an empty
+/// vector if any of the changes apply to portions of the match that are
+/// ineligible for rewriting (certain interactions with macros, for example).
+/// Fails if any invariants are violated relating to bound nodes in the match.
+Expected<SmallVector<Transformation, 1>>
+applyRewriteRule(const ast_matchers::MatchFinder::MatchResult &Result,
+                 llvm::ArrayRef<TextChange> Changes);
 } // namespace tooling
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLING_REFACTOR_TRANSFORMER_H_

clang/lib/Tooling/Refactoring/Transformer.cpp

case NodePart::Name:
     return typeError(
         Target, Node.getNodeKind(),
         "NodePart::Name applied to neither DeclRefExpr, NamedDecl nor "
         "CXXCtorInitializer");
   }
   llvm_unreachable("Unexpected case in NodePart type.");
 }
 
-Expected<Transformation>
-tooling::applyRewriteRule(const RewriteRule &Rule,
-                          const ast_matchers::MatchFinder::MatchResult &Match) {
-  if (Match.Context->getDiagnostics().hasErrorOccurred())
-    return Transformation();
-
-  auto &NodesMap = Match.Nodes.getMap();
-  auto It = NodesMap.find(Rule.Target);
-  assert (It != NodesMap.end() && "Rule.Target must be bound in the match.");
-
-  Expected<CharSourceRange> TargetOrErr =
-      getTargetRange(Rule.Target, It->second, Rule.TargetKind, Rule.TargetPart,
-                     *Match.Context);
-  if (auto Err = TargetOrErr.takeError())
+RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
+                              SmallVector<TextChange, 1> Changes) {
+  M.setAllowBind(true);
+  // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
+  return RewriteRule{*M.tryBind(RewriteRule::RootId), std::move(Changes)};
+}
+
+Expected<SmallVector<Transformation, 1>>
+tooling::applyRewriteRule(const MatchResult &Result,
+                          llvm::ArrayRef<TextChange> Changes) {
+  SmallVector<Transformation, 1> Transformations;
+  auto &NodesMap = Result.Nodes.getMap();
+  for (const auto &Change : Changes) {
+    auto It = NodesMap.find(Change.Target);
+    assert(It != NodesMap.end() && "Change target must be bound in the match.");
+
+    Expected<CharSourceRange> RangeOrErr = getTargetRange(
+        Change.Target, It->second, Change.Kind, Change.Part, *Result.Context);
+    if (auto Err = RangeOrErr.takeError())
       return std::move(Err);
-  auto &Target = *TargetOrErr;
-  if (Target.isInvalid() ||
-      isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
-    return Transformation();
-
-  return Transformation{Target, Rule.Replacement(Match)};
+    auto &TargetRange = *RangeOrErr;
+    if (TargetRange.isInvalid() ||
+        isOriginMacroBody(*Result.SourceManager, TargetRange.getBegin()))
+      return SmallVector<Transformation, 0>();
+    Transformations.emplace_back(TargetRange, Change.Replacement(Result));
+  }
+  return Transformations;
 }
 
 constexpr llvm::StringLiteral RewriteRule::RootId;
 
-RewriteRuleBuilder RewriteRuleBuilder::replaceWith(TextGenerator T) {
-  Rule.Replacement = std::move(T);
+TextChangeBuilder TextChangeBuilder::to(TextGenerator T) {
+  Change.Replacement = std::move(T);
   return *this;
 }
 
-RewriteRuleBuilder RewriteRuleBuilder::because(TextGenerator T) {
-  Rule.Explanation = std::move(T);
+TextChangeBuilder TextChangeBuilder::because(TextGenerator T) {
+  Change.Explanation = std::move(T);
   return *this;
 }
 
 void Transformer::registerMatchers(MatchFinder *MatchFinder) {
   MatchFinder->addDynamicMatcher(Rule.Matcher, this);
 }
 
 void Transformer::run(const MatchResult &Result) {
-  auto ChangeOrErr = applyRewriteRule(Rule, Result);
-  if (auto Err = ChangeOrErr.takeError()) {
-    llvm::errs() << "Rewrite failed: " << llvm::toString(std::move(Err))
+  if (Result.Context->getDiagnostics().hasErrorOccurred())
+    return;
+
+  // Verify the existence and validity of the AST node that roots this change.
+  auto &NodesMap = Result.Nodes.getMap();
+  auto Root = NodesMap.find(RewriteRule::RootId);
+  assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
+  SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
+      Root->second.getSourceRange().getBegin());
+  assert(RootLoc.isValid() && "Invalid location for Root node of match.");
+
+  auto TransformationsOrErr = applyRewriteRule(Result, Rule.Changes);
+  if (auto Err = TransformationsOrErr.takeError()) {
+    llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
                  << "\n";
     return;
   }
-  auto &Change = *ChangeOrErr;
-  auto &Range = Change.Range;
-  if (Range.isInvalid()) {
+  auto &Transformations = *TransformationsOrErr;
+  if (Transformations.empty()) {
     // No rewrite applied (but no error encountered either).
+    RootLoc.print(llvm::errs() << "note: skipping match at loc ",
+                  *Result.SourceManager);
+    llvm::errs() << "\n";
     return;
   }
-  AtomicChange AC(*Result.SourceManager, Range.getBegin());
-  if (auto Err = AC.replace(*Result.SourceManager, Range, Change.Replacement))
+
+  // Convert the result to an AtomicChange.
+  AtomicChange AC(*Result.SourceManager, RootLoc);
+  for (const auto &T : Transformations)
+    if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
       AC.setError(llvm::toString(std::move(Err)));
+      break;
+    }
+
   Consumer(AC);
 }

clang/unittests/Tooling/TransformerTest.cpp

 //===- unittest/Tooling/TransformerTest.cpp -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Refactoring/Transformer.h"
 
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
-namespace clang {
-namespace tooling {
-namespace {
-using ast_matchers::anyOf;
-using ast_matchers::argumentCountIs;
-using ast_matchers::callee;
-using ast_matchers::callExpr;
-using ast_matchers::cxxMemberCallExpr;
-using ast_matchers::cxxMethodDecl;
-using ast_matchers::cxxRecordDecl;
-using ast_matchers::declRefExpr;
-using ast_matchers::expr;
-using ast_matchers::functionDecl;
-using ast_matchers::hasAnyName;
-using ast_matchers::hasArgument;
-using ast_matchers::hasDeclaration;
-using ast_matchers::hasElse;
-using ast_matchers::hasName;
-using ast_matchers::hasType;
-using ast_matchers::ifStmt;
-using ast_matchers::member;
-using ast_matchers::memberExpr;
-using ast_matchers::namedDecl;
-using ast_matchers::on;
-using ast_matchers::pointsTo;
-using ast_matchers::to;
-using ast_matchers::unless;
 
-using llvm::StringRef;
+using namespace clang;
+using namespace tooling;
+using namespace ast_matchers;
 
+namespace {
 constexpr char KHeaderContents[] = R"cc(
   struct string {
     string(const char*);
     char* c_str();
     int size();
   };
   int strlen(const char*);
 
   namespace proto {
   struct PCFProto {
     int foo();
   };
   struct ProtoCommandLineFlag : PCFProto {
     PCFProto& GetProto();
   };
   }  // namespace proto
+  class Logger {};
+  void operator<<(Logger& l, string msg);
+  Logger& log(int level);
 )cc";
 
 static ast_matchers::internal::Matcher<clang::QualType>
 isOrPointsTo(const clang::ast_matchers::DeclarationMatcher &TypeMatcher) {
   return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
 }
 
 static std::string format(StringRef Code) {

 protected:
   TransformerTest() { appendToHeader(KHeaderContents); }
 };
 
 // Given string s, change strlen($s.c_str()) to $s.size().
 static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::string"));
-  return buildRule(
-             callExpr(
-                 callee(functionDecl(hasName("strlen"))),
-                 hasArgument(0, cxxMemberCallExpr(
-                                    on(expr(hasType(isOrPointsTo(StringType)))
-                                           .bind(StringExpr)),
-                                    callee(cxxMethodDecl(hasName("c_str")))))))
-      // Specify the intended type explicitly, because the matcher "type" of
-      // `callExpr()` is `Stmt`, not `Expr`.
-      .as<clang::Expr>()
-      .replaceWith("REPLACED")
-      .because("Use size() method directly on string.");
+  return makeRule(
+      callExpr(callee(functionDecl(hasName("strlen"))),
+                                  callee(cxxMethodDecl(hasName("c_str")))))),
+      changeRoot<clang::Expr>()
+          .to("REPLACED")
+          .because("Use size() method directly on string."));
 }
 
 TEST_F(TransformerTest, StrlenSize) {
   std::string Input = "int f(string s) { return strlen(s.c_str()); }";
   std::string Expected = "int f(string s) { return REPLACED; }";
   testRule(ruleStrlenSize(), Input, Expected);
 }
 

 #define ID(e) e
     int f(string s) { return ID(REPLACED); })cc";
   testRule(ruleStrlenSize(), Input, Expected);
 }
 
 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
-  RewriteRule Rule =
-      buildRule(
-          cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(hasName(
-                                        "proto::ProtoCommandLineFlag"))))
-                                   .bind(Flag)),
-                            unless(callee(cxxMethodDecl(hasName("GetProto"))))))
-          .change<clang::Expr>(Flag)
-          .replaceWith("EXPR")
-          .because("Use GetProto() to access proto fields.");
+  RewriteRule Rule = makeRule(
+      cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
+                                    hasName("proto::ProtoCommandLineFlag"))))
+                        unless(callee(cxxMethodDecl(hasName("GetProto"))))),
+      changeTextOf<clang::Expr>(Flag).to("EXPR").because(
+          "Use GetProto() to access proto fields."));
 
   std::string Input = R"cc(
     proto::ProtoCommandLineFlag flag;
     int x = flag.foo();
     int y = flag.GetProto().foo();
   )cc";
   std::string Expected = R"cc(
     proto::ProtoCommandLineFlag flag;
     int x = EXPR.foo();
     int y = flag.GetProto().foo();
   )cc";
 
   testRule(std::move(Rule), Input, Expected);
 }
 
 TEST_F(TransformerTest, NodePartNameNamedDecl) {
   StringRef Fun = "fun";
-  RewriteRule Rule = buildRule(functionDecl(hasName("bad")).bind(Fun))
-                         .change<clang::FunctionDecl>(Fun, NodePart::Name)
-                         .replaceWith("good");
+  RewriteRule Rule = makeRule(
+      functionDecl(hasName("bad")).bind(Fun),
+      changeTextOf<clang::FunctionDecl>(Fun, NodePart::Name).to("good"));
 
   std::string Input = R"cc(
     int bad(int x);
     int bad(int x) { return x * x; }
   )cc";
   std::string Expected = R"cc(
     int good(int x);
     int good(int x) { return x * x; }
std::string Expected = R"cc(
     template <typename T>
     T bad(T x) {
       return x;
     }
     int neutral(int x) { return good<int>(x) * x; }
   )cc";
 
   StringRef Ref = "ref";
-  testRule(buildRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref))
-               .change<clang::Expr>(Ref, NodePart::Name)
-               .replaceWith("good"),
+  testRule(makeRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref),
+                    changeTextOf<clang::Expr>(Ref, NodePart::Name).to("good")),
            Input, Expected);
 }
 
 TEST_F(TransformerTest, NodePartNameDeclRefFailure) {
   std::string Input = R"cc(
     struct Y {
       int operator*();
     };
     int neutral(int x) {
       Y y;
       int (Y::*ptr)() = &Y::operator*;
       return *y + x;
     }
   )cc";
 
   StringRef Ref = "ref";
-  testRule(buildRule(declRefExpr(to(functionDecl())).bind(Ref))
-               .change<clang::Expr>(Ref, NodePart::Name)
-               .replaceWith("good"),
+  testRule(makeRule(declRefExpr(to(functionDecl())).bind(Ref),
+                    changeTextOf<clang::Expr>(Ref, NodePart::Name).to("good")),
            Input, Input);
 }
 
 TEST_F(TransformerTest, NodePartMember) {
   StringRef E = "expr";
-  RewriteRule Rule = buildRule(memberExpr(member(hasName("bad"))).bind(E))
-                         .change<clang::Expr>(E, NodePart::Member)
-                         .replaceWith("good");
+  RewriteRule Rule =
+      makeRule(memberExpr(member(hasName("bad"))).bind(E),
+               changeTextOf<clang::Expr>(E, NodePart::Member).to("good"));
 
   std::string Input = R"cc(
     struct S {
       int bad;
     };
     int g() {
       S s;
       return s.bad;
std::string Expected = R"cc(
     };
     int g() {
       T t;
       return t.S::good;
     }
   )cc";
 
   StringRef E = "expr";
-  testRule(buildRule(memberExpr().bind(E))
-               .change<clang::Expr>(E, NodePart::Member)
-               .replaceWith("good"),
+  testRule(makeRule(memberExpr().bind(E),
+                    changeTextOf<clang::Expr>(E, NodePart::Member).to("good")),
            Input, Expected);
 }
 
 TEST_F(TransformerTest, NodePartMemberMultiToken) {
   std::string Input = R"cc(
     struct Y {
       int operator*();
       int good();
std::string Expected = R"cc(
     int neutral(int x) {
       Y y;
       y.template good<int>(3);
       return y.good();
     }
   )cc";
 
   StringRef MemExpr = "member";
-  testRule(buildRule(memberExpr().bind(MemExpr))
-               .change<clang::Expr>(MemExpr, NodePart::Member)
-               .replaceWith("good"),
+  testRule(
+      makeRule(memberExpr().bind(MemExpr),
+               changeTextOf<clang::Expr>(MemExpr, NodePart::Member).to("good")),
+      Input, Expected);
+}
+
+TEST_F(TransformerTest, MultiChange) {
+  std::string Input = R"cc(
+    void foo() {
+      if (10 > 1.0)
+        log(1) << "oh no!";
+      else
+        log(0) << "ok";
+    }
+  )cc";
+  std::string Expected = R"cc(
+    void foo() {
+      if (true) { /* then */ }
+      else { /* else */ }
+    }
+  )cc";
+
+  StringRef C = "C", T = "T", E = "E";
+  testRule(makeRule(ifStmt(hasCondition(expr().bind(C)),
+                           hasThen(stmt().bind(T)), hasElse(stmt().bind(E))),
+                    {changeTextOf<Expr>(C).to("true"),
+                     changeTextOf<Stmt>(T).to("{ /* then */ }"),
+                     changeTextOf<Stmt>(E).to("{ /* else */ }")}),
            Input, Expected);
 }
 
 //
 // Negative tests (where we expect no transformation to occur).
 //
 
 TEST_F(TransformerTest, NoTransformationInMacro) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
     int f(string s) { return MACRO(s); })cc";
   testRule(ruleStrlenSize(), Input, Input);
 }
 
 // This test handles the corner case where a macro called within another macro
 // expands to matching code, but the matched code is an argument to the nested
 // macro.  A simple check of isMacroArgExpansion() vs. isMacroBodyExpansion()
 // will get this wrong, and transform the code. This test verifies that no such
 // transformation occurs.
 TEST_F(TransformerTest, NoTransformationInNestedMacro) {
   std::string Input = R"cc(
 #define NESTED(e) e
 #define MACRO(str) NESTED(strlen((str).c_str()))
     int f(string s) { return MACRO(s); })cc";
   testRule(ruleStrlenSize(), Input, Input);
 }
 } // namespace
-} // namespace tooling
-} // namespace clang