clang-tidy: Detect use-after-move in CXXCtorInitializer

Fixes https://github.com/llvm/llvm-project/issues/51844

Differential Revision: https://reviews.llvm.org/D146288

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

@@ -58,11 +58,11 @@ class UseAfterMoveFinder {
 public:
   UseAfterMoveFinder(ASTContext *TheContext);
 
-  // Within the given function body, finds the first use of 'MovedVariable' that
+  // Within the given code block, finds the first use of 'MovedVariable' that
   // occurs after 'MovingCall' (the expression that performs the move). If a
   // use-after-move is found, writes information about it to 'TheUseAfterMove'.
   // Returns whether a use-after-move was found.
-  bool find(Stmt *FunctionBody, const Expr *MovingCall,
+  bool find(Stmt *CodeBlock, const Expr *MovingCall,
             const ValueDecl *MovedVariable, UseAfterMove *TheUseAfterMove);
 
 private:
@@ -104,7 +104,7 @@ static StatementMatcher inDecltypeOrTemplateArg() {
 UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext)
     : Context(TheContext) {}
 
-bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
+bool UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall,
                               const ValueDecl *MovedVariable,
                               UseAfterMove *TheUseAfterMove) {
   // Generate the CFG manually instead of through an AnalysisDeclContext because
@@ -118,12 +118,11 @@ bool UseAfterMoveFinder::find(Stmt *FunctionBody, const Expr *MovingCall,
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   std::unique_ptr<CFG> TheCFG =
-      CFG::buildCFG(nullptr, FunctionBody, Context, Options);
+      CFG::buildCFG(nullptr, CodeBlock, Context, Options);
   if (!TheCFG)
     return false;
 
-  Sequence =
-      std::make_unique<ExprSequence>(TheCFG.get(), FunctionBody, Context);
+  Sequence = std::make_unique<ExprSequence>(TheCFG.get(), CodeBlock, Context);
   BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Context);
   Visited.clear();
 
@@ -398,20 +397,28 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+  // try_emplace is a common maybe-moving function that returns a
+  // bool to tell callers whether it moved. Ignore std::move inside
+  // try_emplace to avoid false positives as we don't track uses of
+  // the bool.
+  auto TryEmplaceMatcher =
+      cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
   auto CallMoveMatcher =
-      callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+      callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))),
                hasArgument(0, declRefExpr().bind("arg")),
+               unless(inDecltypeOrTemplateArg()),
+               unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
                anyOf(hasAncestor(compoundStmt(
                          hasParent(lambdaExpr().bind("containing-lambda")))),
-                     hasAncestor(functionDecl().bind("containing-func"))),
-               unless(inDecltypeOrTemplateArg()),
-               // try_emplace is a common maybe-moving function that returns a
-               // bool to tell callers whether it moved. Ignore std::move inside
-               // try_emplace to avoid false positives as we don't track uses of
-               // the bool.
-               unless(hasParent(cxxMemberCallExpr(
-                   callee(cxxMethodDecl(hasName("try_emplace")))))))
-          .bind("call-move");
+                     hasAncestor(functionDecl(anyOf(
+                         cxxConstructorDecl(
+                             hasAnyConstructorInitializer(withInitializer(
+                                 expr(anyOf(equalsBoundNode("call-move"),
+                                            hasDescendant(expr(
+                                                equalsBoundNode("call-move")))))
+                                     .bind("containing-ctor-init"))))
+                             .bind("containing-ctor"),
+                         functionDecl().bind("containing-func"))))));
 
   Finder->addMatcher(
       traverse(
@@ -434,6 +441,10 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
 }
 
 void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ContainingCtor =
+      Result.Nodes.getNodeAs<CXXConstructorDecl>("containing-ctor");
+  const auto *ContainingCtorInit =
+      Result.Nodes.getNodeAs<Expr>("containing-ctor-init");
   const auto *ContainingLambda =
       Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
   const auto *ContainingFunc =
@@ -445,23 +456,38 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   if (!MovingCall || !MovingCall->getExprLoc().isValid())
     MovingCall = CallMove;
 
-  Stmt *FunctionBody = nullptr;
-  if (ContainingLambda)
-    FunctionBody = ContainingLambda->getBody();
-  else if (ContainingFunc)
-    FunctionBody = ContainingFunc->getBody();
-  else
-    return;
-
   // Ignore the std::move if the variable that was passed to it isn't a local
   // variable.
   if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod())
     return;
 
-  UseAfterMoveFinder Finder(Result.Context);
-  UseAfterMove Use;
-  if (Finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use))
-    emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+  // Collect all code blocks that could use the arg after move.
+  llvm::SmallVector<Stmt *> CodeBlocks{};
+  if (ContainingCtor) {
+    CodeBlocks.push_back(ContainingCtor->getBody());
+    if (ContainingCtorInit) {
+      // Collect the constructor initializer expressions.
+      bool BeforeMove{true};
+      for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+        if (BeforeMove && Init->getInit()->IgnoreImplicit() ==
+                              ContainingCtorInit->IgnoreImplicit())
+          BeforeMove = false;
+        if (!BeforeMove)
+          CodeBlocks.push_back(Init->getInit());
+      }
+    }
+  } else if (ContainingLambda) {
+    CodeBlocks.push_back(ContainingLambda->getBody());
+  } else if (ContainingFunc) {
+    CodeBlocks.push_back(ContainingFunc->getBody());
+  }
+
+  for (Stmt *CodeBlock : CodeBlocks) {
+    UseAfterMoveFinder Finder(Result.Context);
+    UseAfterMove Use;
+    if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use))
+      emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
+  }
 }
 
 } // namespace clang::tidy::bugprone

clang-tools-extra/docs/ReleaseNotes.rst

@@ -162,6 +162,10 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/suspicious-include>` check.
   Global options of the same name should be used instead.
 
+- Improved :doc:`bugprone-use-after-move
+  <clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
+  initializers.
+
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`google-build-namespaces
   <clang-tidy/checks/google/build-namespaces>` check.

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

@@ -369,6 +369,18 @@ void lambdas() {
     };
     a.foo();
   }
+  // Don't warn if 'a' is a copy inside a synchronous lambda
+  {
+    A a;
+    A copied{[a] mutable { return std::move(a); }()};
+    a.foo();
+  }
+  // False negative (should warn if 'a' is a ref inside a synchronous lambda)
+  {
+    A a;
+    A moved{[&a] mutable { return std::move(a); }()};
+    a.foo();
+  }
   // Warn if the use consists of a capture that happens after a move.
   {
     A a;
@@ -1367,6 +1379,120 @@ void typeId() {
 }
 } // namespace UnevalContext
 
+class CtorInit {
+public:
+  CtorInit(std::string val)
+      : a{val.empty()},    // fine
+        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
+  {}
+
+private:
+  bool a;
+  std::string s;
+  bool b;
+};
+
+class CtorInitLambda {
+public:
+  CtorInitLambda(std::string val)
+      : a{val.empty()},    // fine
+        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
+        c{[] {
+          std::string str{};
+          std::move(str);
+          return str.empty();
+          // CHECK-NOTES: [[@LINE-1]]:18: warning: 'str' used after it was moved
+          // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
+        }()} {
+    std::move(val);
+    // CHECK-NOTES: [[@LINE-1]]:15: warning: 'val' used after it was moved
+    // CHECK-NOTES: [[@LINE-13]]:9: note: move occurred here
+    std::string val2{};
+    std::move(val2);
+    val2.empty();
+    // CHECK-NOTES: [[@LINE-1]]:5: warning: 'val2' used after it was moved
+    // CHECK-NOTES: [[@LINE-3]]:5: note: move occurred here
+  }
+
+private:
+  bool a;
+  std::string s;
+  bool b;
+  bool c;
+  bool d{};
+};
+
+class CtorInitOrder {
+public:
+  CtorInitOrder(std::string val)
+      : a{val.empty()}, // fine
+        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-4]]:11: note: the use happens in a later loop iteration than the move
+
+private:
+  bool a;
+  std::string s;
+  bool b;
+};
+
+struct Obj {};
+struct CtorD {
+  CtorD(Obj b);
+};
+
+struct CtorC {
+  CtorC(Obj b);
+};
+
+struct CtorB {
+  CtorB(Obj &b);
+};
+
+struct CtorA : CtorB, CtorC, CtorD {
+  CtorA(Obj b) : CtorB{b}, CtorC{std::move(b)}, CtorD{b} {}
+  // CHECK-NOTES: [[@LINE-1]]:55: warning: 'b' used after it was moved
+  // CHECK-NOTES: [[@LINE-2]]:34: note: move occurred here
+};
+
+struct Base {
+  Base(Obj b) : bb{std::move(b)} {}
+  template <typename Call> Base(Call &&c) : bb{c()} {};
+
+  Obj bb;
+};
+
+struct Derived : Base, CtorC {
+  Derived(Obj b)
+      : Base{[&] mutable { return std::move(b); }()},
+        // False negative: The lambda/std::move was executed, so it should warn
+        // below
+        CtorC{b} {}
+};
+
+struct Derived2 : Base, CtorC {
+  Derived2(Obj b)
+      : Base{[&] mutable { return std::move(b); }},
+        // This was a move, but it doesn't warn below, because it can't know if
+        // the lambda/std::move was actually called
+        CtorC{b} {}
+};
+
+struct Derived3 : Base, CtorC {
+  Derived3(Obj b)
+      : Base{[c = std::move(b)] mutable { return std::move(c); }}, CtorC{b} {}
+  // CHECK-NOTES: [[@LINE-1]]:74: warning: 'b' used after it was moved
+  // CHECK-NOTES: [[@LINE-2]]:19: note: move occurred here
+};
+
 class PR38187 {
 public:
   PR38187(std::string val) : val_(std::move(val)) {