[clang-tidy] Improve bugprone.use-after-move interaction with explicit destructor call. (llvm#188866)

It is valid (although niche) to call an explicit destructor after moving
the object.

Author: serge-sans-paille

clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp

@@ -82,6 +82,21 @@ class UseAfterMoveFinder {
   llvm::SmallPtrSet<const CFGBlock *, 8> Visited;
 };
 
+AST_MATCHER_P(Expr, hasParentIgnoringParenImpCasts,
+              ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+  const Expr *E = &Node;
+  do {
+    const DynTypedNodeList Parents = Finder->getASTContext().getParents(*E);
+    if (Parents.size() != 1)
+      return false;
+    E = Parents[0].get<Expr>();
+    if (!E)
+      return false;
+  } while (isa<ImplicitCastExpr, ParenExpr>(E));
+
+  return InnerMatcher.matches(*E, Finder, Builder);
+}
+
 } // namespace
 
 static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) {
@@ -401,9 +416,12 @@ void UseAfterMoveFinder::getDeclRefs(
       }
     };
 
-    auto DeclRefMatcher = declRefExpr(hasDeclaration(equalsNode(MovedVariable)),
-                                      unless(inDecltypeOrTemplateArg()))
-                              .bind("declref");
+    auto DeclRefMatcher =
+        declRefExpr(hasDeclaration(equalsNode(MovedVariable)),
+                    unless(inDecltypeOrTemplateArg()),
+                    unless(hasParentIgnoringParenImpCasts(
+                        memberExpr(hasDeclaration(cxxDestructorDecl())))))
+            .bind("declref");
 
     AddDeclRefs(match(traverse(TK_AsIs, findAll(DeclRefMatcher)), *S->getStmt(),
                       *Context));

clang-tools-extra/docs/ReleaseNotes.rst

@@ -275,6 +275,8 @@ Changes in existing checks
   - Add support for annotation of user-defined types as having the same
     moved-from semantics as standard smart pointers.
 
+  - Do not report explicit call to destructor after move as an invalid use.
+
 - Improved :doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines
   <clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines>`
   check by adding the `AllowExplicitObjectParameters` option. When enabled,

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

@@ -193,7 +193,7 @@ Use
 ---
 
 Any occurrence of the moved variable that is not a reinitialization (see below)
-is considered to be a use.
+or an explicit call to the variable destructor is considered to be a use.
 
 An exception to this are objects of type ``std::unique_ptr``,
 ``std::shared_ptr``, ``std::weak_ptr``, ``std::optional``, and ``std::any``,

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

@@ -89,6 +89,36 @@ void selfMove() {
   a.foo();
 }
 
+void * operator new(size_t, void *p);
+
+// Don't flag an explicit destructor call
+void explicitDestructor() {
+  alignas(A) char storage[sizeof(A)];
+  A& a = *new (storage) A();
+  std::move(a);
+  a.~A(); // It's always valid to destruct a moved object.
+
+  using B = AnnotatedContainer<int>;
+  alignas(B) char other_storage[sizeof(B)];
+  B& a_p = *new (other_storage) B();
+  std::move(a_p);
+  a_p.~B(); // Same as above, but with a template class.
+
+  A& b = *new (storage) A();
+  std::move(b);
+  (b).~A(); // Parenthesis should not change the behavior.
+  b.foo(); // But destruction is not a reinitialization.
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'b' used after it was moved
+  // CHECK-NOTES: [[@LINE-4]]:3: note: move occurred here
+
+  A& c = *new (storage) A();
+  std::move(c);
+  c.foo();
+  // CHECK-NOTES: [[@LINE-1]]:3: warning: 'c' used after it was moved
+  // CHECK-NOTES: [[@LINE-3]]:3: note: move occurred here
+  c.~A();
+}
+
 // A warning should only be emitted for one use-after-move.
 void onlyFlagOneUseAfterMove() {
   A a;