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

[ymandel]

Summary

Changes the signature of the 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.

[ilya-biryukov]

Why would we consider this a legitimate failure, rather than a programming error?
Same argument could be made about any form of format-string-like functions, e.g. llvm::formatv or sprintf.
Yet, they return strings and not Expected<string> or their equivalent.

[ymandel]

In D61015#1478539, @ilya-biryukov wrote:

Why would we consider this a legitimate failure, rather than a programming error?
Same argument could be made about any form of format-string-like functions, e.g. llvm::formatv or sprintf.
Yet, they return strings and not Expected<string> or their equivalent.

It's not that it's a legitimate failure so much as an invalid argument -- that is, the caller caused it and therefore should handle the failure. For a standalone tool making the call, I'd argue they should treat it as a programming error and assert to crash. However, a server that takes the input from, say, a user will want to propagate that back to the user.

As for formatv and sprintf -- I think the difference is that this has a more dynamic design. Both of those take explicit arguments at the callsite, vs. TextGenerator which takes a MatchResult. But, that's just a detail -- the key thing is that we're trying to support a usecase where the input may be flawed and we want to fail gracefully. We do so at the cost of the added complexity of Expected<>.

Alternatives:

1. add a separate validation function. But, this will complicate the design since we'd need to pass two functions rather than one. That is, TextGenerator would need to be a pair of functions.
2. return an empty string on failure. This has the typical tradeoffs of using a sentinel value.

[ilya-biryukov]

I'd argue it's the server's job to validate the inputs in that case.

The code that landed so far clearly looks like a C++ DSL to describe transformations of the source. While it can be used a dependency in the server-side, I don't see why doing user-input checking should be done by the library, rather than the server itself.
User input would have to be transformed into the calls of the C++ API somehow, that looks like a proper layer to do the validation.

[ymandel]

In D61015#1478586, @ilya-biryukov wrote:

I'd argue it's the server's job to validate the inputs in that case.

The code that landed so far clearly looks like a C++ DSL to describe transformations of the source. While it can be used a dependency in the server-side, I don't see why doing user-input checking should be done by the library, rather than the server itself.
User input would have to be transformed into the calls of the C++ API somehow, that looks like a proper layer to do the validation.

The problem is that validation can't* be done in the abstract. It has to be done with respect to a specific match result. Unfortunately, the server won't be layered directly on top of the call to the TextGenerator -- the rewrite rule is interposed between them. That is, the client of the TG is the rewriterule and that's where the validation has to happen, but the TG is opaque to the rewrite rule, so it can't hardcode that validation logic. So, we'd need to change TextGenerator to bundle a validation function and string generator. Is it still worth it in that case?

*"can't" is a bit strong. I could imagine a design which allows full analysis and validation of rewrite rules before they are executed, but it would be far more sophisticated than the current design.

[ilya-biryukov]

In D61015#1478886, @ymandel wrote:

The problem is that validation can't* be done in the abstract. It has to be done with respect to a specific match result. Unfortunately, the server won't be layered directly on top of the call to the TextGenerator -- the rewrite rule is interposed between them. That is, the client of the TG is the rewriterule and that's where the validation has to happen, but the TG is opaque to the rewrite rule, so it can't hardcode that validation logic. So, we'd need to change TextGenerator to bundle a validation function and string generator. Is it still worth it in that case?

*"can't" is a bit strong. I could imagine a design which allows full analysis and validation of rewrite rules before they are executed, but it would be far more sophisticated than the current design.

Could you provide more details into how the server app is layered? Specifically, what are the user inputs and how do they get translated into the transformer library calls?
I imagine their should be a syntax for textual representation of the generator, the representation should allow referring to the named match results. While parsing this representation, it should be possible to collect all matches of named nodes and make sure there are corresponding binding in the rewrite rule.

If the setup is different, I may be missing where the complexity comes from and I would love to learn about it.

[ymandel]

Updated comment to more explicity describe motivation for new signature.

[ymandel]

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.

[ymandel]

Updates comments on Transformer to make explicit the error reporting.

[ilya-biryukov]

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

47 ↗

(On Diff #197255)

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.

58 ↗

(On Diff #197255)

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

238 ↗

(On Diff #197255)

s/it's/its

clang/lib/Tooling/Refactoring/Transformer.cpp

164 ↗

(On Diff #197255)

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);

205 ↗

(On Diff #197255)

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<AtomicChange> in our callback,
2. provide a separate callback to consume errors.

WDYT about picking one of those two?

[ymandel]

clang/lib/Tooling/Refactoring/Transformer.cpp

205 ↗

(On Diff #197255)

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.

[ymandel]

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

Updates testing code to use new ChangeConsumer signature.

[ymandel]

clang/lib/Tooling/Refactoring/Transformer.cpp

164 ↗

(On Diff #197255)

Here and elsewhere.

[ilya-biryukov]

Thanks! LGTM

[thakis]

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?

[ymandel]

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.

cfe/trunk/include/clang/Tooling/Refactoring/Transformer.h

enum class NodePart {
   Node,
   /// Given a \c MemberExpr, selects the member's token.
   Member,
   /// Given a \c NamedDecl or \c CxxCtorInitializer, selects that token of the
   /// relevant name, not including qualifiers.
   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.
 // Includes: an ID for the (bound) node, a selector for source related to the
 // node, a replacement and, optionally, an explanation for the edit.
 //
 // * Target: the source code impacted by the rule. This identifies an AST node,
 //   or part thereof (\c Part), whose source range indicates the extent of the

 translateEdits(const ast_matchers::MatchFinder::MatchResult &Result,
                llvm::ArrayRef<ASTEdit> Edits);
 
 /// Handles the matcher and callback registration for a single rewrite rule, as
 /// defined by the arguments of the constructor.
 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)) {}
 
   /// 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

cfe/trunk/lib/Tooling/Refactoring/Transformer.cpp

 tooling::translateEdits(const MatchResult &Result,
                         llvm::ArrayRef<ASTEdit> Edits) {
   SmallVector<Transformation, 1> Transformations;
   auto &NodesMap = Result.Nodes.getMap();
   for (const auto &Edit : Edits) {
     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;
 }
 
 RewriteRule tooling::makeRule(ast_matchers::internal::DynTypedMatcher M,
                               SmallVector<ASTEdit, 1> Edits) {
   M.setAllowBind(true);
void Transformer::run(const MatchResult &Result) {
   // Verify the existence and validity of the AST node that roots this rule.
   auto &NodesMap = Result.Nodes.getMap();
   auto Root = NodesMap.find(RewriteRule::RootId);
   assert(Root != NodesMap.end() && "Transformation failed: missing root node.");
   SourceLocation RootLoc = Result.SourceManager->getExpansionLoc(
       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);
     llvm::errs() << "\n";
     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));
 }

cfe/trunk/unittests/Tooling/TransformerTest.cpp

 //===- unittest/Tooling/TransformerTest.cpp -------------------------------===//
 //
 // 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/Refactoring/Transformer.h"
 
 #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"
 
 using namespace clang;
 using namespace tooling;
 using namespace ast_matchers;
 
 namespace {
+using ::testing::IsEmpty;
+
 constexpr char KHeaderContents[] = R"cc(
   struct string {
     string(const char*);
     char* c_str();
     int size();
   };
   int strlen(const char*);
 
protected:
 
   llvm::Optional<std::string> rewrite(StringRef Input) {
     std::string Code = ("#include \"header.h\"\n" + Input).str();
     auto Factory = newFrontendActionFactory(&MatchFinder);
     if (!runToolOnCodeWithArgs(
             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:
   FileContentMappings FileContents = {{"header.h", ""}};
 };
 
 class TransformerTest : public ClangRefactoringTestBase {
 protected:
testRule(makeRule(ifStmt(hasCondition(expr().bind(C)),
            Input, Expected);
 }
 
 //
 // 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<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:
   StringRef O = "O", L = "L";
   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).
 TEST_F(TransformerTest, OverlappingEditsMultipleMatches) {
   std::string Input = "int conflictOneRule() { return -7; }";
   // 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) {
   std::string Input = R"cc(
 #define MACRO(str) strlen((str).c_str())
     int f(string s) { return MACRO(s); })cc";
   testRule(ruleStrlenSize(), Input, Input);
 }