Mark expected, unexpected, and ALL exception types as [[nodiscard]]
#5174
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A reddit thread has persuaded me to explore marking entire types as
[[nodiscard]], which previously we reserved for guard types only.We're very careful about avoiding
[[nodiscard]]false positives. Marking an entire type affects all user-defined functions returning that type (as well as all temporary constructions), with no[[discard]]antidote available for function declarations. (Individual callsites can always be(void)suppressed.) For existing types like C++11error_code, I continue to believe that too much time has passed for it to be reasonable to mark the entire type now.☑️
<expected>⛔However, while explaining our original decision again, I remembered that
<expected>is actually still new to C++23. I forgot because we shipped it in VS 2022 17.3 (Aug 2022), over twomillenniayears ago. As it's a new type, and we haven't finalized C++23, we can set policies for this new type without fear of introducing a blizzard of[[nodiscard]]warnings in legacy codebases. Early adopters will be writing their first functions returningstd::expected, so we can keep all of their callsites clean from day one.I am marking
expected<T, E>,expected<void, E>, andunexpected<E>. The idea is that asexpectedis an alternative to exception handling, it should be difficult to unintentionally ignore errors. If a user-defined function has chosen to return theexpectedtype, all callers should be inspecting the return value. We are essentially saying that all user-defined functions returningexpectedshould be marked[[nodiscard]], and are making that decision for all of them, for all time. (Markingunexpectedis going to be the least useful because it'll be used the least, but as it contains error information that's intended to construct anexpected, the same arguments about not dropping it on the floor apply.)❕ Exception types
A different rationale applies here. These are legacy types, many going back to C++98, but they are rarely used as values. If a function is returning an exception type by value, it is very likely a "maker" function (crafting a string literal, etc.). Returning an exception by value and then discarding it is highly suspicious, as it looks like it was meant to be thrown. Similarly, constructing a temporary exception and dropping it on the floor is extremely likely to be a bug.
The following code previously compiled without warnings (except C4702 "unreachable code", which is noisy and might be silenced). Now it emits a warning, just like a discarded guard temporary:
✅ Test updates
std::expectedllvm/llvm-project#119174 upstream. libc++ was clean except for their monadicexpectedtests.P2505R5_monadic_functions_for_std_expected. This test had already exercisedexpected::transform, and contained a solitary call at the end of the function. It looked like this might have been an editing relic of the original PR when it was introduced, as the existingexpected::transformcoverage is quite sufficient and this last lambda isn't exercising anything new. Rather than(void)cast it, or expand it into a thorough test, I'm just dropping it.💯 The exception hierarchy
I believe I've audited our entire exception hierarchy, including internal exceptions. If I missed anything in product code, please let me know:
exception_Parallelism_resources_exhaustedbad_allocbad_array_new_lengthbad_castbad_any_castbad_exceptionbad_expected_access<void>bad_expected_accessbad_function_callbad_optional_accessbad_typeid__non_rtti_objectbad_variant_accessbad_weak_ptrlogic_errordomain_errorfuture_errorinvalid_argumentlength_errorout_of_rangeruntime_error_System_errorsystem_errorios_base::failurestd::experimental::filesystem::filesystem_errorstd::filesystem::filesystem_errorambiguous_local_timeformat_errornonexistent_local_timeoverflow_errorrange_errorregex_errorunderflow_errorInternally, I've also marked the following types that are defined in VCRuntime. (I've found and marked all of them, although I still need to build and test those changes, which I'll do while mirroring.)
__non_rtti_objectbad_allocbad_array_new_lengthbad_castbad_exceptionbad_typeidexception