[LibTooling] Extend Transformer to support multiple simultaneous changes.
authorYitzhak Mandelbaum <yitzhakm@google.com>
Thu, 18 Apr 2019 17:52:24 +0000 (17:52 +0000)
committerYitzhak Mandelbaum <yitzhakm@google.com>
Thu, 18 Apr 2019 17:52:24 +0000 (17:52 +0000)
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.

Reviewers: ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: jfb, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D60408

llvm-svn: 358697

clang/include/clang/Tooling/Refactoring/Transformer.h
clang/lib/Tooling/Refactoring/Transformer.cpp
clang/unittests/Tooling/TransformerTest.cpp

index bae1a75..e99e279 100644 (file)
 #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 {
@@ -47,6 +47,98 @@ enum class NodePart {
 using TextGenerator =
     std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
 
+/// Wraps a string as a TextGenerator.
+inline TextGenerator text(std::string M) {
+  return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+}
+
+// Description of a source-code edit, expressed in terms of an AST node.
+// Includes: an ID for the (bound) node, a selector for source related to the
+// node, a replacement and, optionally, an explanation for the edit.
+//
+// * 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.
+//
+// * Note: (optional) a note specifically for this edit, potentially referencing
+//   elements of the match.  This will be displayed to the user, where possible;
+//   for example, in clang-tidy diagnostics.  Use of notes should be rare --
+//   explanations of the entire rewrite should be set in the rule
+//   (`RewriteRule::Explanation`) instead.  Notes serve the rare cases wherein
+//   edit-specific diagnostics are required.
+//
+// `ASTEdit` should be built using the `change` convenience fucntions. For
+// example,
+// \code
+//   change<FunctionDecl>(fun, NodePart::Name, "Frodo")
+// \endcode
+// Or, if we use Stencil for the TextGenerator:
+// \code
+//   change<Stmt>(thenNode, stencil::cat("{", thenNode, "}"))
+//   change<Expr>(call, NodePart::Args, stencil::cat(x, ",", y))
+//     .note("argument order changed.")
+// \endcode
+// Or, if you are changing the node corresponding to the rule's matcher, you can
+// use the single-argument override of \c change:
+// \code
+//   change<Expr>("different_expr")
+// \endcode
+struct ASTEdit {
+  // 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 Note;
+};
+
+// Convenience functions for creating \c ASTEdits.  They all must be explicitly
+// instantiated with the desired AST type.  Each overload includes both \c
+// std::string and \c TextGenerator versions.
+
+// FIXME: For overloads taking a \c NodePart, constrain the valid values of \c
+// Part based on the type \c T.
+template <typename T>
+ASTEdit change(StringRef Target, NodePart Part, TextGenerator Replacement) {
+  ASTEdit E;
+  E.Target = Target.str();
+  E.Kind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
+  E.Part = Part;
+  E.Replacement = std::move(Replacement);
+  return E;
+}
+
+template <typename T>
+ASTEdit change(StringRef Target, NodePart Part, std::string Replacement) {
+  return change<T>(Target, Part, text(std::move(Replacement)));
+}
+
+/// Variant of \c change for which the NodePart defaults to the whole node.
+template <typename T>
+ASTEdit change(StringRef Target, TextGenerator Replacement) {
+  return change<T>(Target, NodePart::Node, std::move(Replacement));
+}
+
+/// Variant of \c change for which the NodePart defaults to the whole node.
+template <typename T>
+ASTEdit change(StringRef Target, std::string Replacement) {
+  return change<T>(Target, text(std::move(Replacement)));
+}
+
+/// Variant of \c change that selects the node of the entire match.
+template <typename T> ASTEdit change(TextGenerator Replacement);
+
+/// Variant of \c change that selects the node of the entire match.
+template <typename T> ASTEdit change(std::string Replacement) {
+  return change<T>(text(std::move(Replacement)));
+}
+
 /// Description of a source-code transformation.
 //
 // A *rewrite rule* describes a transformation of source code. It has the
@@ -55,19 +147,10 @@ using TextGenerator =
 // * 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.
+// * Edits: a set of Edits to the source code, described with ASTEdits.
 //
 // * Explanation: explanation of the rewrite.  This will be displayed to the
-//   user, where possible (for example, in clang-tidy fix descriptions).
+//   user, where possible; for example, in clang-tidy diagnostics.
 //
 // Rules have an additional, implicit, component: the parameters. These are
 // portions of the pattern which are left unspecified, yet named so that we can
@@ -77,92 +160,37 @@ using TextGenerator =
 // 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;
+  // at least `RootId`.
+  ast_matchers::internal::DynTypedMatcher Matcher;
+  SmallVector<ASTEdit, 1> Edits;
   TextGenerator Explanation;
 
   // 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.
+  // matcher is should always be bound to this id.
   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;
-  }
-
-  /// (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));
+/// Convenience function for constructing a \c RewriteRule. Takes care of
+/// binding the matcher to RootId.
+RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                     SmallVector<ASTEdit, 1> Edits);
+
+/// Convenience overload of \c makeRule for common case of only one edit.
+inline RewriteRule makeRule(ast_matchers::internal::DynTypedMatcher M,
+                            ASTEdit Edit) {
+  SmallVector<ASTEdit, 1> Edits;
+  Edits.emplace_back(std::move(Edit));
+  return makeRule(std::move(M), std::move(Edits));
 }
 
-template <typename T> RewriteRuleBuilder RewriteRuleBuilder::as() {
-  Rule.TargetKind = ast_type_traits::ASTNodeKind::getFromNodeKind<T>();
-  return *this;
-}
-
-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;
+// Define this overload of `change` here because RewriteRule::RootId is not in
+// scope at the declaration point above.
+template <typename T> ASTEdit change(TextGenerator Replacement) {
+  return change<T>(RewriteRule::RootId, NodePart::Node, std::move(Replacement));
 }
 
 /// A source "transformation," represented by a character range in the source to
@@ -172,13 +200,22 @@ struct Transformation {
   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
+/// Attempts to translate `Edits`, which are in terms of AST nodes bound in the
+/// match `Result`, into Transformations, which are in terms of the source code
+/// text.  This function is a low-level part of the API, provided to support
+/// interpretation of a \c RewriteRule in a tool, like \c Transformer, rather
+/// than direct use by end users.
+///
+/// Returns an empty vector if any of the edits apply to portions of the source
+/// 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<Transformation>
-applyRewriteRule(const RewriteRule &Rule,
-                 const ast_matchers::MatchFinder::MatchResult &Match);
+/// the match.  However, it does not fail in the case of conflicting edits --
+/// conflict handling is left to clients.  We recommend use of the \c
+/// AtomicChange or \c Replacements classes for assistance in detecting such
+/// conflicts.
+Expected<SmallVector<Transformation, 1>>
+translateEdits(const ast_matchers::MatchFinder::MatchResult &Result,
+               llvm::ArrayRef<ASTEdit> Edits);
 
 /// Handles the matcher and callback registration for a single rewrite rule, as
 /// defined by the arguments of the constructor.
@@ -187,7 +224,9 @@ public:
   using ChangeConsumer =
       std::function<void(const clang::tooling::AtomicChange &Change)>;
 
-  /// \param Consumer Receives each successful rewrites as an \c AtomicChange.
+  /// \param Consumer Receives each successful rewrite as an \c AtomicChange.
+  /// Note that clients are responsible for handling the case that independent
+  /// \c AtomicChanges conflict with each other.
   Transformer(RewriteRule Rule, ChangeConsumer Consumer)
       : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
 
index b46fa3e..3a7a3ff 100644 (file)
@@ -144,60 +144,78 @@ getTargetRange(StringRef Target, const DynTypedNode &Node, ASTNodeKind Kind,
   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())
-    return std::move(Err);
-  auto &Target = *TargetOrErr;
-  if (Target.isInvalid() ||
-      isOriginMacroBody(*Match.SourceManager, Target.getBegin()))
-    return Transformation();
-
-  return Transformation{Target, Rule.Replacement(Match)};
+Expected<SmallVector<Transformation, 1>>
+tooling::translateEdits(const MatchResult &Result,
+                        llvm::ArrayRef<ASTEdit> Edits) {
+  SmallVector<Transformation, 1> Transformations;
+  auto &NodesMap = Result.Nodes.getMap();
+  for (const auto &Edit : Edits) {
+    auto It = NodesMap.find(Edit.Target);
+    assert(It != NodesMap.end() && "Edit target must be bound in the match.");
+
+    Expected<CharSourceRange> RangeOrErr = getTargetRange(
+        Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context);
+    if (auto Err = RangeOrErr.takeError())
+      return std::move(Err);
+    Transformation T;
+    T.Range = *RangeOrErr;
+    if (T.Range.isInvalid() ||
+        isOriginMacroBody(*Result.SourceManager, T.Range.getBegin()))
+      return SmallVector<Transformation, 0>();
+    T.Replacement = Edit.Replacement(Result);
+    Transformations.push_back(std::move(T));
+  }
+  return Transformations;
 }
 
-constexpr llvm::StringLiteral RewriteRule::RootId;
-
-RewriteRuleBuilder RewriteRuleBuilder::replaceWith(TextGenerator T) {
-  Rule.Replacement = std::move(T);
-  return *this;
+RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
+                              SmallVector<ASTEdit, 1> Edits) {
+  M.setAllowBind(true);
+  // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
+  return RewriteRule{*M.tryBind(RewriteRule::RootId), std::move(Edits)};
 }
 
-RewriteRuleBuilder RewriteRuleBuilder::because(TextGenerator T) {
-  Rule.Explanation = std::move(T);
-  return *this;
-}
+constexpr llvm::StringLiteral RewriteRule::RootId;
 
 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 rule.
+  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 = translateEdits(Result, Rule.Edits);
+  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))
-    AC.setError(llvm::toString(std::move(Err)));
+
+  // 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);
 }
index a25ce32..b35c195 100644 (file)
 #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*);
@@ -59,6 +34,9 @@ constexpr char KHeaderContents[] = R"cc(
     PCFProto& GetProto();
   };
   }  // namespace proto
+  class Logger {};
+  void operator<<(Logger& l, string msg);
+  Logger& log(int level);
 )cc";
 
 static ast_matchers::internal::Matcher<clang::QualType>
@@ -141,18 +119,15 @@ protected:
 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.");
+  auto R = makeRule(
+      callExpr(callee(functionDecl(hasName("strlen"))),
+               hasArgument(0, cxxMemberCallExpr(
+                                  on(expr(hasType(isOrPointsTo(StringType)))
+                                         .bind(StringExpr)),
+                                  callee(cxxMethodDecl(hasName("c_str")))))),
+      change<clang::Expr>("REPLACED"));
+  R.Explanation = text("Use size() method directly on string.");
+  return R;
 }
 
 TEST_F(TransformerTest, StrlenSize) {
@@ -181,15 +156,12 @@ TEST_F(TransformerTest, StrlenSizeMacro) {
 // 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"))))
+                               .bind(Flag)),
+                        unless(callee(cxxMethodDecl(hasName("GetProto"))))),
+      change<clang::Expr>(Flag, "EXPR"));
 
   std::string Input = R"cc(
     proto::ProtoCommandLineFlag flag;
@@ -207,9 +179,9 @@ TEST_F(TransformerTest, Flag) {
 
 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),
+               change<clang::FunctionDecl>(Fun, NodePart::Name, "good"));
 
   std::string Input = R"cc(
     int bad(int x);
@@ -240,9 +212,8 @@ TEST_F(TransformerTest, NodePartNameDeclRef) {
   )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),
+                    change<clang::Expr>(Ref, NodePart::Name, "good")),
            Input, Expected);
 }
 
@@ -259,17 +230,15 @@ TEST_F(TransformerTest, NodePartNameDeclRefFailure) {
   )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),
+                    change<clang::Expr>(Ref, NodePart::Name, "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),
+                              change<clang::Expr>(E, NodePart::Member, "good"));
 
   std::string Input = R"cc(
     struct S {
@@ -322,9 +291,8 @@ TEST_F(TransformerTest, NodePartMemberQualified) {
   )cc";
 
   StringRef E = "expr";
-  testRule(buildRule(memberExpr().bind(E))
-               .change<clang::Expr>(E, NodePart::Member)
-               .replaceWith("good"),
+  testRule(makeRule(memberExpr().bind(E),
+                    change<clang::Expr>(E, NodePart::Member, "good")),
            Input, Expected);
 }
 
@@ -355,9 +323,32 @@ TEST_F(TransformerTest, NodePartMemberMultiToken) {
   )cc";
 
   StringRef MemExpr = "member";
-  testRule(buildRule(memberExpr().bind(MemExpr))
-               .change<clang::Expr>(MemExpr, NodePart::Member)
-               .replaceWith("good"),
+  testRule(makeRule(memberExpr().bind(MemExpr),
+                    change<clang::Expr>(MemExpr, NodePart::Member, "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"(
+    void foo() {
+      if (true) { /* then */ }
+      else { /* else */ }
+    }
+  )";
+
+  StringRef C = "C", T = "T", E = "E";
+  testRule(makeRule(ifStmt(hasCondition(expr().bind(C)),
+                           hasThen(stmt().bind(T)), hasElse(stmt().bind(E))),
+                    {change<Expr>(C, "true"), change<Stmt>(T, "{ /* then */ }"),
+                     change<Stmt>(E, "{ /* else */ }")}),
            Input, Expected);
 }
 
@@ -365,6 +356,52 @@ TEST_F(TransformerTest, NodePartMemberMultiToken) {
 // Negative tests (where we expect no transformation to occur).
 //
 
+// Tests for a conflict in edits from a single match for a rule.
+TEST_F(TransformerTest, OverlappingEditsInRule) {
+  std::string Input = "int conflictOneRule() { return 3 + 7; }";
+  // Try to change the whole binary-operator expression AND one its operands:
+  StringRef O = "O", L = "L";
+  Transformer T(
+      makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
+               {change<Expr>(O, "DELETE_OP"), change<Expr>(L, "DELETE_LHS")}),
+      [this](const AtomicChange &C) { Changes.push_back(C); });
+  T.registerMatchers(&MatchFinder);
+  // The rewrite process fails...
+  EXPECT_TRUE(rewrite(Input));
+  // ... but one AtomicChange was consumed:
+  ASSERT_EQ(Changes.size(), 1);
+  EXPECT_TRUE(Changes[0].hasError());
+}
+
+// Tests for a conflict in edits across multiple matches (of the same rule).
+TEST_F(TransformerTest, OverlappingEditsMultipleMatches) {
+  std::string Input = "int conflictOneRule() { return -7; }";
+  // Try to change the whole binary-operator expression AND one its operands:
+  StringRef E = "E";
+  Transformer T(makeRule(expr().bind(E), change<Expr>(E, "DELETE_EXPR")),
+                [this](const AtomicChange &C) { Changes.push_back(C); });
+  T.registerMatchers(&MatchFinder);
+  // The rewrite process fails because the changes conflict with each other...
+  EXPECT_FALSE(rewrite(Input));
+  // ... but all changes are (individually) fine:
+  ASSERT_EQ(Changes.size(), 2);
+  EXPECT_FALSE(Changes[0].hasError());
+  EXPECT_FALSE(Changes[1].hasError());
+}
+
+TEST_F(TransformerTest, ErrorOccurredMatchSkipped) {
+  // Syntax error in the function body:
+  std::string Input = "void errorOccurred() { 3 }";
+  Transformer T(
+      makeRule(functionDecl(hasName("errorOccurred")), change<Decl>("DELETED;")),
+      [this](const AtomicChange &C) { Changes.push_back(C); });
+  T.registerMatchers(&MatchFinder);
+  // The rewrite process itself fails...
+  EXPECT_FALSE(rewrite(Input));
+  // ... and no changes are produced in the process.
+  EXPECT_THAT(Changes, ::testing::IsEmpty());
+}
+
 TEST_F(TransformerTest, NoTransformationInMacro) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
@@ -385,5 +422,3 @@ TEST_F(TransformerTest, NoTransformationInNestedMacro) {
   testRule(ruleStrlenSize(), Input, Input);
 }
 } // namespace
-} // namespace tooling
-} // namespace clang