[libTooling] In Transformer, allow atomic changes to span multiple files.

[ymandel]

Summary

Currently, all changes returned by a single application of a rule must fit in
one atomic change and therefore must apply to one file. However, there are
patterns in which a single rule will want to modify multiple files; for example,
a header and implementation to change a declaration and its definition. This
patch relaxes Transformer, libTooling's interpreter of RewriteRules, to support
multiple changes.

[gribozavr2]

clang/lib/Tooling/Transformer/Transformer.cpp

65–79

The test shows an example transformer that removes code, so the header insertion logic is not triggered there. However, for a change that would be adding code, is it correct to insert the header into every file being edited? I think not necessarily. Or do you prefer to deal with this issue when we have a sample use case?

[ymandel]

Thanks for the review.

clang/lib/Tooling/Transformer/Transformer.cpp

65–79

You're right -- these two features don't mix well. Once we support multiple files per transformation, we should change the header manipluation to be change-specific rather than apply to the whole rule. That will require some re-factoring of the APIs.

For now, I'll put in a FIXME, since this is not (yet) a high demand feature and we'll just note the limitations.

[ymandel]

adds fixme comment

[ymandel]

fixes rebase mistake in previous diff

clang/lib/Tooling/Transformer/Transformer.cpp

 //===--- Transformer.cpp - Transformer library implementation ---*- C++ -*-===//
 //
 // 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/Transformer/Transformer.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Refactoring/AtomicChange.h"
 #include "llvm/Support/Error.h"
+#include <map>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
 
void Transformer::run(const MatchFinder::MatchResult &Result) {
   if (Transformations->empty()) {
     // No rewrite applied (but no error encountered either).
     transformer::detail::getRuleMatchLoc(Result).print(
         llvm::errs() << "note: skipping match at loc ", *Result.SourceManager);
     llvm::errs() << "\n";
     return;
   }
 
-  // Record the results in the AtomicChange, anchored at the location of the
-  // first change.
-  AtomicChange AC(*Result.SourceManager,
-                  (*Transformations)[0].Range.getBegin());
+  // Group the transformations, by file, into AtomicChanges, each anchored by
+  // the location of the first change in that file.
+  std::map<FileID, AtomicChange> ChangesByFileID;
   for (const auto &T : *Transformations) {
+    auto ID = Result.SourceManager->getFileID(T.Range.getBegin());
+    auto Iter = ChangesByFileID
+                    .emplace(ID, AtomicChange(*Result.SourceManager,
+                                              T.Range.getBegin()))
+                    .first;
+    auto &AC = Iter->second;
     if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
       Consumer(std::move(Err));
       return;
     }
   }
 
+  for (auto &IDChangePair : ChangesByFileID) {
+    auto &AC = IDChangePair.second;
+    // FIXME: this will add includes to *all* changed files, which may not be
+    // the intent. We should upgrade the representation to allow associating
+    // headers with specific edits.
     for (const auto &I : Case.AddedIncludes) {
       auto &Header = I.first;
       switch (I.second) {
       case transformer::IncludeFormat::Quoted:
         AC.addHeader(Header);
         break;
       case transformer::IncludeFormat::Angled:
         AC.addHeader((llvm::Twine("<") + Header + ">").str());
         break;
       }
     }
 
     Consumer(std::move(AC));
   }
+}

clang/unittests/Tooling/TransformerTest.cpp

TEST(TransformerDeathTest, OrderedRuleTypes) {
   EXPECT_DEATH(transformer::detail::buildMatchers(QualTypeRule),
                "Matcher must be.*node matcher");
 
   RewriteRule TypeRule = makeRule(arrayType(), changeTo(cat("T")));
   EXPECT_DEATH(transformer::detail::buildMatchers(TypeRule),
                "Matcher must be.*node matcher");
 }
 #endif
+
+// Edits are able to span multiple files; in this case, a header and an
+// implementation file.
+TEST_F(TransformerTest, MultipleFiles) {
+  std::string Header = R"cc(void RemoveThisFunction();)cc";
+  std::string Source = R"cc(#include "input.h"
+                            void RemoveThisFunction();)cc";
+  Transformer T(
+      makeRule(functionDecl(hasName("RemoveThisFunction")), changeTo(cat(""))),
+      consumer());
+  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}}));
+
+  std::sort(Changes.begin(), Changes.end(),
+            [](const AtomicChange &L, const AtomicChange &R) {
+              return L.getFilePath() < R.getFilePath();
+            });
+
+  ASSERT_EQ(Changes[0].getFilePath(), "./input.h");
+  EXPECT_THAT(Changes[0].getInsertedHeaders(), IsEmpty());
+  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(R"cc(;)cc"));
+
+  ASSERT_EQ(Changes[1].getFilePath(), "input.cc");
+  EXPECT_THAT(Changes[1].getInsertedHeaders(), IsEmpty());
+  EXPECT_THAT(Changes[1].getRemovedHeaders(), IsEmpty());
+  UpdatedCode = clang::tooling::applyAllReplacements(
+      Source, Changes[1].getReplacements());
+  ASSERT_TRUE(static_cast<bool>(UpdatedCode))
+      << "Could not update code: " << llvm::toString(UpdatedCode.takeError());
+  EXPECT_EQ(format(*UpdatedCode), format(R"cc(#include "input.h"
+                        ;)cc"));
+}
 } // namespace