https://github.com/zeyi2 created https://github.com/llvm/llvm-project/pull/173192
This PR is not ready for review yet. TODOs: - Add documentation - Enhance testcases - Code cleanup >From dd6ebb87189cac5f6f6188843a98dbadced5a469 Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Sun, 21 Dec 2025 18:54:09 +0800 Subject: [PATCH 1/3] [clang-tidy] Improve bugprone-use-after-move to handle lambdas --- .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 201 ++++++++++++------ 1 file changed, 140 insertions(+), 61 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index b2e08fe688a1b..1f42b29aa6d96 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -8,8 +8,10 @@ #include "UseAfterMoveCheck.h" +#include "clang/AST/DeclCXX.h" #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/LambdaCapture.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Analysis/Analyses/CFGReachabilityAnalysis.h" #include "clang/Analysis/CFG.h" @@ -31,6 +33,112 @@ using matchers::hasUnevaluatedContext; namespace { +AST_MATCHER_P(CXXRecordDecl, hasCaptureByReference, const ValueDecl *, + TargetDecl) { + if (!Node.isLambda()) + return false; + + for (const auto &Capture : Node.captures()) { + if (Capture.capturesVariable() && Capture.getCaptureKind() == LCK_ByRef && + Capture.getCapturedVar() == TargetDecl) { + return true; + } + } + return false; +} + +static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { + return anyOf(hasAnyName("::std::move", "::std::forward"), + matchers::matchesAnyListedName(InvalidationFunctions)); +} + +StatementMatcher +makeReinitMatcher(const ValueDecl *MovedVariable, + llvm::ArrayRef<StringRef> InvalidationFunctions) { + const auto DeclRefMatcher = + declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); + + static const auto StandardContainerTypeMatcher = + hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(cxxRecordDecl(hasAnyName( + "::std::basic_string", "::std::vector", "::std::deque", + "::std::forward_list", "::std::list", "::std::set", "::std::map", + "::std::multiset", "::std::multimap", "::std::unordered_set", + "::std::unordered_map", "::std::unordered_multiset", + "::std::unordered_multimap")))))); + + static const auto StandardResettableOwnerTypeMatcher = hasType( + hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl( + hasAnyName("::std::unique_ptr", "::std::shared_ptr", + "::std::weak_ptr", "::std::optional", "::std::any")))))); + + // Matches different types of reinitialization. + return stmt( + anyOf( + // Assignment. In addition to the overloaded assignment + // operator, test for built-in assignment as well, since + // template functions may be instantiated to use std::move() on + // built-in types. + binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)), + // Declaration. We treat this as a type of reinitialization + // too, so we don't need to treat it separately. + declStmt(hasDescendant(equalsNode(MovedVariable))), + // clear() and assign() on standard containers. + cxxMemberCallExpr( + on(expr(DeclRefMatcher, StandardContainerTypeMatcher)), + // To keep the matcher simple, we check for assign() calls + // on all standard containers, even though only vector, + // deque, forward_list and list have assign(). If assign() + // is called on any of the other containers, this will be + // flagged by a compile error anyway. + callee(cxxMethodDecl(hasAnyName("clear", "assign")))), + // reset() on standard smart pointers. + cxxMemberCallExpr(on(expr(DeclRefMatcher, + StandardResettableOwnerTypeMatcher)), + callee(cxxMethodDecl(hasName("reset")))), + // Methods that have the [[clang::reinitializes]] attribute. + cxxMemberCallExpr(on(DeclRefMatcher), + callee(cxxMethodDecl( + hasAttr(clang::attr::Reinitializes)))), + // Passing variable to a function as a non-const pointer. + callExpr(forEachArgumentWithParam( + unaryOperator(hasOperatorName("&"), + hasUnaryOperand(DeclRefMatcher)), + unless( + parmVarDecl(hasType(pointsTo(isConstQualified())))))), + // Passing variable to a function as a non-const lvalue + // reference (unless that function is std::move()). + callExpr(forEachArgumentWithParam( + traverse(TK_AsIs, DeclRefMatcher), + unless(parmVarDecl(hasType( + references(qualType(isConstQualified())))))), + unless(callee(functionDecl( + getNameMatcher(InvalidationFunctions))))))) + .bind("reinit"); +} + + +bool isVariableResetInLambda(const Stmt *Body, const ValueDecl *MovedVariable, + ASTContext *Context, + llvm::ArrayRef<StringRef> InvalidationFunctions) { + if (!Body) + return false; + + // If the variable is not mentioned at all in the lambda body, + // it cannot be reinitialized. + const auto VariableMentionMatcher = stmt(anyOf( + hasDescendant(declRefExpr(hasDeclaration(equalsNode(MovedVariable)))), + hasDescendant(memberExpr(hasDeclaration(equalsNode(MovedVariable)))))); + + if (match(VariableMentionMatcher, *Body, *Context).empty()) + return false; + + const auto ReinitMatcher = + makeReinitMatcher(MovedVariable, InvalidationFunctions); + + return !match(findAll(ReinitMatcher), *Body, *Context).empty(); +} + /// Contains information about a use-after-move. struct UseAfterMove { // The DeclRefExpr that constituted the use of the object. @@ -81,11 +189,6 @@ class UseAfterMoveFinder { } // namespace -static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { - return anyOf(hasAnyName("::std::move", "::std::forward"), - matchers::matchesAnyListedName(InvalidationFunctions)); -} - // Matches nodes that are // - Part of a decltype argument or class template argument (we check this by // seeing if they are children of a TypeLoc), or @@ -313,63 +416,15 @@ void UseAfterMoveFinder::getReinits( const CFGBlock *Block, const ValueDecl *MovedVariable, llvm::SmallPtrSetImpl<const Stmt *> *Stmts, llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs) { - auto DeclRefMatcher = - declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); - - auto StandardContainerTypeMatcher = hasType(hasUnqualifiedDesugaredType( - recordType(hasDeclaration(cxxRecordDecl(hasAnyName( - "::std::basic_string", "::std::vector", "::std::deque", - "::std::forward_list", "::std::list", "::std::set", "::std::map", - "::std::multiset", "::std::multimap", "::std::unordered_set", - "::std::unordered_map", "::std::unordered_multiset", - "::std::unordered_multimap")))))); + const auto ReinitMatcher = + makeReinitMatcher(MovedVariable, InvalidationFunctions); - auto StandardResettableOwnerTypeMatcher = hasType( - hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl( - hasAnyName("::std::unique_ptr", "::std::shared_ptr", - "::std::weak_ptr", "::std::optional", "::std::any")))))); - - // Matches different types of reinitialization. - auto ReinitMatcher = - stmt(anyOf( - // Assignment. In addition to the overloaded assignment operator, - // test for built-in assignment as well, since template functions - // may be instantiated to use std::move() on built-in types. - binaryOperation(hasOperatorName("="), hasLHS(DeclRefMatcher)), - // Declaration. We treat this as a type of reinitialization too, - // so we don't need to treat it separately. - declStmt(hasDescendant(equalsNode(MovedVariable))), - // clear() and assign() on standard containers. - cxxMemberCallExpr( - on(expr(DeclRefMatcher, StandardContainerTypeMatcher)), - // To keep the matcher simple, we check for assign() calls - // on all standard containers, even though only vector, - // deque, forward_list and list have assign(). If assign() - // is called on any of the other containers, this will be - // flagged by a compile error anyway. - callee(cxxMethodDecl(hasAnyName("clear", "assign")))), - // reset() on standard smart pointers. - cxxMemberCallExpr( - on(expr(DeclRefMatcher, StandardResettableOwnerTypeMatcher)), - callee(cxxMethodDecl(hasName("reset")))), - // Methods that have the [[clang::reinitializes]] attribute. - cxxMemberCallExpr( - on(DeclRefMatcher), - callee(cxxMethodDecl(hasAttr(clang::attr::Reinitializes)))), - // Passing variable to a function as a non-const pointer. - callExpr(forEachArgumentWithParam( - unaryOperator(hasOperatorName("&"), - hasUnaryOperand(DeclRefMatcher)), - unless(parmVarDecl(hasType(pointsTo(isConstQualified())))))), - // Passing variable to a function as a non-const lvalue reference - // (unless that function is std::move()). - callExpr(forEachArgumentWithParam( - traverse(TK_AsIs, DeclRefMatcher), - unless(parmVarDecl(hasType( - references(qualType(isConstQualified())))))), - unless(callee(functionDecl( - getNameMatcher(InvalidationFunctions))))))) - .bind("reinit"); + // Match calls to lambdas that capture the moved variable by reference. + const auto LambdaCallMatcher = + cxxOperatorCallExpr( + hasOverloadedOperatorName("()"), + callee(cxxMethodDecl(ofClass(hasCaptureByReference(MovedVariable))))) + .bind("lambda-call"); Stmts->clear(); DeclRefs->clear(); @@ -394,6 +449,30 @@ void UseAfterMoveFinder::getReinits( DeclRefs->insert(TheDeclRef); } } + + // Check for calls to lambdas that capture the moved variable + // by reference and reinitialize it within their body. + const SmallVector<BoundNodes, 1> LambdaMatches = + match(findAll(LambdaCallMatcher), *S->getStmt(), *Context); + + for (const auto &Match : LambdaMatches) { + const auto *Operator = + Match.getNodeAs<CXXOperatorCallExpr>("lambda-call"); + + if (Operator && BlockMap->blockContainingStmt(Operator) == Block) { + const auto *MD = + dyn_cast_or_null<CXXMethodDecl>(Operator->getDirectCallee()); + if (!MD) + continue; + + const auto *RD = MD->getParent(); + const auto *LambdaBody = MD->getBody(); + if (RD && RD->isLambda() && LambdaBody && + isVariableResetInLambda(LambdaBody, MovedVariable, Context, + InvalidationFunctions)) + Stmts->insert(Operator); + } + } } } >From 95a3a9ffc7ee339b617cb09f958302513cc513ae Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Sun, 21 Dec 2025 21:56:14 +0800 Subject: [PATCH 2/3] [clang-tidy] Reformatting --- .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 117 +++++++++--------- 1 file changed, 57 insertions(+), 60 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 87941eeba2840..cd4bf400d3985 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -33,6 +33,54 @@ using matchers::hasUnevaluatedContext; namespace { +/// Contains information about a use-after-move. +struct UseAfterMove { + // The DeclRefExpr that constituted the use of the object. + const DeclRefExpr *DeclRef; + + // Is the order in which the move and the use are evaluated undefined? + 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 +/// various internal helper functions). +class UseAfterMoveFinder { +public: + UseAfterMoveFinder(ASTContext *TheContext, + llvm::ArrayRef<StringRef> InvalidationFunctions); + + // 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. + std::optional<UseAfterMove> find(Stmt *CodeBlock, const Expr *MovingCall, + const DeclRefExpr *MovedVariable); + +private: + std::optional<UseAfterMove> findInternal(const CFGBlock *Block, + const Expr *MovingCall, + const ValueDecl *MovedVariable); + void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable, + llvm::SmallVectorImpl<const DeclRefExpr *> *Uses, + llvm::SmallPtrSetImpl<const Stmt *> *Reinits); + void getDeclRefs(const CFGBlock *Block, const Decl *MovedVariable, + llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs); + void getReinits(const CFGBlock *Block, const ValueDecl *MovedVariable, + llvm::SmallPtrSetImpl<const Stmt *> *Stmts, + llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs); + + ASTContext *Context; + llvm::ArrayRef<StringRef> InvalidationFunctions; + std::unique_ptr<ExprSequence> Sequence; + std::unique_ptr<StmtToBlockMap> BlockMap; + llvm::SmallPtrSet<const CFGBlock *, 8> Visited; +}; + AST_MATCHER_P(CXXRecordDecl, hasCaptureByReference, const ValueDecl *, TargetDecl) { if (!Node.isLambda()) @@ -52,22 +100,21 @@ static auto getNameMatcher(llvm::ArrayRef<StringRef> InvalidationFunctions) { matchers::matchesAnyListedName(InvalidationFunctions)); } -StatementMatcher +static StatementMatcher makeReinitMatcher(const ValueDecl *MovedVariable, llvm::ArrayRef<StringRef> InvalidationFunctions) { const auto DeclRefMatcher = declRefExpr(hasDeclaration(equalsNode(MovedVariable))).bind("declref"); - static const auto StandardContainerTypeMatcher = - hasType(hasUnqualifiedDesugaredType( - recordType(hasDeclaration(cxxRecordDecl(hasAnyName( - "::std::basic_string", "::std::vector", "::std::deque", - "::std::forward_list", "::std::list", "::std::set", "::std::map", - "::std::multiset", "::std::multimap", "::std::unordered_set", - "::std::unordered_map", "::std::unordered_multiset", - "::std::unordered_multimap")))))); + const auto StandardContainerTypeMatcher = hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(cxxRecordDecl(hasAnyName( + "::std::basic_string", "::std::vector", "::std::deque", + "::std::forward_list", "::std::list", "::std::set", "::std::map", + "::std::multiset", "::std::multimap", "::std::unordered_set", + "::std::unordered_map", "::std::unordered_multiset", + "::std::unordered_multimap")))))); - static const auto StandardResettableOwnerTypeMatcher = hasType( + const auto StandardResettableOwnerTypeMatcher = hasType( hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl( hasAnyName("::std::unique_ptr", "::std::shared_ptr", "::std::weak_ptr", "::std::optional", "::std::any")))))); @@ -117,7 +164,6 @@ makeReinitMatcher(const ValueDecl *MovedVariable, .bind("reinit"); } - bool isVariableResetInLambda(const Stmt *Body, const ValueDecl *MovedVariable, ASTContext *Context, llvm::ArrayRef<StringRef> InvalidationFunctions) { @@ -139,57 +185,8 @@ bool isVariableResetInLambda(const Stmt *Body, const ValueDecl *MovedVariable, return !match(findAll(ReinitMatcher), *Body, *Context).empty(); } -/// Contains information about a use-after-move. -struct UseAfterMove { - // The DeclRefExpr that constituted the use of the object. - const DeclRefExpr *DeclRef; - - // Is the order in which the move and the use are evaluated undefined? - 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 -/// various internal helper functions). -class UseAfterMoveFinder { -public: - UseAfterMoveFinder(ASTContext *TheContext, - llvm::ArrayRef<StringRef> InvalidationFunctions); - - // 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. - std::optional<UseAfterMove> find(Stmt *CodeBlock, const Expr *MovingCall, - const DeclRefExpr *MovedVariable); - -private: - std::optional<UseAfterMove> findInternal(const CFGBlock *Block, - const Expr *MovingCall, - const ValueDecl *MovedVariable); - void getUsesAndReinits(const CFGBlock *Block, const ValueDecl *MovedVariable, - llvm::SmallVectorImpl<const DeclRefExpr *> *Uses, - llvm::SmallPtrSetImpl<const Stmt *> *Reinits); - void getDeclRefs(const CFGBlock *Block, const Decl *MovedVariable, - llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs); - void getReinits(const CFGBlock *Block, const ValueDecl *MovedVariable, - llvm::SmallPtrSetImpl<const Stmt *> *Stmts, - llvm::SmallPtrSetImpl<const DeclRefExpr *> *DeclRefs); - - ASTContext *Context; - llvm::ArrayRef<StringRef> InvalidationFunctions; - std::unique_ptr<ExprSequence> Sequence; - std::unique_ptr<StmtToBlockMap> BlockMap; - llvm::SmallPtrSet<const CFGBlock *, 8> Visited; -}; - } // namespace - // Matches nodes that are // - Part of a decltype argument or class template argument (we check this by // seeing if they are children of a TypeLoc), or >From 63d18cddec02015bb399da487a136dee4a335d8e Mon Sep 17 00:00:00 2001 From: mtx <[email protected]> Date: Sun, 21 Dec 2025 22:29:45 +0800 Subject: [PATCH 3/3] Add testcases --- .../checkers/bugprone/use-after-move.cpp | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) 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 b2df2638106e0..b1ed032cbb27b 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 @@ -1703,3 +1703,112 @@ void Run() { } } // namespace custom_invalidation + +void lambdaReinit() { + { + std::string s; + auto g = [&]() { + s = std::string(); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + if (!s.empty()) { + std::move(s); + } + } + } + } + + { + std::string s; + auto g = [&]() { + s.clear(); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + if (!s.empty()) { + std::move(s); + } + } + } + } + + { + std::string s; + auto g = [&]() { + s.assign(10, 'a'); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + if (!s.empty()) { + std::move(s); + } + } + } + } + + { + std::string s; + auto g = [&]() { + return !s.empty(); + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + std::move(s); + // CHECK-NOTES: [[@LINE-1]]:19: warning: 's' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:19: note: the use happens in a later loop + } + } + } + + { + std::string s; + auto g = [s]() mutable { + s = std::string(); + return true; + }; + for (int i = 0; i < 10; ++i) { + if (g()) { + std::move(s); + // CHECK-NOTES: [[@LINE-1]]:19: warning: 's' used after it was moved + // CHECK-NOTES: [[@LINE-2]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:19: note: the use happens in a later loop + } + } + } + + { + std::string s; + while ([&]() { s = std::string(); return true; }()) { + // CHECK-NOTES: [[@LINE-1]]:13: warning: 's' used after it was moved + // CHECK-NOTES: [[@LINE+3]]:9: note: move occurred here + // CHECK-NOTES: [[@LINE-3]]:13: note: the use happens in a later loop + if (!s.empty()) { + std::move(s); + } + } + } +} + +void lambdaReinitInDeadCode() { + // FIXME: This is a known False Negative. The check currently + // lacks the ability to do control flow analysis. + { + std::string s; + auto g = [&]() { + if (false) { + s = std::string(); + } + }; + std::move(s); + + g(); + + s.clear(); + // CHECK-NOTES-NOT: [[@LINE-2]]:5: warning: 's' used after it was moved + } +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
