unwrap considered harmful
#12660
Comments
james7132
added
P-Crash
|
See #10166 for a major culprit. |
|
Another API design issue related to this is |
|
It was also moved to be consistent with That said, I am personally allergic to using |
|
Perhaps instead of Regardless, I agree with this change. |
|
|
|
I think that |
|
Another source of panics is when systems with The alternatives off the top of my head are a little awkward, though:
|
|
I would be in favor of renaming |
alice-i-cecile
added
A-Cross-Cutting
|
#1255 is a big source of dumb panics as I refactor as well. |
|
Removing the controversial label, as it seems like everyone's in agreement that this is something we should be doing. |
james7132
removed
the
X-Controversial
|
How would you like PRs submitted for this? I have a branch where I started removing unwrap()s in most of the smaller crates but it turned out to be quite huge. Maybe this issue can track which crates have been "cleaned"? |
|
@yyogo one crate at a time please: although for bevy_ecs it's likely best to do it one module at a time. |
|
If we're doing this one crate at a time a tracking issue to keep track which crates have been cleaned up would be useful. |
In that situation, wouldn't it make more sense to deprecate |
|
Maybe relevant:
I also find myself reaching for |
|
when replacing an
if the message starts being too long, an error code can be used to link to a page with more details, see https://github.com/bevyengine/bevy/tree/main/errors |
|
I'm also on board for making Bevy less panicky. But I don't think we should seriously consider removing panicking APIs or renaming First, without Results + let Ok(window) = windows.get_single() else {
return;
};
let Some(cursor) = window.cursor_position() else {
return;
};
let Ok(board) = boards.get_single() else {
return;
};
let Ok((camera, camera_transform)) = cameras.get_single() else {
return;
};
let Some(ray) = camera.viewport_to_world_2d(camera_transform, cursor) else {
return;
};// Panicking variant
let window = windows.single();
let cursor = window.cursor_position().unwrap();
let board = boards.single();
let (camera, camera_transform)) = cameras.single();
let ray = camera.viewport_to_world_2d(camera_transform, cursor).unwrap();Note that I've just copy/pasted some code from one of my games. Some of the Porting this to a Result system would fail, naively: fn system(.....) -> Result<()> {
let window = windows.get_single()?;
// this returns Option and therefore fails
let cursor = window.cursor_position()?;
let board = boards.get_single()?;
let (camera, camera_transform) = cameras.get_single()?;
// this returns Option and therefore fails
let ray = camera.viewport_to_world_2d(camera_transform, cursor)?;
Ok(())
}This is a Rust restriction (unfortunately). It is impossible to build a Result / error type that handles both Results and Options. Bevy APIs (including core ECS apis) sometimes return Result and sometimes return Option (ex: Queries return Results, World apis and Assets return Options). In these cases you either need to fn system(.....) -> Option<()> {
let window = windows.get_single().ok()?;
let cursor = window.cursor_position()?;
let board = boards.get_single().ok()?;
let (camera, camera_transform) = cameras.get_single().ok()?;
let ray = camera.viewport_to_world_2d(camera_transform, cursor)?;
}fn system(.....) -> Result<()> {
let window = windows.get_single()?;
let cursor = window.cursor_position().ok_or(MyErr)?;
let board = boards.get_single()?;
let (camera, camera_transform) = cameras.get_single().?;
let ray = camera.viewport_to_world_2d(camera_transform, cursor).ok_or(MyErr)?;
Ok(())
}The system above actually isn't too bad. But imagine doing this when interleaving Queries / Assets / EntityMut, etc in the same system. If we're going to embrace error handling, I think we need to convert most internal APIs to use Results instead of Option to avoid the compatibility issues. Using them together in their current form is an exercise in frustration and ergonomically unsatisfying. |
|
Note that Result systems are already supported. However you need to manually convert them to "normal systems" using app.add_systems(Update, a_system.map(error));
fn a_system(query: Query<&A>) -> Result<()> {
let a = query.get_single()?;
Ok(())
}We'd want to make this automatic if we're going to encourage this pattern: app.add_systems(Update, a_system); |
|
We'd also want our own |
|
I'm also partial to this in my own code as I don't love typing/reading const OK: Result<()> = Ok(());
fn system() -> Result<()> {
OK
} |
Perhaps a |
|
Also worth considering is the same game might want differing behaviour for different |
I think removing them would definitely require easy "error handling" for systems, but I think renaming them could have a relatively minor impact as long as it's not I think overall it's quite problematic to have panicking versions be the obvious function because they are at |
Yes, but this will completely obscure where the error occurred, making it borderline unusuable IMO. Apparently |
|
While it's more in "user code/systems", I wanted to contribute what I've been doing for ages now. Don't know how relevant to "engine" code it is, but since adopting this pattern I practically never reach for The practical result of how these macros simplify/speed up writing gameplay logic is that I never use The advantages are
Downsides
Example macros macro_rules! get_mut {
($q:expr,$r:expr) => {
match $q.get_mut($r) {
Ok(m) => m,
_ => return,
}
};
}
macro_rules! get_single {
($q:expr) => {
match $q.get_single() {
Ok(m) => m,
_ => return,
}
};
}
// get!, get_single_mut! some!, ok! ...etcFor less critical/WIP code I'm usually fine with "nothing happening" if the expected values aren't available. I do not want to panic or unwrap for these, I would have just written a When I am happy with the behaviour and want to commit to that code I replace the macros with a fn grid_debug_gizmo(
mut gizmos: Gizmos,
window: Query<&Window>,
camera: Query<(&Camera, &GlobalTransform), With<crate::MainCamera>>,
) {
let window = get_single!(window);
let (camera, camera_t) = get_single!(camera);
let cursor_ui_pos = some!(window.cursor_position());
let cursor_world_pos = some!(camera.viewport_to_world_2d(camera_t, cursor_ui_pos));
gizmos.rect_2d(
cursor_world_pos.grid_aligned_centered(),
0.0,
Vec2::splat(GRID_SIZE + 4.0),
Color::PINK,
);
}The nice feature of the macros fn handle_game_scene_switch(location: Query<&PawnLocation>, etc) {
query.iter().for_each(|(e, p)| {
let location = get!(location, p.get());
setup_local_map_pawn_renderer(&mut cmd, e, &asset_server, location, etc);
});
} |
|
I find these |
|
I just ran into this which panics because the rotation is denormalized.
I'm still digging into how my transform's rotation ends up this way, but it was really surprising that Transform::forward can panic (because it calls local_z which can panic because of Quat multiplication #12981). Should something as ubiquitous as Transform note in its docs that it can panic due to Quat's panics? Also, separately, am I crazy? I'm running into Quat panics but I'm not explicitly enabling |
By @mweatherley on Discord. Strongly agree that we shouldn't be panicking there though, simply loudly erroring. |
|
Yeah, I agree with Alice that these situations should loudly warn instead of panicking outright. In some of these cases (e.g. ramirezmike's example) it's important for the issue to be fixed upstream because the Transform is actually invalid (i.e. unrecoverable internal state) instead of just being somewhat denormalized. |
|
Just outlined what I think our plan should be here: #14275 (comment) |
# Objective - It's possible to have errors in a draw command, but these errors are ignored ## Solution - Return a result with the error ## Changelog Renamed `RenderCommandResult::Failure` to `RenderCommandResult::Skip` Added a `reason` string parameter to `RenderCommandResult::Failure` ## Migration Guide If you were using `RenderCommandResult::Failure` to just ignore an error and retry later, use `RenderCommandResult::Skip` instead. This wasn't intentional, but this PR should also help with #12660 since we can turn a few unwraps into error messages now. --------- Co-authored-by: Charlotte McElwain <charlotte.c.mcelwain@gmail.com>
# Objective Error handling in bevy is hard. See for reference #11562, #10874 and #12660. The goal of this PR is to make it better, by allowing users to optionally return `Result` from systems as outlined by Cart in <#14275 (comment)>. ## Solution This PR introduces a new `ScheuleSystem` type to represent systems that can be added to schedules. Instances of this type contain either an infallible `BoxedSystem<(), ()>` or a fallible `BoxedSystem<(), Result>`. `ScheuleSystem` implements `System<In = (), Out = Result>` and replaces all uses of `BoxedSystem` in schedules. The async executor now receives a result after executing a system, which for infallible systems is always `Ok(())`. Currently it ignores this result, but more useful error handling could also be implemented. Aliases for `Error` and `Result` have been added to the `bevy_ecs` prelude, as well as const `OK` which new users may find more friendly than `Ok(())`. ## Testing - Currently there are not actual semantics changes that really require new tests, but I added a basic one just to make sure we don't break stuff in the future. - The behavior of existing systems is totally unchanged, including logging. - All of the existing systems tests pass, and I have not noticed anything strange while playing with the examples ## Showcase The following minimal example prints "hello world" once, then completes. ```rust use bevy::prelude::*; fn main() { App::new().add_systems(Update, hello_world_system).run(); } fn hello_world_system() -> Result { println!("hello world"); Err("string")?; println!("goodbye world"); OK } ``` ## Migration Guide This change should be pretty much non-breaking, except for users who have implemented their own custom executors. Those users should use `ScheduleSystem` in place of `BoxedSystem<(), ()>` and import the `System` trait where needed. They can choose to do whatever they wish with the result. ## Current Work + [x] Fix tests & doc comments + [x] Write more tests + [x] Add examples + [X] Draft release notes ## Draft Release Notes As of this release, systems can now return results. First a bit of background: Bevy has hisotrically expected systems to return the empty type `()`. While this makes sense in the context of the ecs, it's at odds with how error handling is typically done in rust: returning `Result::Error` to indicate failure, and using the short-circuiting `?` operator to propagate that error up the call stack to where it can be properly handled. Users of functional languages will tell you this is called "monadic error handling". Not being able to return `Results` from systems left bevy users with a quandry. They could add custom error handling logic to every system, or manually pipe every system into an error handler, or perhaps sidestep the issue with some combination of fallible assignents, logging, macros, and early returns. Often, users would just litter their systems with unwraps and possible panics. While any one of these approaches might be fine for a particular user, each of them has their own drawbacks, and none makes good use of the language. Serious issues could also arrise when two different crates used by the same project made different choices about error handling. Now, by returning results, systems can defer error handling to the application itself. It looks like this: ```rust // Previous, handling internally app.add_systems(my_system) fn my_system(window: Query<&Window>) { let Ok(window) = query.get_single() else { return; }; // ... do something to the window here } // Previous, handling externally app.add_systems(my_system.pipe(my_error_handler)) fn my_system(window: Query<&Window>) -> Result<(), impl Error> { let window = query.get_single()?; // ... do something to the window here Ok(()) } // Previous, panicking app.add_systems(my_system) fn my_system(window: Query<&Window>) { let window = query.single(); // ... do something to the window here } // Now app.add_systems(my_system) fn my_system(window: Query<&Window>) -> Result { let window = query.get_single()?; // ... do something to the window here Ok(()) } ``` There are currently some limitations. Systems must either return `()` or `Result<(), Box<dyn Error + Send + Sync + 'static>>`, with no in-between. Results are also ignored by default, and though implementing a custom handler is possible, it involves writing your own custom ecs executor (which is *not* recomended). Systems should return errors when they cannot perform their normal behavior. In turn, errors returned to the executor while running the schedule will (eventually) be treated as unexpected. Users and library authors should prefer to return errors for anything that disrupts the normal expected behavior of a system, and should only handle expected cases internally. We have big plans for improving error handling further: + Allowing users to change the error handling logic of the default executors. + Adding source tracking and optional backtraces to errors. + Possibly adding tracing-levels (Error/Warn/Info/Debug/Trace) to errors. + Generally making the default error logging more helpful and inteligent. + Adding monadic system combininators for fallible systems. + Possibly removing all panicking variants from our api. --------- Co-authored-by: Zachary Harrold <zac@harrold.com.au>
What problem does this solve or what need does it fill?
Unwrap should generally be avoided. We already encourage contributors to avoid the use of
Option::unwrap, yet crashes from panics are still one of the most common categories of issues we're seeing now.What solution would you like?
unwrap_usedlint at a workspace level, and addallow-unwrap-in-tests = trueto Clippy.toml.expector unwrap_or_else with a panic where possible, otherwise properly gracefully handle the error.What alternative(s) have you considered?
Leaving it as is. Keep crashing from unwraps.
The text was updated successfully, but these errors were encountered: