Tracking issue: Replace `unwrap` with `expect`

[ghost]

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 #3913)
• [ ] bevy_asset (claimed #3892)
• [ ] bevy_audio (claimed #3913)
• [ ] bevy_core (claimed #3913)
• [ ] bevy_core_pipeline (claimed #3913)
• [ ] bevy_crevice
• [x] bevy_derive (no unwraps)
• [ ] bevy_diagnostic (claimed #3975)
• [x] bevy_dylib (no unwraps)
• [ ] bevy_dynamic_plugin (crate get's removed by #3893)
• [ ] bevy_ecs
• [ ] bevy_ecs_compile_fail_tests
• [x] bevy_gilrs (no unwraps)
• [ ] bevy_gltf
• [x] bevy_input (no unwraps)
• [x] bevy_internal (no unwraps)
• [ ] bevy_log(claimed #3957)
• [ ] bevy_macro_utils
• [x] bevy_math (no unwraps)
• [ ] bevy_pbr (claimed #4046)
• [ ] bevy_reflect
• [ ] bevy_render
• [ ] bevy_scene (claimed #3975)
• [ ] bevy_sprite (claimed #4043)
• [ ] bevy_tasks
• [ ] bevy_text
• [ ] bevy_transform
• [ ] bevy_ui
• [x] bevy_utils (no unwraps)
• [ ] bevy_window (claimed #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?

[ghost]

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.

[ghost]

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.

[ghost]

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.

[ghost]

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

[ghost]

Good call! Added this to the description as well.

[Laozey]

Hi ! My friend and I want to tackle the unwraps in bevy_input for our first contribution to bevy. Is it possible ?

[ghost]

Welcome! Yes of course. The bevy_input crate isn't claimed yet so you are good to go.

[Axighi]

I'd like to take bevy_log

[Laozey]

We've checked the unwraps in the bevy_input crate and we only found these. Is it necessary to change those to expects ? In this case unwrap seems to be appropriate. rg -C 4 unwrap # in bevy_input crate

src/gamepad.rs
112-impl GamepadSettings {
113-    pub fn get_button_settings(&self, button: GamepadButton) -> &ButtonSettings {
114-        self.button_settings
115-            .get(&button)
116:            .unwrap_or(&self.default_button_settings)
117-    }
118-
119-    pub fn get_axis_settings(&self, axis: GamepadAxis) -> &AxisSettings {
120-        self.axis_settings
121-            .get(&axis)
122:            .unwrap_or(&self.default_axis_settings)
123-    }
124-
125-    pub fn get_button_axis_settings(&self, button: GamepadButton) -> &ButtonAxisSettings {
126-        self.button_axis_settings
127-            .get(&button)
128:            .unwrap_or(&self.default_button_axis_settings)
129-    }
130-}

[alice-i-cecile]

@Laozey that's correct, those are fine. Unwrap_or variants also don't have the same problems as bare unwraps either.

[ghost]

We've checked the unwraps in the bevy_input crate and we only found these. Is it necessary to change those to expects ? In this case unwrap seems to be appropriate.

@Laozey True. Like @alice-i-cecile already said unwrap_or doesn't have the same problem as a bare unwrap because it doesn't panic when the value (in this case the setting) is None (see https://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap_or). Since there is nothing else left in that crate I'll mark it as done. Thanks for the help!

[mnmaita]

Taking the bevy_sprite crate, if that's fine.

[omarbassam88]

Is there any crates that are still not tackled that I can start working on as a first contribution?

[omarbassam88]

Can I claim the bevy_pbr crate.? I can see it has a lot of unwraps and no one seems to claim it?

[ghost]

Taking the bevy_sprite crate, if that's fine.

@mnmaita Yup, that's absolutely fine. Thank you!

Is there any crates that are still not tackled that I can start working on as a first contribution?

@omarbassam88 I have a list of every crate in the description of this issue and also if they are done (marked by the checkbox) or already claimed (mentioned in parenthesis after the crate name). You can start working on any crate that isn't done or claimed yet, but please let us know which ones you decide to do so we avoid having multiple PR's that work on the same crate.

[ghost]

Can I claim the bevy_pbr crate.? I can see it has a lot of unwraps and no one seems to claim it?

@omarbassam88 Of course! It's not done or claimed yet. Thanks for the help :)

[alice-i-cecile]

IMO we need a unified error message for these PRs for "resource not found". Or, perhaps more usefully, a common error type that is returned and unwrapped. "Could not find resource" isn't actually more helpful than an unwrap unfortunately.

[ghost]

Please do not continue working on this tracking issue for now. The reason for this is mentioned at the top of the issue description!

[cart]

Something to consider before throwing expects onto more "internal" non-user facing code: static strings are compiled into the binary. I don't think we want to bloat bevy apps with a bunch of "internal error messages", especially in cases where the context of the code is as good (if not better) for debugging than the message in the expect (for a Bevy Engine dev debugging a user-reported error). Here is an example of an "internal expect" change that we should consider not doing: https://github.com/bevyengine/bevy/pull/3913/files#r815492767

[cart]

I think we should consider only using expect messages for "panics that are in normal user-facing dev-flows"

[alice-i-cecile]

Here is an example of an "internal expect" change that we should consider not doing: https://github.com/bevyengine/bevy/pull/3913/files#r815492767

Yep: IMO APIs like that should just unwrap a good error type, or have an infallible version.

[ghost]

Something to consider before throwing expects onto more "internal" non-user facing code: static strings are compiled into the binary. I don't think we want to bloat bevy apps with a bunch of "internal error messages", especially in cases where the context of the code is as good (if not better) for debugging than the message in the expect (for a Bevy Engine dev debugging a user-reported error). Here is an example of an "internal expect" change that we should consider not doing: https://github.com/bevyengine/bevy/pull/3913/files#r815492767

Yeah seems reasonable. If we think there is value in adding an error message inside of internal stuff, should that be done using unwrap_or_else instead? This is for example done inside of the bevy_ecs crate and it also notes that using unwrap_or_else avoids allocation unless a failure occurs. Would this still cause the same problem you mentioned?

Example: https://github.com/bevyengine/bevy/blob/main/crates/bevy_ecs/src/world/mod.rs#L207-L209

I think we should consider only using expect messages for "panics that are in normal user-facing dev-flows"

Yeah this should definitely be our main goal, but I also think there is great value in having helpful error messages inside of the internal code. Obviously only to an extend, but this could really make the life of new or inexperienced contributors easier.

Yep: IMO APIs like that should just unwrap a good error type, or have an infallible version.

Agreed. Good error types should be one of the main takeaways of this tracking issue. Your get_resource change for example already shows the great potential of changes like this.

[ghost]

I updated the description of the tracking issue and added a new General error handling section. Everyone that already worked on this tracking issue can now rebase their changes and continue working on it. Please read the General error handling section and maybe consider creating better error handling for specific things when you see yourself copy pasting error messages all over the place.

[cart]

Yeah seems reasonable. If we think there is value in adding an error message inside of internal stuff, should that be done using unwrap_or_else instead? This is for example done inside of the bevy_ecs crate and it also notes that using unwrap_or_else avoids allocation unless a failure occurs. Would this still cause the same problem you mentioned?

unwrap_or_else would still suffer from the same problem, as that function would still have the string compiled into the binary. If you see a "string literal" in rust code, that will be added to the binary.

An alternative here that is almost as good from a debugging perspective: continue using unwraps for "non-user-facing internal errors", but add a comment above them explaining why the unwrap is ok. People debugging internal errors will have the code in front of them anyway.

Yep: IMO APIs like that should just unwrap a good error type, or have an infallible version.

This won't fit every situation. For improving something like an "internal option unwrap", a "good error type" would still involve defining string literals for each error variant, plus the additional complexity of mapping that option to the error type. Creating an infallible version just "moves" the problem somewhere else (and adds implementation complexity). Imo there will always be class of thing that gets worse by using those approaches. I think I prefer "comments above expects unwraps" in those cases.

I also think there might be cases where the context of the unwrap is "clear enough" to someone looking at the code. If it ever feels like someone is adding a comment above an unwrap "because its a rule and not because it adds clarity", we should push back.

[alice-i-cecile]

That's solid advice.

I think we should close out this issue (and the linked PRs), and add this advice to the style guide.

[ghost]

An alternative here that is almost as good from a debugging perspective: continue using unwraps for "non-user-facing internal errors", but add a comment above them explaining why the unwrap is ok. People debugging internal errors will have the code in front of them anyway.

Yeah that'll probably work just fine for internal code.

I think we should close out this issue (and the linked PRs), and add this advice to the style guide.I think we should close out this issue (and the linked PRs), and add this advice to the style guide.

Agreed. This tracking issue sadly cause more harm than good. It was a great learning experience though!