[LibTooling] Change Transformer's TextGenerator to a partial function.

Summary:
Changes the signature of the TextGenerator std::function to return an Expected<std::string>
instead of std::string to allow for (non-fatal) failures.  Previously, we
expected that any failures would be expressed with assertions. However, that's
unfriendly to running the code in servers or other places that don't want their
library calls to crash the program.

Correspondingly, updates Transformer's handling of failures in TextGenerators
and the signature of `ChangeConsumer`.

Reviewers: ilya-biryukov

Subscribers: cfe-commits

Tags: #clang

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@359574 91177308-0d34-0410-b5e6-96231b3b80d8

3 files changed, 93 insertions, 49 deletions

diff --git a/include/clang/Tooling/Refactoring/Transformer.h b/include/clang/Tooling/Refactoring/Transformer.h
index e99e279360..454a3b821b 100644
--- a/include/clang/Tooling/Refactoring/Transformer.h
+++ b/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,16 @@ enum class NodePart {
   Name,
 };
 
-using TextGenerator =
-    std::function<std::string(const ast_matchers::MatchFinder::MatchResult &)>;
+// 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.
 inline TextGenerator text(std::string M) {
-  return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+  return [M](const ast_matchers::MatchFinder::MatchResult &)
+             -> Expected<std::string> { return M; };
 }
 
 // Description of a source-code edit, expressed in terms of an AST node.
@@ -222,11 +226,13 @@ translateEdits(const ast_matchers::MatchFinder::MatchResult &Result,
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
   using ChangeConsumer =
-      std::function<void(const clang::tooling::AtomicChange &Change)>;
+      std::function<void(Expected<clang::tooling::AtomicChange> Change)>;
 
-  /// \param Consumer Receives each successful rewrite as an \c AtomicChange.
-  /// Note that clients are responsible for handling the case that independent
-  /// \c AtomicChanges conflict with each other.
+  /// \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(RewriteRule Rule, ChangeConsumer Consumer)
       : Rule(std::move(Rule)), Consumer(std::move(Consumer)) {}
 
diff --git a/lib/Tooling/Refactoring/Transformer.cpp b/lib/Tooling/Refactoring/Transformer.cpp
index 57f6659857..57a836567d 100644
--- a/lib/Tooling/Refactoring/Transformer.cpp
+++ b/lib/Tooling/Refactoring/Transformer.cpp
@@ -153,16 +153,19 @@ tooling::translateEdits(const MatchResult &Result,
     auto It = NodesMap.find(Edit.Target);
     assert(It != NodesMap.end() && "Edit target must be bound in the match.");
 
-    Expected<CharSourceRange> RangeOrErr = getTargetRange(
+    Expected<CharSourceRange> Range = getTargetRange(
         Edit.Target, It->second, Edit.Kind, Edit.Part, *Result.Context);
-    if (auto Err = RangeOrErr.takeError())
-      return std::move(Err);
-    Transformation T;
-    T.Range = *RangeOrErr;
-    if (T.Range.isInvalid() ||
-        isOriginMacroBody(*Result.SourceManager, T.Range.getBegin()))
+    if (!Range)
+      return Range.takeError();
+    if (Range->isInvalid() ||
+        isOriginMacroBody(*Result.SourceManager, Range->getBegin()))
       return SmallVector<Transformation, 0>();
-    T.Replacement = Edit.Replacement(Result);
+    auto Replacement = Edit.Replacement(Result);
+    if (!Replacement)
+      return Replacement.takeError();
+    Transformation T;
+    T.Range = *Range;
+    T.Replacement = std::move(*Replacement);
     Transformations.push_back(std::move(T));
   }
   return Transformations;
@@ -194,14 +197,13 @@ void Transformer::run(const MatchResult &Result) {
       Root->second.getSourceRange().getBegin());
   assert(RootLoc.isValid() && "Invalid location for Root node of match.");
 
-  auto TransformationsOrErr = translateEdits(Result, Rule.Edits);
-  if (auto Err = TransformationsOrErr.takeError()) {
-    llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
-                 << "\n";
+  auto Transformations = translateEdits(Result, Rule.Edits);
+  if (!Transformations) {
+    Consumer(Transformations.takeError());
     return;
   }
-  auto &Transformations = *TransformationsOrErr;
-  if (Transformations.empty()) {
+
+  if (Transformations->empty()) {
     // No rewrite applied (but no error encountered either).
     RootLoc.print(llvm::errs() << "note: skipping match at loc ",
                   *Result.SourceManager);
@@ -209,14 +211,14 @@ void Transformer::run(const MatchResult &Result) {
     return;
   }
 
-  // Convert the result to an AtomicChange.
+  // Record the results in the AtomicChange.
   AtomicChange AC(*Result.SourceManager, RootLoc);
-  for (const auto &T : Transformations) {
+  for (const auto &T : *Transformations) {
     if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
-      AC.setError(llvm::toString(std::move(Err)));
-      break;
+      Consumer(std::move(Err));
+      return;
     }
   }
 
-  Consumer(AC);
+  Consumer(std::move(AC));
 }
diff --git a/unittests/Tooling/TransformerTest.cpp b/unittests/Tooling/TransformerTest.cpp
index 6cc2c79e56..774184da83 100644
--- a/unittests/Tooling/TransformerTest.cpp
+++ b/unittests/Tooling/TransformerTest.cpp
@@ -10,6 +10,8 @@
 
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Errc.h"
+#include "llvm/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -18,6 +20,8 @@ using namespace tooling;
 using namespace ast_matchers;
 
 namespace {
+using ::testing::IsEmpty;
+
 constexpr char KHeaderContents[] = R"cc(
   struct string {
     string(const char*);
@@ -84,26 +88,43 @@ protected:
             Factory->create(), Code, std::vector<std::string>(), "input.cc",
             "clang-tool", std::make_shared<PCHContainerOperations>(),
             FileContents)) {
+      llvm::errs() << "Running tool failed.\n";
+      return None;
+    }
+    if (ErrorCount != 0) {
+      llvm::errs() << "Generating changes failed.\n";
       return None;
     }
-    auto ChangedCodeOrErr =
+    auto ChangedCode =
         applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
-    if (auto Err = ChangedCodeOrErr.takeError()) {
-      llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
-                   << "\n";
+    if (!ChangedCode) {
+      llvm::errs() << "Applying changes failed: "
+                   << llvm::toString(ChangedCode.takeError()) << "\n";
       return None;
     }
-    return *ChangedCodeOrErr;
+    return *ChangedCode;
+  }
+
+  Transformer::ChangeConsumer consumer() {
+    return [this](Expected<AtomicChange> C) {
+      if (C) {
+        Changes.push_back(std::move(*C));
+      } else {
+        consumeError(C.takeError());
+        ++ErrorCount;
+      }
+    };
   }
 
   void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
-    Transformer T(std::move(Rule),
-                  [this](const AtomicChange &C) { Changes.push_back(C); });
+    Transformer T(std::move(Rule), consumer());
     T.registerMatchers(&MatchFinder);
     compareSnippets(Expected, rewrite(Input));
   }
 
   clang::ast_matchers::MatchFinder MatchFinder;
+  // Records whether any errors occurred in individual changes.
+  int ErrorCount = 0;
   AtomicChanges Changes;
 
 private:
@@ -357,6 +378,23 @@ TEST_F(TransformerTest, MultiChange) {
 //
 
 // Tests for a conflict in edits from a single match for a rule.
+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)),
+                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:
@@ -364,13 +402,11 @@ TEST_F(TransformerTest, OverlappingEditsInRule) {
   Transformer T(
       makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
                {change<Expr>(O, "DELETE_OP"), change<Expr>(L, "DELETE_LHS")}),
-      [this](const AtomicChange &C) { Changes.push_back(C); });
+      consumer());
   T.registerMatchers(&MatchFinder);
-  // The rewrite process fails...
-  EXPECT_TRUE(rewrite(Input));
-  // ... but one AtomicChange was consumed:
-  ASSERT_EQ(Changes.size(), 1u);
-  EXPECT_TRUE(Changes[0].hasError());
+  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).
@@ -379,27 +415,27 @@ TEST_F(TransformerTest, OverlappingEditsMultipleMatches) {
   // 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")),
-                [this](const AtomicChange &C) { Changes.push_back(C); });
+                consumer());
   T.registerMatchers(&MatchFinder);
   // The rewrite process fails because the changes conflict with each other...
   EXPECT_FALSE(rewrite(Input));
-  // ... but all changes are (individually) fine:
-  ASSERT_EQ(Changes.size(), 2u);
-  EXPECT_FALSE(Changes[0].hasError());
-  EXPECT_FALSE(Changes[1].hasError());
+  // ... 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;")),
-      [this](const AtomicChange &C) { Changes.push_back(C); });
+  Transformer T(makeRule(functionDecl(hasName("errorOccurred")),
+                         change<Decl>("DELETED;")),
+                consumer());
   T.registerMatchers(&MatchFinder);
   // The rewrite process itself fails...
   EXPECT_FALSE(rewrite(Input));
-  // ... and no changes are produced in the process.
-  EXPECT_THAT(Changes, ::testing::IsEmpty());
+  // ... and no changes or errors are produced in the process.
+  EXPECT_THAT(Changes, IsEmpty());
+  EXPECT_EQ(ErrorCount, 0);
 }
 
 TEST_F(TransformerTest, NoTransformationInMacro) {