[libTooling] Further simplify Stencil type and introduce MatchComputation.

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.

Reviewers: gribozavr

Subscribers: cfe-commits

Tags: #clang

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

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

@@ -85,7 +85,7 @@ void TransformerClangTidyCheck::check(
   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";

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

@@ -51,6 +51,53 @@ MatchConsumer<T> ifBound(std::string ID, MatchConsumer<T> TrueC,
     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 {

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

@@ -30,7 +30,7 @@
 
 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
@@ -223,11 +223,7 @@ inline ASTEdit insertAfter(RangeSelector S, TextGenerator Replacement) {
 }
 
 /// Removes the source selected by \p S.
-inline ASTEdit remove(RangeSelector S) {
+ASTEdit remove(RangeSelector S);
-  return change(std::move(S),
-                [](const ast_matchers::MatchFinder::MatchResult &)
-                    -> Expected<std::string> { return ""; });
-}
 
 /// The following three functions are a low-level part of the RewriteRule
 /// API. We expose them for use in implementing the fixtures that interpret
@@ -294,10 +290,7 @@ namespace tooling {
 /// Wraps a string as a TextGenerator.
 using TextGenerator = transformer::TextGenerator;
 
-inline TextGenerator text(std::string M) {
+TextGenerator text(std::string M);
-  return [M](const ast_matchers::MatchFinder::MatchResult &)
-             -> Expected<std::string> { return M; };
-}
 
 using transformer::addInclude;
 using transformer::applyFirst;

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

@@ -32,33 +32,8 @@
 
 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
+using StencilInterface = MatchComputation<std::string>;
-  /// 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;
-};
 
 /// A sequence of code fragments, references to parameters and code-generation
 /// operations that together can be evaluated to (a fragment of) source code or
@@ -68,27 +43,13 @@ protected:
 /// 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 {
+using Stencil = std::shared_ptr<StencilInterface>;
-  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);
-  }
-};
 
 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; }

clang/lib/Tooling/Transformer/RewriteRule.cpp

@@ -43,7 +43,7 @@ transformer::detail::translateEdits(const MatchResult &Result,
     // 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;
@@ -61,6 +61,28 @@ ASTEdit transformer::changeTo(RangeSelector S, TextGenerator 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{
@@ -176,3 +198,7 @@ transformer::detail::findSelectedCase(const MatchResult &Result,
 }
 
 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

@@ -272,14 +272,6 @@ public:
 };
 } // 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) {
@@ -287,52 +279,50 @@ Stencil transformer::detail::makeStencil(RangeSelector 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(
+  return std::make_shared<StencilImpl<SelectorData>>(std::move(Selector));
-      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>>(
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Parens, Id));
+      UnaryNodeOperator::Parens, Id);
 }
 
 Stencil transformer::deref(llvm::StringRef ExprId) {
-  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Deref, ExprId));
+      UnaryNodeOperator::Deref, ExprId);
 }
 
 Stencil transformer::addressOf(llvm::StringRef ExprId) {
-  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
+  return std::make_shared<StencilImpl<UnaryOperationData>>(
-      UnaryNodeOperator::Address, ExprId));
+      UnaryNodeOperator::Address, ExprId);
 }
 
 Stencil transformer::access(StringRef BaseId, Stencil Member) {
-  return Stencil(
+  return std::make_shared<StencilImpl<AccessData>>(BaseId, std::move(Member));
-      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>>(
+  return std::make_shared<StencilImpl<IfBoundData>>(Id, std::move(TrueStencil),
-      Id, std::move(TrueStencil), std::move(FalseStencil)));
+                                                    std::move(FalseStencil));
 }
 
 Stencil transformer::run(MatchConsumer<std::string> Fn) {
-  return Stencil(
+  return std::make_shared<StencilImpl<MatchConsumer<std::string>>>(
-      std::make_shared<StencilImpl<MatchConsumer<std::string>>>(std::move(Fn)));
+      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

@@ -24,7 +24,6 @@ using ::llvm::Failed;
 using ::llvm::HasValue;
 using ::llvm::StringError;
 using ::testing::AllOf;
-using ::testing::Eq;
 using ::testing::HasSubstr;
 using MatchResult = MatchFinder::MatchResult;
 
@@ -135,26 +134,6 @@ TEST_F(StencilTest, SingleStatement) {
                        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)

clang/unittests/Tooling/TransformerTest.cpp

@@ -575,13 +575,16 @@ 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 &)
+  class AlwaysFail : public transformer::MatchComputation<std::string> {
-      -> llvm::Expected<std::string> {
+    llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &,
-    return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+                     std::string *) const override {
+      return llvm::createStringError(llvm::errc::invalid_argument, "ERROR");
+    }
+    std::string toString() const override { return "AlwaysFail"; }
   };
-  Transformer T(
+  Transformer T(makeRule(binaryOperator().bind(O),
-      makeRule(binaryOperator().bind(O), changeTo(node(O), AlwaysFail)),
+                         changeTo(node(O), std::make_shared<AlwaysFail>())),
-      consumer());
+                consumer());
   T.registerMatchers(&MatchFinder);
   EXPECT_FALSE(rewrite(Input));
   EXPECT_THAT(Changes, IsEmpty());