[clang-tidy] Let bugprone-use-after-move ignore the moved variable in callee

In C++17, callee is guaranteed to be sequenced before arguments.

This eliminates false positives in bugprone-use-after-move where a variable
is used in the callee and moved from in the arguments.

We introduce one special case: If the callee is a MemberExpr with a DeclRefExpr as its base, we consider it to be sequenced after the arguments. This is because the variable referenced in the base will only actually be accessed when the call happens, i.e. once all of the arguments have been evaluated. This has no basis in the C++ standard, but it reflects actual behavior that is relevant to a use-after-move scenario:
```
a.bar(consumeA(std::move(a));
In this example, we end up accessing a after it has been moved from, even though nominally the callee a.bar is evaluated before the argument consumeA(std::move(a)).
```
Treating this scenario correctly has required rewriting the logic in bugprone-use-after-move that governs whether the use happens in a later loop iteration than the move. This was previously based on an unsound heuristic (does the use come lexically before the move?); we now use a more rigourous criterion based on reachability in the CFG.

Fixes #57758
Fixes #59612

Author: martinboehme

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

@@ -11,9 +11,11 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 
 #include "../utils/ExprSequence.h"
 #include "../utils/Matchers.h"
@@ -34,7 +36,12 @@ struct UseAfterMove {
   const DeclRefExpr *DeclRef;
 
   // Is the order in which the move and the use are evaluated undefined?
-  bool EvaluationOrderUndefined;
+  bool EvaluationOrderUndefined = false;
+
+  // Does the use happen in a later loop iteration than the move?
+  //
+  // We default to false and change it to true if required in find().
+  bool UseHappensInLaterLoopIteration = false;
 };
 
 /// Finds uses of a variable after a move (and maintains state required by the
@@ -48,7 +55,7 @@ class UseAfterMoveFinder {
   // use-after-move is found, writes information about it to 'TheUseAfterMove'.
   // Returns whether a use-after-move was found.
   bool find(Stmt *CodeBlock, const Expr *MovingCall,
-            const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
+            const DeclRefExpr *MovedVariable, UseAfterMove *TheUseAfterMove);
 
 private:
   bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
@@ -89,7 +96,7 @@ UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
     : Context(TheContext) {}
 
 bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
-                              const ValueDecl *MovedVariable,
+                              const DeclRefExpr *MovedVariable,
                               UseAfterMove *TheUseAfterMove) {
   // Generate the CFG manually instead of through an AnalysisDeclContext because
   // it seems the latter can't be used to generate a CFG for the body of a
@@ -110,15 +117,32 @@ bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
   BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
   Visited.clear();
 
-  const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
-  if (!Block) {
+  const CFGBlock *MoveBlock = BlockMap->blockContainingStmt(MovingCall);
+  if (!MoveBlock) {
     // This can happen if MovingCall is in a constructor initializer, which is
     // not included in the CFG because the CFG is built only from the function
     // body.
-    Block = &TheCFG->getEntry();
+    MoveBlock = &TheCFG->getEntry();
   }
 
-  return findInternal(Block, MovingCall, MovedVariable, TheUseAfterMove);
+  bool Found = findInternal(MoveBlock, MovingCall, MovedVariable->getDecl(),
+                            TheUseAfterMove);
+
+  if (Found) {
+    if (const CFGBlock *UseBlock =
+            BlockMap->blockContainingStmt(TheUseAfterMove->DeclRef)) {
+      // Does the use happen in a later loop iteration than the move?
+      // - If they are in the same CFG block, we know the use happened in a
+      //   later iteration if we visited that block a second time.
+      // - Otherwise, we know the use happened in a later iteration if the
+      //   move is reachable from the use.
+      CFGReverseBlockReachabilityAnalysis CFA(*TheCFG);
+      TheUseAfterMove->UseHappensInLaterLoopIteration =
+          UseBlock == MoveBlock ? Visited.contains(UseBlock)
+                                : CFA.isReachable(UseBlock, MoveBlock);
+    }
+  }
+  return Found;
 }
 
 bool UseAfterMoveFinder::findInternal(const CFGBlock *Block,
@@ -394,7 +418,7 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
         "there is no guarantee about the order in which they are evaluated",
         DiagnosticIDs::Note)
         << IsMove;
-  } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
+  } else if (Use.UseHappensInLaterLoopIteration) {
     Check->diag(UseLoc,
                 "the use happens in a later loop iteration than the "
                 "%select{forward|move}0",
@@ -495,7 +519,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   for (Stmt *CodeBlock : CodeBlocks) {
     UseAfterMoveFinder Finder(Result.Context);
     UseAfterMove Use;
-    if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+    if (Finder.find(CodeBlock, MovingCall, Arg, &Use))
       emitDiagnostic(MovingCall, Arg, Use, this, Result.Context,
                      determineMoveType(MoveDecl));
   }

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

@@ -55,12 +55,18 @@ bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
                          ASTContext *Context) {
   if (Descendant == Ancestor)
     return true;
-  for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
-    if (isDescendantOrEqual(Parent, Ancestor, Context))
-      return true;
-  }
+  return llvm::any_of(getParentStmts(Descendant, Context),
+                      [Ancestor, Context](const Stmt *Parent) {
+                        return isDescendantOrEqual(Parent, Ancestor, Context);
+                      });
+}
 
-  return false;
+bool isDescendantOfArgs(const Stmt *Descendant, const CallExpr *Call,
+                        ASTContext *Context) {
+  return llvm::any_of(Call->arguments(),
+                      [Descendant, Context](const Expr *Arg) {
+                        return isDescendantOrEqual(Descendant, Arg, Context);
+                      });
 }
 
 llvm::SmallVector<const InitListExpr *>
@@ -95,9 +101,59 @@ bool ExprSequence::inSequence(const Stmt *Before, const Stmt *After) const {
       return true;
   }
 
+  SmallVector<const Stmt *, 1> BeforeParents = getParentStmts(Before, Context);
+
+  // Since C++17, the callee of a call expression is guaranteed to be sequenced
+  // before all of the arguments.
+  // We handle this as a special case rather than using the general
+  // `getSequenceSuccessor` logic above because the callee expression doesn't
+  // have an unambiguous successor; the order in which arguments are evaluated
+  // is indeterminate.
+  for (const Stmt *Parent : BeforeParents) {
+    // Special case: If the callee is a `MemberExpr` with a `DeclRefExpr` as its
+    // base, we consider it to be sequenced _after_ the arguments. This is
+    // because the variable referenced in the base will only actually be
+    // accessed when the call happens, i.e. once all of the arguments have been
+    // evaluated. This has no basis in the C++ standard, but it reflects actual
+    // behavior that is relevant to a use-after-move scenario:
+    //
+    // ```
+    // a.bar(consumeA(std::move(a));
+    // ```
+    //
+    // In this example, we end up accessing `a` after it has been moved from,
+    // even though nominally the callee `a.bar` is evaluated before the argument
+    // `consumeA(std::move(a))`. Note that this is not specific to C++17, so
+    // we implement this logic unconditionally.
+    if (const auto *Call = dyn_cast<CXXMemberCallExpr>(Parent)) {
+      if (is_contained(Call->arguments(), Before) &&
+          isa<DeclRefExpr>(
+              Call->getImplicitObjectArgument()->IgnoreParenImpCasts()) &&
+          isDescendantOrEqual(After, Call->getImplicitObjectArgument(),
+                              Context))
+        return true;
+
+      // We need this additional early exit so that we don't fall through to the
+      // more general logic below.
+      if (const auto *Member = dyn_cast<MemberExpr>(Before);
+          Member && Call->getCallee() == Member &&
+          isa<DeclRefExpr>(Member->getBase()->IgnoreParenImpCasts()) &&
+          isDescendantOfArgs(After, Call, Context))
+        return false;
+    }
+
+    if (!Context->getLangOpts().CPlusPlus17)
+      continue;
+
+    if (const auto *Call = dyn_cast<CallExpr>(Parent);
+        Call && Call->getCallee() == Before &&
+        isDescendantOfArgs(After, Call, Context))
+      return true;
+  }
+
   // If 'After' is a parent of 'Before' or is sequenced after one of these
   // parents, we know that it is sequenced after 'Before'.
-  for (const Stmt *Parent : getParentStmts(Before, Context)) {
+  for (const Stmt *Parent : BeforeParents) {
     if (Parent == After || inSequence(Parent, After))
       return true;
   }

clang-tools-extra/docs/ReleaseNotes.rst

@@ -254,7 +254,10 @@ 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``. Fixed sequencing of designated initializers. Fixed
+  sequencing of callees: In C++17 and later, the callee of a function is guaranteed
+  to be sequenced before the arguments, so don't warn if the use happens in the
+  callee and the move happens in one of the arguments.
 
 - Improved :doc:`cppcoreguidelines-missing-std-forward
   <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer

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

@@ -1,3 +1,4 @@
+// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
 // RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- -- -fno-delayed-template-parsing
 
 typedef decltype(nullptr) nullptr_t;
@@ -135,6 +136,7 @@ class A {
   A &operator=(A &&);
 
   void foo() const;
+  void bar(int i) const;
   int getInt() const;
 
   operator bool() const;
@@ -576,6 +578,19 @@ void useAndMoveInLoop() {
       std::move(a);
     }
   }
+  // Same as above, but the use and the move are in different CFG blocks.
+  {
+    A a;
+    for (int i = 0; i < 10; ++i) {
+      if (i < 10)
+        a.foo();
+      // CHECK-NOTES: [[@LINE-1]]:9: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here
+      // CHECK-NOTES: [[@LINE-3]]:9: note: the use happens in a later loop
+      if (i < 10)
+        std::move(a);
+    }
+  }
   // However, this case shouldn't be flagged -- the scope of the declaration of
   // 'a' is important.
   {
@@ -1352,6 +1367,40 @@ void ifWhileAndSwitchSequenceInitDeclAndCondition() {
   }
 }
 
+// In a function call, the expression that determines the callee is sequenced
+// before the arguments -- but only in C++17 and later.
+namespace CalleeSequencedBeforeArguments {
+int consumeA(std::unique_ptr<A> a);
+int consumeA(A &&a);
+
+void calleeSequencedBeforeArguments() {
+  {
+    std::unique_ptr<A> a;
+    a->bar(consumeA(std::move(a)));
+    // CHECK-NOTES-CXX11: [[@LINE-1]]:5: warning: 'a' used after it was moved
+    // CHECK-NOTES-CXX11: [[@LINE-2]]:21: note: move occurred here
+    // CHECK-NOTES-CXX11: [[@LINE-3]]:5: note: the use and move are unsequenced
+  }
+  {
+    std::unique_ptr<A> a;
+    std::unique_ptr<A> getArg(std::unique_ptr<A> a);
+    getArg(std::move(a))->bar(a->getInt());
+    // CHECK-NOTES: [[@LINE-1]]:31: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:12: note: move occurred here
+    // CHECK-NOTES-CXX11: [[@LINE-3]]:31: note: the use and move are unsequenced
+  }
+  {
+    A a;
+    // Nominally, the callee `a.bar` is evaluated before the argument
+    // `consumeA(std::move(a))`, but in effect `a` is only accessed after the
+    // call to `A::bar()` happens, i.e. after the argument has been evaluted.
+    a.bar(consumeA(std::move(a)));
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+  }
+}
+} // namespace CalleeSequencedBeforeArguments
+
 // Some statements in templates (e.g. null, break and continue statements) may
 // be shared between the uninstantiated and instantiated versions of the
 // template and therefore have multiple parents. Make sure the sequencing code
@@ -1469,7 +1518,6 @@ class CtorInitOrder {
         // 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-4]]:11: note: the use happens in a later loop iteration than the move
 
 private:
   bool a;