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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D61015#1484669 , @thakis wrote:

> This breaks a test:  
> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/12112/steps/check-llvm%20check-clang%20stage3%2Fmsan/logs/stdio
>
>   [--] 1 test from TransformerTest
>   [ RUN  ] TransformerTest.NodePartNameDeclRefFailure
>   
> /b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:66:
>  Failure
>   Value of: MaybeActual
> Actual: false
>   Expected: true
>   Rewrite failed. Expecting: 
>   struct Y {
> int operator*();
>   };
>   int neutral(int x) {
> Y y;
> int (Y::*ptr)() = &Y::operator*;
> return *y + x;
>   }
>
>   [  FAILED  ] TransformerTest.NodePartNameDeclRefFailure (83 ms)
>   [--] 1 test from TransformerTest (83 ms total)
>
>
> Can you take a look?


Fixed in r359578.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks a test:  
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/12112/steps/check-llvm%20check-clang%20stage3%2Fmsan/logs/stdio

  [--] 1 test from TransformerTest
  [ RUN  ] TransformerTest.NodePartNameDeclRefFailure
  
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm/tools/clang/unittests/Tooling/TransformerTest.cpp:66:
 Failure
  Value of: MaybeActual
Actual: false
  Expected: true
  Rewrite failed. Expecting: 
  struct Y {
int operator*();
  };
  int neutral(int x) {
Y y;
int (Y::*ptr)() = &Y::operator*;
return *y + x;
  }

  [  FAILED  ] TransformerTest.NodePartNameDeclRefFailure (83 ms)
  [--] 1 test from TransformerTest (83 ms total)

Can you take a look?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
ymandel marked an inline comment as done.
Closed by commit rL359574: [LibTooling] Change Transformer's TextGenerator 
to a partial function. (authored by ymandel, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61015?vs=197316&id=197361#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
  cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
  cfe/trunk/unittests/Tooling/TransformerTest.cpp

Index: cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
===
--- cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
+++ cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,16 @@
   Name,
 };
 
-using TextGenerator =
-std::function;
+// 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(
+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 { return M; };
 }
 
 // Description of a source-code edit, expressed in terms of an AST node.
@@ -222,11 +226,13 @@
 class Transformer : public ast_matchers::MatchFinder::MatchCallback {
 public:
   using ChangeConsumer =
-  std::function;
+  std::function 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)) {}
 
Index: cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp
@@ -153,16 +153,19 @@
 auto It = NodesMap.find(Edit.Target);
 assert(It != NodesMap.end() && "Edit target must be bound in the match.");
 
-Expected RangeOrErr = getTargetRange(
+Expected 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();
-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 @@
   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 @@
 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(

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 4 inline comments as done.
ymandel added inline comments.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
   return SmallVector();
-T.Replacement = Edit.Replacement(Result);
+auto ReplacementOrErr = Edit.Replacement(Result);
+if (auto Err = ReplacementOrErr.takeError())

ilya-biryukov wrote:
> Maybe follow a typical pattern for handling errors here (to avoid  `OrErr` 
> suffixes and an extra `Err` variable)? I.e.
> ```
> auto Replacement = Edit.Replacement(Result);
> if (!Replacement)
>   return Replacement.takeError();
> T.Replacement = std::move(*Replacement);
> ```
> 
Here and elsewhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197316.
ymandel marked 2 inline comments as done.
ymandel added a comment.

Addresses comments, including error handling style and signature of 
ChangeConsumer.

Updates testing code to use new ChangeConsumer signature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/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 ast_matchers;
 
 namespace {
+using ::testing::IsEmpty;
+
 constexpr char KHeaderContents[] = R"cc(
   struct string {
 string(const char*);
@@ -84,26 +88,43 @@
 Factory->create(), Code, std::vector(), "input.cc",
 "clang-tool", std::make_shared(),
 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 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:
@@ -356,6 +377,23 @@
 // Negative tests (where we expect no transformation to occur).
 //
 
+// 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 {
+return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+  };
+  Transformer T(makeRule(binaryOperator().bind(O), change(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; }";
@@ -364,13 +402,11 @@
   Transformer T(
   makeRule(binaryOperator(hasLHS(expr().bind(L))).bind(O),
{change(O, "DELETE_OP"), change(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 @@
   // Try to change the whole binary-operator expression AND one its operands:
   StringRef E = "E";
   Transformer T(makeRule(expr().bind(E), change(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());

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 2 inline comments as done.
ymandel added inline comments.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:205
   if (auto Err = TransformationsOrErr.takeError()) {
-llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+AC.setError(llvm::toString(std::move(Err)));
+Consumer(AC);

ilya-biryukov wrote:
> This looks super-complicated.
> Having `Error` in `AtomicChange` seems like a bad idea in the first place, 
> why would we choose to use it here?
> 
> The following alternatives would encourage clients to handle errors properly:
> 1. accept an `Expected` in our callback,
> 2. provide a separate callback to consume errors.
> 
> WDYT about picking one of those two?
Agreed! I was using `setError` on the assumption that it was the "standard" way 
to express errors.  Given that it seems to be totally ignored otherwise, let's 
go with option 1. I'll update the revision.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:47
 
-using TextGenerator =
-std::function;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results.  For example, a typo in the name of a bound node or a mismatch in

NIT: maybe shorten a bit, still capturing the essence? Something like the 
following should be enough:

```
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.
```



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:58
 
-/// Wraps a string as a TextGenerator.
+/// Wraps a string as a (trivially successful) TextGenerator.
 inline TextGenerator text(std::string M) {

Maybe drop `trivially successful`? Does not seem to be super-important.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:238
+  /// occurs in constructing a single \c AtomicChange then the change is still
+  /// passed to \c Consumer, but it's error will be set.  Note that clients are
+  /// responsible for handling the case that independent \c AtomicChanges

s/it's/its



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:164
   return SmallVector();
-T.Replacement = Edit.Replacement(Result);
+auto ReplacementOrErr = Edit.Replacement(Result);
+if (auto Err = ReplacementOrErr.takeError())

Maybe follow a typical pattern for handling errors here (to avoid  `OrErr` 
suffixes and an extra `Err` variable)? I.e.
```
auto Replacement = Edit.Replacement(Result);
if (!Replacement)
  return Replacement.takeError();
T.Replacement = std::move(*Replacement);
```




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:205
   if (auto Err = TransformationsOrErr.takeError()) {
-llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+AC.setError(llvm::toString(std::move(Err)));
+Consumer(AC);

This looks super-complicated.
Having `Error` in `AtomicChange` seems like a bad idea in the first place, why 
would we choose to use it here?

The following alternatives would encourage clients to handle errors properly:
1. accept an `Expected` in our callback,
2. provide a separate callback to consume errors.

WDYT about picking one of those two?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197255.
ymandel added a comment.

Updates comments on Transformer to make explicit the error reporting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/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"
 
@@ -356,6 +358,27 @@
 // Negative tests (where we expect no transformation to occur).
 //
 
+// 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 {
+return llvm::make_error(llvm::errc::invalid_argument,
+   "ERROR");
+  };
+  Transformer T(makeRule(binaryOperator().bind(O), change(O, AlwaysFail)),
+[this](const AtomicChange &C) { Changes.push_back(C); });
+  T.registerMatchers(&MatchFinder);
+  // Rewriting doesn't change the input, but produces an AtomicChange that
+  // carries an error (`rewrite()` doesn't fail outright, because it doesn't
+  // check for errors in the changes).
+  compareSnippets(Input, rewrite(Input));
+  ASSERT_EQ(Changes.size(), 1u);
+  EXPECT_TRUE(Changes[0].hasError());
+}
+
 // 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; }";
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -157,12 +157,16 @@
 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()))
+auto &Range = *RangeOrErr;
+if (Range.isInvalid() ||
+isOriginMacroBody(*Result.SourceManager, Range.getBegin()))
   return SmallVector();
-T.Replacement = Edit.Replacement(Result);
+auto ReplacementOrErr = Edit.Replacement(Result);
+if (auto Err = ReplacementOrErr.takeError())
+  return std::move(Err);
+Transformation T;
+T.Range = Range;
+T.Replacement = std::move(*ReplacementOrErr);
 Transformations.push_back(std::move(T));
   }
   return Transformations;
@@ -194,12 +198,15 @@
   Root->second.getSourceRange().getBegin());
   assert(RootLoc.isValid() && "Invalid location for Root node of match.");
 
+  AtomicChange AC(*Result.SourceManager, RootLoc);
+
   auto TransformationsOrErr = translateEdits(Result, Rule.Edits);
   if (auto Err = TransformationsOrErr.takeError()) {
-llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+AC.setError(llvm::toString(std::move(Err)));
+Consumer(AC);
 return;
   }
+
   auto &Transformations = *TransformationsOrErr;
   if (Transformations.empty()) {
 // No rewrite applied (but no error encountered either).
@@ -209,8 +216,7 @@
 return;
   }
 
-  // Convert the result to an AtomicChange.
-  AtomicChange AC(*Result.SourceManager, RootLoc);
+  // Record the results in the AtomicChange.
   for (const auto &T : Transformations) {
 if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
   AC.setError(llvm::toString(std::move(Err)));
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,21 @@
   Name,
 };
 
-using TextGenerator =
-std::function;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results.  For example, a typo in the name of a bound node or a mismatch in
+// the node's type can lead to a failure in the string generation code.  We
+// allow the generator to return \c Expected, rather than assert on such
+// failures, so that the Transformer client can choose how to handle the error.
+// For example, if used in a UI (for example

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197250.
ymandel added a comment.
Herald added a subscriber: jfb.

Add test for new behavior.

In the process, tweak the handling of errors from TextGenerators in Transformer:
instead of printing to `llvm::errs`, we set the error in the AtomicChange.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/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"
 
@@ -356,6 +358,27 @@
 // Negative tests (where we expect no transformation to occur).
 //
 
+// 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 {
+return llvm::make_error(llvm::errc::invalid_argument,
+   "ERROR");
+  };
+  Transformer T(makeRule(binaryOperator().bind(O), change(O, AlwaysFail)),
+[this](const AtomicChange &C) { Changes.push_back(C); });
+  T.registerMatchers(&MatchFinder);
+  // Rewriting doesn't change the input, but produces an AtomicChange that
+  // carries an error (`rewrite()` doesn't fail outright, because it doesn't
+  // check for errors in the changes).
+  compareSnippets(Input, rewrite(Input));
+  ASSERT_EQ(Changes.size(), 1u);
+  EXPECT_TRUE(Changes[0].hasError());
+}
+
 // 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; }";
Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -157,12 +157,16 @@
 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()))
+auto &Range = *RangeOrErr;
+if (Range.isInvalid() ||
+isOriginMacroBody(*Result.SourceManager, Range.getBegin()))
   return SmallVector();
-T.Replacement = Edit.Replacement(Result);
+auto ReplacementOrErr = Edit.Replacement(Result);
+if (auto Err = ReplacementOrErr.takeError())
+  return std::move(Err);
+Transformation T;
+T.Range = Range;
+T.Replacement = std::move(*ReplacementOrErr);
 Transformations.push_back(std::move(T));
   }
   return Transformations;
@@ -194,12 +198,15 @@
   Root->second.getSourceRange().getBegin());
   assert(RootLoc.isValid() && "Invalid location for Root node of match.");
 
+  AtomicChange AC(*Result.SourceManager, RootLoc);
+
   auto TransformationsOrErr = translateEdits(Result, Rule.Edits);
   if (auto Err = TransformationsOrErr.takeError()) {
-llvm::errs() << "Transformation failed: " << llvm::toString(std::move(Err))
- << "\n";
+AC.setError(llvm::toString(std::move(Err)));
+Consumer(AC);
 return;
   }
+
   auto &Transformations = *TransformationsOrErr;
   if (Transformations.empty()) {
 // No rewrite applied (but no error encountered either).
@@ -209,8 +216,7 @@
 return;
   }
 
-  // Convert the result to an AtomicChange.
-  AtomicChange AC(*Result.SourceManager, RootLoc);
+  // Record the results in the AtomicChange.
   for (const auto &T : Transformations) {
 if (auto Err = AC.replace(*Result.SourceManager, T.Range, T.Replacement)) {
   AC.setError(llvm::toString(std::move(Err)));
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,21 @@
   Name,
 };
 
-using TextGenerator =
-std::function;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results.  For example, a typo in the name of a bound node or a mismatch in
+// the node's type can lead to a failure in the string generation code.  We
+// allow the generator to return \c Expected, rather 

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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 197146.
ymandel added a comment.

Updated comment to more explicity describe motivation for new signature.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61015/new/

https://reviews.llvm.org/D61015

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/Transformer.cpp


Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -157,12 +157,16 @@
 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()))
+auto &Range = *RangeOrErr;
+if (Range.isInvalid() ||
+isOriginMacroBody(*Result.SourceManager, Range.getBegin()))
   return SmallVector();
-T.Replacement = Edit.Replacement(Result);
+auto ReplacementOrErr = Edit.Replacement(Result);
+if (auto Err = ReplacementOrErr.takeError())
+  return std::move(Err);
+Transformation T;
+T.Range = Range;
+T.Replacement = std::move(*ReplacementOrErr);
 Transformations.push_back(std::move(T));
   }
   return Transformations;
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,21 @@
   Name,
 };
 
-using TextGenerator =
-std::function;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results.  For example, a typo in the name of a bound node or a mismatch in
+// the node's type can lead to a failure in the string generation code.  We
+// allow the generator to return \c Expected, rather than assert on such
+// failures, so that the Transformer client can choose how to handle the error.
+// For example, if used in a UI (for example, clang-query or a web app), in
+// which the user specifies the rewrite rule, the backend might choose to 
return
+// a diagnostic error, rather than crash.
+using TextGenerator = std::function(
+const ast_matchers::MatchFinder::MatchResult &)>;
 
-/// Wraps a string as a TextGenerator.
+/// Wraps a string as a (trivially successful) TextGenerator.
 inline TextGenerator text(std::string M) {
-  return [M](const ast_matchers::MatchFinder::MatchResult &) { return M; };
+  return [M](const ast_matchers::MatchFinder::MatchResult &)
+ -> Expected { return M; };
 }
 
 // Description of a source-code edit, expressed in terms of an AST node.


Index: clang/lib/Tooling/Refactoring/Transformer.cpp
===
--- clang/lib/Tooling/Refactoring/Transformer.cpp
+++ clang/lib/Tooling/Refactoring/Transformer.cpp
@@ -157,12 +157,16 @@
 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()))
+auto &Range = *RangeOrErr;
+if (Range.isInvalid() ||
+isOriginMacroBody(*Result.SourceManager, Range.getBegin()))
   return SmallVector();
-T.Replacement = Edit.Replacement(Result);
+auto ReplacementOrErr = Edit.Replacement(Result);
+if (auto Err = ReplacementOrErr.takeError())
+  return std::move(Err);
+Transformation T;
+T.Range = Range;
+T.Replacement = std::move(*ReplacementOrErr);
 Transformations.push_back(std::move(T));
   }
   return Transformations;
Index: clang/include/clang/Tooling/Refactoring/Transformer.h
===
--- clang/include/clang/Tooling/Refactoring/Transformer.h
+++ clang/include/clang/Tooling/Refactoring/Transformer.h
@@ -44,12 +44,21 @@
   Name,
 };
 
-using TextGenerator =
-std::function;
+// \c TextGenerator may fail, because it processes dynamically-bound match
+// results.  For example, a typo in the name of a bound node or a mismatch in
+// the node's type can lead to a failure in the string generation code.  We
+// allow the generator to return \c Expected, rather than assert on such
+// failures, so that the Transformer client can choose how to handle the error.
+// For example, if used in a UI (for example, clang-query or a web app), in
+// which the user specifies the rewrite rule, the backend might choose to return
+// a diagnostic error, rather than crash.
+using TextGenerator = std::function(
+const ast_matchers::MatchFinder::MatchResult &)>;
 
-/// Wraps a string as a TextGenerator.
+/// Wraps a str