From b32845cb94a81e6fd8b01a8631e3d276c9fc9e35 Mon Sep 17 00:00:00 2001 From: AMS21 Date: Mon, 4 Mar 2024 19:56:15 +0100 Subject: [PATCH] [clang-tidy] Let `bugprone-use-after-move` also handle calls to `std::forward` (#82673) Add support for std::forward. Fixes #82023 --- .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 57 ++++++++++++++----- clang-tools-extra/docs/ReleaseNotes.rst | 4 ++ .../checks/bugprone/use-after-move.rst | 12 ++++ .../checkers/bugprone/use-after-move.cpp | 37 ++++++++++++ 4 files changed, 95 insertions(+), 15 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index c5b6b541096ca9..b91ad0f1822955 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -330,7 +330,8 @@ void UseAfterMoveFinder::getReinits( traverse(TK_AsIs, DeclRefMatcher), unless(parmVarDecl(hasType( references(qualType(isConstQualified())))))), - unless(callee(functionDecl(hasName("::std::move"))))))) + unless(callee(functionDecl( + hasAnyName("::std::move", "::std::forward"))))))) .bind("reinit"); Stmts->clear(); @@ -359,24 +360,46 @@ void UseAfterMoveFinder::getReinits( } } +enum class MoveType { + Move, // std::move + Forward, // std::forward +}; + +static MoveType determineMoveType(const FunctionDecl *FuncDecl) { + if (FuncDecl->getName() == "move") + return MoveType::Move; + if (FuncDecl->getName() == "forward") + return MoveType::Forward; + + llvm_unreachable("Invalid move type"); +} + static void emitDiagnostic(const Expr *MovingCall, const DeclRefExpr *MoveArg, const UseAfterMove &Use, ClangTidyCheck *Check, - ASTContext *Context) { - SourceLocation UseLoc = Use.DeclRef->getExprLoc(); - SourceLocation MoveLoc = MovingCall->getExprLoc(); + ASTContext *Context, MoveType Type) { + 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 moved") - << MoveArg->getDecl()->getName(); - Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note); + 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", + DiagnosticIDs::Note) + << IsMove; if (Use.EvaluationOrderUndefined) { - Check->diag(UseLoc, - "the use and move are unsequenced, i.e. there is no guarantee " - "about the order in which they are evaluated", - DiagnosticIDs::Note); + Check->diag( + UseLoc, + "the use and %select{forward|move}0 are unsequenced, i.e. " + "there is no guarantee about the order in which they are evaluated", + DiagnosticIDs::Note) + << IsMove; } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) { Check->diag(UseLoc, - "the use happens in a later loop iteration than the move", - DiagnosticIDs::Note); + "the use happens in a later loop iteration than the " + "%select{forward|move}0", + DiagnosticIDs::Note) + << IsMove; } } @@ -388,7 +411,9 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) { auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace")))); auto CallMoveMatcher = - callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))), + callExpr(argumentCountIs(1), + callee(functionDecl(hasAnyName("::std::move", "::std::forward")) + .bind("move-decl")), hasArgument(0, declRefExpr().bind("arg")), unless(inDecltypeOrTemplateArg()), unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"), @@ -436,6 +461,7 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { const auto *CallMove = Result.Nodes.getNodeAs("call-move"); const auto *MovingCall = Result.Nodes.getNodeAs("moving-call"); const auto *Arg = Result.Nodes.getNodeAs("arg"); + const auto *MoveDecl = Result.Nodes.getNodeAs("move-decl"); if (!MovingCall || !MovingCall->getExprLoc().isValid()) MovingCall = CallMove; @@ -470,7 +496,8 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) { UseAfterMoveFinder Finder(Result.Context); UseAfterMove Use; if (Finder.find(CodeBlock, MovingCall, Arg->getDecl(), &Use)) - emitDiagnostic(MovingCall, Arg, Use, this, Result.Context); + emitDiagnostic(MovingCall, Arg, Use, this, Result.Context, + determineMoveType(MoveDecl)); } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 70ae23ba185af4..1e1949756e0e2c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -144,6 +144,10 @@ Changes in existing checks ` check by updating the parameter `CheckedFunctions` to support regexp. +- Improved :doc:`bugprone-use-after-move + ` check to also handle + calls to ``std::forward``. + - Improved :doc:`cppcoreguidelines-missing-std-forward ` check by no longer giving false positives for deleted functions. 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 8509292eff9947..08bb5374bab1f4 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 @@ -177,6 +177,18 @@ When analyzing the order in which moves, uses and reinitializations happen (see section `Unsequenced moves, uses, and reinitializations`_), the move is assumed to occur in whichever function the result of the ``std::move`` is passed to. +The check also handles perfect-forwarding with ``std::forward`` so the +following code will also trigger a use-after-move warning. + +.. code-block:: c++ + + void consume(int); + + void f(int&& i) { + consume(std::forward(i)); + consume(std::forward(i)); // use-after-move + } + Use --- 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 00b1da1e727e4f..7d9f63479a1b4e 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 @@ -111,6 +111,18 @@ constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept { return static_cast::type &&>(__t); } +template +constexpr _Tp&& +forward(typename std::remove_reference<_Tp>::type& __t) noexcept { + return static_cast<_Tp&&>(__t); +} + +template +constexpr _Tp&& +forward(typename std::remove_reference<_Tp>::type&& __t) noexcept { + return static_cast<_Tp&&>(__t); +} + } // namespace std class A { @@ -1525,3 +1537,28 @@ class PR38187 { private: std::string val_; }; + +namespace issue82023 +{ + +struct S { + S(); + S(S&&); +}; + +void consume(S s); + +template +void forward(T&& t) { + consume(std::forward(t)); + consume(std::forward(t)); + // CHECK-NOTES: [[@LINE-1]]:27: warning: 't' used after it was forwarded + // CHECK-NOTES: [[@LINE-3]]:11: note: forward occurred here +} + +void create() { + S s; + forward(std::move(s)); +} + +} // namespace issue82023