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

[li.zhe.hua]

Summary

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.

[ymandel]

Looks good. But, please add tests. Thanks!

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

27

Maybe ChangeSetConsumer to be more clearly a different word than ChangeConsumer?

28

Maybe document the choice of MutableArrayRef vs passing by value? I think this is the right choice, but it's not totally obvious. My justification is that we assume most consumers are reading the data and not storing the whole array.

[li.zhe.hua]

Update based on comments.

Update test to switch off depreated constructor.
Fix assert.

[li.zhe.hua]

Add test

[li.zhe.hua]

In D119745#3320154, @ymandel wrote:

Looks good. But, please add tests. Thanks!

Done; added a test for a multi-AtomicChange edit.

[li.zhe.hua]

Attempt fix for Windows compilation issue

Work around ambiguous function call by foregoing the delgating constructor.

[li.zhe.hua]

Remove old constructor

Not sure how to resolve the ambiguous constructor issue on MSVC. This is
probably SFINAE-able, but not sure how to repro locally to iterate.

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

 #include <utility>
 
 namespace clang {
 namespace tooling {
 /// Handles the matcher and callback registration for a single `RewriteRule`, as
 /// 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)>;
+  using ChangeConsumer = std::function<void(Expected<AtomicChange> Change)>;
 
+  /// 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)>;
+
+  /// Deprecated. Use the constructor accepting \c ChangesConsumer.
+  ///
   /// \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
   /// 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)) {}
+      : Transformer(std::move(Rule),
+                    [Consumer = std::move(Consumer)](
+                        Expected<llvm::MutableArrayRef<AtomicChange>> Changes) {
+                      if (Changes)
+                        for (auto &Change : *Changes)
+                          Consumer(std::move(Change));
+                      else
+                        Consumer(Changes.takeError());
+                    }) {}
+
+  /// \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, 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.
   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:
   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
 
 #endif // LLVM_CLANG_TOOLING_TRANSFORMER_TRANSFORMER_H_

clang/lib/Tooling/Transformer/Transformer.cpp

case transformer::EditKind::Range:
       }
       break;
     case transformer::EditKind::AddInclude:
       AC.addHeader(T.Replacement);
       break;
     }
   }
 
+  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

 using namespace tooling;
 using namespace ast_matchers;
 namespace {
 using ::clang::transformer::addInclude;
 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;
 using ::clang::transformer::node;
 using ::clang::transformer::remove;
 using ::clang::transformer::rewriteDescendants;
 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 {
     string(const char*);
     char* c_str();
     int size();
   };
   int strlen(const char*);
llvm::Optional<std::string> rewrite(StringRef Input) {
     if (!ChangedCode) {
       llvm::errs() << "Applying changes failed: "
                    << llvm::toString(ChangedCode.takeError()) << "\n";
       return None;
     }
     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: "
                      << llvm::toString(C.takeError()) << "\n";
         ++ErrorCount;
       }
     };
   }
testRule(
                       hasElse(stmt().bind(E))),
                {changeTo(node(std::string(C)), cat("true")),
                 changeTo(statement(std::string(T)), cat("{ /* then */ }")),
                 changeTo(statement(std::string(E)), cat("{ /* else */ }"))}),
       Input, Expected);
 }
 
 TEST_F(TransformerTest, EditList) {
-  using clang::transformer::editList;
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
         log(1) << "oh no!";
       else
         log(0) << "ok";
     }
   )cc";
testRule(makeRule(ifStmt(hasCondition(expr().bind(C)),
                               changeTo(statement(std::string(T)),
                                        cat("{ /* then */ }")),
                               changeTo(statement(std::string(E)),
                                        cat("{ /* else */ }"))})),
            Input, Expected);
 }
 
 TEST_F(TransformerTest, Flatten) {
-  using clang::transformer::editList;
   std::string Input = R"cc(
     void foo() {
       if (10 > 1.0)
         log(1) << "oh no!";
       else
         log(0) << "ok";
     }
   )cc";
TEST_F(TransformerTest, AddIncludeMultipleFiles) {
   EXPECT_THAT(Changes[0].getRemovedHeaders(), IsEmpty());
   llvm::Expected<std::string> UpdatedCode =
       clang::tooling::applyAllReplacements(Header,
                                            Changes[0].getReplacements());
   ASSERT_TRUE(static_cast<bool>(UpdatedCode))
       << "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