[clang-tidy] Extend misc-use-after-move to support unique_ptr and shared_ptr.

[mboehme]

Summary

As a unique_ptr or shared_ptr that has been moved from is guaranteed to be null,
we only warn if the pointer is dereferenced.

[aaron.ballman]

clang-tidy/misc/UseAfterMoveCheck.cpp

467 ↗

(On Diff #76081)

This logic could be improved as:

if (const auto *ID = ND->getIdentifier())
  return ID->getName();
return "";

However, I'm not certain that this function is needed at all -- it's pretty simple and is only used from isStandardSmartPointer().

482 ↗

(On Diff #76081)

Shouldn't this improvement also include weak_ptr?

542 ↗

(On Diff #76081)

And ::std::weak_ptr?

[alexfh]

LG modulo comments.

clang-tidy/misc/UseAfterMoveCheck.cpp

498 ↗

(On Diff #76081)

Use ArrayRef instead of const SmallVectorImpl<> &.

[mboehme]

• Responses to reviewer comments

[mboehme]

Please take another look.

clang-tidy/misc/UseAfterMoveCheck.cpp

467 ↗

(On Diff #76081)

However, I'm not certain that this function is needed at all -- it's pretty simple and is only used from isStandardSmartPointer().

Good point -- I've inlined it there.

482 ↗

(On Diff #76081)

Good point -- done.

[aaron.ballman]

LGTM, thank you!

clang-tools-extra/trunk/clang-tidy/misc/UseAfterMoveCheck.cpp

void UseAfterMoveFinder::getUsesAndReinits(
 
   // Sort the uses by their occurrence in the source code.
   std::sort(Uses->begin(), Uses->end(),
             [](const DeclRefExpr *D1, const DeclRefExpr *D2) {
               return D1->getExprLoc() < D2->getExprLoc();
             });
 }
 
+bool isStandardSmartPointer(const ValueDecl *VD) {
+  const Type *TheType = VD->getType().getTypePtrOrNull();
+  if (!TheType)
+    return false;
+
+  const CXXRecordDecl *RecordDecl = TheType->getAsCXXRecordDecl();
+  if (!RecordDecl)
+    return false;
+
+  const IdentifierInfo *ID = RecordDecl->getIdentifier();
+  if (!ID)
+    return false;
+
+  StringRef Name = ID->getName();
+  if (Name != "unique_ptr" && Name != "shared_ptr" && Name != "weak_ptr")
+    return false;
+
+  return RecordDecl->getDeclContext()->isStdNamespace();
+}
+
 void UseAfterMoveFinder::getDeclRefs(
     const CFGBlock *Block, const Decl *MovedVariable,
     llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
   DeclRefs->clear();
   for (const auto &Elem : *Block) {
     Optional<CFGStmt> S = Elem.getAs<CFGStmt>();
     if (!S)
       continue;
 
-    SmallVector<BoundNodes, 1> Matches =
-        match(findAll(declRefExpr(hasDeclaration(equalsNode(MovedVariable)),
-                                  unless(inDecltypeOrTemplateArg()))
-                          .bind("declref")),
-              *S->getStmt(), *Context);
-
+    auto addDeclRefs = [this, Block,
+                        DeclRefs](const ArrayRef<BoundNodes> Matches) {
       for (const auto &Match : Matches) {
         const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref");
-      if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block)
+        const auto *Operator = Match.getNodeAs<CXXOperatorCallExpr>("operator");
+        if (DeclRef && BlockMap->blockContainingStmt(DeclRef) == Block) {
+          // Ignore uses of a standard smart pointer that don't dereference the
+          // pointer.
+          if (Operator || !isStandardSmartPointer(DeclRef->getDecl())) {
             DeclRefs->insert(DeclRef);
           }
         }
       }
+    };
+
+    auto DeclRefMatcher = declRefExpr(hasDeclaration(equalsNode(MovedVariable)),
+                                      unless(inDecltypeOrTemplateArg()))
+                              .bind("declref");
+
+    addDeclRefs(match(findAll(DeclRefMatcher), *S->getStmt(), *Context));
+    addDeclRefs(match(
+        findAll(cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("*"),
+                                          hasOverloadedOperatorName("->"),
+                                          hasOverloadedOperatorName("[]")),
+                                    hasArgument(0, DeclRefMatcher))
+                    .bind("operator")),
+        *S->getStmt(), *Context));
+  }
+}
 
 void UseAfterMoveFinder::getReinits(
     const CFGBlock *Block, const ValueDecl *MovedVariable,
     llvm::SmallPtrSetImpl<const Stmt *> *Stmts,
     llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) {
   auto DeclRefMatcher =
       declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref");
 
   auto StandardContainerTypeMatcher = hasType(cxxRecordDecl(
       hasAnyName("::std::basic_string", "::std::vector", "::std::deque",
                  "::std::forward_list", "::std::list", "::std::set",
                  "::std::map", "::std::multiset", "::std::multimap",
                  "::std::unordered_set", "::std::unordered_map",
                  "::std::unordered_multiset", "::std::unordered_multimap")));
 
+  auto StandardSmartPointerTypeMatcher = hasType(cxxRecordDecl(
+      hasAnyName("::std::unique_ptr", "::std::shared_ptr", "::std::weak_ptr")));
+
   // Matches different types of reinitialization.
   auto ReinitMatcher =
       stmt(anyOf(
                // Assignment. In addition to the overloaded assignment operator,
                // test for built-in assignment as well, since template functions
                // may be instantiated to use std::move() on built-in types.
                binaryOperator(hasOperatorName("="), hasLHS(DeclRefMatcher)),
                cxxOperatorCallExpr(hasOverloadedOperatorName("="),
                                    hasArgument(0, DeclRefMatcher)),
                // Declaration. We treat this as a type of reinitialization too,
                // so we don't need to treat it separately.
                declStmt(hasDescendant(equalsNode(MovedVariable))),
                // clear() and assign() on standard containers.
                cxxMemberCallExpr(
                    on(allOf(DeclRefMatcher, StandardContainerTypeMatcher)),
                    // To keep the matcher simple, we check for assign() calls
                    // on all standard containers, even though only vector,
                    // deque, forward_list and list have assign(). If assign()
                    // is called on any of the other containers, this will be
                    // flagged by a compile error anyway.
                    callee(cxxMethodDecl(hasAnyName("clear", "assign")))),
+               // reset() on standard smart pointers.
+               cxxMemberCallExpr(
+                   on(allOf(DeclRefMatcher, StandardSmartPointerTypeMatcher)),
+                   callee(cxxMethodDecl(hasName("reset")))),
                // Passing variable to a function as a non-const pointer.
                callExpr(forEachArgumentWithParam(
                    unaryOperator(hasOperatorName("&"),
                                  hasUnaryOperand(DeclRefMatcher)),
                    unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))),
                // Passing variable to a function as a non-const lvalue reference
                // (unless that function is std::move()).
                callExpr(forEachArgumentWithParam(
Check->diag(UseLoc,
                 DiagnosticIDs::Note);
   }
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     return;
 
-  auto StandardSmartPointerTypeMatcher = hasType(
-      cxxRecordDecl(hasAnyName("::std::unique_ptr", "::std::shared_ptr")));
-
   auto CallMoveMatcher =
       callExpr(
           callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
-          hasArgument(
-              0,
-              declRefExpr(unless(StandardSmartPointerTypeMatcher)).bind("arg")),
+          hasArgument(0, declRefExpr().bind("arg")),
           anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
                 hasAncestor(functionDecl().bind("containing-func"))),
           unless(inDecltypeOrTemplateArg()))
           .bind("call-move");
 
   Finder->addMatcher(
       // To find the Stmt that we assume performs the actual move, we look for
       // the direct ancestor of the std::move() that isn't one of the node

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-use-after-move.rst

.. code-block:: c++
     if (i == 1) {
       messages.emplace_back(std::move(str));
       str = "";
     }
     if (i == 2) {
       std::cout << str;
     }
 
-No warnings are emitted for objects of type ``std::unique_ptr`` and
-``std::shared_ptr``, as they have defined move behavior. (Objects of these
-classes are guaranteed to be empty after they have been moved from.)
-
 Subsections below explain more precisely what exactly the check considers to be
 a move, use, and reinitialization.
 
 Unsequenced moves, uses, and reinitializations
 ----------------------------------------------
 
 In many cases, C++ does not make any guarantees about the order in which
 sub-expressions of a statement are evaluated. This means that in code like the

 to occur in whichever function the result of the ``std::move`` is passed to.
 
 Use
 ---
 
 Any occurrence of the moved variable that is not a reinitialization (see below)
 is considered to be a use.
 
+An exception to this are objects of type ``std::unique_ptr``,
+``std::shared_ptr`` and ``std::weak_ptr``, which have defined move behavior
+(objects of these classes are guaranteed to be empty after they have been moved
+from). Therefore, an object of these classes `` will only be considered to be
+used if it is dereferenced, i.e. if ``operator*``, ``operator->`` or
+``operator[]`` (in the case of ``std::unique_ptr<T []>``) is called on it.
+
 If multiple uses occur after a move, only the first of these is flagged.
 
 Reinitialization
 ----------------
 
 The check considers a variable to be reinitialized in the following cases:
 
   - The variable occurs on the left-hand side of an assignment.
 
   - The variable is passed to a function as a non-const pointer or non-const
     lvalue reference. (It is assumed that the variable may be an out-parameter
     for the function.)
 
   - ``clear()`` or ``assign()`` is called on the variable and the variable is of
     one of the standard container types ``basic_string``, ``vector``, ``deque``,
     ``forward_list``, ``list``, ``set``, ``map``, ``multiset``, ``multimap``,
     ``unordered_set``, ``unordered_map``, ``unordered_multiset``,
     ``unordered_multimap``.
 
+  - ``reset()`` is called on the variable and the variable is of type
+    ``std::unique_ptr``, ``std::shared_ptr`` or ``std::weak_ptr``.
+
 If the variable in question is a struct and an individual member variable of
 that struct is written to, the check does not consider this to be a
 reinitialization -- even if, eventually, all member variables of the struct are
 written to. For example:
 
   .. code-block:: c++
 
     struct S {

clang-tools-extra/trunk/test/clang-tidy/misc-use-after-move.cpp

 // RUN: %check_clang_tidy %s misc-use-after-move %t -- -- -std=c++11 -fno-delayed-template-parsing
 
 typedef decltype(nullptr) nullptr_t;
 
 namespace std {
 typedef unsigned size_t;
 
 template <typename T>
 struct unique_ptr {
   unique_ptr();
   T *get() const;
+  explicit operator bool() const;
+  void reset(T *ptr);
+  T &operator*() const;
+  T *operator->() const;
+  T& operator[](size_t i) const;
 };
 
 template <typename T>
 struct shared_ptr {
   shared_ptr();
   T *get() const;
+  explicit operator bool() const;
+  void reset(T *ptr);
+  T &operator*() const;
+  T *operator->() const;
+};
+
+template <typename T>
+struct weak_ptr {
+  weak_ptr();
+  bool expired() const;
 };
 
 #define DECLARE_STANDARD_CONTAINER(name) \
   template <typename T>                  \
   struct name {                          \
     name();                              \
     void clear();                        \
     bool empty();                        \

 // Checks also works on function parameters that have a use-after move.
 void parameters(A a) {
   std::move(a);
   a.foo();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
   // CHECK-MESSAGES: [[@LINE-3]]:3: note: move occurred here
 }
 
-void uniquePtrAndSharedPtr() {
-  // Use-after-moves on std::unique_ptr<> or std::shared_ptr<> aren't flagged.
+void standardSmartPtr() {
+  // std::unique_ptr<>, std::shared_ptr<> and std::weak_ptr<> are guaranteed to
+  // be null after a std::move. So the check only flags accesses that would
+  // dereference the pointer.
   {
     std::unique_ptr<A> ptr;
     std::move(ptr);
     ptr.get();
+    static_cast<bool>(ptr);
+    *ptr;
+    // CHECK-MESSAGES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved
+    // CHECK-MESSAGES: [[@LINE-5]]:5: note: move occurred here
+  }
+  {
+    std::unique_ptr<A> ptr;
+    std::move(ptr);
+    ptr->foo();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+  }
+  {
+    std::unique_ptr<A> ptr;
+    std::move(ptr);
+    ptr[0];
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
   }
   {
     std::shared_ptr<A> ptr;
     std::move(ptr);
     ptr.get();
+    static_cast<bool>(ptr);
+    *ptr;
+    // CHECK-MESSAGES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved
+    // CHECK-MESSAGES: [[@LINE-5]]:5: note: move occurred here
+  }
+  {
+    std::shared_ptr<A> ptr;
+    std::move(ptr);
+    ptr->foo();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
   }
-  // This is also true if the std::unique_ptr<> or std::shared_ptr<> is wrapped
-  // in a typedef.
+  {
+    // std::weak_ptr<> cannot be dereferenced directly, so we only check that
+    // member functions may be called on it after a move.
+    std::weak_ptr<A> ptr;
+    std::move(ptr);
+    ptr.expired();
+  }
+  // Make sure we recognize std::unique_ptr<> or std::shared_ptr<> if they're
+  // wrapped in a typedef.
   {
     typedef std::unique_ptr<A> PtrToA;
     PtrToA ptr;
     std::move(ptr);
     ptr.get();
   }
   {
     typedef std::shared_ptr<A> PtrToA;
     PtrToA ptr;
     std::move(ptr);
     ptr.get();
   }
-  // And it's also true if the template argument is a little more involved.
+  // And we don't get confused if the template argument is a little more
+  // involved.
   {
     struct B {
       typedef A AnotherNameForA;
     };
     std::unique_ptr<B::AnotherNameForA> ptr;
     std::move(ptr);
     ptr.get();
   }
+  // We don't give any special treatment to types that are called "unique_ptr"
+  // or "shared_ptr" but are not in the "::std" namespace.
+  {
+    struct unique_ptr {
+      void get();
+    } ptr;
+    std::move(ptr);
+    ptr.get();
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-MESSAGES: [[@LINE-3]]:5: note: move occurred here
+  }
 }
 
 // The check also works in member functions.
 class Container {
   void useAfterMoveInMemberFunction() {
     A a;
     std::move(a);
     a.foo();
// An intervening assign() on a different container does not reinitialize.
     std::move(container1);
     container2.assign(0, 0);
     container1.empty();
     // CHECK-MESSAGES: [[@LINE-1]]:5: warning: 'container1' used after it was
     // CHECK-MESSAGES: [[@LINE-4]]:5: note: move occurred here
   }
 }
 
+// Resetting the standard smart pointer types using reset() is treated as a
+// re-initialization. (We don't test std::weak_ptr<> because it can't be
+// dereferenced directly.)
+void standardSmartPointerResetIsReinit() {
+  {
+    std::unique_ptr<A> ptr;
+    std::move(ptr);
+    ptr.reset(new A);
+    *ptr;
+  }
+  {
+    std::shared_ptr<A> ptr;
+    std::move(ptr);
+    ptr.reset(new A);
+    *ptr;
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Tests related to order of evaluation within expressions
 
 // Relative sequencing of move and use.
 void passByRvalueReference(int i, A &&a);
 void passByValue(int i, A a);
 void passByValue(A a, int i);
 A g(A, A &&);