Fallible System Parameters

[JonahPlusPlus]

Objective

There is no way to implement SystemParam for Option<T> and Result<T, E> in downstream crates because of orphan rules.

Solution

Make SystemParam generic for T: Fallibility. The sealed trait Fallibility describes the return value of SystemParamState::get_param via the generic associated type Scope<T>. Users can then use the marker structs Infallible, Optional, and Resultful, which define Scope<T> to be T, Option<T> and Result<T, &dyn Error>.

&dyn Error was chosen because there was no way to constrain the type parameter E (this would require higher-ranked types).

The point of doing it this way instead of creating OptionalSystemParam and ResultfulSystemParam traits is that there would otherwise be significant overlap between traits as SystemParamFetch and SystemParamState get merged into SystemParam.

---

Changelog

• Changed SystemParam to SystemParam<T: Fallibility>.
• Changed SystemParamState to SystemParamState<T: Fallibility>.
• Changed instances of SystemParam to SystemParam<Infallible> or SystemParam<Optional>, when appropriate.
• Changed instances of SystemParamState to SystemParamState<Infallible> or SystemParamState<Optional>, when appropriate.
• Added sealed Fallibility trait.
• Added Infallible, Optional, and Resultful markers.

Migration Guide

• Instances of SystemParam and SystemParamState have to be changed to their respective generic type.

[JonahPlusPlus]

Pros:

• Greatly improves the DRY-ness of code.
• Makes it easy to implement system params with different levels of fallibility.

Cons:

• Sometimes it's necessary to specify which SystemParam. (Also would have been an issue with specialized traits)
• Since Rust doesn't have negative bounds to disable blanket implementations, it's necessary to implement SystemParam<Infallible> through SystemParam<Optional> for SystemParam<Resultful>, so there is a small conversion when using Result<T> as T, though the effect on performance should be negligible.

[JonahPlusPlus]

Unresolved Questions:

• Should the SystemParam derive macro have an attribute for fallibility? Ex:

#[derive(SystemParam)]
#[fallibility(Optional)]
struct MyParam {
  ...
}

• Should there be a type alias for Result<T, &dyn Error>?
• Should Scope have another name?

Edit: All my questions were answered below.

[JonahPlusPlus]

Here is a prototype of this system, which shows the core ideas behind it.

[JonahPlusPlus]

Okay, this is ready to be reviewed and merged.

[JonahPlusPlus]

@alice-i-cecile Would you like to review this? (I'm not sure on @JoJoJet's progress on his PR, but I would like to get this merged so he can freely rebase)

[JonahPlusPlus]

Okay, I've changed it so Box<dyn Error> is now &dyn Error, where the lifetime is 'state.

[alice-i-cecile]

Definitely on board with the goals here: this abstracts a common and important pattern and allows third-party crates to get in on the fun.

I like the use of a generic with a sealed trait bound to me: I think that's a good way to implement this and IMO the overall comprehensibility of this code is improved. I particularly like how much of the unsafe code is clearer and simpler.

#6919 will make this code much clearer: I'd like to merge that ASAP regardless of what we do here.

Some nits to clean up before merging though: see comments.

[alice-i-cecile]

Should the SystemParam derive macro have an attribute for fallibility? Ex:

Yes

Should there be a type alias for Result<T, &dyn Error>?

No: this is clear and uses common idioms.

Should Scope have another name?

Yes, or a good doc comment. It's hard to understand what it's doing here.

[alice-i-cecile]

The derive macro would be nice, but I won't block on it. If it doesn't get done in this PR please open an issue!

[JonahPlusPlus]

Okay, I got the macro done and modified the documentation.

[joseph-gio]

I'm on board with the optional SystemParam changes -- it's clearly useful, and necessary for allowing users to bypass the orphan rule (I liked the state that #6854 was in). However, I am very skeptical as to the how useful Resultful params are.

Here are some of my thoughts

1. &dyn Error has the same problem as Box<dyn Error>, in that you can't do much with it except for log or unwrap it.
   • Put another way: the only reason you would want Result as a SystemParam is if you can actually handle the error case. It's not really possible to meaningfully handle a dyn Error, so it doesn't make sense to use it as a SystemParam.
2. Results are usually returned or bubbled up from a function. Passing a Result as an argument looks very questionable -- usually, errors should be handled by the caller instead of passed into a function (in this case, the 'caller' would be the SystemParam impl for that type).
3. Having &dyn Error as the error type feels quite magical and non-obvious.

Feel free to wait for more opinions, but I don't think Resultful params should be included.

[JonahPlusPlus]

I think having Resultful in this state is better than nothing. The error-handling with it is not very ergonomic, but it allows for complex error-handling, which is something users couldn't do before. So I would rather have Resultful like this and give users the opportunity to decide for themselves whether using it is right for their use case.

In defense of Result<T, &dyn Error>, I have two major points:

1. dyn Error can be downcast to a concrete type, which then can be used to handle the error case.
2. Easily derivable: fields that implement SystemParam<Resultful> can be easily handled; there is no need to create an new error enum to store them.

[JonahPlusPlus]

Here is a small program demonstrating Resultful:

use bevy::{
    ecs::system::{
        Infallible, OptionState, Optional, ResState, Resultful, SystemMeta, SystemParam,
        SystemParamState,
    },
    prelude::*,
};
use std::error::Error;

#[derive(Debug, Resource)]
struct SomeResource {
    value: u32,
}

#[derive(Debug)]
struct MyParam<'w> {
    value: Res<'w, SomeResource>,
}

impl<'w> SystemParam<Resultful> for MyParam<'w> {
    type State = MyParamState;
}

struct MyParamState {
    res_state: ResState<SomeResource>,
}

unsafe impl SystemParamState<Resultful> for MyParamState {
    fn init(world: &mut World, system_meta: &mut bevy::ecs::system::SystemMeta) -> Self {
        Self {
            res_state: <ResState<SomeResource> as SystemParamState<Optional>>::init(
                world,
                system_meta,
            ),
        }
    }

    type Item<'w, 's> = MyParam<'w>;

    #[inline]
    unsafe fn get_param<'w, 's>(
        state: &'s mut Self,
        system_meta: &SystemMeta,
        world: &'w World,
        change_tick: u32,
    ) -> Result<Self::Item<'w, 's>, &'s dyn Error> {
        // Since `Res<T>` doesn't implement `SystemParam<Resultful>`, we have to use `Option<Res<T>>` and map it to a result.
        let some_res = <<Res<SomeResource> as SystemParam<Optional>>::State as SystemParamState<
            Optional,
        >>::get_param(&mut state.res_state, system_meta, world, change_tick);

        match some_res {
            Some(r) => Ok(MyParam { value: r }),
            None => Err(&MyError(45)),
        }
    }
}

#[derive(Debug)]
struct MyError(u32);

impl std::fmt::Display for MyError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "MyError has {}!", self.0)
    }
}

impl Error for MyError {}

#[derive(SystemParam, Debug)]
#[fallibility(Resultful)]
struct MySpecialParam<'w> {
    param: MyParam<'w>,
}

fn main() {
    App::new()
        .insert_resource(SomeResource { value: 653 })
        .add_system(foo)
        .run();
}

fn foo(foo: Option<MySpecialParam>) {
    println!("{foo:?}");
}

[joseph-gio]

The error-handling with it is not very ergonomic, but it allows for complex error-handling, which is something users couldn't do before

You can do complex error handling already, you just can't use std's Result type. Defining your own result-ish type might be annoying, but IMO it's much better than downcasting -- strong typing is preferred.

Also, using &'s dyn Error as the type is incredibly restrictive -- it means that you can only return an error that is stored in the param's state. Err(&MyError(45)) only works in your example due to static promotion -- it fails if you swap out 45 with a runtime value.

Easily derivable: fields that implement SystemParam<Resultful> can be easily handled; there is no need to create an new error enum to store them.

We shouldn't erase type information for the sake of making the derive macro easier to write.

Also, the derive macro is too restrictive to be very useful right now. Resultful params can only contain other resultful params, which means a simple type like this cannot be derived:

#[derive(SystemParam)]
#[fallibility(Resultful)]
struct Ouch<'w, 's> {
    r: Res<'w, R>, // error[E0277]: the trait bound `Res<'__w, R>: SystemParam<Resultful>` is not satisfied
    fallible: MyFallibleParam<'w, 's>,
}

...I hope I'm not coming across as combative, I'm just trying to help give feedback :)

[JonahPlusPlus]

You can do complex error handling already, you just can't use std's Result type. Defining your own result-ish type might be annoying, but IMO it's much better than downcasting -- strong typing is preferred.

Ah, that's right. That's not really nice either IMO, but I guess it works.
If users want to create their own Result-like type, this doesn't stop them.

using &'s dyn Error as the type is incredibly restrictive -- it means that you can only return an error that is stored in the param's state.

Hmm, good point. I'll look into this.

We shouldn't erase type information for the sake of making the derive macro easier to write.

No, I was just making the point that it isn't entirely bad.

the derive macro is too restrictive to be very useful right now. Resultful params can only contain other resultful params

Yeah, I guess I can add a field level attribute to override how it's handled.

I hope I'm not coming across as combative, I'm just trying to help give feedback

Of course, thanks. I agree, &dyn Error isn't ideal, but until there is a way to constrain the error type, this is the best solution I can find for this type of system. So until then, I think we should at least support this option.

[JonahPlusPlus]

Now, say we were able to use Result<T, E>, then deriving would require users to define From for E for all error types in the fields.
(Oh, this might sound like I am making a case for &dyn Error, but I am trying to show how Resultful can be fixed in the future)

[JonahPlusPlus]

Hmm, so I thought using a generic trait would be a good idea to keep the code base DRY, but it makes implementing Result<T, E> difficult. The options I found are: keep Result<T, &dyn Error>, add another trait to constrain E, or remove the generic trait in favor of separate traits. None of these solutions are ideal, but the last is the best for users, so I'm closing this PR and will start another one.