[clang-tidy] performance-noexcept-move-constructor do not handle conditional noexcept properly

[Nechda]

Hi!

It seems that this issue has been discussed in #68101 and partially resolved. The problem is now observed in clang version 19.1.0 and possibly in the trunk as well.

To summarize, I have an external class that does not have a noexcept move constructor. When I try to mark the move constructor for my class using type traits, the check fails.

Previously, a workaround solved the case where the class in question had a template parameter. This allowed the check to allow the use of type traits with noexcept. Maybe the check should only be triggered when the user explicitly specifies noexcept(false)?

#include <type_traits>

/* External class */
struct NotMarkedNoexcept {
    int x;
    NotMarkedNoexcept() = default;
    // NOLINTNEXTLINE(performance-noexcept-move-constructor)
    NotMarkedNoexcept(NotMarkedNoexcept&& rhs);
};

struct Complicated {
    static constexpr bool NOEXCEPT_MOVE = std::is_nothrow_move_constructible_v<NotMarkedNoexcept>;

    Complicated() = default;
    Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Warning

    NotMarkedNoexcept inner;
};

int main() {}

[<source>:14:41: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]](javascript:;)
   14 |     Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Warning
      |                                         ^
1 warning generated.
Suppressed 1 warnings (1 NOLINT).

https://godbolt.org/z/E4E65jdhh

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Dmitry Nechitaev (Nechda)

Hi!

It seems that this issue has been discussed in #68101 and partially resolved. The problem is now observed in clang version 19.1.0 and possibly in the trunk as well.

To summarize, I have an external class that does not have a noexcept move constructor. When I try to mark the move constructor for my class using type traits, the check fails.

Previously, a workaround solved the case where the class in question had a template parameter. This allowed the check to allow the use of type traits with noexcept. Maybe the check should only be triggered when the user explicitly specifies noexcept(false)?

#include <type_traits>

/* External class */
struct NotMarkedNoexcept {
    int x;
    NotMarkedNoexcept() = default;
    // NOLINTNEXTLINE(performance-noexcept-move-constructor)
    NotMarkedNoexcept(NotMarkedNoexcept&& rhs);
};

struct Complicated {
    static constexpr bool NOEXCEPT_MOVE = std::is_nothrow_move_constructible_v<NotMarkedNoexcept>;

    Complicated() = default;
    Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Warning

    NotMarkedNoexcept inner;
};

int main() {}

[<source>:14:41: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor]](javascript:;)
   14 |     Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Warning
      |                                         ^
1 warning generated.
Suppressed 1 warnings (1 NOLINT).

https://godbolt.org/z/E4E65jdhh

[Nechda]

cc @AMS21 @PiotrZSL

[AMS21]

I haven't looked at it, but my best guess that the noexcept checks don't resolve any templates which would be needed to resolve the noexcept expression.

Although if memory serves if we can't resolve an expression we return Unknown and should just ignore it...

[Nechda]

if we can't resolve an expression we return Unknown and should just ignore it

Yes, according to your previous commit, this logic is clear.
https://github.com/llvm/llvm-project/pull/68359/files#diff-fb42ca512b87267a21b8a5676608906e18453409a69965aa23840415d27bb9c3R154-R160

But in my example, we can resolve the expression and check is failed. Moreover, if we invert the expression in "noexcept", then the check will pass.

--- example.cpp
+++ example.cpp
@@ -12,7 +12,7 @@
     static constexpr bool NOEXCEPT_MOVE = std::is_nothrow_move_constructible_v<NotMarkedNoexcept>;

     Complicated() = default;
-    Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Warning
+    Complicated(Complicated&&) noexcept(!NOEXCEPT_MOVE) = default; // Silent

     NotMarkedNoexcept inner;
 };

https://godbolt.org/z/as5q3GMvb

[AMS21]

Wait this seems correct to me. The move constructor of Complicated is marked as throwing and thus we get a warning from clang-tidy.

Adding some static asserts clearly shows this:

#include <type_traits>

struct NotMarkedNoexcept {
    int x;
    NotMarkedNoexcept() = default;
    // NOLINTNEXTLINE(performance-noexcept-move-constructor)
    NotMarkedNoexcept(NotMarkedNoexcept&& rhs);
};

static_assert(!std::is_nothrow_move_constructible_v<NotMarkedNoexcept>);

struct Complicated {
    static constexpr bool NOEXCEPT_MOVE = std::is_nothrow_move_constructible_v<NotMarkedNoexcept>;
    static_assert(!NOEXCEPT_MOVE); // i.e. NOEXCEPT_MOVE == false

    Complicated() = default;
    Complicated(Complicated&&) noexcept(NOEXCEPT_MOVE) = default; // Evaluates at compile time to 'noexcept(false)'

    NotMarkedNoexcept inner;
};

static_assert(!std::is_nothrow_move_constructible_v<Complicated>);

int main() {}

https://godbolt.org/z/4KdWj74Gd

The warning even explicitly states that the expression inside the noexcept specifier evaluated to false thus making the class Complicated not noexcept move constructible.

This also explains why inverting the expression "fixes" the problem since then the class does indeed become noexcept move constructible.

So I would argue that the check does work as expected or am I missing something here?

[Nechda]

So I would argue that the check does work as expected or am I missing something here?

I understand why the warning is triggered in my example, but is this warning really necessary for noexcept(expr), which can be evaluated at compile time to noexcept(false)? At the same time, we ignore such checks for classes that have a template argument.

It looks like, as soon as clang-tidy is able to evaluate noexcept(expr), even for templated classes, we may trigger check inside the STL classes. For example, if someone writes a templated class with a move-constructor, marked as noexcept(expr), and then uses it in std::vector.

My proposal is to add an option to allow noexcept(expr), even if this expression can be folded to false. This would force users to mark their move constructors as noexcept, while at the same time, the check would not trigger if the move constructor is not actually noexcept. What do you think about it?

[AMS21]

I understand why the warning is triggered in my example, but is this warning really necessary for noexcept(expr), which can be evaluated at compile time to noexcept(false)?

In my opinion yes. For the exact reason that fooled me the first time I read your example. This looks like it's enabling noexcept conditionally when in fact it's always false. What you have written here is essentially just a more complicated way of writing noexcept(false).

At the same time, we ignore such checks for classes that have a template argument.

This is not exactly true, we only ignore those classes for which the noexcept specification is value-dependent. So something like noexcept(std::is_nothrow_move_constructible_v<T>) does not trigger this warning.

It looks like, as soon as clang-tidy is able to evaluate noexcept(expr), even for templated classes, we may trigger check inside the STL classes. For example, if someone writes a templated class with a move-constructor, marked as noexcept(expr), and then uses it in std::vector.

And that's exactly a reason why I don't think it would make a lot of sense for this check to do that, since you would just get a lot of false-positives and for a container to have it's move constructors noexceptness depend on the underlying type is the correct approach in my opinion.

This would also mean that using my_vector<int> woudn't trigger the warning but as soon as you use my_vector<my_class> you could get it, which again in my opinion would be weird and unhelpful, especially as the "fault" here would lie with implementation of my_class and not the container.

My proposal is to add an option to allow noexcept(expr), even if this expression can be folded to false. This would force users to mark their move constructors as noexcept, while at the same time, the check would not trigger if the move constructor is not actually noexcept. What do you think about it?

To be honest I don't really see the value in such an option. But that might be because of my very strong opinions about exceptions in C++.

Just to be clear, I've seen your PR and I'm not at all against implementing such an option. Even if I don't see the value, others might and with the option being defaulted to false there should be no harm done and it would be a net benefit for all users.