[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

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

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

238

s/it's/its

clang/lib/Tooling/Refactoring/Transformer.cpp

164

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

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

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

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.

clang/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 &)>;
+// \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<Expected<std::string>(
+    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<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

 
 /// 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)>;
 
-  /// \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 as an \c AtomicChange.  If an error
+  /// 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
+  /// 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

clang/lib/Tooling/Refactoring/Transformer.cpp

tooling::translateEdits(const MatchResult &Result,
   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(
         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<Transformation, 0>();
-    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;
 }
 
 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.");
 
+  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).
     RootLoc.print(llvm::errs() << "note: skipping match at loc ",
                   *Result.SourceManager);
     llvm::errs() << "\n";
     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)));
       break;
     }
   }
 
   Consumer(AC);
 }

clang/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 {
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::make_error<llvm::StringError>(llvm::errc::invalid_argument,
+                                               "ERROR");
+  };
+  Transformer T(makeRule(binaryOperator().bind(O), change<Expr>(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; }";
   // 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); });