[libTooling] Add `run` combinator to Stencils.

[ymandel]

Summary

This revision adds run, a StencilPart that runs a user-defined function that
computes a result over MatchFinder::MatchResult.

[ymandel]

clang-format

[gribozavr]

clang/include/clang/Tooling/Refactoring/Stencil.h

181

We could reimplement all other stencils through run() and eliminate StencilPartInterface.

The idea is to make StencilPart contain a std::shared_ptr<MatchConsumer<std::string>>. Then run() can be implemented as creating a StencilPart directly, and everything else can be implemented in terms of run().

WDYT?

clang/lib/Tooling/Refactoring/Stencil.cpp

107

Not very appropriate for this review -- but why do Stencils support equality comparison? As you note, with things like run it makes little sense to support it as an API. However, since a user does not know whether a Stencil can contain run or not, it is not very meaningful to compare them at all.

So if equality is not used for anything important, consider removing it?

Not to even mention that as implemented, equality is not reflexive.

[ymandel]

clang/include/clang/Tooling/Refactoring/Stencil.h

181

Answering both questions together: the equality function is designed to make testing of the format-string parser feasible (or, at least, reasonable). As is, the only reason not to do what you suggest above is exactly the equality function -- otherwise, StencilPart is just std::shared_ptr<MatchConsumer<std::string>>.

I can also plausibly imagine that we'll extend the interface to include a "print" function as well at some point.

However, currently the parser hasn't been upstreamed and the print function is only an idea, so it would be reasonable to drop StencilPartInterface and all the associated baggage and we could reinstate it later as needed. Or, if you can think of a good way to test the parser that doesn't require an equality function, that too would be convincing.

[gribozavr]

clang/include/clang/Tooling/Refactoring/Stencil.h

181

Ilya suggested that rather than comparing objects, a better way is to compare pretty-printed strings. First, it avoids the need for equality (which might get used for other things), and second, test failures will be more debuggable because we will have an obvious diff in the pretty printed string.

With that, I agree that StencilPartInterface has to stay since we are going to have at least two operations.

[ymandel]

Thanks for the review!

clang/include/clang/Tooling/Refactoring/Stencil.h

181

Yes, I like that! Pretty printing has obvious other benefits, which equality doesn't. Will do in followup patch. Thanks!

clang/include/clang/Tooling/Refactoring/Stencil.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_REFACTOR_STENCIL_H_
 #define LLVM_CLANG_TOOLING_REFACTOR_STENCIL_H_
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Refactoring/MatchConsumer.h"
 #include "clang/Tooling/Refactoring/RangeSelector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace tooling {

 
 /// Chooses between the two strings, based on whether \p ID is bound in the
 /// match.
 inline StencilPart ifBound(llvm::StringRef Id, llvm::StringRef TrueText,
                            llvm::StringRef FalseText) {
   return ifBound(Id, text(TrueText), text(FalseText));
 }
 
+/// Wraps a MatchConsumer in a StencilPart, so that it can be used in a Stencil.
+/// This supports user-defined extensions to the Stencil language.
+StencilPart run(MatchConsumer<std::string> C);
+
 /// For debug use only; semantics are not guaranteed.
 ///
 /// \returns the string resulting from calling the node's print() method.
 StencilPart dPrint(llvm::StringRef Id);
 } // namespace stencil
 } // namespace tooling
 } // namespace clang
 #endif // LLVM_CLANG_TOOLING_REFACTOR_STENCIL_H_

clang/lib/Tooling/Refactoring/Stencil.cpp

 #include <string>
 
 using namespace clang;
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
 using llvm::errc;
 using llvm::Error;
+using llvm::Expected;
 using llvm::StringError;
 
 // A down_cast function to safely down cast a StencilPartInterface to a subclass
 // D. Returns nullptr if P is not an instance of D.
 template <typename D> const D *down_cast(const StencilPartInterface *P) {
   if (P == nullptr || D::typeId() != P->typeId())
     return nullptr;
   return static_cast<const D *>(P);

 bool isEqualData(const AccessData &A, const AccessData &B) {
   return A.BaseId == B.BaseId && A.Member == B.Member;
 }
 
 bool isEqualData(const IfBoundData &A, const IfBoundData &B) {
   return A.Id == B.Id && A.TruePart == B.TruePart && A.FalsePart == B.FalsePart;
 }
 
+// Equality is not defined over MatchConsumers, which are opaque.
+bool isEqualData(const MatchConsumer<std::string> &A,
+                 const MatchConsumer<std::string> &B) {
+  return false;
+}
+
 // The `evalData()` overloads evaluate the given stencil data to a string, given
 // the match result, and append it to `Result`. We define an overload for each
 // type of stencil data.
 
 Error evalData(const RawTextData &Data, const MatchFinder::MatchResult &,
                std::string *Result) {
   Result->append(Data.Text);
   return Error::success();

 
 Error evalData(const IfBoundData &Data, const MatchFinder::MatchResult &Match,
                std::string *Result) {
   auto &M = Match.Nodes.getMap();
   return (M.find(Data.Id) != M.end() ? Data.TruePart : Data.FalsePart)
       .eval(Match, Result);
 }
 
+Error evalData(const MatchConsumer<std::string> &Fn,
+               const MatchFinder::MatchResult &Match, std::string *Result) {
+  Expected<std::string> Value = Fn(Match);
+  if (!Value)
+    return Value.takeError();
+  *Result += *Value;
+  return Error::success();
+}
+
 template <typename T>
 class StencilPartImpl : public StencilPartInterface {
   T Data;
 
 public:
   template <typename... Ps>
   explicit StencilPartImpl(Ps &&... Args)
       : StencilPartInterface(StencilPartImpl::typeId()),
return StencilPart(
       std::make_shared<StencilPartImpl<AccessData>>(BaseId, std::move(Member)));
 }
 
 StencilPart stencil::ifBound(StringRef Id, StencilPart TruePart,
                              StencilPart FalsePart) {
   return StencilPart(std::make_shared<StencilPartImpl<IfBoundData>>(
       Id, std::move(TruePart), std::move(FalsePart)));
 }
+
+StencilPart stencil::run(MatchConsumer<std::string> Fn) {
+  return StencilPart(
+      std::make_shared<StencilPartImpl<MatchConsumer<std::string>>>(
+          std::move(Fn)));
+}

clang/unittests/Tooling/StencilTest.cpp

 using ::testing::AllOf;
 using ::testing::Eq;
 using ::testing::HasSubstr;
 using MatchResult = MatchFinder::MatchResult;
 using stencil::access;
 using stencil::cat;
 using stencil::dPrint;
 using stencil::ifBound;
+using stencil::run;
 using stencil::text;
 
 // In tests, we can't directly match on llvm::Expected since its accessors
 // mutate the object. So, we collapse it to an Optional.
 static llvm::Optional<std::string> toOptional(llvm::Expected<std::string> V) {
   if (V)
     return *V;
   ADD_FAILURE() << "Losing error in conversion to IsSomething: "
TEST_F(StencilTest, AccessOpImplicitThis) {
   auto StmtMatch =
       matchStmt(Snippet, returnStmt(hasReturnValue(ignoringImplicit(memberExpr(
                              hasObjectExpression(expr().bind("obj")))))));
   ASSERT_TRUE(StmtMatch);
   const Stencil Stencil = cat(access("obj", "field"));
   EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result), HasValue("field"));
 }
 
+TEST_F(StencilTest, RunOp) {
+  StringRef Id = "id";
+  auto SimpleFn = [Id](const MatchResult &R) {
+    return std::string(R.Nodes.getNodeAs<Stmt>(Id) != nullptr ? "Bound"
+                                                              : "Unbound");
+  };
+  testExpr(Id, "3;", cat(run(SimpleFn)), "Bound");
+}
+
 TEST(StencilEqualityTest, Equality) {
   auto Lhs = cat("foo", dPrint("dprint_id"));
   auto Rhs = cat("foo", dPrint("dprint_id"));
   EXPECT_EQ(Lhs, Rhs);
 }
 
 TEST(StencilEqualityTest, InEqualityDifferentOrdering) {
   auto Lhs = cat("foo", dPrint("node"));
   auto Rhs = cat(dPrint("node"), "foo");
   EXPECT_NE(Lhs, Rhs);
 }
 
 TEST(StencilEqualityTest, InEqualityDifferentSizes) {
   auto Lhs = cat("foo", dPrint("node"), "bar", "baz");
   auto Rhs = cat("foo", dPrint("node"), "bar");
   EXPECT_NE(Lhs, Rhs);
 }
 
 // node is opaque and therefore cannot be examined for equality.
 TEST(StencilEqualityTest, InEqualitySelection) {
   auto S1 = cat(node("node"));
   auto S2 = cat(node("node"));
   EXPECT_NE(S1, S2);
 }
+
+// `run` is opaque.
+TEST(StencilEqualityTest, InEqualityRun) {
+  auto F = [](const MatchResult &R) { return "foo"; };
+  auto S1 = cat(run(F));
+  auto S2 = cat(run(F));
+  EXPECT_NE(S1, S2);
+}
 } // namespace