[libTooling] Further simplify `Stencil` type and introduce `MatchComputation`.

[ymandel]

Summary

This revision introduces a new interface MatchComputation which generalizes
the Stencil interface and replaces the std::function interface of
MatchConsumer. With this revision, Stencil (as an abstraction) becomes just
one collection of implementations of
MatchComputation<std::string>. Correspondingly, we remove the Stencil class
entirely in favor of a simple type alias, deprecate MatchConsumer and change
all functions that accepted MatchConsumer<std::string> to use
MatchComputation<std::string> instead.

[ymandel]

tweak.

[ymandel]

syntax fixes (to compile)

clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp

llvm::errs() << "Rewrite failed: "
                  << llvm::toString(Transformations.takeError()) << "\n";
     return;
   }
 
   // No rewrite applied, but no error encountered either.
   if (Transformations->empty())
     return;
 
-  Expected<std::string> Explanation = Case.Explanation(Result);
+  Expected<std::string> Explanation = Case.Explanation->eval(Result);
   if (!Explanation) {
     llvm::errs() << "Error in explanation: "
                  << llvm::toString(Explanation.takeError()) << "\n";
     return;
   }
 
   // Associate the diagnostic with the location of the first change.
   DiagnosticBuilder Diag =

clang/include/clang/Tooling/Transformer/MatchConsumer.h

 template <typename T>
 MatchConsumer<T> ifBound(std::string ID, MatchConsumer<T> TrueC,
                          MatchConsumer<T> FalseC) {
   return [=](const ast_matchers::MatchFinder::MatchResult &Result) {
     auto &Map = Result.Nodes.getMap();
     return (Map.find(ID) != Map.end() ? TrueC : FalseC)(Result);
   };
 }
+
+/// A failable computation over nodes bound by AST matchers, with (limited)
+/// reflection via the `toString` method.
+///
+/// The computation should report any errors though its return value (rather
+/// than terminating the program) to enable usage in interactive scenarios like
+/// clang-query.
+///
+/// This is a central abstraction of the Transformer framework. It is a
+/// generalization of `MatchConsumer` and intended to replace it.
+template <typename T> class MatchComputation {
+public:
+  virtual ~MatchComputation() = default;
+
+  /// Evaluates the computation and (potentially) updates the accumulator \c
+  /// Result.  \c Result is undefined in the case of an error. `Result` is an
+  /// out parameter to optimize case where the computation involves composing
+  /// the result of sub-computation evaluations.
+  virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
+                           T *Result) const = 0;
+
+  /// Convenience version of `eval`, for the case where the computation is being
+  /// evaluated on its own.
+  llvm::Expected<T> eval(const ast_matchers::MatchFinder::MatchResult &R) const;
+
+  /// Constructs a string representation of the computation, for informational
+  /// purposes. The representation must be deterministic, but is not required to
+  /// be unique.
+  virtual std::string toString() const = 0;
+
+protected:
+  MatchComputation() = default;
+
+  // Since this is an abstract class, copying/assigning only make sense for
+  // derived classes implementing `clone()`.
+  MatchComputation(const MatchComputation &) = default;
+  MatchComputation &operator=(const MatchComputation &) = default;
+};
+
+template <typename T>
+llvm::Expected<T> MatchComputation<T>::eval(
+    const ast_matchers::MatchFinder::MatchResult &R) const {
+  T Output;
+  if (auto Err = eval(R, &Output))
+    return std::move(Err);
+  return Output;
+}
 } // namespace transformer
 
 namespace tooling {
 // DEPRECATED: Temporary alias supporting client migration to the `transformer`
 // namespace.
 using transformer::ifBound;
 } // namespace tooling
 } // namespace clang
 #endif // LLVM_CLANG_TOOLING_TRANSFORMER_MATCH_CONSUMER_H_

clang/include/clang/Tooling/Transformer/RewriteRule.h

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
 #include <functional>
 #include <string>
 #include <utility>
 
 namespace clang {
 namespace transformer {
-using TextGenerator = MatchConsumer<std::string>;
+using TextGenerator = std::shared_ptr<MatchComputation<std::string>>;
 
 // 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
 //   replacement applied by the replacement term.  By default, the extent is the

 
 /// Inserts \p Replacement after \p S, leaving the source selected by \S
 /// unchanged.
 inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
   return changeTo(after(std::move(S)), std::move(Replacement));
 }
 
 /// Removes the source selected by \p S.
-inline ASTEdit remove(RangeSelector S) {
-  return change(std::move(S),
-                [](const ast_matchers::MatchFinder::MatchResult &)
-                    -> Expected<std::string> { return ""; });
-}
+ASTEdit remove(RangeSelector S);
 
 /// The following three functions are a low-level part of the RewriteRule
 /// API. We expose them for use in implementing the fixtures that interpret
 /// RewriteRule, like Transformer and TransfomerTidy, or for more advanced
 /// users.
 //
 // FIXME: These functions are really public, if advanced, elements of the
 // RewriteRule API.  Recast them as such.  Or, just declare these functions

 } // namespace transformer
 
 namespace tooling {
 // DEPRECATED: These are temporary aliases supporting client migration to the
 // `transformer` namespace.
 /// Wraps a string as a TextGenerator.
 using TextGenerator = transformer::TextGenerator;
 
-inline TextGenerator text(std::string M) {
-  return [M](const ast_matchers::MatchFinder::MatchResult &)
-             -> Expected<std::string> { return M; };
-}
+TextGenerator text(std::string M);
 
 using transformer::addInclude;
 using transformer::applyFirst;
 using transformer::change;
 using transformer::insertAfter;
 using transformer::insertBefore;
 using transformer::makeRule;
 using transformer::remove;

clang/include/clang/Tooling/Transformer/Stencil.h

 #include "clang/Tooling/Transformer/RangeSelector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace transformer {
-class StencilInterface {
-public:
-  virtual ~StencilInterface() = default;
-
-  /// Evaluates this part to a string and appends it to \c Result.  \c Result is
-  /// undefined in the case of an error. `Result` is an out parameter to
-  /// optimize the expected common case of evaluating a sequence of generators.
-  virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
-                           std::string *Result) const = 0;
-
-  /// Convenience version of `eval`, for the case where the generator is being
-  /// evaluated on its own.
-  llvm::Expected<std::string>
-  eval(const ast_matchers::MatchFinder::MatchResult &R) const;
-
-  /// Constructs a string representation of the StencilInterface. The
-  /// representation must be deterministic, but is not required to be unique.
-  virtual std::string toString() const = 0;
-
-protected:
-  StencilInterface() = default;
-
-  // Since this is an abstract class, copying/assigning only make sense for
-  // derived classes implementing `clone()`.
-  StencilInterface(const StencilInterface &) = default;
-  StencilInterface &operator=(const StencilInterface &) = default;
-};
+
+using StencilInterface = MatchComputation<std::string>;
 
 /// A sequence of code fragments, references to parameters and code-generation
 /// operations that together can be evaluated to (a fragment of) source code or
 /// a diagnostic message, given a match result.
 ///
 /// We use a `shared_ptr` to allow for easy and cheap copying of stencils.
 /// Since `StencilInterface` is an immutable interface, the sharing doesn't
 /// impose any risks. Otherwise, we would have to add a virtual `copy` method to
 /// the API and implement it for all derived classes.
-class Stencil {
-  std::shared_ptr<StencilInterface> Impl;
-
-public:
-  Stencil() = default;
-  explicit Stencil(std::shared_ptr<StencilInterface> Ptr)
-      : Impl(std::move(Ptr)) {}
-
-  StencilInterface *operator->() const { return Impl.get(); }
-
-  llvm::Expected<std::string>
-  operator()(const ast_matchers::MatchFinder::MatchResult &R) {
-    return Impl->eval(R);
-  }
-};
+using Stencil = std::shared_ptr<StencilInterface>;
 
 namespace detail {
 /// Convenience function to construct a \c Stencil. Overloaded for common cases
 /// so that user doesn't need to specify which factory function to use. This
 /// pattern gives benefits similar to implicit constructors, while maintaing a
-/// higher degree of explictness.
+/// higher degree of explicitness.
 Stencil makeStencil(llvm::StringRef Text);
 Stencil makeStencil(RangeSelector Selector);
 inline Stencil makeStencil(Stencil S) { return S; }
 } // namespace detail
 
 /// Constructs the string representing the concatenation of the given \p
 /// Parts. If only one element is passed in \p Parts, returns that element.
 Stencil catVector(std::vector<Stencil> Parts);

clang/lib/Tooling/Transformer/RewriteRule.cpp

for (const auto &Edit : Edits) {
     if (!Range)
       return Range.takeError();
     llvm::Optional<CharSourceRange> EditRange =
         tooling::getRangeForEdit(*Range, *Result.Context);
     // FIXME: let user specify whether to treat this case as an error or ignore
     // it as is currently done.
     if (!EditRange)
       return SmallVector<Transformation, 0>();
-    auto Replacement = Edit.Replacement(Result);
+    auto Replacement = Edit.Replacement->eval(Result);
     if (!Replacement)
       return Replacement.takeError();
     transformer::detail::Transformation T;
     T.Range = *EditRange;
     T.Replacement = std::move(*Replacement);
     Transformations.push_back(std::move(T));
   }
   return Transformations;
 }
 
 ASTEdit transformer::changeTo(RangeSelector S, TextGenerator Replacement) {
   ASTEdit E;
   E.TargetRange = std::move(S);
   E.Replacement = std::move(Replacement);
   return E;
 }
 
+namespace {
+/// A \c TextGenerator that always returns a fixed string.
+class SimpleTextGenerator : public MatchComputation<std::string> {
+  std::string S;
+
+public:
+  SimpleTextGenerator(std::string S) : S(std::move(S)) {}
+  llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &,
+                   std::string *Result) const override {
+    Result->append(S);
+    return llvm::Error::success();
+  }
+  std::string toString() const override {
+    return (llvm::Twine("text(\"") + S + "\")").str();
+  }
+};
+} // namespace
+
+ASTEdit transformer::remove(RangeSelector S) {
+  return change(std::move(S), std::make_shared<SimpleTextGenerator>(""));
+}
+
 RewriteRule transformer::makeRule(DynTypedMatcher M, SmallVector<ASTEdit, 1> Edits,
                               TextGenerator Explanation) {
   return RewriteRule{{RewriteRule::Case{
       std::move(M), std::move(Edits), std::move(Explanation), {}}}};
 }
 
 void transformer::addInclude(RewriteRule &Rule, StringRef Header,
                          IncludeFormat Format) {
for (size_t i = 0, N = Rule.Cases.size(); i < N; ++i) {
     std::string Tag = ("Tag" + Twine(i)).str();
     if (NodesMap.find(Tag) != NodesMap.end())
       return Rule.Cases[i];
   }
   llvm_unreachable("No tag found for this rule.");
 }
 
 constexpr llvm::StringLiteral RewriteRule::RootID;
+
+TextGenerator tooling::text(std::string M) {
+  return std::make_shared<SimpleTextGenerator>(std::move(M));
+}

clang/lib/Tooling/Transformer/Stencil.cpp

Error eval(const MatchFinder::MatchResult &Match,
              std::string *Result) const override {
     return evalData(Data, Match, Result);
   }
 
   std::string toString() const override { return toStringData(Data); }
 };
 } // namespace
 
-llvm::Expected<std::string>
-StencilInterface::eval(const MatchFinder::MatchResult &R) const {
-  std::string Output;
-  if (auto Err = eval(R, &Output))
-    return std::move(Err);
-  return Output;
-}
-
 Stencil transformer::detail::makeStencil(StringRef Text) { return text(Text); }
 
 Stencil transformer::detail::makeStencil(RangeSelector Selector) {
   return selection(std::move(Selector));
 }
 
 Stencil transformer::text(StringRef Text) {
-  return Stencil(std::make_shared<StencilImpl<RawTextData>>(Text));
+  return std::make_shared<StencilImpl<RawTextData>>(Text);
 }
 
 Stencil transformer::selection(RangeSelector Selector) {
-  return Stencil(
-      std::make_shared<StencilImpl<SelectorData>>(std::move(Selector)));
+  return std::make_shared<StencilImpl<SelectorData>>(std::move(Selector));
 }
 
 Stencil transformer::dPrint(StringRef Id) {
-  return Stencil(std::make_shared<StencilImpl<DebugPrintNodeData>>(Id));
+  return std::make_shared<StencilImpl<DebugPrintNodeData>>(Id);
 }
 
 Stencil transformer::expression(llvm::StringRef Id) {
-  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Parens, Id));
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
+      UnaryNodeOperator::Parens, Id);
 }
 
 Stencil transformer::deref(llvm::StringRef ExprId) {
-  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Deref, ExprId));
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
+      UnaryNodeOperator::Deref, ExprId);
 }
 
 Stencil transformer::addressOf(llvm::StringRef ExprId) {
-  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Address, ExprId));
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
+      UnaryNodeOperator::Address, ExprId);
 }
 
 Stencil transformer::access(StringRef BaseId, Stencil Member) {
-  return Stencil(
-      std::make_shared<StencilImpl<AccessData>>(BaseId, std::move(Member)));
+  return std::make_shared<StencilImpl<AccessData>>(BaseId, std::move(Member));
 }
 
 Stencil transformer::ifBound(StringRef Id, Stencil TrueStencil,
                              Stencil FalseStencil) {
-  return Stencil(std::make_shared<StencilImpl<IfBoundData>>(
-      Id, std::move(TrueStencil), std::move(FalseStencil)));
+  return std::make_shared<StencilImpl<IfBoundData>>(Id, std::move(TrueStencil),
+                                                    std::move(FalseStencil));
 }
 
 Stencil transformer::run(MatchConsumer<std::string> Fn) {
-  return Stencil(
-      std::make_shared<StencilImpl<MatchConsumer<std::string>>>(std::move(Fn)));
+  return std::make_shared<StencilImpl<MatchConsumer<std::string>>>(
+      std::move(Fn));
 }
 
 Stencil transformer::catVector(std::vector<Stencil> Parts) {
   // Only one argument, so don't wrap in sequence.
   if (Parts.size() == 1)
     return std::move(Parts[0]);
-  return Stencil(std::make_shared<StencilImpl<SequenceData>>(std::move(Parts)));
+  return std::make_shared<StencilImpl<SequenceData>>(std::move(Parts));
 }

clang/unittests/Tooling/StencilTest.cpp

 using namespace transformer;
 using namespace ast_matchers;
 
 namespace {
 using ::llvm::Failed;
 using ::llvm::HasValue;
 using ::llvm::StringError;
 using ::testing::AllOf;
-using ::testing::Eq;
 using ::testing::HasSubstr;
 using MatchResult = MatchFinder::MatchResult;
 
 // Create a valid translation-unit from a statement.
 static std::string wrapSnippet(StringRef StatementCode) {
   return ("struct S { int field; }; auto stencil_test_snippet = []{" +
           StatementCode + "};")
       .str();
TEST_F(StencilTest, SingleStatement) {
   ASSERT_TRUE(StmtMatch);
   // Invert the if-then-else.
   auto Stencil = cat("if (!", node(Condition), ") ", statement(Else), " else ",
                      statement(Then));
   EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result),
                        HasValue("if (!true) return 0; else return 1;"));
 }
 
-// Tests `stencil`.
-TEST_F(StencilTest, StencilFactoryFunction) {
-  StringRef Condition("C"), Then("T"), Else("E");
-  const std::string Snippet = R"cc(
-    if (true)
-      return 1;
-    else
-      return 0;
-  )cc";
-  auto StmtMatch = matchStmt(
-      Snippet, ifStmt(hasCondition(expr().bind(Condition)),
-                      hasThen(stmt().bind(Then)), hasElse(stmt().bind(Else))));
-  ASSERT_TRUE(StmtMatch);
-  // Invert the if-then-else.
-  auto Consumer = cat("if (!", node(Condition), ") ", statement(Else), " else ",
-                      statement(Then));
-  EXPECT_THAT_EXPECTED(Consumer(StmtMatch->Result),
-                       HasValue("if (!true) return 0; else return 1;"));
-}
-
 TEST_F(StencilTest, UnboundNode) {
   const std::string Snippet = R"cc(
     if (true)
       return 1;
     else
       return 0;
   )cc";
   auto StmtMatch = matchStmt(Snippet, ifStmt(hasCondition(stmt().bind("a1")),

clang/unittests/Tooling/TransformerTest.cpp

 // 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> {
+  class AlwaysFail : public transformer::MatchComputation<std::string> {
+    llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &,
+                     std::string *) const override {
       return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+    }
+    std::string toString() const override { return "AlwaysFail"; }
   };
-  Transformer T(
-      makeRule(binaryOperator().bind(O), changeTo(node(O), AlwaysFail)),
+  Transformer T(makeRule(binaryOperator().bind(O),
+                         changeTo(node(O), std::make_shared<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) {