Replace unwraps with expects in bevy_app, bevy_audio, bevy_core, and bevy_core_pipeline by tyleranton · Pull Request #3913 · bevyengine/bevy

tyleranton · 2022-02-10T23:29:40Z

Objective

Panics in bevy are often particularly frustrating and obtuse.
Tracking issue: Replace unwrap with expect #3899

Solution

Provide somewhat more helpful error messages by replacing unwraps with expects.

Meta

This is my first time contributing and really looking at the code base, so I wasn't completely confident in all of my expect error messages. Feedback is welcome and appreciated!

Ixentus

Looks good. Just some nits.

tyleranton · 2022-02-11T16:23:55Z

Is amending commits okay here since it's simple, or are new commits preferred? Thanks for taking time to review this!

ghost · 2022-02-11T18:22:43Z

That shouldn't really matter. The fastest way to commit the suggested changes is to use the commit suggestion button directly below the suggestion. This will create new commits, which is the way I saw it being done most of the time. Feel free to use whatever method is more convenient for you. For bigger changes I would prefer a new commit though.

mockersf · 2022-02-12T01:21:36Z

+    let audio_output = world
+        .get_non_send::<AudioOutput<Source>>()
+        .expect("Could not find `AudioOutput` in the `World`.");
+    let mut audio = world
+        .get_resource_mut::<Audio<Source>>()
+        .expect("Could not find `Audio` in the `World`.");


In both those cases, the issue would be either:

the user is calling this system without adding the AudioPlugin

the user is calling this system on a custom Source without having it set up first like in

bevy/crates/bevy_audio/src/lib.rs

Lines 57 to 59 in 0ccb9dd

app.init_non_send_resource::<AudioOutput<AudioSource>>()

.add_asset::<AudioSource>()

.init_resource::<Audio<AudioSource>>()

It could be helpful to explain how to fix the issue

Would this be a case where adding to the error codes is best for an explanation?

…ting-unwraps

tyleranton · 2022-02-25T02:31:16Z

Just wanted to bump this for further review
@mockersf @Ixentus @KDecay

ghost

Did a pass over everything. I think you might be missing some unwraps. For example in the bevy_core_pipeline there is an unwrap() that didn't get addressed. Could you do a second pass over the crates and see if you find any more unwraps that should be changed? Thank you <3

ghost · 2022-02-26T15:18:10Z

+            0,
+            *world
+                .get_resource::<Count>()
+                .expect("Could not find `Count` in the `World`.")


Suggested change

.expect("Could not find `Count` in the `World`.")

.expect("Could not get the `Count` resource from the `World`")

ghost · 2022-02-26T15:18:19Z

+            1,
+            *world
+                .get_resource::<Count>()
+                .expect("Could not find `Count` in the `World`.")


Suggested change

.expect("Could not find `Count` in the `World`.")

.expect("Could not get the `Count` resource from the `World`")

ghost · 2022-02-26T15:18:31Z

+            3,
+            *world
+                .get_resource::<Count>()
+                .expect("Could not find `Count` in the `World`.")


Suggested change

.expect("Could not find `Count` in the `World`.")

.expect("Could not get the `Count` resource from the `World`")

ghost · 2022-02-26T16:03:04Z

-            let draw_functions = world.get_resource::<DrawFunctions<AlphaMask3d>>().unwrap();
+            let draw_functions = world
+                .get_resource::<DrawFunctions<AlphaMask3d>>()
+                .expect("Could not get `DrawFunctions` resource from the `World`.");


Suggested change

.expect("Could not get `DrawFunctions` resource from the `World`.");

.expect("Could not get the `DrawFunctions<AlphaMask3d>` resource from the `World`. It can be added using the `CorePipelinePlugin`, or the `DefaultPlugins`");

ghost · 2022-02-26T16:03:38Z

-                let draw_function = draw_functions.get_mut(item.draw_function).unwrap();
+                let draw_function = draw_functions
+                    .get_mut(item.draw_function)
+                    .expect("Could not get draw function.");


Suggested change

.expect("Could not get draw function.");

.unwrap_or_else(|| {

panic!("Could not get the draw function for id: {}", item.draw_function.0);

});

ghost · 2022-02-26T16:05:02Z

            let draw_functions = world
                .get_resource::<DrawFunctions<Transparent3d>>()
-                .unwrap();
+                .expect("Could not get `DrawFunctions` resource from the `World`.");


Suggested change

.expect("Could not get `DrawFunctions` resource from the `World`.");

.expect("Could not get the `DrawFunctions<Transparent3d>` resource from the `World`. It can be added using the `CorePipelinePlugin`, or the `DefaultPlugins`");

ghost · 2022-02-26T16:05:44Z

-                let draw_function = draw_functions.get_mut(item.draw_function).unwrap();
+                let draw_function = draw_functions
+                    .get_mut(item.draw_function)
+                    .expect("Could not get draw function.");


Suggested change

.expect("Could not get draw function.");

.unwrap_or_else(|| {

panic!("Could not get the draw function for id: {}", item.draw_function.0);

});

ghost · 2022-02-26T16:07:03Z

-        let extracted_cameras = world.get_resource::<ExtractedCameraNames>().unwrap();
+        let extracted_cameras = world
+            .get_resource::<ExtractedCameraNames>()
+            .expect("Could not get `ExtractedCameraNames` resource from the `World`.");


Suggested change

.expect("Could not get `ExtractedCameraNames` resource from the `World`.");

.expect("Could not get the `ExtractedCameraNames` resource from the `World`. It can be added using the `CameraPlugin`, `RenderPlugin` or the `DefaultPlugins`");

Co-authored-by: KDecay <KDecayMusic@protonmail.com>

tyleranton · 2022-02-26T19:36:56Z

@KDecay wow thanks for going through all of this. Would committing all of these suggestions separately be too noisy? I know you said it's common to use that feature, just wasn't sure if volume mattered here. Appreciate the review!

Edit: Just saw the request to pause work on the tracking issue, so you can disregard the above. Thanks again!

ghost · 2022-02-27T17:57:41Z

@KDecay wow thanks for going through all of this. Would committing all of these suggestions separately be too noisy? I know you said it's common to use that feature, just wasn't sure if volume mattered here. Appreciate the review!

@tylerdotdev You can also combine the small suggestions into one larger commit to reduce noise.

Edit: Just saw the request to pause work on the tracking issue, so you can disregard the above. Thanks again!

Yup, we figured that it would be beneficial to get some of the extremely common cases out of the way first (e.g. failing on getting a resource). This will remove a lot of expects in all of the PR's related to the tracking issue which means that we can focus on the error messages themselves instead of painfully trying to keep everything consistent throughout all of the PR's.

cart · 2022-02-27T21:13:35Z

                MainPass3dNode::IN_VIEW,
            )
-            .unwrap();
+            .expect("Could not add slot edge to `RenderGraph`.");


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 like these 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).

alice-i-cecile · 2022-04-11T03:59:15Z

Closing per discussion in #3899 <3

tyleranton added 4 commits February 10, 2022 16:41

replaces unwraps with expects in bevy_app

fd941cc

replaces unwraps with expects in bevy_audio

8def886

replaces unwraps with expects in bevy_core

a04e8a1

replaces unwraps with expects in bevy_core_pipeline

82d361d

github-actions Bot added the S-Needs-Triage This issue needs to be labelled label Feb 10, 2022

Ixentus suggested changes Feb 11, 2022

View reviewed changes

ghost mentioned this pull request Feb 11, 2022

Tracking issue: Replace unwrap with expect #3899

Closed

31 tasks

IceSentry added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 11, 2022

tyleranton added 2 commits February 11, 2022 12:58

fix expect typos and error messages

b31ffb8

fix expect typos and error messages

045ef66