feat(ecs): add Assume and Unpack traits for Result conversion

[JeanMertz]

Adds the following traits, and implements them for Option:

/// Unpack `Self<T>` to `T`, otherwise return [`Unpack::Error`].
///
/// This can be a drop-in replacement for `unwrap`, combined with the question mark operator and
/// [`Result`](super::Result) return type, to get the same ergonomics as `unwrap` but without the
/// panicking behavior (when using a non-panicking error handler).
pub trait Unpack<T> {
    /// The error type returned by [`Unpack::unpack`].
    ///
    /// Typically implements the [`Error`] trait, allowing it to match Bevy's fallible system
    /// [`Result`](super::Result) return type.
    type Error;

    /// Convert `Self<T>` to a `Result<T, Self::Error>`.
    fn unpack(self) -> Result<T, Self::Error>;
}

/// Assume that `Self<T>` is `T`, otherwise return the provided error.
///
/// This can be a drop-in replacement for `expect`, combined with the question mark operator and
/// [`Result`](super::Result) return type, to get the same ergonomics as `expect` but without the
/// panicking behavior (when using a non-panicking error handler).
pub trait Assume<T> {
    /// The error type returned by [`Assume::assume`].
    ///
    /// Typically implements the [`Error`] trait, allowing it to match Bevy's fallible system
    /// [`Result`](super::Result) return type.
    type Error;

    /// Convert `Self<T>` to a `Result<T, Self::Error>`.
    fn assume<E: Into<Self::Error>>(self, err: E) -> Result<T, Self::Error>;
}

This allows for a more concise and readable way of handling Option values in Bevy systems that return Results, by applying the question mark operator:

fn my_system(world: &World) -> bevy::ecs::Result {
    world.get::<MyComponent>(Entity::PLACEHOLDER).unpack()?;

    world.get_resource::<MyResource>().assume("resource exists")?;

    Ok(())
}

In future commits, more of Bevy's API will be converted to return Results directly, but there will always be (non-)Bevy methods that return Option values, so this extension trait will have its place, regardless.

This was suggested by @cart here: #14275 (comment)

[mockersf]

c) we come up with a better place to combine all of this.

I think the better place for this trait at least is Rust std. Could you try opening the discussion there?

[JeanMertz]

I think the better place for this trait at least is Rust std. Could you try opening the discussion there?

I’m happy to, but I don’t suppose you propose waiting for that resolution? Any suggestion where it should live until/when it lands in Rust std?

[mockersf]

I’m happy to, but I don’t suppose you propose waiting for that resolution?

Yes I do? If it happens fast, it could be in rust 1.86 which may happen before (or around) the next version of Bevy

[JeanMertz]

I’m happy to, but I don’t suppose you propose waiting for that resolution?

Yes I do? If it happens fast, it could be in rust 1.86 which may happen before (or around) the next version of Bevy

I see. I would propose that both can be done in parallel; add the convenience methods in Bevy to help with fallible system adoption, and concurrently propose upstreaming the methods. If they are upstreamed, we can remove the custom impl here, if it's not upstreamed, then we don't need to change anything on our side.

Regardless, I'll find some time next week to propose the methods upstream.

[mockersf]

I see. I would propose that both can be done in parallel; add the convenience methods in Bevy to help with fallible system adoption, and concurrently propose upstreaming the methods. If they are upstreamed, we can remove the custom impl here, if it's not upstreamed, then we don't need to change anything on our side.

I would prefer to wait, this is not that much better that it's worth changing then reverting

[alice-i-cecile]

I like the end result, but I'm not super keen on shipping this in Bevy itself. Extension traits on super common types like Option are likely to badly confuse beginners. Would enthusiastically support using this if it was in the std though!

[JeanMertz]

I haven’t looked too deep yet, but NoneError existed in Rust before, but was removed and there have apparently been many lengthy discussions on it. So I would not hold my breath on that landing any time soon. I’ll dig a bit deeper on Monday and see if opening a PR is worth the effort or not.

[JeanMertz]

Alright, I looked into it more, I really don't see any chance of this getting upstreamed.

For fn assume<E: Into<Box<dyn Error>>>(self, err: E) -> Result<T, Box<dyn Error>>;, this is close to Rust's ok_or, except that there E is generic, whereas we explicitly take anything that can be converted into the specific error type returned by our fallible systems. Now, the question mark operator ? already applies From::from to the error value, but there are still cases where you have to append a map_err(Into::into) to make it work. So you go from:

optional.assume(SomeError)?

to (worst case):

optional.ok_or(SomeError).map_err(Into::into)?

(I should note that optional.assume(SomeError) doesn't read correctly, while optional.assume("should exist") does, as does optional.ok_or(SomeError), but that's a different discussion)

While I could make a case for assume to be valuable (given that our Error type is just a type alias for Box<dyn Error>), it's also the case that std::result::Result<T, E> doesn't make any assumptions about what type E is, in fact there is no method in there that expects E: Error, as far as I can tell. That is what makes assume more convenient in our case, because it expects its input to be convertible to our system error return type.

For fn unpack(self) -> Result<T, NoneError>;, there are many discussions to be found on the topic. But here's basically the most succinct explanation on why NoneError was removed from Rust. Their reasoning for removal is sound, but it does mean our fallible systems are a little bit less ergonomical without this extension trait. I don't feel it's worth it for me to re-open that discussion, nor do I want to waste people's time re-hashing resolved discussions.

---

Having said all that. I'm happy to close this and use the extension trait in my own projects. I added this because it was proposed in #14275 (comment), but if we think the gain in convenience is too small for the cost of people having to learn non-std-Rust methods, then I can see that reasoning as well.

[alice-i-cecile]

We'll leave this open and let @cart decide when he gets around to it :) This is just a matter of taste IMO.

[cart]

I think this is a good move, with some minor changes.

[cart]

I also think the odds of these being included in std are slim. And assume, once Bevy's error type isn't just a boxed core Error, couldn't possibly live in std. And even if unpack were to land in std, it would likely be closer to the "years" timeframe than the "months" timeframe.

[alice-i-cecile]

Agreed with Cart's feedback, and I'm fine to follow his direction on this. I agree with his / your assumption that upstreaming will be hard.

[JeanMertz]

This PR has been rebased on main and all feedback has been resolved.

[alice-i-cecile]

These docs will not be very helpful on mouse-over tooltips.

[alice-i-cecile]

A round of documentation hardening for you :) New users are going to see these traits / methods pretty prominently, so I want to make sure that they're approachable and have the context needed to defend the design.