[libc++] mark std::expected as nodiscard

[hulxv]

Fix #130656

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mohamed Emad (hulxv)

Changes

Fix #130656

---

Full diff: https://github.com/llvm/llvm-project/pull/130820.diff

2 Files Affected:

• (modified) libcxx/include/__cxx03/__expected/expected.h (+1-1)
• (modified) libcxx/include/__expected/expected.h (+1-1)

diff --git a/libcxx/include/__cxx03/__expected/expected.h b/libcxx/include/__cxx03/__expected/expected.h
index 1d54bb9f6edeb..ce9676c68c2dc 100644
--- a/libcxx/include/__cxx03/__expected/expected.h
+++ b/libcxx/include/__cxx03/__expected/expected.h
@@ -58,7 +58,7 @@ _LIBCPP_PUSH_MACROS
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp, class _Err>
-class expected;
+class [[nodiscrad]] expected;
 
 template <class _Tp>
 struct __is_std_expected : false_type {};
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index 03bbd1623ed5c..8dd32668bcb33 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -60,7 +60,7 @@ _LIBCPP_PUSH_MACROS
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp, class _Err>
-class expected;
+class [[nodiscard]] expected;
 
 template <class _Tp>
 struct __is_std_expected : false_type {};

[mordante]

Thanks for working on this!

This needs tests to verify that the nodiscard works as expected.
libcxx/test/std/containers/associative/multimap/empty.verify.cpp
contains an example test.

[hulxv]

Thanks for working on this!

This needs tests to verify that the nodiscard works as expected. libcxx/test/std/containers/associative/multimap/empty.verify.cpp contains an example test.

Thanks for guidance! I will add it now

[frederick-vs-ja]

We should also add [[nodiscard]] to this partial specialization:

llvm-project/libcxx/include/__expected/expected.h

Lines 1355 to 1358 in bd748b3

	template <class _Tp, class _Err>
	requires is_void_v<_Tp>
	class expected<_Tp, _Err> : private __expected_void_base<_Err> {
	static_assert(__valid_std_unexpected<_Err>::value,

Superseded

Per CI failures, we should add void cast to function calls in these files to suppress warnings:

• libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp
• libcxx/utilities/expected/expected.expected/or_else.mandates.verify.cpp
• libcxx/utilities/expected/expected.void/and_then.mandates.verify.cpp
• libcxx/utilities/expected/expected.void/or_else.mandates.verify.cpp

[ldionne]

I'm conflicted about making this change -- I suspect this could end up being really verbose. Before we actually land this, can you please ping @llvm/libcxx-vendors to give folks a chance to run this change internally first? I'd like to try that out to get some signal.

[zmodem]

Chromium isn't on C++23 yet, so we're not using std::expected and I'm not really familiar with the class, but I did notice that our own "backport" is marked [[nodiscard]]: https://source.chromium.org/chromium/chromium/src/+/main:base/types/expected.h;l=273;drc=9596f8af68e7ad21da9bf94249e8a314edf09329

[Mick235711]

If we are doing this, should we add a reason for why std::expected is marked as [[nodiscard]]? I believe it would be more helpful if a reason is supplied in the warning message for users who encountered a warning.
Although libc++ doesn't seem to use [[nodiscard("reason")]] at all currently. Is there a policy for that internally?

[mordante]

If we are doing this, should we add a reason for why std::expected is marked as [[nodiscard]]? I believe it would be more helpful if a reason is supplied in the warning message for users who encountered a warning. Although libc++ doesn't seem to use [[nodiscard("reason")]] at all currently. Is there a policy for that internally?

AFAIK we never considered to add a reason. That feature was added in C++20, while nodiscard itself if C++17.

I'm not against adding a reason. FYI @philnik777 proposed to add reasons to deprecated.

[hulxv]

I'm conflicted about making this change -- I suspect this could end up being really verbose. Before we actually land this, can you please ping @llvm/libcxx-vendors to give folks a chance to run this change internally first? I'd like to try that out to get some signal.

I came from a Rust background and we have Result that does the same behaviour as std::expected, the compiler warning us directly if this result isn't handled or assigned to a variable

let res: Result<T,E> = result_return_result(); // no warning
result_return_result(); // WARNING!!!
result_return_result().unwrap(); // no warning. ".unwrap()" works as a handler and returns the expected value

So, I don't find it a verbose thing and it's normal from my point of view

[strega-nil-re]

It seems like it's pretty much always a bug to not check an expected. I certainly have my opinions from MS-STL ([[nodiscard]] all the things!) but I don't see why anyone would ever want to implicitly ignore an expected.

[R-Goc]

I also feel like implicitly ignoring an expected is always a bug. It would be nice to see a reason added to it.

[StephanTLavavej]

A data point that may be of interest - MSVC's STL marked std::expected (and std::unexpected, and all exception types) as [[nodiscard]] with microsoft/STL#5174. There's some rationale discussion in the PR. This recently shipped in VS 2022 17.14 Preview 1 and so far we haven't gotten any negative feedback about a blizzard of warnings.

[philnik777]

While I like the idea in general, I really dislike that there is no way for the user to say "it's fine to discard the expected from this function", so we force the user to either not use expected when it may be OK to discard, or add casts to void on every single call site. Neither option seems great.

[Mick235711]

While I like the idea in general, I really dislike that there is no way for the user to say "it's fine to discard the expected from this function", so we force the user to either not use expected when it may be OK to discard, or add casts to void on every single call site. Neither option seems great.

Well, in C++26, users will be able to do

std::ignore = some_func_returning_expected();

to signify that it is OK to discard an std::expected return, which seems much more readable than casting to void. (libc++ had already implemented this change to std::ignore)

Whether this is an acceptable solution to force on users is up for debate, though.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[mordante]

@hulxv can you please fix the CI before requesting reviews again? If you need help with the CI issues feel free to reach out on Discord. I tend not to look at patches when there are a lot of CI issues.

[huixie90]

Is there any paper adding ‘[[candiscard]]’ floating around ? I’d much prefer to making it possible for the function owner to make the function possible to discard the returned value, before we adding the blanket nodiscard for everyone. Imagine if we make printf nodiscard for the reason of not checking the error code? It will simply end up with lots of #pragma gcc diagnostic

[Mick235711]

Is there any paper adding ‘[[candiscard]]’ floating around ? I’d much prefer to making it possible for the function owner to make the function possible to discard the returned value, before we adding the blanket nodiscard for everyone. Imagine if we make printf nodiscard for the reason of not checking the error code? It will simply end up with lots of #pragma gcc diagnostic

Yes, there is P2992R1 proposing the [[discard]] attribute. However, that depends on the ability to put attributes on expressions, which is a rather large and controversial step to take. Because of this, in St. Louis (2024-06) P2992R1 was rejected by EWG with a rather split vote.

[huixie90]

Is there any paper adding ‘[[candiscard]]’ floating around ? I’d much prefer to making it possible for the function owner to make the function possible to discard the returned value, before we adding the blanket nodiscard for everyone. Imagine if we make printf nodiscard for the reason of not checking the error code? It will simply end up with lots of #pragma gcc diagnostic

Yes, there is P2992R1 proposing the [[discard]] attribute. However, that depends on the ability to put attributes on expressions, which is a rather large and controversial step to take. Because of this, in St. Louis (2024-06) P2992R1 was rejected by EWG with a rather split vote.

Thank is not the same though. that [[discard]] is somewhat similar to [[maybe_unused]] and it is intended to be used by the function caller. What I really like is for authors of the functions that return expected to override the class level expected's [[nodiscard]], and to say that my function's expected can be discarded