[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

273

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

273

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;
+
+  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 = []{" +
-          StatementCode + "};")
-      .str();
+  constexpr char Preface[] = R"cc(
+    namespace N { class C {}; }
+    namespace { class AnonC {}; }
+    struct S { int Field; };
+    struct Smart {
+      S* operator->() const;
+      S& operator*() const;
+    };
+    auto stencil_test_snippet = []{
+  )cc";
+  return (Preface + StatementCode + "};").str();
 }
 
 static DeclarationMatcher wrapMatcher(const StatementMatcher &Matcher) {
   return varDecl(hasName("stencil_test_snippet"),
                  hasDescendant(compoundStmt(hasAnySubstatement(Matcher))));
 }
 
 struct TestMatch {
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");
+}
+
+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");
 }