[clang-tools-extra] [clang-tidy] Fix smart pointers handling in bugprone-use-after-move (PR #94869)

Piotr Zegar via cfe-commits cfe-commits at lists.llvm.org
Sat Jun 8 12:58:36 PDT 2024


https://github.com/PiotrZSL created https://github.com/llvm/llvm-project/pull/94869

- Removed custom smart pointers handling (were hiding issues)
- Changed 'move occurred here' note location to always point to 'std::move'

Closes #90174


>From 1179f63d792da2462e1490c1b0a59cf1e6e47349 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Sat, 8 Jun 2024 19:41:51 +0000
Subject: [PATCH 1/3] [clang-tidy] Fix smart pointers handling in
 bugprone-use-after-move

- Removed custom smart pointers handling (were hidding issues)
- Changed 'move occurred here' note location to always point to 'std::move'
- Fixed issue when 'std::move' of 'x' on initialization list would be
  incorrectly detected as use after move.

Closes #90174
---
 .../clang-tidy/bugprone/UseAfterMoveCheck.cpp |  39 +-----
 .../checkers/bugprone/use-after-move.cpp      | 114 ++++++++++++++----
 2 files changed, 95 insertions(+), 58 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index b91ad0f182295..593ed11d8ac43 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -215,26 +215,6 @@ void UseAfterMoveFinder::getUsesAndReinits(
   });
 }
 
-bool isStandardSmartPointer(const ValueDecl *VD) {
-  const Type *TheType = VD->getType().getNonReferenceType().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) {
@@ -248,13 +228,8 @@ void UseAfterMoveFinder::getDeclRefs(
                         DeclRefs](const ArrayRef<BoundNodes> Matches) {
       for (const auto &Match : Matches) {
         const auto *DeclRef = Match.getNodeAs<DeclRefExpr>("declref");
-        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);
-          }
+          DeclRefs->insert(DeclRef);
         }
       }
     };
@@ -265,11 +240,6 @@ void UseAfterMoveFinder::getDeclRefs(
 
     AddDeclRefs(match(traverse(TK_AsIs, findAll(DeclRefMatcher)), *S->getStmt(),
                       *Context));
-    AddDeclRefs(match(findAll(cxxOperatorCallExpr(
-                                  hasAnyOverloadedOperatorName("*", "->", "[]"),
-                                  hasArgument(0, DeclRefMatcher))
-                                  .bind("operator")),
-                      *S->getStmt(), *Context));
   }
 }
 
@@ -411,10 +381,9 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   auto TryEmplaceMatcher =
       cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
   auto CallMoveMatcher =
-      callExpr(argumentCountIs(1),
+      callExpr(argumentCountIs(1), hasArgument(0, declRefExpr().bind("arg")),
                callee(functionDecl(hasAnyName("::std::move", "::std::forward"))
                           .bind("move-decl")),
-               hasArgument(0, declRefExpr().bind("arg")),
                unless(inDecltypeOrTemplateArg()),
                unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
                anyOf(hasAncestor(compoundStmt(
@@ -464,7 +433,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl");
 
   if (!MovingCall || !MovingCall->getExprLoc().isValid())
-    MovingCall = CallMove;
+    MovingCall = ContainingCtorInit ? ContainingCtorInit : CallMove;
 
   // Ignore the std::move if the variable that was passed to it isn't a local
   // variable.
@@ -496,7 +465,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
     UseAfterMoveFinder Finder(Result.Context);
     UseAfterMove Use;
     if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
-      emitDiagnostic(MovingCall, Arg, Use, this, Result.Context,
+      emitDiagnostic(CallMove, Arg, Use, this, Result.Context,
                      determineMoveType(MoveDecl));
   }
 }
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 7d9f63479a1b4..3d06dc008d353 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -8,6 +8,7 @@ typedef unsigned size_t;
 template <typename T>
 struct unique_ptr {
   unique_ptr();
+  unique_ptr(T* ptr);
   T *get() const;
   explicit operator bool() const;
   void reset(T *ptr);
@@ -123,6 +124,10 @@ forward(typename std::remove_reference<_Tp>::type&& __t) noexcept {
   return static_cast<_Tp&&>(__t);
 }
 
+template <typename T, typename... Args> auto make_unique(Args &&...args) {
+  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
+}
+
 } // namespace std
 
 class A {
@@ -220,10 +225,8 @@ void standardSmartPtr() {
     std::unique_ptr<A> ptr;
     std::move(ptr);
     ptr.get();
-    static_cast<bool>(ptr);
-    *ptr;
-    // CHECK-NOTES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved
-    // CHECK-NOTES: [[@LINE-5]]:5: note: move occurred here
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   {
     std::unique_ptr<A> ptr;
@@ -243,10 +246,8 @@ void standardSmartPtr() {
     std::shared_ptr<A> ptr;
     std::move(ptr);
     ptr.get();
-    static_cast<bool>(ptr);
-    *ptr;
-    // CHECK-NOTES: [[@LINE-1]]:6: warning: 'ptr' used after it was moved
-    // CHECK-NOTES: [[@LINE-5]]:5: note: move occurred here
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   {
     std::shared_ptr<A> ptr;
@@ -261,6 +262,8 @@ void standardSmartPtr() {
     std::weak_ptr<A> ptr;
     std::move(ptr);
     ptr.expired();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   // Make sure we recognize std::unique_ptr<> or std::shared_ptr<> if they're
   // wrapped in a typedef.
@@ -269,12 +272,16 @@ void standardSmartPtr() {
     PtrToA ptr;
     std::move(ptr);
     ptr.get();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   {
     typedef std::shared_ptr<A> PtrToA;
     PtrToA ptr;
     std::move(ptr);
     ptr.get();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   // And we don't get confused if the template argument is a little more
   // involved.
@@ -285,6 +292,8 @@ void standardSmartPtr() {
     std::unique_ptr<B::AnotherNameForA> ptr;
     std::move(ptr);
     ptr.get();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   // Make sure we treat references to smart pointers correctly.
   {
@@ -292,12 +301,16 @@ void standardSmartPtr() {
     std::unique_ptr<A>& ref_to_ptr = ptr;
     std::move(ref_to_ptr);
     ref_to_ptr.get();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'ref_to_ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   {
     std::unique_ptr<A> ptr;
     std::unique_ptr<A>&& rvalue_ref_to_ptr = std::move(ptr);
     std::move(rvalue_ref_to_ptr);
     rvalue_ref_to_ptr.get();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'rvalue_ref_to_ptr' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
   }
   // We don't give any special treatment to types that are called "unique_ptr"
   // or "shared_ptr" but are not in the "::std" namespace.
@@ -329,7 +342,7 @@ void moveInDeclaration() {
   A another_a(std::move(a));
   a.foo();
   // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved
-  // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+  // CHECK-NOTES: [[@LINE-3]]:15: note: move occurred here
 }
 
 // We see the std::move if it's inside an initializer list. Initializer lists
@@ -1068,10 +1081,10 @@ void sequencingOfMoveAndUse() {
     A a;
     g(g(a, std::move(a)), g(a, std::move(a)));
     // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:27: note: move occurred here
+    // CHECK-NOTES: [[@LINE-2]]:32: note: move occurred here
     // CHECK-NOTES: [[@LINE-3]]:9: note: the use and move are unsequenced
     // CHECK-NOTES: [[@LINE-4]]:29: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-5]]:7: note: move occurred here
+    // CHECK-NOTES: [[@LINE-5]]:12: note: move occurred here
     // CHECK-NOTES: [[@LINE-6]]:29: note: the use and move are unsequenced
   }
   // This case is fine because the actual move only happens inside the call to
@@ -1088,7 +1101,7 @@ void sequencingOfMoveAndUse() {
     int v[3];
     v[a.getInt()] = intFromA(std::move(a));
     // CHECK-NOTES: [[@LINE-1]]:7: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:21: note: move occurred here
+    // CHECK-NOTES: [[@LINE-2]]:30: note: move occurred here
     // CHECK-NOTES: [[@LINE-3]]:7: note: the use and move are unsequenced
   }
   {
@@ -1096,7 +1109,7 @@ void sequencingOfMoveAndUse() {
     int v[3];
     v[intFromA(std::move(a))] = intFromInt(a.i);
     // CHECK-NOTES: [[@LINE-1]]:44: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:7: note: move occurred here
+    // CHECK-NOTES: [[@LINE-2]]:16: note: move occurred here
     // CHECK-NOTES: [[@LINE-3]]:44: note: the use and move are unsequenced
   }
 }
@@ -1168,7 +1181,7 @@ void commaOperatorSequences() {
     (a = A()), A(std::move(a));
     a.foo();
     // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-3]]:16: note: move occurred here
+    // CHECK-NOTES: [[@LINE-3]]:18: note: move occurred here
   }
 }
 
@@ -1210,7 +1223,7 @@ void initializerListSequences() {
     A a;
     S2 s2{.a = std::move(a), .i = a.getInt()};
     // CHECK-NOTES: [[@LINE-1]]:35: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+    // CHECK-NOTES: [[@LINE-2]]:16: note: move occurred here
   }
   {
     // Check the case where the constructed type has a constructor and the
@@ -1264,7 +1277,7 @@ void logicalOperatorsSequence() {
     A a;
     if (A(std::move(a)).getInt() > 0 && a.getInt() > 0) {
       // CHECK-NOTES: [[@LINE-1]]:41: warning: 'a' used after it was moved
-      // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here
+      // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
       A().foo();
     }
   }
@@ -1278,7 +1291,7 @@ void logicalOperatorsSequence() {
     A a;
     if (A(std::move(a)).getInt() > 0 || a.getInt() > 0) {
       // CHECK-NOTES: [[@LINE-1]]:41: warning: 'a' used after it was moved
-      // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here
+      // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
       A().foo();
     }
   }
@@ -1324,7 +1337,7 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() {
   for (int i = 0; i < 10; ++i) {
     if (A a1; A(std::move(a1)).getInt() > a1.getInt()) {}
     // CHECK-NOTES: [[@LINE-1]]:43: warning: 'a1' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:15: note: move occurred here
+    // CHECK-NOTES: [[@LINE-2]]:17: note: move occurred here
     // CHECK-NOTES: [[@LINE-3]]:43: note: the use and move are unsequenced
   }
   for (int i = 0; i < 10; ++i) {
@@ -1419,7 +1432,7 @@ class CtorInit {
         s{std::move(val)},
         b{val.empty()}
   // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
-  // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here
+  // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
   {}
 
 private:
@@ -1435,7 +1448,7 @@ class CtorInitLambda {
         s{std::move(val)},
         b{[&] { return val.empty(); }()},
         // CHECK-NOTES: [[@LINE-1]]:12: warning: 'val' used after it was moved
-        // CHECK-NOTES: [[@LINE-3]]:9: note: move occurred here
+        // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
         c{[] {
           std::string str{};
           std::move(str);
@@ -1445,7 +1458,7 @@ class CtorInitLambda {
         }()} {
     std::move(val);
     // CHECK-NOTES: [[@LINE-1]]:15: warning: 'val' used after it was moved
-    // CHECK-NOTES: [[@LINE-13]]:9: note: move occurred here
+    // CHECK-NOTES: [[@LINE-13]]:11: note: move occurred here
     std::string val2{};
     std::move(val2);
     val2.empty();
@@ -1468,7 +1481,7 @@ class CtorInitOrder {
         b{val.empty()},
         // CHECK-NOTES: [[@LINE-1]]:11: warning: 'val' used after it was moved
         s{std::move(val)} {} // wrong order
-  // CHECK-NOTES: [[@LINE-1]]:9: note: move occurred here
+  // CHECK-NOTES: [[@LINE-1]]:11: note: move occurred here
   // CHECK-NOTES: [[@LINE-4]]:11: note: the use happens in a later loop iteration than the move
 
 private:
@@ -1531,7 +1544,7 @@ class PR38187 {
   PR38187(std::string val) : val_(std::move(val)) {
     val.empty();
     // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val' used after it was moved
-    // CHECK-NOTES: [[@LINE-3]]:30: note: move occurred here
+    // CHECK-NOTES: [[@LINE-3]]:35: note: move occurred here
   }
 
 private:
@@ -1562,3 +1575,58 @@ void create() {
 }
 
 } // namespace issue82023
+
+namespace PR90174 {
+
+struct A {};
+
+struct SinkA {
+  SinkA(std::unique_ptr<A>);
+};
+
+class ClassB {
+  ClassB(std::unique_ptr<A> aaa) : aa(std::move(aaa)) {
+    a = std::make_unique<SinkA>(std::move(aaa));
+    // CHECK-NOTES: [[@LINE-1]]:43: warning: 'aaa' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:39: note: move occurred here
+  }
+  std::unique_ptr<A> aa;
+  std::unique_ptr<SinkA> a;
+};
+
+void s(const std::unique_ptr<A> &);
+
+template <typename T, typename... Args> auto my_make_unique(Args &&...args) {
+  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
+}
+
+void natively(std::unique_ptr<A> x) {
+  std::unique_ptr<A> tmp = std::move(x);
+  std::unique_ptr<SinkA> y2{new SinkA(std::move(x))};
+  // CHECK-NOTES: [[@LINE-1]]:49: warning: 'x' used after it was moved
+  // CHECK-NOTES: [[@LINE-3]]:28: note: move occurred here
+}
+
+void viaStdMakeUnique(std::unique_ptr<A> x) {
+  std::unique_ptr<A> tmp = std::move(x);
+  std::unique_ptr<SinkA> y2 =
+      std::make_unique<SinkA>(std::move(x));
+  // CHECK-NOTES: [[@LINE-1]]:41: warning: 'x' used after it was moved
+  // CHECK-NOTES: [[@LINE-4]]:28: note: move occurred here
+}
+
+void viaMyMakeUnique(std::unique_ptr<A> x) {
+  std::unique_ptr<A> tmp = std::move(x);
+  std::unique_ptr<SinkA> y2 = my_make_unique<SinkA>(std::move(x));
+  // CHECK-NOTES: [[@LINE-1]]:63: warning: 'x' used after it was moved
+  // CHECK-NOTES: [[@LINE-3]]:28: note: move occurred here
+}
+
+void viaMyMakeUnique2(std::unique_ptr<A> x) {
+  std::unique_ptr<A> tmp = std::move(x);
+  s(x);
+  // CHECK-NOTES: [[@LINE-1]]:5: warning: 'x' used after it was moved
+  // CHECK-NOTES: [[@LINE-3]]:28: note: move occurred here
+}
+
+}

>From 30e5074e9727661dff24ee0b190b1e89a3f622de Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Sat, 8 Jun 2024 19:49:04 +0000
Subject: [PATCH 2/3] Add release notes & documentation

---
 clang-tools-extra/docs/ReleaseNotes.rst                    | 3 ++-
 .../docs/clang-tidy/checks/bugprone/use-after-move.rst     | 7 -------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 277a6e75da2ac..f9b84a6df532e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -254,7 +254,8 @@ Changes in existing checks
 
 - Improved :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>` check to also handle
-  calls to ``std::forward``.
+  calls to ``std::forward``. Smart pointers are now handled like any other
+  objects allowing to detect more cases.
 
 - Improved :doc:`cppcoreguidelines-missing-std-forward
   <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
index 08bb5374bab1f..faf9e4dddc12c 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst
@@ -195,13 +195,6 @@ 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

>From dfad1e2ff4495d50d5da8f5604550dc367579f3c Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Sat, 8 Jun 2024 19:58:13 +0000
Subject: [PATCH 3/3] Remove not needed change

---
 clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index 593ed11d8ac43..4f1ea32da20f4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -433,7 +433,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MoveDecl = Result.Nodes.getNodeAs<FunctionDecl>("move-decl");
 
   if (!MovingCall || !MovingCall->getExprLoc().isValid())
-    MovingCall = ContainingCtorInit ? ContainingCtorInit : CallMove;
+    MovingCall = CallMove;
 
   // Ignore the std::move if the variable that was passed to it isn't a local
   // variable.



More information about the cfe-commits mailing list