[libTooling] Add various Stencil combinators for expressions.

[ymandel]

Summary

This revision adds three new Stencil combinators:

• expression, which idiomatically constructs the source for an expression, including wrapping the expression's source in parentheses if needed.
• deref, which constructs an idiomatic dereferencing expression.
• addressOf, which constructs an idiomatic address-taking expression.

[gribozavr]

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

149 ↗

(On Diff #222747)

Should the comment cross-reference expression() and say that the user probably wants that instead?

clang/lib/Tooling/Refactoring/Stencil.cpp

71 ↗

(On Diff #222747)

Add "unary" to the name somewhere?

[ymandel]

responded to comments

[ymandel]

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

149 ↗

(On Diff #222747)

That depends on what selector they're using. For selection(node(ExprId)), yes I think that expression(ExprId) is going to be better in most cases. But, for other selectors, no. So, I'm not sure that the cross-reference will be generally useful. WDYT?

Also, it occurs to me that we have an asymmetry for statements and expressions. Getting the source of a statement is
selection(statement(Id)) versus expression(Id) for expressions. However, in the context of cat, which takes RangeSelector directly, they look the same, because selection isn't needed.

[gribozavr]

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

149 ↗

(On Diff #222747)

Yeah, you're right -- there are different cases, and selection() is not just for expressions. Maybe just point out that digging out raw source code should be considered a fallback option, and other AST-aware and context-aware stencils should be preferred where they exist.

[ymandel]

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

149 ↗

(On Diff #222747)

I'm still not sure what advice we want to give. In general, selection() is the way to go any time you want to propagate code from source to the target, particularly for cases where there is no corresponding node. So, in many cases it will be the only (and correct) option, rather than a fallback.

So, I'm inclined to leave it as is. The next major task I'm working on for Transformer is to write documentation for the set of related langauges here (that is, user-manual style documentation). I think that effort should help clarify what we want in some of these comments. WDYT?

[gribozavr]

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

149 ↗

(On Diff #222747)

Okay, probably I don't understand it sufficiently then. Feel free to leave it as is, especially since this comment was there before this patch.

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

 //===--- Stencil.h - Stencil class ------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 ///
-/// /file
+/// \file
 /// This file defines the *Stencil* abstraction: a code-generating object,
 /// parameterized by named references to (bound) AST nodes.  Given a match
 /// result, a stencil can be evaluated to a string of source code.
 ///
 /// A stencil is similar in spirit to a format string: it is composed of a
 /// series of raw text strings, references to nodes (the parameters) and helper
 /// code-generation operations.
 ///

 StencilPart selection(RangeSelector Selector);
 
 /// \returns the source corresponding to the identified node.
 /// FIXME: Deprecated. Write `selection(node(Id))` instead.
 inline StencilPart node(llvm::StringRef Id) {
   return selection(tooling::node(Id));
 }
 
+/// 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);
+
+/// Constructs an idiomatic dereferencing of the expression bound to \p ExprId.
+/// \p ExprId is wrapped in parentheses, if needed.
+StencilPart 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);
+
 /// 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) {
   return access(BaseId, text(Member));

cfe/trunk/lib/Tooling/Refactoring/Stencil.cpp

 #include <atomic>
 #include <memory>
 #include <string>
 
 using namespace clang;
 using namespace tooling;
 
 using ast_matchers::MatchFinder;
+using ast_type_traits::DynTypedNode;
 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);
 }
 
-static llvm::Expected<ast_type_traits::DynTypedNode>
+static llvm::Expected<DynTypedNode>
 getNode(const ast_matchers::BoundNodes &Nodes, StringRef Id) {
   auto &NodesMap = Nodes.getMap();
   auto It = NodesMap.find(Id);
   if (It == NodesMap.end())
     return llvm::make_error<llvm::StringError>(llvm::errc::invalid_argument,
                                                "Id not bound: " + Id);
   return It->second;
 }
 
 namespace {
 // An arbitrary fragment of code within a stencil.
 struct RawTextData {
   explicit RawTextData(std::string T) : Text(std::move(T)) {}
   std::string Text;
 };
 
 // A debugging operation to dump the AST for a particular (bound) AST node.
 struct DebugPrintNodeData {
   explicit DebugPrintNodeData(std::string S) : Id(std::move(S)) {}
   std::string Id;
 };
 
+// Operators that take a single node Id as an argument.
+enum class UnaryNodeOperator {
+  Parens,
+  Deref,
+  Address,
+};
+
+// Generic container for stencil operations with a (single) node-id argument.
+struct UnaryOperationData {
+  UnaryOperationData(UnaryNodeOperator Op, std::string Id)
+      : Op(Op), Id(std::move(Id)) {}
+  UnaryNodeOperator Op;
+  std::string Id;
+};
+
 // 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 {

 bool isEqualData(const RawTextData &A, const RawTextData &B) {
   return A.Text == B.Text;
 }
 
 bool isEqualData(const DebugPrintNodeData &A, const DebugPrintNodeData &B) {
   return A.Id == B.Id;
 }
 
+bool isEqualData(const UnaryOperationData &A, const UnaryOperationData &B) {
+  return A.Op == B.Op && A.Id == B.Id;
+}
+
 // Equality is not (yet) defined for \c RangeSelector.
 bool isEqualData(const SelectorData &, const SelectorData &) { return false; }
 
 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) {
Error evalData(const DebugPrintNodeData &Data,
   auto NodeOrErr = getNode(Match.Nodes, Data.Id);
   if (auto Err = NodeOrErr.takeError())
     return Err;
   NodeOrErr->print(Os, PrintingPolicy(Match.Context->getLangOpts()));
   *Result += Os.str();
   return Error::success();
 }
 
+Error evalData(const UnaryOperationData &Data,
+               const MatchFinder::MatchResult &Match, std::string *Result) {
+  const auto *E = Match.Nodes.getNodeAs<Expr>(Data.Id);
+  if (E == nullptr)
+    return llvm::make_error<StringError>(
+        errc::invalid_argument, "Id not bound or not Expr: " + Data.Id);
+  llvm::Optional<std::string> Source;
+  switch (Data.Op) {
+  case UnaryNodeOperator::Parens:
+    Source = buildParens(*E, *Match.Context);
+    break;
+  case UnaryNodeOperator::Deref:
+    Source = buildDereference(*E, *Match.Context);
+    break;
+  case UnaryNodeOperator::Address:
+    Source = buildAddressOf(*E, *Match.Context);
+    break;
+  }
+  if (!Source)
+    return llvm::make_error<StringError>(
+        errc::invalid_argument,
+        "Could not construct expression source from ID: " + Data.Id);
+  *Result += *Source;
+  return Error::success();
+}
+
 Error evalData(const SelectorData &Data, const MatchFinder::MatchResult &Match,
                std::string *Result) {
   auto Range = Data.Selector(Match);
   if (!Range)
     return Range.takeError();
   *Result += getText(*Range, *Match.Context);
   return Error::success();
 }
StencilPart stencil::selection(RangeSelector Selector) {
   return StencilPart(
       std::make_shared<StencilPartImpl<SelectorData>>(std::move(Selector)));
 }
 
 StencilPart stencil::dPrint(StringRef Id) {
   return StencilPart(std::make_shared<StencilPartImpl<DebugPrintNodeData>>(Id));
 }
 
+StencilPart stencil::expression(llvm::StringRef Id) {
+  return StencilPart(std::make_shared<StencilPartImpl<UnaryOperationData>>(
+      UnaryNodeOperator::Parens, Id));
+}
+
+StencilPart stencil::deref(llvm::StringRef ExprId) {
+  return StencilPart(std::make_shared<StencilPartImpl<UnaryOperationData>>(
+      UnaryNodeOperator::Deref, ExprId));
+}
+
+StencilPart stencil::addressOf(llvm::StringRef ExprId) {
+  return StencilPart(std::make_shared<StencilPartImpl<UnaryOperationData>>(
+      UnaryNodeOperator::Address, ExprId));
+}
+
 StencilPart stencil::access(StringRef BaseId, StencilPart Member) {
   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)));
 }

cfe/trunk/unittests/Tooling/StencilTest.cpp

 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
 using namespace tooling;
 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;
 using stencil::access;
+using stencil::addressOf;
 using stencil::cat;
+using stencil::deref;
 using stencil::dPrint;
+using stencil::expression;
 using stencil::ifBound;
 using stencil::run;
 using stencil::text;
 
 // Create a valid translation-unit from a statement.
 static std::string wrapSnippet(StringRef StatementCode) {
   return ("struct S { int field; }; auto stencil_test_snippet = []{" +
           StatementCode + "};")
auto StmtMatch = matchStmt(
                                         .bind("decl")))
                      .bind("expr")))
             .bind("stmt"));
     ASSERT_TRUE(StmtMatch);
     if (auto ResultOrErr = Stencil.eval(StmtMatch->Result)) {
       ADD_FAILURE() << "Expected failure but succeeded: " << *ResultOrErr;
     } else {
       auto Err = llvm::handleErrors(ResultOrErr.takeError(),
-                                    [&Matcher](const llvm::StringError &Err) {
+                                    [&Matcher](const StringError &Err) {
                                       EXPECT_THAT(Err.getMessage(), Matcher);
                                     });
       if (Err) {
         ADD_FAILURE() << "Unhandled error: " << llvm::toString(std::move(Err));
       }
     }
   }
 

 // 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));
 }
 
+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),
+                       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");
 }
 
 TEST_F(StencilTest, IfBoundOpUnbound) {
   StringRef Id = "id";
   testExpr(Id, "3;", cat(ifBound("other", text("5"), text("7"))), "7");
 }
 
+TEST_F(StencilTest, ExpressionOpNoParens) {
+  StringRef Id = "id";
+  testExpr(Id, "3;", cat(expression(Id)), "3");
+}
+
+// Don't parenthesize a parens expression.
+TEST_F(StencilTest, ExpressionOpNoParensParens) {
+  StringRef Id = "id";
+  testExpr(Id, "(3);", cat(expression(Id)), "(3)");
+}
+
+TEST_F(StencilTest, ExpressionOpBinaryOpParens) {
+  StringRef Id = "id";
+  testExpr(Id, "3+4;", cat(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")),
+              AllOf(HasSubstr("ACACA"), HasSubstr("not bound")));
+}
+
+TEST_F(StencilTest, DerefPointer) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; x;", cat(deref(Id)), "*x");
+}
+
+TEST_F(StencilTest, DerefBinOp) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; x + 1;", cat(deref(Id)), "*(x + 1)");
+}
+
+TEST_F(StencilTest, DerefAddressExpr) {
+  StringRef Id = "id";
+  testExpr(Id, "int x; &x;", cat(deref(Id)), "x");
+}
+
+TEST_F(StencilTest, AddressOfValue) {
+  StringRef Id = "id";
+  testExpr(Id, "int x; x;", cat(addressOf(Id)), "&x");
+}
+
+TEST_F(StencilTest, AddressOfDerefExpr) {
+  StringRef Id = "id";
+  testExpr(Id, "int *x; *x;", cat(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");
 }