[libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.

[ymandel]

Summary

The new overloads apply directly to a node, like the
clang::ast_matchers::match functions, Rather than generating an
EditGenerator combinator.

[gribozavr2]

clang/include/clang/Tooling/Transformer/RewriteRule.h

388

I agree that these functions provide very useful functionality, however I feel uneasy about putting a function that returns an EditGenerator and a function that actually executes the refactoring into the same overload set. The first is a part of a DSL, the second is a regular function. Do you think this is a problem worth solving?

IDK what exactly to suggest though. A namespace for DSL functions like we have in AST matchers?

[ymandel]

Moved new rewriteDescendants overloads to detail namespace

[ymandel]

fix diff base; make small clang-tidy suggested change

[ymandel]

Thanks for the review!

clang/include/clang/Tooling/Transformer/RewriteRule.h

388

That's a really good point. These new functions are definitely a part of an (implicit) lower-level library that improves manipulation of the AST directly. We don't have any appropriate library yet for this -- SourceCode is in this spirit but even simpler than RewriteRule, which in fact depends on it.

For now, I'll put these in the detail which is (in some sense) the collection of low level functions for which we need to solve this same problem. WDYT? (I wasn't quite clear about the comparison with ast matchers, since both the matchers DSL and the match functions (on which this is loosely based) are in the same namespace).

[gribozavr2]

clang/include/clang/Tooling/Transformer/RewriteRule.h

388

I wasn't quite clear about the comparison with ast matchers, since both the matchers DSL and the match functions (on which this is loosely based) are in the same namespace

Oh right -- I mistakenly thought that clang::ast_matchers only contains the DSL. I think your choice to put functions into detail looks good.

clang/include/clang/Tooling/Transformer/RewriteRule.h

 /// 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 {
+/// The following overload set is a version of `rewriteDescendants` that
+/// operates directly on the AST, rather than generating a Transformer
+/// combinator. It applies `Rule` to all descendants of `Node`, although not
+/// `Node` itself. `Rule` can refer to nodes bound in `Result`.
+///
+/// For example, assuming that "body" is bound to a function body in MatchResult
+/// `Results`, this will produce edits to change all appearances of `x` in that
+/// body to `3`.
+/// ```
+/// auto InlineX =
+///     makeRule(declRefExpr(to(varDecl(hasName("x")))), changeTo(cat("3")));
+/// const auto *Node = Results.Nodes.getNodeAs<Stmt>("body");
+/// auto Edits = rewriteDescendants(*Node, InlineX, Results);
+/// ```
+/// @{
+llvm::Expected<SmallVector<Edit, 1>>
+rewriteDescendants(const Decl &Node, RewriteRule Rule,
+                   const ast_matchers::MatchFinder::MatchResult &Result);
+
+llvm::Expected<SmallVector<Edit, 1>>
+rewriteDescendants(const Stmt &Node, RewriteRule Rule,
+                   const ast_matchers::MatchFinder::MatchResult &Result);
+
+llvm::Expected<SmallVector<Edit, 1>>
+rewriteDescendants(const TypeLoc &Node, RewriteRule Rule,
+                   const ast_matchers::MatchFinder::MatchResult &Result);
+
+llvm::Expected<SmallVector<Edit, 1>>
+rewriteDescendants(const DynTypedNode &Node, RewriteRule Rule,
+                   const ast_matchers::MatchFinder::MatchResult &Result);
+/// @}
+
 /// Builds a single matcher for the rule, covering all of the rule's cases.
 /// Only supports Rules whose cases' matchers share the same base "kind"
 /// (`Stmt`, `Decl`, etc.)  Deprecated: use `buildMatchers` instead, which
 /// supports mixing matchers of different kinds.
 ast_matchers::internal::DynTypedMatcher buildMatcher(const RewriteRule &Rule);
 
 /// Builds a set of matchers that cover the rule.
 ///

clang/lib/Tooling/Transformer/RewriteRule.cpp

public:
   RewriteRule Rule;
 
   // Initialize to a non-error state.
   Expected<SmallVector<Edit, 1>> Edits = SmallVector<Edit, 1>();
 };
 } // namespace
 
 template <typename T>
-static llvm::Expected<SmallVector<clang::transformer::Edit, 1>>
+llvm::Expected<SmallVector<clang::transformer::Edit, 1>>
 rewriteDescendantsImpl(const T &Node, RewriteRule Rule,
                        const MatchResult &Result) {
   ApplyRuleCallback Callback(std::move(Rule));
   MatchFinder Finder;
   Callback.registerMatchers<T>(Result.Nodes, &Finder);
   Finder.match(Node, *Result.Context);
   return std::move(Callback.Edits);
 }
 
+llvm::Expected<SmallVector<clang::transformer::Edit, 1>>
+transformer::detail::rewriteDescendants(const Decl &Node, RewriteRule Rule,
+                                        const MatchResult &Result) {
+  return rewriteDescendantsImpl(Node, std::move(Rule), Result);
+}
+
+llvm::Expected<SmallVector<clang::transformer::Edit, 1>>
+transformer::detail::rewriteDescendants(const Stmt &Node, RewriteRule Rule,
+                                        const MatchResult &Result) {
+  return rewriteDescendantsImpl(Node, std::move(Rule), Result);
+}
+
+llvm::Expected<SmallVector<clang::transformer::Edit, 1>>
+transformer::detail::rewriteDescendants(const TypeLoc &Node, RewriteRule Rule,
+                                        const MatchResult &Result) {
+  return rewriteDescendantsImpl(Node, std::move(Rule), Result);
+}
+
+llvm::Expected<SmallVector<clang::transformer::Edit, 1>>
+transformer::detail::rewriteDescendants(const DynTypedNode &DNode,
+                                        RewriteRule Rule,
+                                        const MatchResult &Result) {
+  if (const auto *Node = DNode.get<Decl>())
+    return rewriteDescendantsImpl(*Node, std::move(Rule), Result);
+  if (const auto *Node = DNode.get<Stmt>())
+    return rewriteDescendantsImpl(*Node, std::move(Rule), Result);
+  if (const auto *Node = DNode.get<TypeLoc>())
+    return rewriteDescendantsImpl(*Node, std::move(Rule), Result);
+
+  return llvm::make_error<llvm::StringError>(
+      llvm::errc::invalid_argument,
+      "type unsupported for recursive rewriting, Kind=" +
+          DNode.getNodeKind().asStringRef());
+}
+
 EditGenerator transformer::rewriteDescendants(std::string NodeId,
                                               RewriteRule Rule) {
-  // FIXME: warn or return error if `Rule` contains any `AddedIncludes`, since
-  // these will be dropped.
   return [NodeId = std::move(NodeId),
           Rule = std::move(Rule)](const MatchResult &Result)
              -> llvm::Expected<SmallVector<clang::transformer::Edit, 1>> {
     const ast_matchers::BoundNodes::IDToNodeMap &NodesMap =
         Result.Nodes.getMap();
     auto It = NodesMap.find(NodeId);
     if (It == NodesMap.end())
       return llvm::make_error<llvm::StringError>(llvm::errc::invalid_argument,
                                                  "ID not bound: " + NodeId);
-    if (auto *Node = It->second.get<Decl>())
-      return rewriteDescendantsImpl(*Node, std::move(Rule), Result);
-    if (auto *Node = It->second.get<Stmt>())
-      return rewriteDescendantsImpl(*Node, std::move(Rule), Result);
-    if (auto *Node = It->second.get<TypeLoc>())
-      return rewriteDescendantsImpl(*Node, std::move(Rule), Result);
-
-    return llvm::make_error<llvm::StringError>(
-        llvm::errc::invalid_argument,
-        "type unsupported for recursive rewriting, ID=\"" + NodeId +
-            "\", Kind=" + It->second.getNodeKind().asStringRef());
+    return detail::rewriteDescendants(It->second, std::move(Rule), Result);
   };
 }
 
 void transformer::addInclude(RewriteRule &Rule, StringRef Header,
                              IncludeFormat Format) {
   for (auto &Case : Rule.Cases)
     Case.Edits = flatten(std::move(Case.Edits), addInclude(Header, Format));
 }

clang/unittests/Tooling/TransformerTest.cpp

 using namespace clang;
 using namespace tooling;
 using namespace ast_matchers;
 namespace {
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 using transformer::cat;
 using transformer::changeTo;
+using transformer::rewriteDescendants;
 using transformer::RewriteRule;
 
 constexpr char KHeaderContents[] = R"cc(
   struct string {
     string(const char*);
     char* c_str();
     int size();
   };
Transformer T(
                rewriteDescendants("type", IntToChar)),
       consumer());
   T.registerMatchers(&MatchFinder);
   EXPECT_FALSE(rewrite(Input));
   EXPECT_THAT(Changes, IsEmpty());
   EXPECT_EQ(ErrorCount, 1);
 }
 
+//
+// We include one test per typed overload. We don't test extensively since that
+// is already covered by the tests above.
+//
+
+TEST_F(TransformerTest, RewriteDescendantsTypedStmt) {
+  // Add an unrelated definition to the header that also has a variable named
+  // "x", to test that the rewrite is limited to the scope we intend.
+  appendToHeader(R"cc(int g(int x) { return x; })cc");
+  std::string Input =
+      "int f(int x) { int y = x; { int z = x * x; } return x; }";
+  std::string Expected =
+      "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+  auto InlineX =
+      makeRule(declRefExpr(to(varDecl(hasName("x")))), changeTo(cat("3")));
+  testRule(makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+                    [&InlineX](const MatchFinder::MatchResult &R) {
+                      const auto *Node = R.Nodes.getNodeAs<Stmt>("body");
+                      assert(Node != nullptr && "body must be bound");
+                      return transformer::detail::rewriteDescendants(
+                          *Node, InlineX, R);
+                    }),
+           Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDecl) {
+  std::string Input =
+      "int f(int x) { int y = x; { int z = x * x; } return x; }";
+  std::string Expected =
+      "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+  auto InlineX =
+      makeRule(declRefExpr(to(varDecl(hasName("x")))), changeTo(cat("3")));
+  testRule(makeRule(functionDecl(hasName("f")).bind("fun"),
+                    [&InlineX](const MatchFinder::MatchResult &R) {
+                      const auto *Node = R.Nodes.getNodeAs<Decl>("fun");
+                      assert(Node != nullptr && "fun must be bound");
+                      return transformer::detail::rewriteDescendants(
+                          *Node, InlineX, R);
+                    }),
+           Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedTypeLoc) {
+  std::string Input = "int f(int *x) { return *x; }";
+  std::string Expected = "int f(char *x) { return *x; }";
+  auto IntToChar =
+      makeRule(typeLoc(loc(qualType(isInteger(), builtinType()))).bind("loc"),
+               changeTo(cat("char")));
+  testRule(
+      makeRule(
+          functionDecl(
+              hasName("f"),
+              hasParameter(0, varDecl(hasTypeLoc(typeLoc().bind("parmType"))))),
+          [&IntToChar](const MatchFinder::MatchResult &R) {
+            const auto *Node = R.Nodes.getNodeAs<TypeLoc>("parmType");
+            assert(Node != nullptr && "parmType must be bound");
+            return transformer::detail::rewriteDescendants(*Node, IntToChar, R);
+          }),
+      Input, Expected);
+}
+
+TEST_F(TransformerTest, RewriteDescendantsTypedDynTyped) {
+  // Add an unrelated definition to the header that also has a variable named
+  // "x", to test that the rewrite is limited to the scope we intend.
+  appendToHeader(R"cc(int g(int x) { return x; })cc");
+  std::string Input =
+      "int f(int x) { int y = x; { int z = x * x; } return x; }";
+  std::string Expected =
+      "int f(int x) { int y = 3; { int z = 3 * 3; } return 3; }";
+  auto InlineX =
+      makeRule(declRefExpr(to(varDecl(hasName("x")))), changeTo(cat("3")));
+  testRule(
+      makeRule(functionDecl(hasName("f"), hasBody(stmt().bind("body"))),
+               [&InlineX](const MatchFinder::MatchResult &R) {
+                 auto It = R.Nodes.getMap().find("body");
+                 assert(It != R.Nodes.getMap().end() && "body must be bound");
+                 return transformer::detail::rewriteDescendants(It->second,
+                                                                InlineX, R);
+               }),
+      Input, Expected);
+}
+
 TEST_F(TransformerTest, InsertBeforeEdit) {
   std::string Input = R"cc(
     int f() {
       return 7;
     }
   )cc";
   std::string Expected = R"cc(
     int f() {