Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark expected, unexpected, and ALL exception types as [[nodiscard]] #5174

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

StephanTLavavej
Copy link
Member

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++11 error_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 two millennia years 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 returning std::expected, so we can keep all of their callsites clean from day one.

I am marking expected<T, E>, expected<void, E>, and unexpected<E>. The idea is that as expected is an alternative to exception handling, it should be difficult to unintentionally ignore errors. If a user-defined function has chosen to return the expected type, all callers should be inspecting the return value. We are essentially saying that all user-defined functions returning expected should be marked [[nodiscard]], and are making that decision for all of them, for all time. (Marking unexpected is going to be the least useful because it'll be used the least, but as it contains error information that's intended to construct an expected, 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:

D:\GitHub\STL\out\x64>type meow.cpp
#include <print>
#include <stdexcept>
using namespace std;

void woof() {
    println("woof() one!");
    runtime_error{"TOO MANY PUPPIES"};
    println("woof() two!");
}

int main() {
    try {
        println("main() before!");
        woof();
        println("main() after!");
    } catch (const runtime_error& e) {
        println("runtime_error: {}", e.what());
    }
}
D:\GitHub\STL\out\x64>cl /EHsc /nologo /W4 /wd4702 /std:c++latest /MTd /Od meow.cpp
meow.cpp
meow.cpp(7): warning C4834: discarding return value of function with [[nodiscard]] attribute

✅ Test updates

  • I submitted [libcxx][test] Silence nodiscard warnings for std::expected llvm/llvm-project#119174 upstream. libc++ was clean except for their monadic expected tests.
  • I dropped an unnecessary call in P2505R5_monadic_functions_for_std_expected. This test had already exercised expected::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 existing expected::transform coverage 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_exhausted
    • bad_alloc
      • bad_array_new_length
    • bad_cast
      • bad_any_cast
    • bad_exception
    • bad_expected_access<void>
      • bad_expected_access
    • bad_function_call
    • bad_optional_access
    • bad_typeid
      • __non_rtti_object
    • bad_variant_access
    • bad_weak_ptr
    • logic_error
      • domain_error
      • future_error
      • invalid_argument
      • length_error
      • out_of_range
    • runtime_error
      • _System_error
        • system_error
          • ios_base::failure
          • std::experimental::filesystem::filesystem_error
          • std::filesystem::filesystem_error
      • ambiguous_local_time
      • format_error
      • nonexistent_local_time
      • overflow_error
      • range_error
      • regex_error
      • underflow_error

⚠️ Note to self when mirroring

Internally, 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_object
  • bad_alloc
  • bad_array_new_length
  • bad_cast
  • bad_exception
  • bad_typeid
  • exception

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Dec 9, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 9, 2024 08:12
@CaseyCarter CaseyCarter removed their assignment Dec 9, 2024
@StephanTLavavej StephanTLavavej self-assigned this Dec 12, 2024
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 7643c27 into microsoft:main Dec 13, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the know-this-card branch December 13, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants