[LibTooling] Update Transformer to use RangeSelector instead of NodePart enum.

[ymandel]

Summary

Transformer provides an enum to indicate the range of source text to be edited.
That support is now redundant with the new (and more general) RangeSelector
library, so we remove the custom enum support in favor of supporting any
RangeSelector.

[ymandel]

reordered some declarations.

[ilya-biryukov]

Looks neat! The only concern I have is about the growing number of overloads in Transformer.h, see the relevant comment.

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

209

The overloads create quite a lot of noise (and we are already starting to see a combinatorial explosion with two classes).

Could we try to overcome this? I see two options:

1. Turn TextGenerator and RangeSelector into classes, which are constructible from string or function<string(MatchResult)>.
2. Remove the string overloads, force users to explicitly construct the parameters.

Option (2) actually is simple and would add a bit of type safety, but will make some client code a bit less readable. Do you think the readability hit is significant?

[ymandel]

Removed most change overloads.

[ymandel]

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

209

Agreed. I wasn't loving all the overloads myself. I removed all the string-related overloads. left the one overload that lets the user implicitly specify the entirety of the match.

I'm ok with the result. Certainly for RangeSelector, it forces a consistency which I like. For the use of text() -- I could go both ways, but i think pure text replacements will be rare outside of tests, so I'm not sure users will be terribly inconvenienced by this.

The only overload i'm unsure about is the one I left, because I think a) it will be a common case and b) it will confuse users to learn to use RewriteRule::RootID. That said, if you have an elegant, but explicit, alternative I'm interested. We could, for example, add combinators matchedNode() and matchedStatement(), defined respectively as node(RewriteRule::RootID) and statement(RewriteRule::RootID). Or, add a`makeRule` overload for the node() case (and let users be explicit when they want the statement() case.).

[ilya-biryukov]

LGTM. See the nit about the comment, might be a good idea to fix it before landing.

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

207

NIT: maybe clarify that "matched portion" is the whole match of a rewrite rule.
It looks like "portion" in the previous overload means "range defined by the selector", while in this overload it means "the whole matched node". It'd be nice to use different words there to avoid confusion and make the comment more useful

209

This one overload actually looks ok and I also agree it'll probably be somewhat common.
Thanks!

[ymandel]

Updated comments.

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 "clang/Tooling/Refactoring/RangeSelector.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>
 
 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,
-};
 
 // Note that \p TextGenerator is allowed to fail, e.g. when trying to access a
 // matched node that was not bound.  Allowing this to fail simplifies error
 // handling for interactive tools like clang-query.
 using TextGenerator = std::function<Expected<std::string>(
     const ast_matchers::MatchFinder::MatchResult &)>;
 
 /// Wraps a string as a TextGenerator.

 //     .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;
+  RangeSelector TargetRange;
   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. A simple rule
 // contains each of the following components:
 //
 // * Matcher: the pattern term, expressed as clang matchers (with Transformer
 //   extensions).
 //
struct RewriteRule {
   struct Case {
     ast_matchers::internal::DynTypedMatcher Matcher;
     SmallVector<ASTEdit, 1> Edits;
     TextGenerator Explanation;
   };
   // We expect RewriteRules will most commonly include only one case.
   SmallVector<Case, 1> Cases;
 
-  // Id used as the default target of each match. The node described by the
+  // ID used as the default target of each match. The node described by the
   // matcher is should always be bound to this id.
-  static constexpr llvm::StringLiteral RootId = "___root___";
+  static constexpr llvm::StringLiteral RootID = "___root___";
 };
 
+/// Replaces a portion of the source text with \p Replacement.
+ASTEdit change(RangeSelector Target, TextGenerator Replacement);
+
+/// These overloads of \c change allow the user to pass a string-type argument
+/// where a \c TextGenerator, \c RangeSelector are otherwise expected.
+inline ASTEdit change(RangeSelector Target, std::string Replacement) {
+  return change(std::move(Target), text(std::move(Replacement)));
+}
+inline ASTEdit change(StringRef Target, TextGenerator Replacement) {
+  return change(node(Target), std::move(Replacement));
+}
+inline ASTEdit change(StringRef Target, std::string Replacement) {
+  return change(node(Target), text(Replacement));
+}
+
+/// These overloads implicitly identify the matched portion of a RewriteRule as
+/// the target of the change.
+inline ASTEdit change(std::string Replacement) {
+  return change(RewriteRule::RootID, std::move(Replacement));
+}
+inline ASTEdit change(TextGenerator Replacement) {
+  return change(RewriteRule::RootID, std::move(Replacement));
+}
+
 /// Convenience function for constructing a simple \c RewriteRule.
 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;

 //     callExpr(callee(eq_fun), callExpr(hasArgument(1, method_call)));
 //
 // RewriteRule R = applyFirst({makeRule(two_calls, two_calls_action),
 //                             makeRule(left_call, left_call_action),
 //                             makeRule(right_call, right_call_action)});
 // ```
 RewriteRule applyFirst(ArrayRef<RewriteRule> Rules);
 
-// 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));
-}
-
 /// The following three functions are a low-level part of the RewriteRule
 /// API. We expose them for use in implementing the fixtures that interpret
 /// RewriteRule, like Transformer and TransfomerTidy, or for more advanced
 /// users.
 //
 // FIXME: These functions are really public, if advanced, elements of the
 // RewriteRule API.  Recast them as such.  Or, just declare these functions
 // public and well-supported and move them out of `detail`.
 namespace detail {
 /// Builds a single matcher for the rule, covering all of the rule's cases.
 ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
 
 /// Returns the \c Case of \c Rule that was selected in the match result.

clang/lib/Tooling/Refactoring/Transformer.cpp

 using namespace clang;
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
 using ast_matchers::internal::DynTypedMatcher;
 using ast_type_traits::ASTNodeKind;
 using ast_type_traits::DynTypedNode;
 using llvm::Error;
-using llvm::Expected;
-using llvm::Optional;
 using llvm::StringError;
-using llvm::StringRef;
-using llvm::Twine;
 
 using MatchResult = MatchFinder::MatchResult;
 
 // Did the text at this location originate in a macro definition (aka. body)?
 // For example,
 //
 //   #define NESTED(x) x
 //   #define MACRO(y) { int y  = NESTED(3); }
while (Loc.isMacroID()) {
     // Otherwise, it must be in an argument, so we continue searching up the
     // invocation stack. getImmediateMacroCallerLoc() gives the location of the
     // argument text, inside the call text.
     Loc = SM.getImmediateMacroCallerLoc(Loc);
   }
   return false;
 }
 
-static llvm::Error invalidArgumentError(Twine Message) {
-  return llvm::make_error<StringError>(llvm::errc::invalid_argument, Message);
-}
-
-static llvm::Error typeError(StringRef Id, const ASTNodeKind &Kind,
-                             Twine Message) {
-  return invalidArgumentError(
-      Message + " (node id=" + Id + " kind=" + Kind.asStringRef() + ")");
-}
-
-static llvm::Error missingPropertyError(StringRef Id, Twine Description,
-                                        StringRef Property) {
-  return invalidArgumentError(Description + " requires property '" + Property +
-                              "' (node id=" + Id + ")");
-}
-
-static Expected<CharSourceRange>
-getTargetRange(StringRef Target, const DynTypedNode &Node, ASTNodeKind Kind,
-               NodePart TargetPart, ASTContext &Context) {
-  switch (TargetPart) {
-  case NodePart::Node: {
-    // For non-expression statements, associate any trailing semicolon with the
-    // statement text.  However, if the target was intended as an expression (as
-    // indicated by its kind) then we do not associate any trailing semicolon
-    // with it.  We only associate the exact expression text.
-    if (Node.get<Stmt>() != nullptr) {
-      auto ExprKind = ASTNodeKind::getFromNodeKind<clang::Expr>();
-      if (!ExprKind.isBaseOf(Kind))
-        return getExtendedRange(Node, tok::TokenKind::semi, Context);
-    }
-    return CharSourceRange::getTokenRange(Node.getSourceRange());
-  }
-  case NodePart::Member:
-    if (auto *M = Node.get<clang::MemberExpr>())
-      return CharSourceRange::getTokenRange(
-          M->getMemberNameInfo().getSourceRange());
-    return typeError(Target, Node.getNodeKind(),
-                     "NodePart::Member applied to non-MemberExpr");
-  case NodePart::Name:
-    if (const auto *D = Node.get<clang::NamedDecl>()) {
-      if (!D->getDeclName().isIdentifier())
-        return missingPropertyError(Target, "NodePart::Name", "identifier");
-      SourceLocation L = D->getLocation();
-      auto R = CharSourceRange::getTokenRange(L, L);
-      // Verify that the range covers exactly the name.
-      // FIXME: extend this code to support cases like `operator +` or
-      // `foo<int>` for which this range will be too short.  Doing so will
-      // require subcasing `NamedDecl`, because it doesn't provide virtual
-      // access to the \c DeclarationNameInfo.
-      if (getText(R, Context) != D->getName())
-        return CharSourceRange();
-      return R;
-    }
-    if (const auto *E = Node.get<clang::DeclRefExpr>()) {
-      if (!E->getNameInfo().getName().isIdentifier())
-        return missingPropertyError(Target, "NodePart::Name", "identifier");
-      SourceLocation L = E->getLocation();
-      return CharSourceRange::getTokenRange(L, L);
-    }
-    if (const auto *I = Node.get<clang::CXXCtorInitializer>()) {
-      if (!I->isMemberInitializer() && I->isWritten())
-        return missingPropertyError(Target, "NodePart::Name",
-                                    "explicit member initializer");
-      SourceLocation L = I->getMemberLocation();
-      return CharSourceRange::getTokenRange(L, L);
-    }
-    return typeError(
-        Target, Node.getNodeKind(),
-        "NodePart::Name applied to neither DeclRefExpr, NamedDecl nor "
-        "CXXCtorInitializer");
-  }
-  llvm_unreachable("Unexpected case in NodePart type.");
-}
-
 Expected<SmallVector<tooling::detail::Transformation, 1>>
 tooling::detail::translateEdits(const MatchResult &Result,
                                 llvm::ArrayRef<ASTEdit> Edits) {
-  SmallVector<Transformation, 1> Transformations;
-  auto &NodesMap = Result.Nodes.getMap();
+  SmallVector<tooling::detail::Transformation, 1> Transformations;
   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> Range = getTargetRange(
-        Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context);
+    Expected<CharSourceRange> Range = Edit.TargetRange(Result);
     if (!Range)
       return Range.takeError();
     if (Range->isInvalid() ||
         isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
       return SmallVector<Transformation, 0>();
     auto Replacement = Edit.Replacement(Result);
     if (!Replacement)
       return Replacement.takeError();
-    Transformation T;
+    tooling::detail::Transformation T;
     T.Range = *Range;
     T.Replacement = std::move(*Replacement);
     Transformations.push_back(std::move(T));
   }
   return Transformations;
 }
 
+ASTEdit tooling::change(RangeSelector S, TextGenerator Replacement) {
+  ASTEdit E;
+  E.TargetRange = std::move(S);
+  E.Replacement = std::move(Replacement);
+  return E;
+}
+
 RewriteRule tooling::makeRule(DynTypedMatcher M,
                               SmallVector<ASTEdit, 1> Edits) {
   return RewriteRule{
       {RewriteRule::Case{std::move(M), std::move(Edits), nullptr}}};
 }
 
 // Determines whether A is a base type of B in the class hierarchy, including
 // the implicit relationship of Type and QualType.
static DynTypedMatcher joinCaseMatchers(const RewriteRule &Rule) {
   return DynTypedMatcher::constructVariadic(
       DynTypedMatcher::VO_AnyOf, CommonKind, taggedMatchers("Tag", Rule.Cases));
 }
 
 DynTypedMatcher tooling::detail::buildMatcher(const RewriteRule &Rule) {
   DynTypedMatcher M = joinCaseMatchers(Rule);
   M.setAllowBind(true);
   // `tryBind` is guaranteed to succeed, because `AllowBind` was set to true.
-  return *M.tryBind(RewriteRule::RootId);
+  return *M.tryBind(RewriteRule::RootID);
 }
 
 // Finds the case that was "selected" -- that is, whose matcher triggered the
 // `MatchResult`.
 const RewriteRule::Case &
 tooling::detail::findSelectedCase(const MatchResult &Result,
                                   const RewriteRule &Rule) {
   if (Rule.Cases.size() == 1)
     return Rule.Cases[0];
 
   auto &NodesMap = Result.Nodes.getMap();
   for (size_t i = 0, N = Rule.Cases.size(); i < N; ++i) {
     std::string Tag = ("Tag" + Twine(i)).str();
     if (NodesMap.find(Tag) != NodesMap.end())
       return Rule.Cases[i];
   }
   llvm_unreachable("No tag found for this rule.");
 }
 
-constexpr llvm::StringLiteral RewriteRule::RootId;
+constexpr llvm::StringLiteral RewriteRule::RootID;
 
 void Transformer::registerMatchers(MatchFinder *MatchFinder) {
   MatchFinder->addDynamicMatcher(tooling::detail::buildMatcher(Rule), this);
 }
 
 void Transformer::run(const MatchResult &Result) {
   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);
+  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 Transformations = tooling::detail::translateEdits(
       Result, tooling::detail::findSelectedCase(Result, Rule).Edits);
   if (!Transformations) {

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/Refactoring/RangeSelector.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
 using namespace tooling;
static RewriteRule ruleStrlenSize() {
   StringRef StringExpr = "strexpr";
   auto StringType = namedDecl(hasAnyName("::basic_string", "::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"));
+      change("REPLACED"));
   R.Cases[0].Explanation = text("Use size() method directly on string.");
   return R;
 }
 
 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);

 // Tests replacing an expression.
 TEST_F(TransformerTest, Flag) {
   StringRef Flag = "flag";
   RewriteRule Rule = makeRule(
       cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
                                     hasName("proto::ProtoCommandLineFlag"))))
                                .bind(Flag)),
                         unless(callee(cxxMethodDecl(hasName("GetProto"))))),
-      change<clang::Expr>(Flag, "EXPR"));
+      change(Flag, "EXPR"));
 
   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 =
-      makeRule(functionDecl(hasName("bad")).bind(Fun),
-               change<clang::FunctionDecl>(Fun, NodePart::Name, "good"));
+  RewriteRule Rule = makeRule(functionDecl(hasName("bad")).bind(Fun),
+                              change(name(Fun), "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(
     T bad(T x) {
       return x;
     }
     int neutral(int x) { return good<int>(x) * x; }
   )cc";
 
   StringRef Ref = "ref";
   testRule(makeRule(declRefExpr(to(functionDecl(hasName("bad")))).bind(Ref),
-                    change<clang::Expr>(Ref, NodePart::Name, "good")),
+                    change(name(Ref), "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";
   Transformer T(makeRule(declRefExpr(to(functionDecl())).bind(Ref),
-                         change<clang::Expr>(Ref, NodePart::Name, "good")),
+                         change(name(Ref), "good")),
                 consumer());
   T.registerMatchers(&MatchFinder);
   EXPECT_FALSE(rewrite(Input));
 }
 
 TEST_F(TransformerTest, NodePartMember) {
   StringRef E = "expr";
   RewriteRule Rule = makeRule(memberExpr(member(hasName("bad"))).bind(E),
-                              change<clang::Expr>(E, NodePart::Member, "good"));
+                              change(member(E), "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(makeRule(memberExpr().bind(E),
-                    change<clang::Expr>(E, NodePart::Member, "good")),
-           Input, Expected);
+  testRule(makeRule(memberExpr().bind(E), change(member(E), "good")), Input,
+           Expected);
 }
 
 TEST_F(TransformerTest, NodePartMemberMultiToken) {
   std::string Input = R"cc(
     struct Y {
       int operator*();
       int good();
       template <typename T> void foo(T t);
std::string Expected = R"cc(
     int neutral(int x) {
       Y y;
       y.template good<int>(3);
       return y.good();
     }
   )cc";
 
   StringRef MemExpr = "member";
-  testRule(makeRule(memberExpr().bind(MemExpr),
-                    change<clang::Expr>(MemExpr, NodePart::Member, "good")),
-           Input, Expected);
+  testRule(
+      makeRule(memberExpr().bind(MemExpr), change(member(MemExpr), "good")),
 }
 
 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 */ }")}),
+                    {change(C, "true"), change(statement(T), "{ /* then */ }"),
+                     change(statement(E), "{ /* else */ }")}),
            Input, Expected);
 }
 
 TEST_F(TransformerTest, OrderedRuleUnrelated) {
   StringRef Flag = "flag";
   RewriteRule FlagRule = makeRule(
       cxxMemberCallExpr(on(expr(hasType(cxxRecordDecl(
                                     hasName("proto::ProtoCommandLineFlag"))))
                                .bind(Flag)),
                         unless(callee(cxxMethodDecl(hasName("GetProto"))))),
-      change<clang::Expr>(Flag, "PROTO"));
+      change(Flag, "PROTO"));
 
   std::string Input = R"cc(
     proto::ProtoCommandLineFlag flag;
     int x = flag.foo();
     int y = flag.GetProto().foo();
     int f(string s) { return strlen(s.c_str()); }
   )cc";
   std::string Expected = R"cc(

 // ruleStrlenSize, so we can tell their effect apart.
 RewriteRule ruleStrlenSizeDistinct() {
   StringRef S;
   return makeRule(
       callExpr(callee(functionDecl(hasName("strlen"))),
                hasArgument(0, cxxMemberCallExpr(
                                   on(expr().bind(S)),
                                   callee(cxxMethodDecl(hasName("c_str")))))),
-      change<clang::Expr>("DISTINCT"));
+      change("DISTINCT"));
 }
 
 TEST_F(TransformerTest, OrderedRuleRelated) {
   std::string Input = R"cc(
     namespace foo {
     struct mystring {
       char* c_str();
     };

 TEST_F(TransformerTest, TextGeneratorFailure) {
   std::string Input = "int conflictOneRule() { return 3 + 7; }";
   // Try to change the whole binary-operator expression AND one its operands:
   StringRef O = "O";
   auto AlwaysFail = [](const ast_matchers::MatchFinder::MatchResult &)
       -> llvm::Expected<std::string> {
     return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
   };
-  Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(O, AlwaysFail)),
+  Transformer T(makeRule(binaryOperator().bind(O), change(O, AlwaysFail)),
                 consumer());
   T.registerMatchers(&MatchFinder);
   EXPECT_FALSE(rewrite(Input));
   EXPECT_THAT(Changes, IsEmpty());
   EXPECT_EQ(ErrorCount, 1);
 }
 
 // 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")}),
+  Transformer T(makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
+                         {change(O, "DELETE_OP"), change(L, "DELETE_LHS")}),
                 consumer());
   T.registerMatchers(&MatchFinder);
   EXPECT_FALSE(rewrite(Input));
   EXPECT_THAT(Changes, IsEmpty());
   EXPECT_EQ(ErrorCount, 1);
 }
 
 // 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")),
-                consumer());
+  Transformer T(makeRule(expr().bind(E), change(E, "DELETE_EXPR")), consumer());
   T.registerMatchers(&MatchFinder);
   // The rewrite process fails because the changes conflict with each other...
   EXPECT_FALSE(rewrite(Input));
   // ... but two changes were produced.
   EXPECT_EQ(Changes.size(), 2u);
   EXPECT_EQ(ErrorCount, 0);
 }
 
 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;")),
-                consumer());
+  Transformer T(
+      makeRule(functionDecl(hasName("errorOccurred")), change("DELETED;")),
   T.registerMatchers(&MatchFinder);
   // The rewrite process itself fails...
   EXPECT_FALSE(rewrite(Input));
   // ... and no changes or errors are produced in the process.
   EXPECT_THAT(Changes, IsEmpty());
   EXPECT_EQ(ErrorCount, 0);
 }