BevyError: Bevy's new catch-all error type

[cart]

Objective

Fixes #18092

Bevy's current error type is a simple type alias for Box<dyn Error + Send + Sync + 'static>. This largely works as a catch-all error, but it is missing a critical feature: the ability to capture a backtrace at the point that the error occurs. The best way to do this is anyhow-style error handling: a new error type that takes advantage of the fact that the ? From conversion happens "inline" to capture the backtrace at the point of the error.

Solution

This PR adds a new BevyError type (replacing our old std::error::Error type alias), which uses the "from conversion backtrace capture" approach:

fn oh_no() -> Result<(), BevyError> {
    // this fails with Rust's built in ParseIntError, which
    // is converted into the catch-all BevyError type
    let number: usize = "hi".parse()?;
    println!("parsed {number}");
    Ok(())
}

This also updates our exported Result type alias to default to BevyError, meaning you can write this instead:

fn oh_no() -> Result {
    let number: usize = "hi".parse()?;
    println!("parsed {number}");
    Ok(())
}

When a BevyError is encountered in a system, it will use Bevy's default system error handler (which panics by default). BevyError does custom "backtrace filtering" by default, meaning we can cut out the massive amount of "rust internals", "async executor internals", and "bevy system scheduler internals" that show up in backtraces. It also trims out the first generally-unnecssary From conversion backtrace lines that make it harder to locate the real error location. The result is a blissfully simple backtrace by default:

The full backtrace can be shown by setting the BEVY_BACKTRACE=full environment variable. Non-BevyError panics still use the default Rust backtrace behavior.

One issue that prevented the truly noise-free backtrace during panics that you see above is that Rust's default panic handler will print the unfiltered (and largely unhelpful real-panic-point) backtrace by default, in addition to our filtered BevyError backtrace (with the helpful backtrace origin) that we capture and print. To resolve this, I have extended Bevy's existing PanicHandlerPlugin to wrap the default panic handler. If we panic from the result of a BevyError, we will skip the default "print full backtrace" panic handler. This behavior can be enabled and disabled using the new error_panic_hook cargo feature in bevy_app (which is enabled by default).

One downside to not using Box<dyn Error> directly is that we can no longer take advantage of the built-in Into impl for strings to errors. To resolve this, I have added the following:

// Before
Err("some error")?

// After
Err(BevyError::message("some error"))?

We can discuss adding shorthand methods or macros for this (similar to anyhow's anyhow!("some error") macro), but I'd prefer to discuss that later.

I have also added the following extension method:

// Before
some_option.ok_or("some error")?;

// After
some_option.ok_or_message("some error")?;

I've also moved all of our existing error infrastructure from bevy_ecs::result to bevy_ecs::error, as I think that is the better home for it

Why not anyhow (or eyre)?

The biggest reason is that anyhow needs to be a "generically useful error type", whereas Bevy is a much narrower scope. By using our own error, we can be significantly more opinionated. For example, anyhow doesn't do the extensive (and invasive) backtrace filtering that BevyError does because it can't operate on Bevy-specific context, and needs to be generically useful.

Bevy also has a lot of operational context (ex: system info) that could be useful to attach to errors. If we have control over the error type, we can add whatever context we want to in a structured way. This could be increasingly useful as we add more visual / interactive error handling tools and editor integrations.

Additionally, the core approach used is simple and requires almost no code. anyhow clocks in at ~2500 lines of code, but the impl here uses 160. We are able to boil this down to exactly what we need, and by doing so we improve our compile times and the understandability of our code.

[BenjaminBrienen]

I would prefer bevy::Error over bevy::BevyError to avoid namespace stuttering. Users can always use bevy::Error as BevyError if they have multiple error types going on in one file. I don't feel this strongly, so feel free to ignore me.

[alice-i-cecile]

I would prefer bevy::Error over bevy::BevyError to avoid namespace stuttering. Users can always use bevy::Error as BevyError if they have multiple error types going on in one file. I don't feel this strongly, so feel free to ignore me.

I don't like shadowing core types / traits like this, even though I know it's a common practice. It doesn't work well with glob imports (like from a prelude), it's harder to review in PRs / skim at a glance, and they're much harder for beginners to google.

[BenjaminBrienen]

Good reasons! Web-searchability is useful and being a core type is quite a special status.

[NthTensor]

I don't like shadowing core types / traits like this, even though I know it's a common practice.

Worth noting we are already doing this with our specialized Result alias. It won't conflict with the default Result because it's just adding defaults, but it could conflicts with other aliases, such as the one from anyhow.

[hukasu]

will BevyError be only used on bevy_ecs? wouldn't it be better to have it at bevy_error?

[cart]

will BevyError be only used on bevy_ecs? wouldn't it be better to have it at bevy_error?

We can consider breaking it out, but it increases the difficulty of consuming it, and there isn't really much of a use for it outside of the ECS context. I'd prefer to defer this until there is a real use case that justifies the added consumption cost.

[hymm]

I think it makes sense for bevy to have it's own error type and the code changes are straightforward.

[hymm]

could we add a test for the string filtering? It feels like it'd be pretty fragile to code changes and rust changes.

[cart]

Writing this test in a way that is actually useful involves setting the RUST_BACKTRACE env variable from the test, which is unsafe to do as it could run in parallel. I'm cool with just accepting that though

[chescock]

One downside to not using Box<dyn Error> directly is that we can no longer take advantage of the built-in Into impl for strings to errors.

Would impl From<&str> for BevyError make that work again? Would it make sense to have that impl?

[alice-i-cecile]

I think we should try out @chescock's idea, but once that's explored I have no objections to merging this. Tests would always be nice, but I don't think they're essential.

[hukasu]

I want to try and make an example using error-stack just to have a point of comparison, no commitment

[cart]

Would impl From<&str> for BevyError make that work again? Would it make sense to have that impl?

This conflicts with the blanket From<E: Error> impl, which we need.

[chescock]

This conflicts with the blanket From<E: Error> impl, which we need.

But then how does it work for Box<dyn Error>??? ... Oh, I see, std is cheating by using a negative impl

// This is required to make `impl From<&str> for Box<dyn Error>` and `impl<E> From<E> for Box<dyn Error>` not overlap.
#[stable(feature = "error_in_core_neg_impl", since = "1.65.0")]
impl !crate::error::Error for &str {}

I think impl<E> From<E> for BevyError where Box<dyn Error>: From<E> is legal, but then you wouldn't be able to put the backtrace inside the box. Makes sense!

[fjkorf]

This is a great PR. Understandable with clear benefits. The code is clear and functional.

[killercup]

I like the direction of this! As a potential bonus feature: I have seen other panic handlers/error types (e.g. color-eyre's) that also capture spans from tracing. Would this be useful for bevy as well?

[cart]

As a potential bonus feature: I have seen other panic handlers/error types (e.g. color-eyre's) that also capture spans from tracing. Would this be useful for bevy as well?

This sounds useful to me! Worth exploring in a followup.

[NthTensor]

Looks great. This rounds out the result handling feature nicely.