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

[MarcoFalke]

Summary

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

[MarcoFalke]

clang-format+doc

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

190

Please keep alphabetical order (by check name) in this section.

[MarcoFalke]

sort doc

[PiotrZSL]

I will check this more deeply during weekend.

[PiotrZSL]

Requesting changes due to lack of support for base class initializes & got some concerns related lambdas used in initializers.

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

400–401

This is correct but consider this:

auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));

auto CallMoveMatcher =
callExpr(argumentCountIs(1), // because matching this will be faster than checking name.
             callee(functionDecl(hasName("::std::move"))),
             unless(inDecltypeOrTemplateArg()),
             expr().bind("call-move"),
             unless(hasParent(TryEmplaceMatcher)),
             anyOf(hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda")))),
                        hasAncestor(functionDecl(anyOf(cxxConstructorDecl(hasAnyConstructorInitializer(
                                                                                                                   withInitializer(expr(
                                                                                                                          anyOf(equalsBounNode("call-move"), 
                                                                                                                                     hasDescendant(equalsBounNode("call-move")))
                                                                                                                     ).bind("containing-ctor-init-stmt")
                                                                                                              ))
                                                                                                             ).bind("containing-ctor"),
                                                                             functionDecl().bind("containing-func")))))
            );

474

consider using here ->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()

479–488

This won't work for std::move used to initialize base class. Assuming that Inits are in specific order, then you just should include all init's after one found, or use things like SourceManager::isBeforeInTranslationUnit, but still probably using something like:

bool PastMove = false;
for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
if (!PastMove  && Init->getInit()->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()) {
    PastMove  = true;
}

if (PastMove && Init->getInit())
{
    CodeBlocks.push_back(Init->getInit());
}

would be better, as it should work for both field inits and base constructor calls

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

1430

Missing tests:

• Test with A derive from B, C, D, and argument is moved into C, but D initialization also uses it.
• Test with lambda call inside init, and in lambda std::move of captured by reference constructor parameter, and lambda could be mutable, and lambda could be saved into member (or passed to base class), or called instantly (better both tests).
• Test with lambda that takes argument into capture by move

In short:

struct Obj {};
struct D
{
    D(Obj b);
};

struct C
{
     C(Obj b);
};

struct B
{
     B(Obj& b);
}

struct A : B, C, D
{
    A(Obj b)
      : B(b), C(std::move(b)), D(b)
    {
    }
};

and second:

struct Base
{
    Base(Obj b) : bb(std::move(b)) {}
     template<typename T>
     Base(T&& b) : bb(b()) {};
   
     Obj bb;
};

struct Derived : Base, C
{
     Derived(Obj b) 
           : Base([&] mutable { return std::move(b); }()), C(b)
   {
   }
};

struct Derived2 : Base, C
{
     Derived2(Obj b) 
           : Base([&] mutable { return std::move(b); }), C(b)
   {
   }
};

struct Derived3 : Base, C
{
     Derived3(Obj b) 
           : Base([c = std::move(b)] mutable { return std::move(c); }), C(b)
   {
   }
};

[MarcoFalke]

Took some review suggestions (Thanks!)

[MarcoFalke]

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

400–401

I get:

error: no matching function for call to object of type 'const internal::ArgumentAdaptingMatcherFunc<clang::ast_matchers::internal::HasDescendantMatcher>'
                    anyOf(equalsBoundNode("call-move"),hasDescendant(equalsBoundNode("call-move")))
                                                       ^~~~~~~~~~~~~
./llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1491:3: note: candidate template ignored: could not match 'Matcher' against 'PolymorphicMatcher'
  operator()(const Matcher<T> &InnerMatcher) const {
  ^
./llvm-project/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1498:3: note: candidate template ignored: could not match 'MapAnyOfHelper' against 'PolymorphicMatcher'
  operator()(const MapAnyOfHelper<T...> &InnerMatcher) const {
  ^
1 error generated.

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

1430

I added the test Derived2, however, I wonder if it should throw a warning. In the general case, I don't think we can know whether Base(Call&&call) will actually call call and execute the std::move, no?

[PiotrZSL]

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

400–401

yee, forgot type: hasDescendant(callExpr(equalsBoundNode("call-move"))))
+- something like this..

[PiotrZSL]

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

1430

For me:
Derived - should throw a warning - same type used before C(b) (but questionable)
Derived 2 -> also should throw warning (but questionable)
Derived 3 -> also should throw warning

Note that Derived & Derived 2 could be considered different issue, like use after free (because we take reference to argument), it's safe to assume that it would be called anyway, because later that argument reference could become invalid. But it could be also fine to not throw issue. For is better to throw & nolint than ignore and crash.
But thats your call.

[MarcoFalke]

address feedback about matcher, clang-format

[MarcoFalke]

Thanks, addressed some more review comments.

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

1430

Ok, maybe it can be done in a follow-up? This should be a separate and pre-existing issue on main already (see also my other added test case about lambdas).

[PiotrZSL]

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

1430

Yes it could be.

[PiotrZSL]

Overall LGTM, give it few days, maybe something will pop up.
I will try to run this change on my project and check if it finds any false-positives or real-issues.
Edit: Windows build failed due to clangd, try rebasing this to newer main branch

[MarcoFalke]

NFC

[MarcoFalke]

Minor NFC, I plan to land this tomorrow or Friday

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

 };
 
 /// Finds uses of a variable after a move (and maintains state required by the
 /// various internal helper functions).
 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:
   bool findInternal(const CFGBlock *Block, const Expr *MovingCall,
                     const ValueDecl *MovedVariable,
                     UseAfterMove *TheUseAfterMove);
   void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable,
                          llvm::SmallVectorImpl<const DeclRefExpr *> *Uses,
return anyOf(hasAncestor(typeLoc()),
                hasAncestor(declRefExpr(
                    to(functionDecl(ast_matchers::isTemplateInstantiation())))),
                hasAncestor(expr(hasUnevaluatedContext())));
 }
 
 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
   // it seems the latter can't be used to generate a CFG for the body of a
   // lambda.
   //
   // We include implicit and temporary destructors in the CFG so that
   // destructors marked [[noreturn]] are handled correctly in the control flow
   // analysis. (These are used in some styles of assertion macros.)
   CFG::BuildOptions Options;
   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();
 
   const CFGBlock *Block = BlockMap->blockContainingStmt(MovingCall);
   if (!Block) {
     // 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.
static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg,
   } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
     Check->diag(UseLoc,
                 "the use happens in a later loop iteration than the move",
                 DiagnosticIDs::Note);
   }
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   auto CallMoveMatcher =
-      callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
-               hasArgument(0, declRefExpr().bind("arg")),
-               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")))))))
+      callExpr(
+          callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+          anyOf(hasAncestor(
+                    stmt(
+                        // For some reason cxxCtorInitializer didn't
+                        // match, so work around by matching the topmost
+                        // Stmt inside the CXXCtorInitializer.
+                        hasParent(cxxConstructorDecl().bind("containing-ctor")),
+                        // Exclude the constructor body
+                        unless(compoundStmt()))
+                        .bind("containing-ctor-init-stmt")),
+                hasAncestor(compoundStmt(
           .bind("call-move");
 
   Finder->addMatcher(
       traverse(
           TK_AsIs,
           // 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 types ignored by ignoringParenImpCasts().
           stmt(
               forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
               // Don't allow an InitListExpr to be the moving call. An
               // InitListExpr has both a syntactic and a semantic form, and the
               // parent-child relationships are different between the two. This
               // could cause an InitListExpr to be analyzed as the moving call
               // in addition to the Expr that we actually want, resulting in two
               // diagnostics with different code locations for the same move.
               unless(initListExpr()),
               unless(expr(ignoringParenImpCasts(equalsBoundNode("call-move")))))
               .bind("moving-call")),
       this);
 }
 
 void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ContainingCtor =
+      Result.Nodes.getNodeAs<CXXConstructorDecl>("containing-ctor");
+  const auto *ContainingCtorInitStmt =
+      Result.Nodes.getNodeAs<Stmt>("containing-ctor-init-stmt");
   const auto *ContainingLambda =
       Result.Nodes.getNodeAs<LambdaExpr>("containing-lambda");
   const auto *ContainingFunc =
       Result.Nodes.getNodeAs<FunctionDecl>("containing-func");
   const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
   const auto *MovingCall = Result.Nodes.getNodeAs<Expr>("moving-call");
   const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg");
 
   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;
 
+  // Collect all code blocks that could use the arg after move.
+  llvm::SmallVector<Stmt *> CodeBlocks{};
+  if (ContainingCtor) {
+    CodeBlocks.push_back(ContainingCtor->getBody());
+    if (ContainingCtorInitStmt) {
+      // Find the constructor initializer's member field.
+      FieldDecl *ContainingCtorField{nullptr};
+      for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+        if (Init->getInit() == ContainingCtorInitStmt) {
+          ContainingCtorField = Init->getMember();
+          break;
+        }
+      }
+      if (ContainingCtorField) {
+        // Collect the constructor initializer expressions.
+        for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+          if (Init->getMember() && Init->getMember()->getFieldIndex() >=
+                                       ContainingCtorField->getFieldIndex()) {
+            if (Init->getInit())
+              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(FunctionBody, MovingCall, Arg->getDecl(), &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

- Deprecated check-local options `HeaderFileExtensions`
   <clang-tidy/checks/misc/definitions-in-headers>` check.
   Global options of the same name should be used instead.
 
 - Deprecated check-local options `HeaderFileExtensions`
   in :doc:`misc-unused-using-decls
   <clang-tidy/checks/misc/unused-using-decls>` 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.
+
 - In :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` count template
   constructors toward hand written constructors so that they are skipped if more
   than one exists.
 
 - Fixed reading `HungarianNotation.CString.*` options in
   :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>` check.

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

 void typeId() {
   Foo Bar;
   // error: you need to include <typeinfo> before using the 'typeid' operator
   // (void) typeid(Foo{std::move(Bar)}).name();
   Foo Other{std::move(Bar)};
 }
 } // 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;
+};
+
+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;
+};
+
 class PR38187 {
 public:
   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
   }
 
 private:
   std::string val_;
 };