Our API suggests that panicking should be the default #14275
Comments
janhohenheim
added
C-Feature
janhohenheim
changed the title
|
I agree with this I think, however it would be unfortunate for singleton-style components to become more verbose. |
|
@JoJoJet you could also do the following, assuming we remove the let singleton = single!(singleton); // early return
let singleton = single!(singleton, panic); // panic if there's not exactly oneThis would require some |
|
The RFC for postfix macros would make this pattern so much nicer. Waiting for a language feature isn't practical, though. let window = window_query.single().or_return!("optional warning msg"); |
|
At work we started using a slightly different pattern. We use the early return with the fallible functions, but we use a debug_assert so it still panics in debug to help us catch those issues. That way it doesn't panic for our users if something unexpected happens. |
|
My take is generally still what I discussed here: #12660 (comment) Specifically, I think we should investigate the viability of the following plan:
Imo it is critical to prove steps (2) and (3) out first and do it "as a single transaction" from a user perspective. Skipping straight to step (3) (while easy) puts the cart before the horse and imo makes Bevy UX worse for a period of time (at best) and at worst, commits us to a path that may not be viable or ideal (due to unknowns not yet discovered). Skipping (2) would mean that I do think the macro approach is an interesting "middle ground", but it feels like we're just re-implementing the That being said, if it doesn't already exist, I think it makes a lot of sense to build the macro approach as a 3rd party crate. That seems like an appropriate "quick no bureaucracy" middle ground solution while we sort out the details. |
|
As a supplement, I think it would also be interesting to investigate an Option extension trait that lets you do this: fn system() -> Result {
let list = vec![1, 2];
let x = list.get(0).unpack()?;
let y = list.get(1).assume("index 1 should exist")?;
OK
}Where |
|
|
|
|
|
Ditto for |
|
IMO better error handling shouldn't block a rename from e.g. |
|
Copying my sentiment from Discord here. The proposed additions to This is probably food for thought for another issue however |
|
(Also just mirroring discord) I do suspect that we may want our own top level general-purpose (anyhow-style catch-all) Bevy error type to allow us to better encapsulate Bevy context (such as system context). However should should definitely consider using anyhow given its maturity and featureset. Worth weighing both options. I agree that |
IMO we can and should start on this today: I've been frustrated by this in the past. |
I still think we might want to land this all as a whole, as some people might be relying on the proliferation of |
|
(if you're saying we should start merging these changes now as they come in) |
Just noting that I previously explored this approach in #8705, but I decided to abandon it becuase it made the stack traces for errors useless, which was worse than just calling |
|
Yep, fine to wait on merging. We should at least get a list for now. |
|
@JoJoJet its worth calling out that the anyhow impl supports this reasonably (using built in rust features), provided you enable the "backtrace" anyhow cargo feature (and run with the RUST_BACKTRACE=1 environment variable): use anyhow::Result;
use thiserror::Error;
fn main() {
let result = foo();
println!("{:?}", result);
}
fn foo() -> Result<()> {
bar()?;
Ok(())
}
fn bar() -> Result<()> {
let x = vec![1, 2];
let y = x.get(2).ok_or(E1)?; // backtrace correctly points here
Ok(())
}
#[derive(Error, Debug)]
#[error("hello")]
struct E1;Prints: |
|
We could also consider adding a bevy feature that sets the environment variable for people: #[cfg(feature = "backtrace")]
std::env::set_var("RUST_BACKTRACE", "1")Just verified that this works. Then if/when we add a "dev" meta-cargo-feature, we could add "backtrace" to it. Then when people do |
|
Ah good point. We could provide a dedicated |
|
@JoJoJet I want to point out that that would again increase the amount of code we duplicate from |
|
I think it would be kind of weird for us to use |
|
Yeah, whatever the solution, usable backtraces are definitely a must-have. |
|
I published a crate for the macro approach: https://github.com/benfrankel/tiny_bail. I used (an earlier version of) these macros during the game jam and they were exceptionally convenient. |
|
I feel like this issue drifted from the original purpose and either needs a rename or split to more issues. My few cents are that we can't take the ergonomic hit from unwrapping everything by hand. The reason I have to write this is, this is causing problems just by existing. |
|
@MiniaczQ I think Cart's point here is that changing it now is really disruptive to users, and will be really disruptive again once we have better error handling. Or is your suggestion that we simply don't follow the old conventions for new API, but leave the old API intact? The new error handling sounds like something a working group could develop, if enough people are willing to participate. But I think we have plenty of those currently running already, so I doubt that would be fruitful at this time. |
|
My suggestion is that issues need to stop being marked as The problem is that it's unclear whether new features should expose panicking APIs or not, and that should be decided right now, because not knowing that is halting progress. |
|
I've only seen this issue now. My take is The solution to this I would prefer is to lean into the ECS paradigm more & treat more things as if they were querying the world. If a This also makes for nicer breaking changes because users already have it set up so that systems with Additionally this kind of default just makes for better graceful behavior for released apps. |
This only resolves errors during resource acquisition, systems can have other types of errors that need to be resolved and it'd be good to have a solution for that too. |
I do think we should do this, and if we can get this behavior in place I'd prefer to encourage the use of the That said, I agree with @MiniaczQ above that Bevy needs better error-handling from within systems in general, and generally feel like we should be using more descriptive |
Which I think should be a different mechanism. Anything that can be treated as querying the world should be treated as querying the world. |
|
@iiYese the reason |
Im thinking we should be building new APIs consistent with the current Bevy API pattern (panicking shortname with get_X for errors/results). Until we have solved the UX issues around error handling, I don't think we're ready to be hard-liners about Results-only apis. Those interested in changing that paradigm should invest in resolving the technical questions outlined here, then start mapping out what a holistic port looks like so we can do this all in one fell swoop. |
|
|
|
@maniwani has an excellent suggestion on the "allow systems to return errors" front:
From Discord |
|
@hymm suggested that the scheduler should treat systems as fallible by default, and convert systems that return |
|
Note that systems can fail externally (missing parameters) or internally (return |
|
I've had a crack at implementing the basic suggestions here: #16589. |
# 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 <[email protected]>
In spirit of #12660
Adapted from #14268
What problem does this solve or what need does it fill?
I believe it is not controversial to say that when an API offers two similar functions with similar names, the shorter will seem like the default. As such, I believe many people will instinctively gravitate to
singleandsingle_mutoverget_singleandget_single_mut. This means we are subtly pushing users to prefer the implicitly panicking version over the fallible one. This is bad, as it leads to games that may run well on the developers machine but then panic in an edge case.It is currently understood in the wider Rust community that panics should be an exceptional case, a nuclear option for when recovery is impossible.
We should do our best to make it as easy to do the right thing by default (See Falling Into The Pit of Success). This means that it should be easier to handle an error by propagating it with
?to a logger or similar than to panic. Our current API does the opposite.This is an issue for the following APIs:
Query::singleQuery::getWorld::resourceWorld::non_send_resource(although eventually this will likely be on App)What solution would you like?
Deprecate and remove panicking variants. Improve ergonomics by adding macros for early returns. Using a macro for
get_singleas an example:Which expands to:
Note that returning
default()will allow the macro to work with systems that return anOptionand getpiped into error handling methods.Similar macros are already being used by some users, as indicated by https://github.com/tbillington/bevy_best_practices?tab=readme-ov-file#getter-macros
Paraphrasing @alice-i-cecile:
What alternative(s) have you considered?
get_variants in documentation_unchecked#[system]macro that modifies the code to allow us to use?in it, like in bevy_mod_sysfailOpen Questions
Naming
There is a sentiment that
get_is used by the standard library in general when returning anOption.Quoting @benfrankel:
In a world where Bevy is not panicking by default and always hands out
Options andResults, I believe there is little reason to stick to aget_prefix. It is unnecessary noise on otherwise very concise functions. As such, I would advise to change Bevy's naming convention and drop it as part of this initiative. This may sound like something for another issue, but I'll argue that it should be discussed at least in the same release cycle. Users will already have to go and touch every instance ofmut,single, etc. so I don't want to further annoy them by having to change them yet a second time when we drop theget_prefix.I realize that this is more controversial however and I'm fine with leaving it. I just want to point out that this is probably the best chance we will get to change it.
Macro parameters
It would be nice to have some influence over what should be done in the unhappy path. For example, we could have a parameter that controls an
error!log:which expands to:
Of course, the use of
error!is arbitrary here and the user may want to usewarn!instead or something else altogether. To accommodate for this, we could pass a closure:Which expands to the same as above. Note that this closure could even dictate what we
return. Sinceerror!returns(), this works out for the common case, and a user is free to build their ownErrorvariant. However at that point, I don't know if a macro call is saving much typing work anymore.One could also combine these variants by using labeled parameters:
This would allow the user to have a terse version for the common case usage and a more verbose one for custom error handling.
continueThis discussion has focused on the case where we want an early return. I think that is the common case, at least in my experience. What about
continuehowever? Do we want a_continuevariant for all macros? An extra parameter? Is there some kind of Rust hack that turns into eithercontinueorreturndepending on context? Would that even be what the user expects to happen?The text was updated successfully, but these errors were encountered: