Tracking issue: Replace `unwrap` with `expect`

Explanation

Calling unwrap on an Option or Result causes the program to panic if the Option is None or the Result is Err. If the program panics because of an unwrap call, the error message isn't really helpful. To help contributors and users of bevy find problems faster we can use expect to our advantage. Calling expect works exactly the same as unwrap, but allows us to pass a string containing an explanation of why the panic occurred or rather what we expected to be the case.

Example

Unwrap

fn main() {
    // Panics with the following message:
    // thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src/main.rs:5:12
    let number: Option<i32> = None;
    number.unwrap();

    // Doesn't panic.
    let number: Option<i32> = Some(1);
    number.unwrap();
}

Expect

fn main() {
    // Panics with the following message:
    // thread 'main' panicked at 'Number should exist', src/main.rs:5:12
    let number: Option<i32> = None;
    number.expect("Number should exist");

    // Doesn't panic.
    let number: Option<i32> = Some(1);
    number.expect("Number should exist");
}

To-Do

Replace most of the unwraps inside of the individual crates with more explanatory expects. Changing the unwrap calls inside of our main code is more important than inside of our tests, but it's appreciated if it is done in both.

General error handling

Through the PR's made for this tracking issue we realized that we have a lot of very similar and common error messages that all have to stay consistent for a good user and developer experience. Since this is hard to maintain we want to introduce better error handling in general for things like getting a resource (#4047).

If you realize that in one of your PR's you are writing the same error message multiple times, you might as well consider doing something similar to #4047 and create better error handling for those things.

Invariants

Not every unwrap call should be replaced with expect. This is because sometimes unwrap is used on an invariant. An invariant is something that doesn't vary and can therefore be assumed to always successfully unwrap without panicking.

Error codes

Whenever possible and reasonable please add the error to the error codes when adding an error message. This should only be done for errors that can't be explained in a single line and therefore need a more in depth explanation of how to fix the error.

No final dot

There shouldn't be a dot at the end of an error message. This is because Rust will add : Err at the end of an expect error message on a Result. On an Option this looks fine, but to keep it consistent don't add a dot there either.

Example

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=aed00b15860c0c31534e110ca7852a4d

// expect on an Option
thread 'main' panicked at 'this is an error message.', src/main.rs:9:16

// expect on a Result
thread 'main' panicked at 'this is an error message.: Err1', src/main.rs:9:16

Crates

• bevy_app (claimed Replace unwraps with expects in bevy_app, bevy_audio, bevy_core, and bevy_core_pipeline #3913)
• bevy_asset (claimed Replace unwraps with expects in bevy_asset #3892)
• bevy_audio (claimed Replace unwraps with expects in bevy_app, bevy_audio, bevy_core, and bevy_core_pipeline #3913)
• bevy_core (claimed Replace unwraps with expects in bevy_app, bevy_audio, bevy_core, and bevy_core_pipeline #3913)
• bevy_core_pipeline (claimed Replace unwraps with expects in bevy_app, bevy_audio, bevy_core, and bevy_core_pipeline #3913)
• bevy_crevice
• bevy_derive (no unwraps)
• bevy_diagnostic (claimed Replace unwraps with expects in bevy_window, bevy_scene and bevy_diagnostics #3975)
• bevy_dylib (no unwraps)
• bevy_dynamic_plugin (crate get's removed by Remove bevy_dynamic_plugin #3893)
• bevy_ecs
• bevy_ecs_compile_fail_tests
• bevy_gilrs (no unwraps)
• bevy_gltf
• bevy_input (no unwraps)
• bevy_internal (no unwraps)
• bevy_log(claimed replace unwrap in bevy_log with expect #3957)
• bevy_macro_utils
• bevy_math (no unwraps)
• bevy_pbr (claimed Replace unwraps with expects in bevy_pbr #4046)
• bevy_reflect
• bevy_render
• bevy_scene (claimed Replace unwraps with expects in bevy_window, bevy_scene and bevy_diagnostics #3975)
• bevy_sprite (claimed Replace unwrap calls with expect in bevy_sprite crate  #4043)
• bevy_tasks
• bevy_text
• bevy_transform
• bevy_ui
• bevy_utils (no unwraps)
• bevy_window (claimed Replace unwraps with expects in bevy_window, bevy_scene and bevy_diagnostics #3975)
• bevy_winit

[alice-i-cecile]

When you need to capture state in the error message, use unwrap_or_else instead of expect, as seen here.

[tyleranton]

I'm interested in this as a first contribution. Should this be tackled as one PR, or one PR per crate?

I'm interested in this as a first contribution.

Awesome! Thank you <3 Great to have you here!

Should this be tackled as one PR, or one PR per crate?

I would suggest making one PR per crate so that reviewing the changes and getting them merged is easier. You could also compile 2-3 crates into one PR if the crates don't have a lot of unwraps inside of them.

[tyleranton]

Okay cool, thanks! I'll start with with bevy_app, bevy_audio, and bevy_core_pipeline as they have few unwraps. bevy_core looks like it's already good unless I'm missing something.

Perfect! Thank you :)

I just checked bevy_core and there are some unwraps in the fixed_timestep file.

[mockersf]

I don't think replacing all unwrap with an expect is the right call. Sometime, unwrap is used on an invariant, in those cases it's more like thing.unwrap_or(|| unreachable!("shouldn't happen")) which isn't exactly the same as a thing.expect("unreachable: shouldn't happen"). But that's probably just a matter of taste...

Also, when possible, please add to the error codes (https://github.com/bevyengine/bevy/tree/main/errors) when adding an error message.

Thanks for the suggestions and your explanation in the bevy discord! I updated the description accordingly.

[beregolas]

Hey, I'm looking for a first contribution as well. Is it okay if I start with a couple of crates on this issue as well, or do you want all of them? In that case I'll look for a different issue. (But this seems good in scope and to get to grips with a repo of this scale, even at work we're nowhere close to this size of a code base ^^)

[alice-i-cecile]

Is it okay if I start with a couple of crates on this issue as well

Yes please! Just let us know which ones you would like to tackle.

Welcome! I answered the same question in a previous comment of mine (see below).

I would suggest making one PR per crate so that reviewing the changes and getting them merged is easier. You could also compile 2-3 crates into one PR if the crates don't have a lot of unwraps inside of them.

[mockersf]

Another suggestion: expect message on an Option<T> could end with a ., but must not on a Result<T, E>. Rust will add a : Err at the end of an expect on a result. General rule could be no final .

See https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=af9d877512343ad1dbba1e22df1d9769