Proposal to replace `_LIBCPP_NODISCARD_EXT` with `[[nodiscard]]`

Hi folks,

There’s been a lot of requests for [[nodiscard]] to be replaced with _LIBCPP_NODISCARD_EXT in recent reviews where the standard doesn’t explicitly declare an entity with the nodiscard attribute. After some investigation, I’ve discovered that this feature is confusing and error-prone. _LIBCPP_NODISCARD_EXT is opt-in, which requires users to know about its existence (I certainly didn’t until reviewers started asking for it, and even then, didn’t expect it to be opt-in), and then has three ways to opt out of its use: _LIBCPP_DISABLE_NODISCARD_EXT (global), -Wno-unused-result (global), and cast to void (local).

Since It’s unclear why we want so many toggle options for this feature, this attribute doesn’t change a user’s program, and users already have two compiler-based opt-out mechanisms for all supported compilers and C++ standards, I think it’s reasonable to remove this attribute in favour of the easier-to-use attribute itself.

Kind regards,

Christopher Di Bella

Hello, Chris, all.

I agree that it would be a good idea to move away from “_LIBCPP_NODISCARD_EXT”. All of our supported compilers have a flag to disable “[[nodiscard]]”, which is probably a better thing for people to use than this inconsistent configuration macro. I also doubt that we have any configuration that tests with this.

Thanks.

Hi folks,

There's been a lot of requests for `[[nodiscard]]` to be replaced with `
_LIBCPP_NODISCARD_EXT` in recent reviews where the standard doesn't
explicitly declare an entity with the nodiscard attribute. After some
investigation, I've discovered that this feature is confusing and
error-prone. `_LIBCPP_NODISCARD_EXT` is opt-in, which requires users to
know about its existence (I certainly didn't until reviewers started asking
for it, and even then, didn't expect it to be opt-in), and then has three
ways to opt out of its use: `_LIBCPP_DISABLE_NODISCARD_EXT` (global), `
-Wno-unused-result` (global), and cast to `void` (local).

Looking at the patch introducing the feature it seems this comment is
the reason to make it an opt-in https://reviews.llvm.org/D45179#1077048

Since It's unclear why we want so many toggle options for this feature, this
attribute doesn't change a user's program
<http://eel.is/c++draft/dcl.attr.nodiscard>, and users already have
two compiler-based
opt-out mechanisms <https://godbolt.org/z/hsWo1v5W9> for all supported
compilers and C++ standards, I think it's reasonable to remove this
attribute in favour of the easier-to-use attribute itself.

As a standard library vendor I think we should be careful against
breaking the code of our users when they upgrade to a newer version of
Clang and libc++. So I'm against just making all extensions available
unconditionally. Even if we think there's no good reason to discard the
return of certain functions somebody might have a valid reason. If we
make this change the user either needs to disable the `nodiscard`
diagnostics globally or adjust their code.

If we want to make this extension available unconditionally I think we
should first change the switch from opt-in to opt-out and document these
changes in the release notes. At a later stage we can remove the opt-out.
That way we can get feedback on the extension and we might note we've
been too liberal with adding `nodiscard`s.

I would like to know what the other vendors do in this area. If
libstdc++ and MSVC STL also have additional `nodiscard`s it would be
nice to align with them. Then, of course, it would be nice if there
were a paper to make these extensions part of the Standard.

Cheers,
Mark de Wever

Hi folks,

I was out for a bit, but now I’m back. I see there’s been a lot of discussion about [[nodiscard]] lately (here and on Discord), and I think we should try to settle that story once and for all. My opinion has two aspects to it:

First, I think that we should apply [[nodiscard]] conservatively, only in the places where it is mandated by the Standard, or where we know almost for sure that it is a vexing bug hidden (like std::vector::empty or ranges::empty). Something like std::vector::size, for example, is not a good candidate for getting [[nodiscard]]. Yes, it doesn’t make much sense to call .size() if you discard the result. However, it is not something that we have evidence of it causing bugs, unlike for empty or things like lock_guard's constructor. Catching vexing cases like .empty() and lock_guard is the primary reason for adding the attribute - policing pretty much all functions that return non-void isn’t.

If we were designing the language from scratch, I’d probably argue that all non-void functions should be [[nodiscard]] implicitly, and then we’d have an attribute to allow discarding the result instead. I think that would be far more ergonomic. But that’s not the language we have, and I think we should try to work with it instead of starting to annotate almost every single function we write with an attribute. Annotating everything has a cost in readability, doesn’t provide obvious value (I’m not convinced that annotating something like .size() will help catch any bug, ever), and will almost certainly cause issues for people trying to update to the new version of the library, in case they use some [[nodiscard]] functions in dubious but valid places. That brings us to the second point about extensions vs mandated [[nodiscard]].

I believe that if we stick with the guideline above of only adding [[nodiscard]] to places that are truly vexing (and so we should basically consider it a bug that the Standard doesn’t mandate [[nodiscard]] on those), the debate of whether to enable [[nodiscard]] extensions by default or not becomes pretty much moot. Indeed, if we only add [[nodiscard]] conservatively to places that are really catching bugs, we shouldn’t trigger any false positives in user code. Hence, nobody should notice that we added these extensions, and if they do, then they’d be thankful that we’ve done so because we’d have caught a bug. For that reason, I believe that it’s fine to just use [[nodiscard]] unconditionally (not guarded behind some sort of extension macro) if we stay conservative about where we use it. Furthermore, under that guideline, I think we should consider writing a paper to make it part of the Standard every time we add [[nodiscard]] as an extension.

So, my preference would be to review the current uses of [[nodiscard]] in the library (most of which have been added recently) and remove those that do not abide to the guideline above. Similarly, we could turn uses of _LIBCPP_NODISCARD_EXT into _LIBCPP_NODISCARD and update the documentation accordingly (alongside with a release note, but we should expect that change to never break anyone if we’re being conservative).

Louis