https://github.com/higher-performance updated https://github.com/llvm/llvm-project/pull/170346
>From 1ecfecc1a4a5af2171ecacca843e30443ad1cabb Mon Sep 17 00:00:00 2001 From: higher-performance <[email protected]> Date: Tue, 2 Dec 2025 13:20:24 -0500 Subject: [PATCH] Extend bugprone-use-after-move check to allow custom invalidation functions --- .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 66 ++++++++++++------- .../clang-tidy/bugprone/UseAfterMoveCheck.h | 7 +- clang-tools-extra/docs/ReleaseNotes.rst | 4 ++ .../checks/bugprone/use-after-move.rst | 10 +++ .../checkers/bugprone/use-after-move.cpp | 56 +++++++++++++++- 5 files changed, 117 insertions(+), 26 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 6d134a0e896a0..cc1fa135c7f39 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -19,6 +19,7 @@ #include "../utils/ExprSequence.h" #include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" #include <optional> using namespace clang::ast_matchers; @@ -48,7 +49,8 @@ struct UseAfterMove { /// various internal helper functions). class UseAfterMoveFinder { public: - UseAfterMoveFinder(ASTContext *TheContext); + 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 @@ -71,6 +73,7 @@ class UseAfterMoveFinder { 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; @@ -78,6 +81,11 @@ 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 @@ -92,8 +100,9 @@ static StatementMatcher inDecltypeOrTemplateArg() { hasAncestor(expr(hasUnevaluatedContext()))); } -UseAfterMoveFinder::UseAfterMoveFinder(ASTContext *TheContext) - : Context(TheContext) {} +UseAfterMoveFinder::UseAfterMoveFinder( + ASTContext *TheContext, llvm::ArrayRef<StringRef> InvalidationFunctions) + : Context(TheContext), InvalidationFunctions(InvalidationFunctions) {} std::optional<UseAfterMove> UseAfterMoveFinder::find(Stmt *CodeBlock, const Expr *MovingCall, @@ -359,7 +368,7 @@ void UseAfterMoveFinder::getReinits( unless(parmVarDecl(hasType( references(qualType(isConstQualified())))))), unless(callee(functionDecl( - hasAnyName("::std::move", "::std::forward"))))))) + getNameMatcher(InvalidationFunctions))))))) .bind("reinit"); Stmts->clear(); @@ -388,9 +397,10 @@ void UseAfterMoveFinder::getReinits( } } -enum class MoveType { - Move, // std::move - Forward, // std::forward +enum MoveType { + Forward = 0, // std::forward + Move = 1, // std::move + Invalidation = 2, // other }; static MoveType determineMoveType(const FunctionDecl *FuncDecl) { @@ -399,7 +409,7 @@ static MoveType determineMoveType(const FunctionDecl *FuncDecl) { if (FuncDecl->getName() == "forward") return MoveType::Forward; - llvm_unreachable("Invalid move type"); + return MoveType::Invalidation; } static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, @@ -408,29 +418,38 @@ static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, const SourceLocation UseLoc = Use.DeclRef->getExprLoc(); const SourceLocation MoveLoc = MovingCall->getExprLoc(); - const bool IsMove = (Type == MoveType::Move); - - Check->diag(UseLoc, "'%0' used after it was %select{forwarded|moved}1") - << MoveArg->getDecl()->getName() << IsMove; - Check->diag(MoveLoc, "%select{forward|move}0 occurred here", + Check->diag(UseLoc, + "'%0' used after it was %select{forwarded|moved|invalidated}1") + << MoveArg->getDecl()->getName() << Type; + Check->diag(MoveLoc, "%select{forward|move|invalidation}0 occurred here", DiagnosticIDs::Note) - << IsMove; + << Type; if (Use.EvaluationOrderUndefined) { Check->diag( UseLoc, - "the use and %select{forward|move}0 are unsequenced, i.e. " + "the use and %select{forward|move|invalidation}0 are unsequenced, i.e. " "there is no guarantee about the order in which they are evaluated", DiagnosticIDs::Note) - << IsMove; + << Type; } else if (Use.UseHappensInLaterLoopIteration) { Check->diag(UseLoc, "the use happens in a later loop iteration than the " - "%select{forward|move}0", + "%select{forward|move|invalidation}0", DiagnosticIDs::Note) - << IsMove; + << Type; } } +UseAfterMoveCheck::UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + InvalidationFunctions(utils::options::parseStringList( + Options.get("InvalidationFunctions", ""))) {} + +void UseAfterMoveCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "InvalidationFunctions", + utils::options::serializeStringList(InvalidationFunctions)); +} + 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 @@ -438,11 +457,14 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { // the bool. auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace")))); + auto Arg = declRefExpr().bind("arg"); + auto IsMemberCallee = callee(functionDecl(unless(isStaticStorageClass()))); auto CallMoveMatcher = - callExpr(argumentCountIs(1), - callee(functionDecl(hasAnyName("::std::move", "::std::forward")) + callExpr(callee(functionDecl(getNameMatcher(InvalidationFunctions)) .bind("move-decl")), - hasArgument(0, declRefExpr().bind("arg")), + anyOf(cxxMemberCallExpr(IsMemberCallee, on(Arg)), + callExpr(unless(cxxMemberCallExpr(IsMemberCallee)), + hasArgument(0, Arg))), unless(inDecltypeOrTemplateArg()), unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"), anyOf(hasAncestor(compoundStmt( @@ -521,7 +543,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { } for (Stmt *CodeBlock : CodeBlocks) { - UseAfterMoveFinder Finder(Result.Context); + UseAfterMoveFinder Finder(Result.Context, InvalidationFunctions); if (auto Use = Finder.find(CodeBlock, MovingCall, Arg)) emitDiagnostic(MovingCall, Arg, *Use, this, Result.Context, determineMoveType(MoveDecl)); diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h index d38b29e09fa8b..1bbf5c00785ff 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.h @@ -20,13 +20,16 @@ namespace clang::tidy::bugprone { /// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html class UseAfterMoveCheck : public ClangTidyCheck { public: - UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + UseAfterMoveCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { return LangOpts.CPlusPlus11; } void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + std::vector<StringRef> InvalidationFunctions; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 79a768e599cfd..32699371446ec 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -400,6 +400,10 @@ Changes in existing checks suffix when the reason starts with the character `>` in the `CustomFunctions` option. +- Improved :doc:`bugprone-use-after-move + <clang-tidy/checks/bugprone/use-after-move>` check by adding + `InvalidationFunctions` option to support custom invalidation functions. + - Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables <clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables>` check by adding a new option `AllowThreadLocal` that suppresses warnings on diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst index 07edd07b1c4c9..f076be19a75a7 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst @@ -253,3 +253,13 @@ For example, if an additional member variable is added to ``S``, it is easy to forget to add the reinitialization for this additional member. Instead, it is safer to assign to the entire struct in one go, and this will also avoid the use-after-move warning. + +Options +------- + +.. option:: InvalidationFunctions + + A semicolon-separated list of names of functions that cause their initial + arguments to be invalidated (e.g., closing a handle). + For member functions, the initial argument is considered to be the implicit + object argument (`this`). Default value is an empty string. 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 87dfec4f68061..9ae02f7e9a158 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 @@ -1,5 +1,13 @@ -// 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 +// RUN: %check_clang_tidy -std=c++11 -check-suffixes=,CXX11 %s bugprone-use-after-move %t -- \ +// RUN: -config='{CheckOptions: { \ +// RUN: bugprone-use-after-move.InvalidationFunctions: "::Database<>::StaticCloseConnection;Database<>::CloseConnection;FriendCloseConnection" \ +// RUN: }}' -- \ +// RUN: -fno-delayed-template-parsing +// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-use-after-move %t -- \ +// RUN: -config='{CheckOptions: { \ +// RUN: bugprone-use-after-move.InvalidationFunctions: "::Database<>::StaticCloseConnection;Database<>::CloseConnection;FriendCloseConnection" \ +// RUN: }}' -- \ +// RUN: -fno-delayed-template-parsing typedef decltype(nullptr) nullptr_t; @@ -1645,3 +1653,47 @@ void create() { } } // namespace issue82023 + +namespace custom_invalidation +{ + +template<class T = int> +struct Database { + template<class...> + void CloseConnection(T = T()) {} + template<class...> + static void StaticCloseConnection(Database&, T = T()) {} + template<class...> + friend void FriendCloseConnection(Database&, T = T()) {} + void Query(); +}; + +void Run() { + using DB = Database<>; + + DB db1; + db1.CloseConnection(); + db1.Query(); + // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db1' used after it was invalidated + // CHECK-NOTES: [[@LINE-3]]:7: note: invalidation occurred here + + DB db2; + DB::StaticCloseConnection(db2); + db2.Query(); + // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db2' used after it was invalidated + // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here + + DB db3; + DB().StaticCloseConnection(db3); + db3.Query(); + // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db3' used after it was invalidated + // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here + + DB db4; + FriendCloseConnection(db4); + db4.Query(); + // CHECK-NOTES: [[@LINE-1]]:3: warning: 'db4' used after it was invalidated + // CHECK-NOTES: [[@LINE-3]]:3: note: invalidation occurred here +} + +} // namespace custom_invalidation _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
