[clang] [[nodiscard]] ignored in std::function due to warning suppression in system headers

[avikivity]

Consider:

#include <functional>

struct [[nodiscard]] A {};

A f();

std::function<void ()> ff(f);

void g() {
    ff();
}

std::function calls f() (or rather, a function pointer that that returns an A), but returns void. So somewhere, something should complain about the nodiscard A being discarded. But it looks like it's explicitly silenced somewhere, causing hard-to-find bugs.

Investigation by the gcc team [1] revealed it's likely due to -Wunused-result being suppressed in system headers, which it probably should not be.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120460

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Avi Kivity (avikivity)

Consider:

#include <functional>

struct [[nodiscard]] A {};

A f();

std::function<void ()> ff(f);

void g() {
    ff();
}

std::function calls f() (or rather, a function pointer that that returns an A), but returns void. So somewhere, something should complain about the nodiscard A being discarded. But it looks like it's explicitly silenced somewhere, causing hard-to-find bugs.

Investigation by the gcc team [1] revealed it's likely due to -Wunused-result being suppressed in system headers, which it probably should not be.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120460

[shafik]

The return value from ff is void if we make it A then it diagnoses as expected: https://godbolt.org/z/5xxfTKnrn

Making the return value void I believe is basically like casting to void which should silence the diagnostic.

[avikivity]

Making the return value void is not explicitly casting to void.

The following code warns:

struct [[nodiscard]] A {};

A f();

void ff() {
    f();
}

std::function does the same, more or less, but loses the warning in the process.

[michoecho]

Making the return value void I believe is basically like casting to void which should silence the diagnostic.

For practical context:
in our (my and @avikivity's) project we have lots of asynchronous code.
We use [[nodiscard]] to prevent accidentally leaking an asynchronous fiber by doing

f();

instead of

co_await f();

Asynchronous functions return a future which is [[nodiscard]], so the first example generates a warning, preventing the mistake.
The warning makes it very hard to leak a future without explicitly casting something to void, which is great.
But we stumbled into this problem with std::function which caused us to leak a future.

Specifically, we have a helper:

void run_test_with_environment(std::function<void(test_env&)> f) { test_env env = ...; f(env); };

and someone recently did an accidental:

run_test_with_environment([] (test_env& env) -> future { ... });

without realizing that this helper isn't meant for asynchronous tests.
So the future-returning lambda was "casted" to std::function<void(test_env&)> and called,
with the returned future getting silently discarded and leaked inside the std::function<...>::operator(),
leading to some confusion.

So it would be nice if a [[nodiscard]] violation inside the internals of std::function generated the warning too,
to avoid mistakes like this.

[shafik]

Making the return value void is not explicitly casting to void.

Unless I am reading incorrectly, which could be b/c library is not my wheelhouse, it is defined in terms of invoke: https://eel.is/c++draft/function.objects#func.wrap.func.inv-1

and indeed INVOKE says:

Define INVOKE(f, t1, t2, …, tN) as static_cast<void>(INVOKE(f, t1, t2, …, tN)) if R is cv void,

[AaronBallman]

Making the return value void is not explicitly casting to void.

Unless I am reading incorrectly, which could be b/c library is not my wheelhouse, it is defined in terms of invoke: https://eel.is/c++draft/function.objects#func.wrap.func.inv-1

and indeed INVOKE says:

Define INVOKE(f, t1, t2, …, tN) as static_cast(INVOKE(f, t1, t2, …, tN)) if R is cv void,

This matches my reading of the tea leaves, but CC @ldionne @mordante @philnik777 for some additional opinions.

[cor3ntin]

A demonstration of the issue https://godbolt.org/z/GnTsrnYfo

[avikivity]

Making the return value void is not explicitly casting to void.

Unless I am reading incorrectly, which could be b/c library is not my wheelhouse, it is defined in terms of invoke: https://eel.is/c++draft/function.objects#func.wrap.func.inv-1

and indeed INVOKE says:

Define INVOKE(f, t1, t2, …, tN) as static_cast(INVOKE(f, t1, t2, …, tN)) if R is cv void,

Then I'd say the standard has a defect.

[jwakely]

FWIW libstdc++ does not use a cast to void, we use:

if constexpr (is_void_v<R>)
  INVOKE(...);
else
  return INVOKE(...);

which would preserve the nodiscard warning (if it wasn't for the system header handling).

Then I'd say the standard has a defect.

I think it's fairly clear that std::invoke_r<void>(...) is an explicit cast to void. If you don't want an explicit conversion to void, don't use an explicit return type there, i.e. std::invoke(...).

I think it's reasonable to think of std::function<void()>:operator() as doing an implicit conversion to void, even if the current definition in terms of INVOKE<void> requires an explicit conversion.

So maybe the static_cast<void> should be part of std::invoke_r<void> not INVOKE<R>, and then INVOKE<void>(...) could be respecified as (INVOKE(...), void()) so that when it's used in std::function etc. we don't suppress nodiscard.

Some uses of INVOKE<R> would want the explicit conversion though, e.g. std::bind<void>(...) is also explicit about ignoring the return type, so maybe that should use std::invoke_r<R> instead of INVOKE<R>.

I think this is worth opening an LWG issue.