https://github.com/nakasan617 updated https://github.com/llvm/llvm-project/pull/192895
>From ed24d3cf7bf69dff222eb0e5a50a78d5968186f9 Mon Sep 17 00:00:00 2001 From: Yuta Nakamura <[email protected]> Date: Sun, 17 May 2026 22:01:40 -0700 Subject: [PATCH] [clang-tidy] Fix false positive in bugprone-use-after-move for std::tie std::tie(a, b) = expr reinitializes all variables passed to std::tie because the tuple assignment operator writes back through the stored references. The check was not recognizing this pattern. Add std::tie assignment as a reinitialization case in makeReinitMatcher(). Update documentation and tests accordingly. Fixes #136105. --- .../clang-tidy/bugprone/UseAfterMoveCheck.cpp | 11 + clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../checks/bugprone/use-after-move.rst | 557 +++++++++--------- .../checkers/bugprone/use-after-move.cpp | 90 +++ 4 files changed, 387 insertions(+), 276 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp index 399442f52bd33..ac2214e225afa 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp @@ -134,6 +134,17 @@ makeReinitMatcher(const ValueDecl *MovedVariable, // built-in types. binaryOperation(hasOperatorName("="), hasLHS(ignoringParenImpCasts(DeclRefMatcher))), + // std::tie() assignment: std::tie(a, b) = expr reinitializes + // all variables passed to std::tie because the tuple + // assignment writes back through the stored references. + // ignoringImplicit strips the MaterializeTemporaryExpr that + // Clang inserts when calling operator= on the prvalue tuple. + binaryOperation( + hasOperatorName("="), + hasLHS(ignoringImplicit( + callExpr(callee(functionDecl(hasName("::std::tie"))), + hasAnyArgument( + ignoringParenImpCasts(DeclRefMatcher)))))), // Declaration. We treat this as a type of reinitialization // too, so we don't need to treat it separately. declStmt(hasDescendant(equalsNode(MovedVariable))), diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 89fb1684bba7c..84215a5f4381e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -433,6 +433,11 @@ Changes in existing checks - Avoid false positives when moving object is reinitialized via the base class's ``operator=``. + - Fix a false positive when a moved-from variable is reinitialized + via a ``std::tie()`` assignment (e.g. ``std::tie(a, b) = f(std::move(a), + std::move(b))``). The tuple assignment writes back through the stored + references, which fully reinitializes the captured variables. + - Improved :doc:`cppcoreguidelines-avoid-capturing-lambda-coroutines <clang-tidy/checks/cppcoreguidelines/avoid-capturing-lambda-coroutines>` check by adding the `AllowExplicitObjectParameters` option. When enabled, 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 da72f742b38d0..798b3e458594b 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 @@ -1,276 +1,281 @@ -.. title:: clang-tidy - bugprone-use-after-move - -bugprone-use-after-move -======================= - -Warns if an object is used after it has been moved, for example: - -.. code-block:: c++ - - std::string str = "Hello, world!\n"; - std::vector<std::string> messages; - messages.emplace_back(std::move(str)); - std::cout << str; - -The last line will trigger a warning that ``str`` is used after it has been -moved. - -The check does not trigger a warning if the object is reinitialized after the -move and before the use. For example, no warning will be output for this code: - -.. code-block:: c++ - - messages.emplace_back(std::move(str)); - str = "Greetings, stranger!\n"; - std::cout << str; - -Subsections below explain more precisely what exactly the check considers to be -a move, use, and reinitialization. - -The check takes control flow into account. A warning is only emitted if the use -can be reached from the move. This means that the following code does not -produce a warning: - -.. code-block:: c++ - - if (condition) { - messages.emplace_back(std::move(str)); - } else { - std::cout << str; - } - -On the other hand, the following code does produce a warning: - -.. code-block:: c++ - - for (int i = 0; i < 10; ++i) { - std::cout << str; - messages.emplace_back(std::move(str)); - } - -(The use-after-move happens on the second iteration of the loop.) - -In some cases, the check may not be able to detect that two branches are -mutually exclusive. For example (assuming that ``i`` is an int): - -.. code-block:: c++ - - if (i == 1) { - messages.emplace_back(std::move(str)); - } - if (i == 2) { - std::cout << str; - } - -In this case, the check will erroneously produce a warning, even though it is -not possible for both the move and the use to be executed. More formally, the -analysis is `flow-sensitive but not path-sensitive -<https://en.wikipedia.org/wiki/Data-flow_analysis#Sensitivities>`_. - -Silencing erroneous warnings ----------------------------- - -An erroneous warning can be silenced by reinitializing the object after the -move: - -.. code-block:: c++ - - if (i == 1) { - messages.emplace_back(std::move(str)); - str = ""; - } - if (i == 2) { - std::cout << str; - } - -If you want to avoid the overhead of actually reinitializing the object, -you can create a dummy function that causes the check to assume the object -was reinitialized: - -.. code-block:: c++ - - template <class T> - void IS_INITIALIZED(T&) {} - -You can use this as follows: - -.. code-block:: c++ - - if (i == 1) { - messages.emplace_back(std::move(str)); - } - if (i == 2) { - IS_INITIALIZED(str); - std::cout << str; - } - -The check will not output a warning in this case because passing the object -to a function as a non-const pointer or reference counts as a reinitialization -(see section `Reinitialization`_ below). - -Unsequenced moves, uses, and reinitializations ----------------------------------------------- - -In many cases, C++ does not make any guarantees about the order in which -sub-expressions of a statement are evaluated. This means that in code like the -following, it is not guaranteed whether the use will happen before or after the -move: - -.. code-block:: c++ - - void f(int i, std::vector<int> v); - std::vector<int> v = { 1, 2, 3 }; - f(v[1], std::move(v)); - -In this kind of situation, the check will note that the use and move are -unsequenced. - -The check will also take sequencing rules into account when reinitializations -occur in the same statement as moves or uses. A reinitialization is only -considered to reinitialize a variable if it is guaranteed to be evaluated after -the move and before the use. - -Move ----- - -The check currently only considers calls of ``std::move`` on local variables or -function parameters. It does not check moves of member variables or global -variables. - -Any call of ``std::move`` on a variable is considered to cause a move of that -variable, even if the result of ``std::move`` is not passed to an rvalue -reference parameter. - -This means that the check will flag a use-after-move even on a type that does -not define a move constructor or move assignment operator. This is intentional. -Developers may use ``std::move`` on such a type in the expectation that the -type will add move semantics in the future. If such a ``std::move`` has the -potential to cause a use-after-move, we want to warn about it even if the type -does not implement move semantics yet. - -Furthermore, if the result of ``std::move`` *is* passed to an rvalue reference -parameter, this will always be considered to cause a move, even if the function -that consumes this parameter does not move from it, or if it does so only -conditionally. For example, in the following situation, the check will assume -that a move always takes place: - -.. code-block:: c++ - - std::vector<std::string> messages; - void f(std::string &&str) { - // Only remember the message if it isn't empty. - if (!str.empty()) { - messages.emplace_back(std::move(str)); - } - } - std::string str = ""; - f(std::move(str)); - -The check will assume that the last line causes a move, even though, in this -particular case, it does not. Again, this is intentional. - -There is one special case: A call to ``std::move`` inside a ``try_emplace`` -call is conservatively assumed not to move. This is to avoid spurious warnings, -as the check has no way to reason about the ``bool`` returned by ``try_emplace``. - -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<int>(i)); - consume(std::forward<int>(i)); // use-after-move - } - -Use ---- - -Any occurrence of the moved variable that is not a reinitialization (see below) -or an explicit call to the variable destructor is considered to be a use. - -An exception to this are objects of type ``std::unique_ptr``, -``std::shared_ptr``, ``std::weak_ptr``, ``std::optional``, and ``std::any``, -which can be reinitialized via ``reset``. For smart pointers specifically, the -moved-from objects have a well-defined state of being ``nullptr``s, and only -``operator*``, ``operator->`` and ``operator[]`` are considered bad accesses as -they would be dereferencing a ``nullptr``. - -User-defined types can be annotated as having the same semantics as standard -smart pointers with ``[[clang::annotate("clang-tidy", -"bugprone-use-after-move", "null_after_move")]]``. This expresses that a -moved-from object of this type is a null pointer. - -If multiple uses occur after a move, only the first of these is flagged. - -Reinitialization ----------------- - -The check considers a variable to be reinitialized in the following cases: - - - The variable occurs on the left-hand side of an assignment. - - - The variable is passed to a function as a non-const pointer or non-const - lvalue reference. (It is assumed that the variable may be an out-parameter - for the function.) - - - ``clear()`` or ``assign()`` is called on the variable and the variable is - of one of the standard container types ``basic_string``, ``vector``, - ``deque``, ``forward_list``, ``list``, ``set``, ``map``, ``multiset``, - ``multimap``, ``unordered_set``, ``unordered_map``, ``unordered_multiset``, - ``unordered_multimap``. - - - ``reset()`` is called on the variable and the variable is of type - ``std::unique_ptr``, ``std::shared_ptr``, ``std::weak_ptr``, - ``std::optional``, or ``std::any``. - - - A member function marked with the ``[[clang::reinitializes]]`` attribute is - called on the variable. - -If the variable in question is a struct and an individual member variable of -that struct is written to, the check does not consider this to be a -reinitialization -- even if, eventually, all member variables of the struct are -written to. For example: - -.. code-block:: c++ - - struct S { - std::string str; - int i; - }; - S s = { "Hello, world!\n", 42 }; - S s_other = std::move(s); - s.str = "Lorem ipsum"; - s.i = 99; - -The check will not consider ``s`` to be reinitialized after the last line; -instead, the line that assigns to ``s.str`` will be flagged as a use-after-move. -This is intentional as this pattern of reinitializing a struct is error-prone. -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 regular expressions matching names of functions - that cause their first arguments to be invalidated (e.g., closing a handle). - For member functions, the first argument is considered to be the implicit - object argument (``this``). Default value is an empty string. - -.. option:: ReinitializationFunctions - - A semicolon-separated list of regular expressions matching names of functions - that reinitialize the object. For member functions, the implicit object - argument (``*this``) is considered to be reinitialized. For non-member or - static member functions, the first argument is considered to be - reinitialized. Default value is an empty string. +.. title:: clang-tidy - bugprone-use-after-move + +bugprone-use-after-move +======================= + +Warns if an object is used after it has been moved, for example: + +.. code-block:: c++ + + std::string str = "Hello, world!\n"; + std::vector<std::string> messages; + messages.emplace_back(std::move(str)); + std::cout << str; + +The last line will trigger a warning that ``str`` is used after it has been +moved. + +The check does not trigger a warning if the object is reinitialized after the +move and before the use. For example, no warning will be output for this code: + +.. code-block:: c++ + + messages.emplace_back(std::move(str)); + str = "Greetings, stranger!\n"; + std::cout << str; + +Subsections below explain more precisely what exactly the check considers to be +a move, use, and reinitialization. + +The check takes control flow into account. A warning is only emitted if the use +can be reached from the move. This means that the following code does not +produce a warning: + +.. code-block:: c++ + + if (condition) { + messages.emplace_back(std::move(str)); + } else { + std::cout << str; + } + +On the other hand, the following code does produce a warning: + +.. code-block:: c++ + + for (int i = 0; i < 10; ++i) { + std::cout << str; + messages.emplace_back(std::move(str)); + } + +(The use-after-move happens on the second iteration of the loop.) + +In some cases, the check may not be able to detect that two branches are +mutually exclusive. For example (assuming that ``i`` is an int): + +.. code-block:: c++ + + if (i == 1) { + messages.emplace_back(std::move(str)); + } + if (i == 2) { + std::cout << str; + } + +In this case, the check will erroneously produce a warning, even though it is +not possible for both the move and the use to be executed. More formally, the +analysis is `flow-sensitive but not path-sensitive +<https://en.wikipedia.org/wiki/Data-flow_analysis#Sensitivities>`_. + +Silencing erroneous warnings +---------------------------- + +An erroneous warning can be silenced by reinitializing the object after the +move: + +.. code-block:: c++ + + if (i == 1) { + messages.emplace_back(std::move(str)); + str = ""; + } + if (i == 2) { + std::cout << str; + } + +If you want to avoid the overhead of actually reinitializing the object, +you can create a dummy function that causes the check to assume the object +was reinitialized: + +.. code-block:: c++ + + template <class T> + void IS_INITIALIZED(T&) {} + +You can use this as follows: + +.. code-block:: c++ + + if (i == 1) { + messages.emplace_back(std::move(str)); + } + if (i == 2) { + IS_INITIALIZED(str); + std::cout << str; + } + +The check will not output a warning in this case because passing the object +to a function as a non-const pointer or reference counts as a reinitialization +(see section `Reinitialization`_ below). + +Unsequenced moves, uses, and reinitializations +---------------------------------------------- + +In many cases, C++ does not make any guarantees about the order in which +sub-expressions of a statement are evaluated. This means that in code like the +following, it is not guaranteed whether the use will happen before or after the +move: + +.. code-block:: c++ + + void f(int i, std::vector<int> v); + std::vector<int> v = { 1, 2, 3 }; + f(v[1], std::move(v)); + +In this kind of situation, the check will note that the use and move are +unsequenced. + +The check will also take sequencing rules into account when reinitializations +occur in the same statement as moves or uses. A reinitialization is only +considered to reinitialize a variable if it is guaranteed to be evaluated after +the move and before the use. + +Move +---- + +The check currently only considers calls of ``std::move`` on local variables or +function parameters. It does not check moves of member variables or global +variables. + +Any call of ``std::move`` on a variable is considered to cause a move of that +variable, even if the result of ``std::move`` is not passed to an rvalue +reference parameter. + +This means that the check will flag a use-after-move even on a type that does +not define a move constructor or move assignment operator. This is intentional. +Developers may use ``std::move`` on such a type in the expectation that the +type will add move semantics in the future. If such a ``std::move`` has the +potential to cause a use-after-move, we want to warn about it even if the type +does not implement move semantics yet. + +Furthermore, if the result of ``std::move`` *is* passed to an rvalue reference +parameter, this will always be considered to cause a move, even if the function +that consumes this parameter does not move from it, or if it does so only +conditionally. For example, in the following situation, the check will assume +that a move always takes place: + +.. code-block:: c++ + + std::vector<std::string> messages; + void f(std::string &&str) { + // Only remember the message if it isn't empty. + if (!str.empty()) { + messages.emplace_back(std::move(str)); + } + } + std::string str = ""; + f(std::move(str)); + +The check will assume that the last line causes a move, even though, in this +particular case, it does not. Again, this is intentional. + +There is one special case: A call to ``std::move`` inside a ``try_emplace`` +call is conservatively assumed not to move. This is to avoid spurious warnings, +as the check has no way to reason about the ``bool`` returned by ``try_emplace``. + +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<int>(i)); + consume(std::forward<int>(i)); // use-after-move + } + +Use +--- + +Any occurrence of the moved variable that is not a reinitialization (see below) +or an explicit call to the variable destructor is considered to be a use. + +An exception to this are objects of type ``std::unique_ptr``, +``std::shared_ptr``, ``std::weak_ptr``, ``std::optional``, and ``std::any``, +which can be reinitialized via ``reset``. For smart pointers specifically, the +moved-from objects have a well-defined state of being ``nullptr``s, and only +``operator*``, ``operator->`` and ``operator[]`` are considered bad accesses as +they would be dereferencing a ``nullptr``. + +User-defined types can be annotated as having the same semantics as standard +smart pointers with ``[[clang::annotate("clang-tidy", +"bugprone-use-after-move", "null_after_move")]]``. This expresses that a +moved-from object of this type is a null pointer. + +If multiple uses occur after a move, only the first of these is flagged. + +Reinitialization +---------------- + +The check considers a variable to be reinitialized in the following cases: + + - The variable occurs on the left-hand side of an assignment. + + - The variable is passed to a function as a non-const pointer or non-const + lvalue reference. (It is assumed that the variable may be an out-parameter + for the function.) + + - ``clear()`` or ``assign()`` is called on the variable and the variable is + of one of the standard container types ``basic_string``, ``vector``, + ``deque``, ``forward_list``, ``list``, ``set``, ``map``, ``multiset``, + ``multimap``, ``unordered_set``, ``unordered_map``, ``unordered_multiset``, + ``unordered_multimap``. + + - ``reset()`` is called on the variable and the variable is of type + ``std::unique_ptr``, ``std::shared_ptr``, ``std::weak_ptr``, + ``std::optional``, or ``std::any``. + + - A member function marked with the ``[[clang::reinitializes]]`` attribute is + called on the variable. + + - The variable is passed as an argument to ``std::tie`` on the left-hand + side of an assignment (e.g. ``std::tie(a, b) = f(...)``). The tuple + assignment operator writes back through the stored references, which + reinitializes each named variable. + +If the variable in question is a struct and an individual member variable of +that struct is written to, the check does not consider this to be a +reinitialization -- even if, eventually, all member variables of the struct are +written to. For example: + +.. code-block:: c++ + + struct S { + std::string str; + int i; + }; + S s = { "Hello, world!\n", 42 }; + S s_other = std::move(s); + s.str = "Lorem ipsum"; + s.i = 99; + +The check will not consider ``s`` to be reinitialized after the last line; +instead, the line that assigns to ``s.str`` will be flagged as a use-after-move. +This is intentional as this pattern of reinitializing a struct is error-prone. +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 regular expressions matching names of functions + that cause their first arguments to be invalidated (e.g., closing a handle). + For member functions, the first argument is considered to be the implicit + object argument (``this``). Default value is an empty string. + +.. option:: ReinitializationFunctions + + A semicolon-separated list of regular expressions matching names of functions + that reinitialize the object. For member functions, the implicit object + argument (``*this``) is considered to be reinitialized. For non-member or + static member functions, the first argument is considered to be + reinitialized. 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 d4e78d359b654..0395650a82d1b 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 @@ -17,6 +17,7 @@ #include <map> #include <set> #include <string> +#include <tuple> #include <unordered_map> #include <unordered_set> #include <utility> @@ -1814,3 +1815,92 @@ namespace GH62206 { (d) = b; // Should not warn } } // namespace GH62206 + + +std::pair<std::string, std::string> makeStringPair(std::string a, + std::string b); + +void stdTieIsReinit() { + std::string a, b; + std::move(a); + std::move(b); + std::tie(a, b) = makeStringPair("x", "y"); + a.size(); + b.size(); +} + +void stdTieWithIgnore() { + std::string a; + std::move(a); + std::tie(a, std::ignore) = makeStringPair("x", "y"); + a.size(); +} + +void stdTieIgnoreFlipped() { + std::string a; + std::move(a); + std::tie(std::ignore, a) = makeStringPair("x", "y"); + a.size(); +} + +void stdTieThreeVars() { + std::string a, b, c; + std::move(a); + std::move(b); + std::move(c); + std::tie(a, b, c) = + std::make_tuple(std::string("x"), std::string("y"), std::string("z")); + a.size(); + b.size(); + c.size(); +} + +void stdTiePartialReinit() { + std::string a, b, c; + std::move(a); + std::move(b); + std::move(c); + std::tie(b) = std::make_tuple(std::string("y")); + b.size(); + std::tie(c, b, std::ignore) = + std::make_tuple(std::string("x"), std::string("y"), std::string("z")); + b.size(); + c.size(); + a.size(); + // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-NOTES: [[@LINE-11]]:3: note: move occurred here +} + +void stdTieInLoop() { + std::string a, b; + while (true) { + std::tie(a, b) = makeStringPair(std::move(a), std::move(b)); + std::tie(a, b) = makeStringPair(std::move(a), std::move(b)); + } +} + +template <typename T> +void stdTieReinitInTemplate() { + T a, b; + std::move(a); + std::move(b); + std::tie(a, b) = std::make_tuple(T(), T()); + a.size(); + b.size(); +} + +template <typename T> +void stdTiePartialReinitInTemplate() { + T a, b; + std::move(a); + std::tie(b) = std::make_tuple(T()); + b.size(); + a.size(); + // CHECK-NOTES: [[@LINE-1]]:3: warning: 'a' used after it was moved + // CHECK-NOTES: [[@LINE-5]]:3: note: move occurred here +} + +void callStdTieTemplateTests() { + stdTieReinitInTemplate<std::string>(); + stdTiePartialReinitInTemplate<std::string>(); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
