[libTooling] Add support for smart pointers to relevant Transformer `Stencil`s.

[ymandel]

Summary

Stencils maybeDeref and maybeAddressOf are designed to handle nodes that may
be pointers. Currently, they only handle native pointers. This patch extends the
support to recognize smart pointers and handle them as well.

[tdl-g]

clang/unittests/Tooling/StencilTest.cpp

277

You're only testing the "QuacksLike" case. I suspect you should have tests that validate the "KnownSmartPointers".

Admittedly, it's a bit redundant since the known smart pointers also QuackLike pointers. Which, I guess, raises the question of why you have the hard-coded list of KnownSmartPointers if they are covered by the QuacksLike behavior and thus can't be meaningfully tested independently.

What do you think?

[ymandel]

added comment known smart pointers.

[ymandel]

Thanks for the review!

clang/unittests/Tooling/StencilTest.cpp

277

You're right -- there isn't really a meaningful test specific to the known cases. I added a comment explaining that hard-coding them is an optimization. WDYT?

[tdl-g]

Ah, if it's just an optimization that makes sense. I still think it's worth having a test case to confirm that one of the specially-handled cases works.

[ymandel]

Added test for unique_ptr specifically.

[ymandel]

In D93637#2479576, @tdl-g wrote:

Ah, if it's just an optimization that makes sense. I still think it's worth having a test case to confirm that one of the specially-handled cases works.

Sure. I added one for unique pointer. but, just one -- i didn't add one per case. My logic is that since it's not part of the contract/API, we're really just using the (single) test to test the internal function isSmartPointerType, so one test is enough. That said, if you disagree, I'm fine with adding more tests. Its easy.

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

 
 /// Constructs an idiomatic dereferencing of the expression bound to \p ExprId.
 /// \p ExprId is wrapped in parentheses, if needed.
 Stencil deref(llvm::StringRef ExprId);
 
 /// If \p ExprId is of pointer type, constructs an idiomatic dereferencing of
 /// the expression bound to \p ExprId, including wrapping it in parentheses, if
 /// needed. Otherwise, generates the original expression source.
-/// FIXME: Identify smart-pointers as pointer types.
 Stencil maybeDeref(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.
 Stencil addressOf(llvm::StringRef ExprId);
 
 /// If \p ExprId is not a pointer type, constructs an expression that
 /// idiomatically takes the address of the expression bound to \p ExprId,
 /// including wrapping \p ExprId in parentheses, if needed. Otherwise, generates
 /// the original expression source.
-/// FIXME: Identify smart-pointers as pointer types.
 Stencil maybeAddressOf(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.
 Stencil access(llvm::StringRef BaseId, Stencil Member);

clang/lib/Tooling/Transformer/Stencil.cpp

static Error printNode(StringRef Id, const MatchFinder::MatchResult &Match,
   return Error::success();
 }
 
 Error evalData(const DebugPrintNodeData &Data,
                const MatchFinder::MatchResult &Match, std::string *Result) {
   return printNode(Data.Id, Match, Result);
 }
 
+// FIXME: Consider memoizing this function using the `ASTContext`.
+static bool isSmartPointerType(QualType Ty, ASTContext &Context) {
+  using namespace ::clang::ast_matchers;
+
+  // Optimization: hard-code common smart-pointer types. This can/should be
+  // removed if we start caching the results of this function.
+  auto KnownSmartPointer =
+      cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr"));
+  const auto QuacksLikeASmartPointer = cxxRecordDecl(
+      hasMethod(cxxMethodDecl(hasOverloadedOperatorName("->"),
+                              returns(qualType(pointsTo(type()))))),
+      hasMethod(cxxMethodDecl(hasOverloadedOperatorName("*"),
+                              returns(qualType(references(type()))))));
+  const auto SmartPointer = qualType(hasDeclaration(
+      cxxRecordDecl(anyOf(KnownSmartPointer, QuacksLikeASmartPointer))));
+  return match(SmartPointer, Ty, Context).size() > 0;
+}
+
 Error evalData(const UnaryOperationData &Data,
                const MatchFinder::MatchResult &Match, std::string *Result) {
   // The `Describe` operation can be applied to any node, not just expressions,
   // so it is handled here, separately.
   if (Data.Op == UnaryNodeOperator::Describe)
     return printNode(Data.Id, Match, 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 = tooling::buildParens(*E, *Match.Context);
     break;
   case UnaryNodeOperator::Deref:
     Source = tooling::buildDereference(*E, *Match.Context);
     break;
   case UnaryNodeOperator::MaybeDeref:
-    if (!E->getType()->isAnyPointerType()) {
-      *Result += tooling::getText(*E, *Match.Context);
-      return Error::success();
+    if (E->getType()->isAnyPointerType() ||
+        isSmartPointerType(E->getType(), *Match.Context)) {
+      // Strip off any operator->. This can only occur inside an actual arrow
+      // member access, so we treat it as equivalent to an actual object
+      // expression.
+      if (const auto *OpCall = dyn_cast<clang::CXXOperatorCallExpr>(E)) {
+        if (OpCall->getOperator() == clang::OO_Arrow &&
+            OpCall->getNumArgs() == 1) {
+          E = OpCall->getArg(0);
+        }
       }
       Source = tooling::buildDereference(*E, *Match.Context);
       break;
+    }
+    *Result += tooling::getText(*E, *Match.Context);
+    return Error::success();
   case UnaryNodeOperator::AddressOf:
     Source = tooling::buildAddressOf(*E, *Match.Context);
     break;
   case UnaryNodeOperator::MaybeAddressOf:
-    if (E->getType()->isAnyPointerType()) {
+    if (E->getType()->isAnyPointerType() ||
+        isSmartPointerType(E->getType(), *Match.Context)) {
+      // Strip off any operator->. This can only occur inside an actual arrow
+      // member access, so we treat it as equivalent to an actual object
+      // expression.
+      if (const auto *OpCall = dyn_cast<clang::CXXOperatorCallExpr>(E)) {
+        if (OpCall->getOperator() == clang::OO_Arrow &&
+            OpCall->getNumArgs() == 1) {
+          E = OpCall->getArg(0);
+        }
+      }
       *Result += tooling::getText(*E, *Match.Context);
       return Error::success();
     }
     Source = tooling::buildAddressOf(*E, *Match.Context);
     break;
   case UnaryNodeOperator::Describe:
     llvm_unreachable("This case is handled at the start of the function");
   }

clang/unittests/Tooling/StencilTest.cpp

 //===- unittest/Tooling/StencilTest.cpp -----------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Transformer/Stencil.h"
 #include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Expr.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/FixIt.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace clang;
 using namespace transformer;
 using namespace ast_matchers;
 
 namespace {
 using ::llvm::Failed;
 using ::llvm::HasValue;
 using ::llvm::StringError;
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using MatchResult = MatchFinder::MatchResult;
 
 // Create a valid translation-unit from a statement.
-static std::string wrapSnippet(StringRef StatementCode) {
-  return ("namespace N { class C {}; } "
-          "namespace { class AnonC {}; } "
-          "struct S { int field; }; auto stencil_test_snippet = []{" +
+static std::string wrapSnippet(StringRef ExtraPreface,
+                               StringRef StatementCode) {
+  constexpr char Preface[] = R"cc(
+    namespace N { class C {}; }
+    namespace { class AnonC {}; }
+    struct S { int Field; };
+    struct Smart {
+      S* operator->() const;
+      S& operator*() const;
+    };
+  )cc";
+  return (Preface + ExtraPreface + "auto stencil_test_snippet = []{" +
           StatementCode + "};")
       .str();
 }
 
 static DeclarationMatcher wrapMatcher(const StatementMatcher &Matcher) {
   return varDecl(hasName("stencil_test_snippet"),
                  hasDescendant(compoundStmt(hasAnySubstatement(Matcher))));
 }
 
 struct TestMatch {
   // The AST unit from which `result` is built. We bundle it because it backs
   // the result. Users are not expected to access it.
   std::unique_ptr<ASTUnit> AstUnit;
   // The result to use in the test. References `ast_unit`.
   MatchResult Result;
 };
 
 // Matches `Matcher` against the statement `StatementCode` and returns the
 // result. Handles putting the statement inside a function and modifying the
 // matcher correspondingly. `Matcher` should match one of the statements in
 // `StatementCode` exactly -- that is, produce exactly one match. However,
 // `StatementCode` may contain other statements not described by `Matcher`.
+// `ExtraPreface` (optionally) adds extra decls to the TU, before the code.
 static llvm::Optional<TestMatch> matchStmt(StringRef StatementCode,
-                                           StatementMatcher Matcher) {
-  auto AstUnit = tooling::buildASTFromCodeWithArgs(wrapSnippet(StatementCode),
-                                                   {"-Wno-unused-value"});
+                                           StatementMatcher Matcher,
+                                           StringRef ExtraPreface = "") {
+  auto AstUnit = tooling::buildASTFromCodeWithArgs(
+      wrapSnippet(ExtraPreface, StatementCode), {"-Wno-unused-value"});
   if (AstUnit == nullptr) {
     ADD_FAILURE() << "AST construction failed";
     return llvm::None;
   }
   ASTContext &Context = AstUnit->getASTContext();
   auto Matches = ast_matchers::match(wrapMatcher(Matcher), Context);
   // We expect a single, exact match for the statement.
   if (Matches.size() != 1) {
TEST_F(StencilTest, MaybeDerefBinOp) {
   testExpr(Id, "int *x; x + 1;", maybeDeref(Id), "*(x + 1)");
 }
 
 TEST_F(StencilTest, MaybeDerefAddressExpr) {
   StringRef Id = "id";
   testExpr(Id, "int x; &x;", maybeDeref(Id), "x");
 }
 
+TEST_F(StencilTest, MaybeDerefSmartPointer) {
+  StringRef Id = "id";
+  std::string Snippet = R"cc(
+    Smart x;
+    x;
+  )cc";
+  testExpr(Id, Snippet, maybeDeref(Id), "*x");
+}
+
+// Tests that unique_ptr specifically is handled.
+TEST_F(StencilTest, MaybeDerefSmartPointerUniquePtr) {
+  StringRef Id = "id";
+  // We deliberately specify `unique_ptr` as empty to verify that it matches
+  // because of its name, rather than its contents.
+  StringRef ExtraPreface =
+      "namespace std { template <typename T> class unique_ptr {}; }\n";
+  StringRef Snippet = R"cc(
+    std::unique_ptr<int> x;
+    x;
+  )cc";
+  auto StmtMatch = matchStmt(Snippet, expr().bind(Id), ExtraPreface);
+  ASSERT_TRUE(StmtMatch);
+  EXPECT_THAT_EXPECTED(maybeDeref(Id)->eval(StmtMatch->Result),
+                       HasValue(std::string("*x")));
+}
+
+TEST_F(StencilTest, MaybeDerefSmartPointerFromMemberExpr) {
+  StringRef Id = "id";
+  std::string Snippet = "Smart x; x->Field;";
+  auto StmtMatch =
+      matchStmt(Snippet, memberExpr(hasObjectExpression(expr().bind(Id))));
+  ASSERT_TRUE(StmtMatch);
+  const Stencil Stencil = maybeDeref(Id);
+  EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result), HasValue("*x"));
+}
+
 TEST_F(StencilTest, MaybeAddressOfPointer) {
   StringRef Id = "id";
   testExpr(Id, "int *x; x;", maybeAddressOf(Id), "x");
 }
 
 TEST_F(StencilTest, MaybeAddressOfValue) {
   StringRef Id = "id";
   testExpr(Id, "int x; x;", addressOf(Id), "&x");
 }
 
 TEST_F(StencilTest, MaybeAddressOfBinOp) {
   StringRef Id = "id";
   testExpr(Id, "int x; x + 1;", maybeAddressOf(Id), "&(x + 1)");
 }
 
 TEST_F(StencilTest, MaybeAddressOfDerefExpr) {
   StringRef Id = "id";
   testExpr(Id, "int *x; *x;", addressOf(Id), "x");
 }
 
+TEST_F(StencilTest, MaybeAddressOfSmartPointer) {
+  StringRef Id = "id";
+  testExpr(Id, "Smart x; x;", maybeAddressOf(Id), "x");
+}
+
+TEST_F(StencilTest, MaybeAddressOfSmartPointerFromMemberCall) {
+  StringRef Id = "id";
+  std::string Snippet = "Smart x; x->Field;";
+  auto StmtMatch =
+      matchStmt(Snippet, memberExpr(hasObjectExpression(expr().bind(Id))));
+  ASSERT_TRUE(StmtMatch);
+  const Stencil Stencil = maybeAddressOf(Id);
+  EXPECT_THAT_EXPECTED(Stencil->eval(StmtMatch->Result), HasValue("x"));
+}
+
+TEST_F(StencilTest, MaybeAddressOfSmartPointerDerefNoCancel) {
+  StringRef Id = "id";
+  testExpr(Id, "Smart x; *x;", maybeAddressOf(Id), "&*x");
+}
+
 TEST_F(StencilTest, AccessOpValue) {
   StringRef Snippet = R"cc(
     S x;
     x;
   )cc";
   StringRef Id = "id";
   testExpr(Id, Snippet, access(Id, "field"), "x.field");
 }