[libTooling] Simplify type structure of `Stencil`s.

[ymandel]

Summary

Currently, stencils are defined as a sequence of StencilParts. This
differentiation adds an unneeded layer of complexity to the definition of
Stencils. This change significantly simplifies the type structure: a stencil is
now conceptually any object implementing StencilInterface and Stencil is
just an alias for pointers to this interface.

To account for the sequencing that was supported by the old Stencil type, we
introduce a sequencing class that implements StencilInterface. That is,
sequences are just another kind of Stencil and no longer have any special
status.

Corresponding to this change in the type structure, we change the way cat is
used (and defined). cat bundles multiple features: it builds a stencil from a
sequence of subcomponents and admits multiple different types for its arguments,
while coercing them into the right type. Previously, cat was also used to
coerce a single StencilPart into a Stencil. With that distinction gone, many
uses of cat (e.g. in the tests) are unnecessary and have, therefore, been
removed.

Finally, we expose the "coercion" functionality with the first-class function
makeStencil, which was, previously, the private method wrap. This way, other
functions with Stencil-typed arguments can leverage makeStencil.

[ymandel]

commented sequence operator

[ymandel]

added simple overload of cat -- needed to support client transition to new type structure.

[ymandel]

rebasing onto parent

[gribozavr2]

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

51

s/Part// (2x)

91

I feel like this should be an implementation detail of cat (or other stencil combinators who want to support such implicit conversions). Users writing stencils should use concrete factory functions below.

158

It is the same operation as cat, right? So WDYT about cat_vector?

161–162

Like I said in the other review, I don't feel great about an overload set with universal references and anything else.

161–162

I think it is better to remove the first sentence.

163

"Creates a MatchConsumer that evaluates a given Stencil."

165

I'm a bit confused by the name. The function returns a MatchConsumer, but it is called "stencil". What is the intended usage?

166

I'm not sure the convenience of being able to avoid to say "cat(...)" at the callsite is worth the API complexity. In other words, my suggestion is to make this function take a single stencil.

clang/lib/Tooling/Transformer/Stencil.cpp

97

I think these comments can be deleted now.

259

There isn't much logic left in StencilImpl.

Would the code be simpler if we made the various Data classes implement StencilInterface directly?

[ymandel]

Thanks for the review! I agreed w/ all the comments, just responding early w/ those I had further thoughts/questions on.

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

91

Agreed, but I wasn't sure if it was worth introducing a "detail" namespace for just one (overloaded) function. I liked when these were private methods of Stencil, but we don't have that option now. Should I use a namespace detail? something else?

158

not quite. cat is "smart" and optimizes case of one argument to just return that argument. in the future, it might also handle the case of 0 arguments specially. sequence just blindly constructs a sequence stencil.

that said, i can see the argument that the user doesn't care about such details and instead of the three functions:

cat(...)
cat(vector)
sequence(vector)

we should just have the two

cat(...)
catVector(vector)

with no "plain" sequence constructor. WDYT?

161–162

Sorry, I'd forgotten we'd discussed this before. I see your point -- I was surprised it worked, but when it did just barelled along. That should have been a red flag. :) I'm happy to rework this, per the comment above,.

165

Intended use is something like makeRule(matcher, change(stencil(...))).

The naming is the same as we previously had for text(). Since MatchConsumer is so general, the function name describes its argument, not its particular type. But, I'm open to other suggestions.

clang/lib/Tooling/Transformer/Stencil.cpp

259

Yes! I plan to do that in a followup revision, though I was really tempted to do it here already. :)

That said, we could also use a llvm::PointerSumType and implement it in functional style, fwiw. I'm fine either way.

[gribozavr2]

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

91

I don't have a strong opinion, but namespace detail should be fine.

158

i can see the argument that the user doesn't care about such details

I agree, and I would also say that we probably don't even want the users of the library to depend on such details either. So I agree, cat(...) and catVector(vector) look like the best option to me.

165

I see -- thanks. change() takes a TextGenerator. Aside from stencils, what are the other text generators? If those are not important, could we change change() to take a stencil directly? Or if those other text generators are important, we could add overloads to change() that accept a stencil.

My thinking here is that the user is not really concerned with the fact that Stencil-to-TextGenerator conversion is happening here. I think the more complicated operation is the combining of stencils -- and we already have cat for that; so adding another way to spell that is better avoided. So, not emphasizing the conversion by making it a part of the change() seems fine, and spelling the operation as cat seems to be an improvement to me.

[ymandel]

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

165

I see -- thanks. change() takes a TextGenerator. Aside from stencils, what are the other text generators? If those are not important, could we change change() to take a stencil directly?

Actually, that was the original design of the library. We changed it in the first submitted version on Ilya's suggestion to avoid the dependency between Transformer and Stencil. In fact, the reason TextGenerator exists is to abstract away that dependency.

Looking back, I think it was a reasonable choice at the time, keeping the commits, etc. independent.
But, based on my experience with the library over the last six months, I'd say these are clearly a single package and we'd be better off re-introducing the dependency in exactly the way you describe.

I think we can start (here) by taking the Stencil as an argument and wrapping it as a TextGenerator. But, I should follow up with a cleanup to actually store the value as a Stencil rather than wrapped as a TextGenerator.

[gribozavr2]

I see. Some decoupling is desirable, I agree. Maybe move StencilInterface and Stencil into a separate header that RewriteRule can depend on, and then define the library of stencils in another header?

[ymandel]

In D69613#1729703, @gribozavr2 wrote:

I see. Some decoupling is desirable, I agree. Maybe move StencilInterface and Stencil into a separate header that RewriteRule can depend on, and then define the library of stencils in another header?

This sounds good. I'll put together a (stacked) revision to do this. It seems worth doing separately given that it will involve a substantial to the RewriteRule library & tests.

[ymandel]

addressed to comments

[ymandel]

In D69613#1730238, @ymandel wrote:

In D69613#1729703, @gribozavr2 wrote:

I see. Some decoupling is desirable, I agree. Maybe move StencilInterface and Stencil into a separate header that RewriteRule can depend on, and then define the library of stencils in another header?

This sounds good. I'll put together a (stacked) revision to do this. It seems worth doing separately given that it will involve a substantial to the RewriteRule library & tests.

In order to keep the client unaffected by the change, I reinstated a simple Stencil class to wrap the shared_ptr and provide the call operator. This means that the code is now backwards compatible and forwards compatible (to the change we've discussed in RewriteRule to be stencil-aware). This was simpler than adding temporary(?) overloads in RewriteRule.h for every function with a TextGenerator argument.

I included an overload of operator-> so that we can collapse it back to just a type alias.

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

166

Based on discussion, removed this entirely.

[ymandel]

remove stray comments; clang-format

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 {
-/// A stencil is represented as a sequence of "parts" that can each individually
-/// generate a code string based on a match result.  The different kinds of
-/// parts include (raw) text, references to bound nodes and assorted operations
-/// on bound nodes.
-///
-/// Users can create custom Stencil operations by implementing this interface.
-class StencilPartInterface {
+class StencilInterface {
 public:
-  virtual ~StencilPartInterface() = default;
+  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.
+  /// 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;
 
-  /// Constructs a string representation of the StencilPart. StencilParts
-  /// generated by the `selection` and `run` functions do not have a unique
-  /// string representation.
+  /// 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:
-  StencilPartInterface() = default;
+  StencilInterface() = default;
 
   // Since this is an abstract class, copying/assigning only make sense for
   // derived classes implementing `clone()`.
-  StencilPartInterface(const StencilPartInterface &) = default;
-  StencilPartInterface &operator=(const StencilPartInterface &) = default;
-};
-
-/// A copyable facade for a std::unique_ptr<StencilPartInterface>. Copies result
-/// in a copy of the underlying pointee object.
-class StencilPart {
-public:
-  explicit StencilPart(std::shared_ptr<StencilPartInterface> Impl)
-      : Impl(std::move(Impl)) {}
-
-  /// See `StencilPartInterface::eval()`.
-  llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
-                   std::string *Result) const {
-    return Impl->eval(Match, Result);
-  }
-
-  std::string toString() const {
-    if (Impl == nullptr)
-      return "";
-    return Impl->toString();
-  }
-
-private:
-  std::shared_ptr<StencilPartInterface> Impl;
+  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,
-/// given a match result.
+/// 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;
-  Stencil(std::vector<StencilPart> Parts) : Parts(std::move(Parts)) {}
-
-  /// Composes a stencil from a series of parts.
-  template <typename... Ts> static Stencil cat(Ts &&... Parts) {
-    Stencil S;
-    S.Parts = {wrap(std::forward<Ts>(Parts))...};
-    return S;
-  }
+  explicit Stencil(std::shared_ptr<StencilInterface> Ptr)
+      : Impl(std::move(Ptr)) {}
 
-  /// Appends data from a \p OtherStencil to this stencil.
-  void append(Stencil OtherStencil);
+  StencilInterface *operator->() const { return Impl.get(); }
 
-  // Evaluates the stencil given a match result. Requires that the nodes in the
-  // result includes any ids referenced in the stencil. References to missing
-  // nodes will result in an invalid_argument error.
   llvm::Expected<std::string>
-  eval(const ast_matchers::MatchFinder::MatchResult &Match) const;
-
-  // Allow Stencils to operate as std::function, for compatibility with
-  // Transformer's TextGenerator.
-  llvm::Expected<std::string>
-  operator()(const ast_matchers::MatchFinder::MatchResult &Result) const {
-    return eval(Result);
-  }
-
-  /// Constructs a string representation of the Stencil. The string is not
-  /// guaranteed to be unique.
-  std::string toString() const {
-    std::vector<std::string> PartStrings;
-    PartStrings.reserve(Parts.size());
-    for (const auto &Part : Parts)
-      PartStrings.push_back(Part.toString());
-    return llvm::join(PartStrings, ", ");
+  operator()(const ast_matchers::MatchFinder::MatchResult &R) {
+    return Impl->eval(R);
   }
+};
 
-private:
-  static StencilPart wrap(llvm::StringRef Text);
-  static StencilPart wrap(RangeSelector Selector);
-  static StencilPart wrap(StencilPart Part) { return Part; }
+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.
+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);
 
-  std::vector<StencilPart> Parts;
-};
+/// Concatenates 0+ stencil pieces into a single stencil. Arguments can be raw
+/// text, ranges in the matched code (\p RangeSelector) or other `Stencil`s.
+template <typename... Ts> Stencil cat(Ts &&... Parts) {
+  return catVector({detail::makeStencil(std::forward<Ts>(Parts))...});
+}
 
 //
 // Functions for conveniently building stencils.
 //
 
-/// Convenience wrapper for Stencil::cat that can be imported with a using decl.
-template <typename... Ts> Stencil cat(Ts &&... Parts) {
-  return Stencil::cat(std::forward<Ts>(Parts)...);
-}
-/// Convenience wrapper for Stencil constructor of the same type. Declaration
-/// outside of the class supports transition of `Stencil` type to an alias
-/// rather than a class.
-inline Stencil catVector(std::vector<StencilPart> Parts) {
-  return Stencil(std::move(Parts));
-}
-
 /// \returns exactly the text provided.
-StencilPart text(llvm::StringRef Text);
+Stencil text(llvm::StringRef Text);
 
 /// \returns the source corresponding to the selected range.
-StencilPart selection(RangeSelector Selector);
+Stencil selection(RangeSelector Selector);
 
 /// Generates the source of the expression bound to \p Id, wrapping it in
 /// parentheses if it may parse differently depending on context. For example, a
 /// binary operation is always wrapped, while a variable reference is never
 /// wrapped.
-StencilPart expression(llvm::StringRef Id);
+Stencil expression(llvm::StringRef Id);
 
 /// Constructs an idiomatic dereferencing of the expression bound to \p ExprId.
 /// \p ExprId is wrapped in parentheses, if needed.
-StencilPart deref(llvm::StringRef ExprId);
+Stencil deref(llvm::StringRef ExprId);
 
 /// Constructs an expression that idiomatically takes the address of the
 /// expression bound to \p ExprId. \p ExprId is wrapped in parentheses, if
 /// needed.
-StencilPart addressOf(llvm::StringRef ExprId);
+Stencil addressOf(llvm::StringRef ExprId);
 
 /// Constructs a `MemberExpr` that accesses the named member (\p Member) of the
 /// object bound to \p BaseId. The access is constructed idiomatically: if \p
 /// BaseId is bound to `e` and \p Member identifies member `m`, then returns
 /// `e->m`, when e is a pointer, `e2->m` when e = `*e2` and `e.m` otherwise.
 /// Additionally, `e` is wrapped in parentheses, if needed.
-StencilPart access(llvm::StringRef BaseId, StencilPart Member);
-inline StencilPart access(llvm::StringRef BaseId, llvm::StringRef Member) {
+Stencil access(llvm::StringRef BaseId, Stencil Member);
+inline Stencil access(llvm::StringRef BaseId, llvm::StringRef Member) {
   return access(BaseId, text(Member));
 }
 
 /// Chooses between the two stencil parts, based on whether \p ID is bound in
 /// the match.
-StencilPart ifBound(llvm::StringRef Id, StencilPart TruePart,
-                    StencilPart FalsePart);
+Stencil ifBound(llvm::StringRef Id, Stencil TrueStencil, Stencil FalseStencil);
 
 /// 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) {
+inline Stencil ifBound(llvm::StringRef Id, llvm::StringRef TrueText,
   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);
+/// Wraps a \c MatchConsumer in a \c Stencil, so that it can be used in a \c
+/// Stencil.  This supports user-defined extensions to the \c Stencil language.
+Stencil 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);
+Stencil dPrint(llvm::StringRef Id);
 } // namespace transformer
-
-namespace tooling {
-// DEPRECATED: These are temporary aliases supporting client migration to the
-// `transformer` namespace.
-using Stencil = transformer::Stencil;
-using StencilPart = transformer::StencilPart;
-namespace stencil {
-using transformer::access;
-using transformer::addressOf;
-using transformer::cat;
-using transformer::deref;
-using transformer::dPrint;
-using transformer::expression;
-using transformer::ifBound;
-using transformer::run;
-using transformer::selection;
-using transformer::text;
-/// \returns the source corresponding to the identified node.
-/// FIXME: Deprecated. Write `selection(node(Id))` instead.
-inline transformer::StencilPart node(llvm::StringRef Id) {
-  return selection(tooling::node(Id));
-}
-} // namespace stencil
-} // namespace tooling
 } // namespace clang
 #endif // LLVM_CLANG_TOOLING_TRANSFORMER_STENCIL_H_

clang/lib/Tooling/Transformer/Stencil.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Expr.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Transformer/SourceCode.h"
 #include "clang/Tooling/Transformer/SourceCodeBuilders.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Errc.h"
 #include <atomic>
 #include <memory>
 #include <string>
 
 using namespace clang;
 using namespace transformer;

 // The fragment of code corresponding to the selected range.
 struct SelectorData {
   explicit SelectorData(RangeSelector S) : Selector(std::move(S)) {}
   RangeSelector Selector;
 };
 
 // A stencil operation to build a member access `e.m` or `e->m`, as appropriate.
 struct AccessData {
-  AccessData(StringRef BaseId, StencilPart Member)
+  AccessData(StringRef BaseId, Stencil Member)
       : BaseId(BaseId), Member(std::move(Member)) {}
   std::string BaseId;
-  StencilPart Member;
+  Stencil Member;
 };
 
 struct IfBoundData {
-  IfBoundData(StringRef Id, StencilPart TruePart, StencilPart FalsePart)
-      : Id(Id), TruePart(std::move(TruePart)), FalsePart(std::move(FalsePart)) {
-  }
+  IfBoundData(StringRef Id, Stencil TrueStencil, Stencil FalseStencil)
+      : Id(Id), TrueStencil(std::move(TrueStencil)),
+        FalseStencil(std::move(FalseStencil)) {}
   std::string Id;
-  StencilPart TruePart;
-  StencilPart FalsePart;
+  Stencil TrueStencil;
+  Stencil FalseStencil;
+};
+
+struct SequenceData {
+  SequenceData(std::vector<Stencil> Stencils) : Stencils(std::move(Stencils)) {}
+  std::vector<Stencil> Stencils;
 };
 
 std::string toStringData(const RawTextData &Data) {
   std::string Result;
   llvm::raw_string_ostream OS(Result);
   OS << "\"";
   OS.write_escaped(Data.Text);
   OS << "\"";
std::string toStringData(const UnaryOperationData &Data) {
   }
   return (OpName + "(\"" + Data.Id + "\")").str();
 }
 
 std::string toStringData(const SelectorData &) { return "selection(...)"; }
 
 std::string toStringData(const AccessData &Data) {
   return (llvm::Twine("access(\"") + Data.BaseId + "\", " +
-          Data.Member.toString() + ")")
+          Data.Member->toString() + ")")
       .str();
 }
 
 std::string toStringData(const IfBoundData &Data) {
   return (llvm::Twine("ifBound(\"") + Data.Id + "\", " +
-          Data.TruePart.toString() + ", " + Data.FalsePart.toString() + ")")
+          Data.TrueStencil->toString() + ", " + Data.FalseStencil->toString() +
+          ")")
       .str();
 }
 
 std::string toStringData(const MatchConsumer<std::string> &) {
   return "run(...)";
 }
 
+std::string toStringData(const SequenceData &Data) {
+  llvm::SmallVector<std::string, 2> Parts;
+  Parts.reserve(Data.Stencils.size());
+  for (const auto &S : Data.Stencils)
+    Parts.push_back(S->toString());
+  return (llvm::Twine("seq(") + llvm::join(Parts, ", ") + ")").str();
+}
+
 // 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();
if (llvm::Optional<std::string> S =
                 ? tooling::buildArrow(*E, *Match.Context)
                 : tooling::buildDot(*E, *Match.Context))
       *Result += *S;
     else
       return llvm::make_error<StringError>(
           errc::invalid_argument,
           "Could not construct object text from ID: " + Data.BaseId);
   }
-  return Data.Member.eval(Match, Result);
+  return Data.Member->eval(Match, Result);
 }
 
 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);
+  return (M.find(Data.Id) != M.end() ? Data.TrueStencil : Data.FalseStencil)
+      ->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 {
+Error evalData(const SequenceData &Data, const MatchFinder::MatchResult &Match,
+               std::string *Result) {
+  for (const auto &S : Data.Stencils)
+    if (auto Err = S->eval(Match, Result))
+      return Err;
+  return Error::success();
+}
+
+template <typename T> class StencilImpl : public StencilInterface {
   T Data;
 
 public:
   template <typename... Ps>
-  explicit StencilPartImpl(Ps &&... Args) : Data(std::forward<Ps>(Args)...) {}
+  explicit StencilImpl(Ps &&... Args) : Data(std::forward<Ps>(Args)...) {}
 
   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
 
-StencilPart Stencil::wrap(StringRef Text) {
-  return transformer::text(Text);
-}
-
-StencilPart Stencil::wrap(RangeSelector Selector) {
-  return transformer::selection(std::move(Selector));
+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;
 }
 
-void Stencil::append(Stencil OtherStencil) {
-  for (auto &Part : OtherStencil.Parts)
-    Parts.push_back(std::move(Part));
-}
+Stencil transformer::detail::makeStencil(StringRef Text) { return text(Text); }
 
-llvm::Expected<std::string>
-Stencil::eval(const MatchFinder::MatchResult &Match) const {
-  std::string Result;
-  for (const auto &Part : Parts)
-    if (auto Err = Part.eval(Match, &Result))
-      return std::move(Err);
-  return Result;
+Stencil transformer::detail::makeStencil(RangeSelector Selector) {
+  return selection(std::move(Selector));
 }
 
-StencilPart transformer::text(StringRef Text) {
-  return StencilPart(std::make_shared<StencilPartImpl<RawTextData>>(Text));
+Stencil transformer::text(StringRef Text) {
+  return Stencil(std::make_shared<StencilImpl<RawTextData>>(Text));
 }
 
-StencilPart transformer::selection(RangeSelector Selector) {
-  return StencilPart(
-      std::make_shared<StencilPartImpl<SelectorData>>(std::move(Selector)));
+Stencil transformer::selection(RangeSelector Selector) {
+  return Stencil(
+      std::make_shared<StencilImpl<SelectorData>>(std::move(Selector)));
 }
 
-StencilPart transformer::dPrint(StringRef Id) {
-  return StencilPart(std::make_shared<StencilPartImpl<DebugPrintNodeData>>(Id));
+Stencil transformer::dPrint(StringRef Id) {
+  return Stencil(std::make_shared<StencilImpl<DebugPrintNodeData>>(Id));
 }
 
-StencilPart transformer::expression(llvm::StringRef Id) {
-  return StencilPart(std::make_shared<StencilPartImpl<UnaryOperationData>>(
+Stencil transformer::expression(llvm::StringRef Id) {
+  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
       UnaryNodeOperator::Parens, Id));
 }
 
-StencilPart transformer::deref(llvm::StringRef ExprId) {
-  return StencilPart(std::make_shared<StencilPartImpl<UnaryOperationData>>(
+Stencil transformer::deref(llvm::StringRef ExprId) {
+  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
       UnaryNodeOperator::Deref, ExprId));
 }
 
-StencilPart transformer::addressOf(llvm::StringRef ExprId) {
-  return StencilPart(std::make_shared<StencilPartImpl<UnaryOperationData>>(
+Stencil transformer::addressOf(llvm::StringRef ExprId) {
+  return Stencil(std::make_shared<StencilImpl<UnaryOperationData>>(
       UnaryNodeOperator::Address, ExprId));
 }
 
-StencilPart transformer::access(StringRef BaseId, StencilPart Member) {
-  return StencilPart(
-      std::make_shared<StencilPartImpl<AccessData>>(BaseId, std::move(Member)));
+Stencil transformer::access(StringRef BaseId, Stencil Member) {
+  return Stencil(
+      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)));
 }
 
-StencilPart transformer::ifBound(StringRef Id, StencilPart TruePart,
-                             StencilPart FalsePart) {
-  return StencilPart(std::make_shared<StencilPartImpl<IfBoundData>>(
-      Id, std::move(TruePart), std::move(FalsePart)));
+Stencil transformer::run(MatchConsumer<std::string> Fn) {
+  return Stencil(
+      std::make_shared<StencilImpl<MatchConsumer<std::string>>>(std::move(Fn)));
 }
 
-StencilPart transformer::run(MatchConsumer<std::string> Fn) {
-  return StencilPart(
-      std::make_shared<StencilPartImpl<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)));
 }

clang/unittests/Tooling/StencilTest.cpp

auto StmtMatch = matchStmt(
                  cxxConstructExpr(
                      hasDeclaration(decl(hasDescendant(cxxCtorInitializer(
                                                            isBaseInitializer())
                                                            .bind("init")))
                                         .bind("decl")))
                      .bind("expr")))
             .bind("stmt"));
     ASSERT_TRUE(StmtMatch);
-    if (auto ResultOrErr = Stencil.eval(StmtMatch->Result)) {
+    if (auto ResultOrErr = Stencil->eval(StmtMatch->Result)) {
       ADD_FAILURE() << "Expected failure but succeeded: " << *ResultOrErr;
     } else {
       auto Err = llvm::handleErrors(ResultOrErr.takeError(),
                                     [&Matcher](const StringError &Err) {
                                       EXPECT_THAT(Err.getMessage(), Matcher);
                                     });
       if (Err) {
         ADD_FAILURE() << "Unhandled error: " << llvm::toString(std::move(Err));
TEST_F(StencilTest, SingleStatement) {
   )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 Stencil = cat("if (!", node(Condition), ") ", statement(Else), " else ",
                      statement(Then));
-  EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result),
+  EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result),
                        HasValue("if (!true) return 0; else return 1;"));
 }
 
-TEST_F(StencilTest, SingleStatementCallOperator) {
+// 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.
-  Stencil S = cat("if (!", node(Condition), ") ", statement(Else), " else ",
+  auto Consumer = cat("if (!", node(Condition), ") ", statement(Else), " else ",
                       statement(Then));
-  EXPECT_THAT_EXPECTED(S(StmtMatch->Result),
+  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")),
                                              hasThen(stmt().bind("a2"))));
   ASSERT_TRUE(StmtMatch);
   auto Stencil = cat("if(!", node("a1"), ") ", node("UNBOUND"), ";");
-  auto ResultOrErr = Stencil.eval(StmtMatch->Result);
+  auto ResultOrErr = Stencil->eval(StmtMatch->Result);
   EXPECT_TRUE(llvm::errorToBool(ResultOrErr.takeError()))
       << "Expected unbound node, got " << *ResultOrErr;
 }
 
 // Tests that a stencil with a single parameter (`Id`) evaluates to the expected
 // string, when `Id` is bound to the expression-statement in `Snippet`.
 void testExpr(StringRef Id, StringRef Snippet, const Stencil &Stencil,
               StringRef Expected) {
   auto StmtMatch = matchStmt(Snippet, expr().bind(Id));
   ASSERT_TRUE(StmtMatch);
-  EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result), HasValue(Expected));
+  EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result), HasValue(Expected));
 }
 
 void testFailure(StringRef Id, StringRef Snippet, const Stencil &Stencil,
                  testing::Matcher<std::string> MessageMatcher) {
   auto StmtMatch = matchStmt(Snippet, expr().bind(Id));
   ASSERT_TRUE(StmtMatch);
-  EXPECT_THAT_EXPECTED(Stencil.eval(StmtMatch->Result),
+  EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result),
                        Failed<StringError>(testing::Property(
                            &StringError::getMessage, MessageMatcher)));
 }
 
 TEST_F(StencilTest, SelectionOp) {
   StringRef Id = "id";
   testExpr(Id, "3;", cat(node(Id)), "3");
 }
 
 TEST_F(StencilTest, IfBoundOpBound) {
   StringRef Id = "id";
-  testExpr(Id, "3;", cat(ifBound(Id, text("5"), text("7"))), "5");
+  testExpr(Id, "3;", ifBound(Id, text("5"), text("7")), "5");
 }
 
 TEST_F(StencilTest, IfBoundOpUnbound) {
   StringRef Id = "id";
-  testExpr(Id, "3;", cat(ifBound("other", text("5"), text("7"))), "7");
+  testExpr(Id, "3;", ifBound("other", text("5"), text("7")), "7");
 }
 
 TEST_F(StencilTest, ExpressionOpNoParens) {
   StringRef Id = "id";
-  testExpr(Id, "3;", cat(expression(Id)), "3");
+  testExpr(Id, "3;", expression(Id), "3");
 }
 
 // Don't parenthesize a parens expression.
 TEST_F(StencilTest, ExpressionOpNoParensParens) {
   StringRef Id = "id";
-  testExpr(Id, "(3);", cat(expression(Id)), "(3)");
+  testExpr(Id, "(3);", expression(Id), "(3)");
 }
 
 TEST_F(StencilTest, ExpressionOpBinaryOpParens) {
   StringRef Id = "id";
-  testExpr(Id, "3+4;", cat(expression(Id)), "(3+4)");
+  testExpr(Id, "3+4;", expression(Id), "(3+4)");
 }
 
 // `expression` shares code with other ops, so we get sufficient coverage of the
 // error handling code with this test. If that changes in the future, more error
 // tests should be added.
 TEST_F(StencilTest, ExpressionOpUnbound) {
   StringRef Id = "id";
-  testFailure(Id, "3;", cat(expression("ACACA")),
+  testFailure(Id, "3;", expression("ACACA"),
               AllOf(HasSubstr("ACACA"), HasSubstr("not bound")));
 }
 
 TEST_F(StencilTest, DerefPointer) {
   StringRef Id = "id";
-  testExpr(Id, "int *x; x;", cat(deref(Id)), "*x");
+  testExpr(Id, "int *x; x;", deref(Id), "*x");
 }
 
 TEST_F(StencilTest, DerefBinOp) {
   StringRef Id = "id";
-  testExpr(Id, "int *x; x + 1;", cat(deref(Id)), "*(x + 1)");
+  testExpr(Id, "int *x; x + 1;", deref(Id), "*(x + 1)");
 }
 
 TEST_F(StencilTest, DerefAddressExpr) {
   StringRef Id = "id";
-  testExpr(Id, "int x; &x;", cat(deref(Id)), "x");
+  testExpr(Id, "int x; &x;", deref(Id), "x");
 }
 
 TEST_F(StencilTest, AddressOfValue) {
   StringRef Id = "id";
-  testExpr(Id, "int x; x;", cat(addressOf(Id)), "&x");
+  testExpr(Id, "int x; x;", addressOf(Id), "&x");
 }
 
 TEST_F(StencilTest, AddressOfDerefExpr) {
   StringRef Id = "id";
-  testExpr(Id, "int *x; *x;", cat(addressOf(Id)), "x");
+  testExpr(Id, "int *x; *x;", addressOf(Id), "x");
 }
 
 TEST_F(StencilTest, AccessOpValue) {
   StringRef Snippet = R"cc(
     S x;
     x;
   )cc";
   StringRef Id = "id";
-  testExpr(Id, Snippet, cat(access(Id, "field")), "x.field");
+  testExpr(Id, Snippet, access(Id, "field"), "x.field");
 }
 
 TEST_F(StencilTest, AccessOpValueExplicitText) {
   StringRef Snippet = R"cc(
     S x;
     x;
   )cc";
   StringRef Id = "id";
-  testExpr(Id, Snippet, cat(access(Id, text("field"))), "x.field");
+  testExpr(Id, Snippet, access(Id, text("field")), "x.field");
 }
 
 TEST_F(StencilTest, AccessOpValueAddress) {
   StringRef Snippet = R"cc(
     S x;
     &x;
   )cc";
   StringRef Id = "id";
-  testExpr(Id, Snippet, cat(access(Id, "field")), "x.field");
+  testExpr(Id, Snippet, access(Id, "field"), "x.field");
 }
 
 TEST_F(StencilTest, AccessOpPointer) {
   StringRef Snippet = R"cc(
     S *x;
     x;
   )cc";
   StringRef Id = "id";
-  testExpr(Id, Snippet, cat(access(Id, "field")), "x->field");
+  testExpr(Id, Snippet, access(Id, "field"), "x->field");
 }
 
 TEST_F(StencilTest, AccessOpPointerDereference) {
   StringRef Snippet = R"cc(
     S *x;
     *x;
   )cc";
   StringRef Id = "id";
-  testExpr(Id, Snippet, cat(access(Id, "field")), "x->field");
+  testExpr(Id, Snippet, access(Id, "field"), "x->field");
 }
 
 TEST_F(StencilTest, AccessOpExplicitThis) {
   using clang::ast_matchers::hasObjectExpression;
   using clang::ast_matchers::memberExpr;
 
   // Set up the code so we can bind to a use of this.
   StringRef Snippet = R"cc(
     class C {
      public:
       int x;
       int foo() { return this->x; }
     };
   )cc";
   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),
+  const Stencil Stencil = access("obj", "field");
+  EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result),
                        HasValue("this->field"));
 }
 
 TEST_F(StencilTest, AccessOpImplicitThis) {
   using clang::ast_matchers::hasObjectExpression;
   using clang::ast_matchers::memberExpr;
 
   // Set up the code so we can bind to a use of (implicit) this.
   StringRef Snippet = R"cc(
     class C {
      public:
       int x;
       int foo() { return x; }
     };
   )cc";
   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"));
+  const Stencil Stencil = 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");
+  testExpr(Id, "3;", run(SimpleFn), "Bound");
 }
 
 TEST(StencilToStringTest, RawTextOp) {
   auto S = cat("foo bar baz");
   StringRef Expected = R"("foo bar baz")";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, RawTextOpEscaping) {
   auto S = cat("foo \"bar\" baz\\n");
   StringRef Expected = R"("foo \"bar\" baz\\n")";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, DebugPrintNodeOp) {
-  auto S = cat(dPrint("Id"));
+  auto S = dPrint("Id");
   StringRef Expected = R"repr(dPrint("Id"))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, ExpressionOp) {
-  auto S = cat(expression("Id"));
+  auto S = expression("Id");
   StringRef Expected = R"repr(expression("Id"))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, DerefOp) {
-  auto S = cat(deref("Id"));
+  auto S = deref("Id");
   StringRef Expected = R"repr(deref("Id"))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, AddressOfOp) {
-  auto S = cat(addressOf("Id"));
+  auto S = addressOf("Id");
   StringRef Expected = R"repr(addressOf("Id"))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, SelectionOp) {
   auto S1 = cat(node("node1"));
-  EXPECT_EQ(S1.toString(), "selection(...)");
+  EXPECT_EQ(S1->toString(), "selection(...)");
 }
 
-TEST(StencilToStringTest, AccessOp) {
-  auto S = cat(access("Id", text("memberData")));
+TEST(StencilToStringTest, AccessOpText) {
+  auto S = access("Id", "memberData");
   StringRef Expected = R"repr(access("Id", "memberData"))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
-TEST(StencilToStringTest, AccessOpStencilPart) {
-  auto S = cat(access("Id", access("subId", "memberData")));
-  StringRef Expected = R"repr(access("Id", access("subId", "memberData")))repr";
-  EXPECT_EQ(S.toString(), Expected);
+TEST(StencilToStringTest, AccessOpSelector) {
+  auto S = access("Id", selection(name("otherId")));
+  StringRef Expected = R"repr(access("Id", selection(...)))repr";
+  EXPECT_EQ(S->toString(), Expected);
+}
+
+TEST(StencilToStringTest, AccessOpStencil) {
+  auto S = access("Id", cat("foo_", "bar"));
+  StringRef Expected = R"repr(access("Id", seq("foo_", "bar")))repr";
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, IfBoundOp) {
-  auto S = cat(ifBound("Id", text("trueText"), access("exprId", "memberData")));
+  auto S = ifBound("Id", text("trueText"), access("exprId", "memberData"));
   StringRef Expected =
       R"repr(ifBound("Id", "trueText", access("exprId", "memberData")))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  EXPECT_EQ(S->toString(), Expected);
 }
 
 TEST(StencilToStringTest, RunOp) {
   auto F1 = [](const MatchResult &R) { return "foo"; };
-  auto S1 = cat(run(F1));
-  EXPECT_EQ(S1.toString(), "run(...)");
+  auto S1 = run(F1);
+  EXPECT_EQ(S1->toString(), "run(...)");
 }
 
-TEST(StencilToStringTest, MultipleOp) {
+TEST(StencilToStringTest, Sequence) {
   auto S = cat("foo", access("x", "m()"), "bar",
                ifBound("x", text("t"), access("e", "f")));
-  StringRef Expected = R"repr("foo", access("x", "m()"), "bar", )repr"
-                       R"repr(ifBound("x", "t", access("e", "f")))repr";
-  EXPECT_EQ(S.toString(), Expected);
+  StringRef Expected = R"repr(seq("foo", access("x", "m()"), "bar", )repr"
+                       R"repr(ifBound("x", "t", access("e", "f"))))repr";
+  EXPECT_EQ(S->toString(), Expected);
+}
+
+TEST(StencilToStringTest, SequenceEmpty) {
+  auto S = cat();
+  StringRef Expected = "seq()";
+  EXPECT_EQ(S->toString(), Expected);
+}
+
+TEST(StencilToStringTest, SequenceSingle) {
+  auto S = cat("foo");
+  StringRef Expected = "\"foo\"";
+  EXPECT_EQ(S->toString(), Expected);
+}
+
+TEST(StencilToStringTest, SequenceFromVector) {
+  auto S = catVector({text("foo"), access("x", "m()"), text("bar"),
+                      ifBound("x", text("t"), access("e", "f"))});
+  StringRef Expected = R"repr(seq("foo", access("x", "m()"), "bar", )repr"
+                       R"repr(ifBound("x", "t", access("e", "f"))))repr";
+  EXPECT_EQ(S->toString(), Expected);
 }
 } // namespace