[libTooling] Change Tranformer's consumer to take multiple changes

Previously, Transformer would invoke the consumer once per file modified per
match, in addition to any errors encountered. The consumer is not aware of which
AtomicChanges come from any particular match. It is unclear which sets of edits
may be related or whether an error invalidates any previously emitted changes.

Modify the signature of the consumer to accept a set of changes. This keeps
related changes (i.e. all edits from a single match) together, and clarifies
that errors don't produce partial changes.

Reviewed By: ymandel

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

Author: tJener

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

@@ -22,16 +22,25 @@ namespace tooling {
 /// defined by the arguments of the constructor.
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
-  using ChangeConsumer =
-      std::function<void(Expected<clang::tooling::AtomicChange> Change)>;
-
-  /// \param Consumer Receives each rewrite or error.  Will not necessarily be
-  /// called for each match; for example, if the rewrite is not applicable
-  /// because of macros, but doesn't fail.  Note that clients are responsible
+  /// Provides the set of changes to the consumer.  The callback is free to move
+  /// or destructively consume the changes as needed.
+  ///
+  /// We use \c MutableArrayRef as an abstraction to provide decoupling, and we
+  /// expect the majority of consumers to copy or move the individual values
+  /// into a separate data structure.
+  using ChangeSetConsumer = std::function<void(
+      Expected<llvm::MutableArrayRef<AtomicChange>> Changes)>;
+
+  /// \param Consumer Receives all rewrites for a single match, or an error.
+  /// Will not necessarily be called for each match; for example, if the rule
+  /// generates no edits but does not fail.  Note that clients are responsible
   /// for handling the case that independent \c AtomicChanges conflict with each
   /// other.
-  Transformer(transformer::RewriteRule Rule, ChangeConsumer Consumer)
-      : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
+  explicit Transformer(transformer::RewriteRule Rule,
+                       ChangeSetConsumer Consumer)
+      : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {
+    assert(this->Consumer && "Consumer is empty");
+  }
 
   /// N.B. Passes `this` pointer to `MatchFinder`.  So, this object should not
   /// be moved after this call.
@@ -43,8 +52,9 @@ class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 
 private:
   transformer::RewriteRule Rule;
-  /// Receives each successful rewrites as an \c AtomicChange.
-  ChangeConsumer Consumer;
+  /// Receives sets of successful rewrites as an
+  /// \c llvm::ArrayRef<AtomicChange>.
+  ChangeSetConsumer Consumer;
 };
 } // namespace tooling
 } // namespace clang

clang/lib/Tooling/Transformer/Transformer.cpp

@@ -65,6 +65,9 @@ void Transformer::run(const MatchFinder::MatchResult &Result) {
     }
   }
 
+  llvm::SmallVector<AtomicChange, 1> Changes;
+  Changes.reserve(ChangesByFileID.size());
   for (auto &IDChangePair : ChangesByFileID)
-    Consumer(std::move(IDChangePair.second));
+    Changes.push_back(std::move(IDChangePair.second));
+  Consumer(llvm::MutableArrayRef<AtomicChange>(Changes));
 }

clang/unittests/Tooling/TransformerTest.cpp

@@ -26,6 +26,7 @@ using ::clang::transformer::applyFirst;
 using ::clang::transformer::before;
 using ::clang::transformer::cat;
 using ::clang::transformer::changeTo;
+using ::clang::transformer::editList;
 using ::clang::transformer::makeRule;
 using ::clang::transformer::member;
 using ::clang::transformer::name;
@@ -36,6 +37,8 @@ using ::clang::transformer::RewriteRule;
 using ::clang::transformer::statement;
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::UnorderedElementsAre;
 
 constexpr char KHeaderContents[] = R"cc(
   struct string {
@@ -120,10 +123,11 @@ class ClangRefactoringTestBase : public testing::Test {
     return *ChangedCode;
   }
 
-  Transformer::ChangeConsumer consumer() {
-    return [this](Expected<AtomicChange> C) {
+  Transformer::ChangeSetConsumer consumer() {
+    return [this](Expected<MutableArrayRef<AtomicChange>> C) {
       if (C) {
-        Changes.push_back(std::move(*C));
+        Changes.insert(Changes.end(), std::make_move_iterator(C->begin()),
+                       std::make_move_iterator(C->end()));
       } else {
         // FIXME: stash this error rather then printing.
         llvm::errs() << "Error generating changes: "
@@ -799,7 +803,6 @@ TEST_F(TransformerTest, MultiChange) {
 }
 
 TEST_F(TransformerTest, EditList) {
-  using clang::transformer::editList;
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
@@ -827,7 +830,6 @@ TEST_F(TransformerTest, EditList) {
 }
 
 TEST_F(TransformerTest, Flatten) {
-  using clang::transformer::editList;
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
@@ -1638,4 +1640,46 @@ TEST_F(TransformerTest, AddIncludeMultipleFiles) {
       << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
   EXPECT_EQ(format(*UpdatedCode), format(Header));
 }
+
+// A single change set can span multiple files.
+TEST_F(TransformerTest, MultiFileEdit) {
+  // NB: The fixture is unused for this test, but kept for the test suite name.
+  std::string Header = R"cc(void Func(int id);)cc";
+  std::string Source = R"cc(#include "input.h"
+                            void Caller() {
+                              int id = 0;
+                              Func(id);
+                            })cc";
+  int ErrorCount = 0;
+  std::vector<AtomicChanges> ChangeSets;
+  clang::ast_matchers::MatchFinder MatchFinder;
+  Transformer T(
+      makeRule(callExpr(callee(functionDecl(hasName("Func"))),
+                        forEachArgumentWithParam(expr().bind("arg"),
+                                                 parmVarDecl().bind("param"))),
+               editList({changeTo(node("arg"), cat("ARG")),
+                         changeTo(node("param"), cat("PARAM"))})),
+      [&](Expected<MutableArrayRef<AtomicChange>> Changes) {
+        if (Changes)
+          ChangeSets.push_back(AtomicChanges(Changes->begin(), Changes->end()));
+        else
+          ++ErrorCount;
+      });
+  T.registerMatchers(&MatchFinder);
+  auto Factory = newFrontendActionFactory(&MatchFinder);
+  EXPECT_TRUE(runToolOnCodeWithArgs(
+      Factory->create(), Source, std::vector<std::string>(), "input.cc",
+      "clang-tool", std::make_shared<PCHContainerOperations>(),
+      {{"input.h", Header}}));
+
+  EXPECT_EQ(ErrorCount, 0);
+  EXPECT_THAT(
+      ChangeSets,
+      UnorderedElementsAre(UnorderedElementsAre(
+          ResultOf([](const AtomicChange &C) { return C.getFilePath(); },
+                   "input.cc"),
+          ResultOf([](const AtomicChange &C) { return C.getFilePath(); },
+                   "./input.h"))));
+}
+
 } // namespace