FallibleSystemParam

[JonahPlusPlus]

Objective

Currently, there is no way to implement SystemParam for Option<T> and Result<T, E> due to orphan rules.

Solution

This PR introduces FallibleSystemParam, which, when implemented, gives blanket implementations of SystemParam for Option<T> and Result<T, E>.

Compare this to #7162, which introduces OptionalSystemParam and ResultfulSystemParam. Here, the design is far simpler and using a type as just T gives a custom panic message. The downside is that there is less control (you have to provide an error type, you can't just implement Option<T>), however, this may be desirable since it keeps code more uniform.

[JonahPlusPlus]

@JoJoJet Here is a draft. I'll be working on docs later.

[joseph-gio]

This is definitely my favorite iteration of fallible system params -- I like how much lower the impact of this PR is, compared to the previous ones. I've scanned through it once, though I'll review it fully later.

All the iteration we've gone through in these PRs tells me that there's a lot of considerations at play here, and that there isn't really an "obviously right" way of doing this. I'm marking this PR as controversial, to make sure we get multiple people signing off on this as a direction.

[JonahPlusPlus]

@JoJoJet Removed the generic error type in favor of individual error types. ResError<T> is a lot easier to write out than MissingSystemParam<Res<'_, T>> and each type can get it's own custom message.

Also added some docs. I'll add an example later on.

Another thing worth considering is whether parts common to both SystemParam and FallibleSystemParam should be placed inside a central macro, making it easier to edit both at once.

[joseph-gio]

Not sure how well this would work in practice, but I have another idea for code reuse. Since every FallibleSystemParam is also a SystemParam, couldn't we just do

pub trait FallibleSystemParam: SystemParam {
    type Error;
    /// # Safety
    /// This can only be called if it would be safe to call `<Self as SystemParam>::get_param`.
    unsafe fn try_get_param<'w, 's>(...) -> Result<Self::Item<'w, 's>, Self::Error>;
}

Doing it in this way prevents duplication of the init_state, new_archetype, and apply functions, as well as their documentation. It also means FallibleSystemParam doesn't have to be unsafe, since those safety invariants are already guaranteed by its supertrait.

This would make it slightly more verbose for implementers, since they would have to implement both the get_param and try_get_param methods, but I think that's probably worth it. Also, individual implementers should be able to just implement get_param in terms of try_get_param.

[joseph-gio]

Since this is an example, I think this safety comment could be a lot more verbose. I think it would be good to explain what the safety invariants are and what they mean, and then to explain how we know the invariants are satisfied.

[JonahPlusPlus]

I think the issue with making FallibleSystemParam depend upon SystemParam is that it's the opposite way around: when FallibleSystemParam is implemented, SystemParam depends upon it via the blanket implementations. By forcing users to implement SystemParam, they lose the benefit of a blanket implementation that uses the error type to give a custom message.

New idea: there were issues with making SystemParam generic in a previous iteration due to the inclusion of Result<T, E> (trying to constrain the error type). However, now that OptionalSystemParam and ResultfulSystemParam have been merged, it may be possible to make SystemParam generic for two marker types Infallible and Fallible.

Edit: Nevermind, it still has issues when constraining the error type

[joseph-gio]

I think the issue with making FallibleSystemParam depend upon SystemParam is that it's the opposite way around: when FallibleSystemParam is implemented, SystemParam depends upon it via the blanket implementations.

Right, my suggestion is that we invert the relationship since it allows us to simplify the API.

By forcing users to implement SystemParam, they lose the benefit of a blanket implementation that uses the error type to give a custom message.

It's a bit unfortunate, but it just makes it slightly more annoying for manual implementers of SystemParam which I think is fine. As a positive, it means that FallibleSystemParam::Error no longer needs any trait bounds for formatting.

This is a pretty niche feature, I think that simplifying the model in any way we can is desirable.

[JonahPlusPlus]

Hmm, I think there is a misunderstanding between us.

I see it as the opposite: by requiring SystemParam for FallibleSystemParam, it would be necessary to remove the blanket implementation of SystemParam for T: FallibleSystemParam, so that users can implement SystemParam for T themselves and specify things like init_state, new_archetype, etc. This makes it harder for implementers of FallibleSystemParam (since they would need to implement get_param manually), not SystemParam, which I assume will be untouched.

While it may be simplifying Bevy's code, I think this make things more complex for end users. I think the best choice for reducing the code inside Bevy is to simply wrap common behavior of the traits in a macro. Alternatively, maybe there could be a SystemParamBase trait that encapsulates behavior common to both.

Also, I think there is a benefit to putting trait bounds on Error. It requires error types to implement basic traits, which makes it easier for users to combine error types later on. In the future, if it seems that these bounds are problematic, we can remove them, but I would like to keep them in for now.

[joseph-gio]

Manual implementers of FallibleSystemParam are a very niche demographic -- I don't think much is lost if they have to implement one more function.

Alternatively, maybe there could be a SystemParamBase trait that encapsulates behavior common to both.

That's an option, but consider: every FallibleSystemParam is also a SystemParam, but every SystemParam is not a FallibleSystemParam. It makes sense to me for SystemParam to be that base trait.

[JonahPlusPlus]

Hmm, I still think a macro would be the best choice. I don't think I've heard your objections to that? My concern is that we would be sacrificing (a small amount, but still an amount of) usability just to reduce internal code size. If there was some benefit to the end user, I would be more receptive, but I don't see any. There was so much work done to combine the SystemParam traits (thank you, btw), that it feels like a regression to go back in that direction of split traits (by that I mean multiple traits needed to implement a system param).

By putting common stuff in a macro, the code size can be reduced and becomes manageable and it keeps usability.

[JonahPlusPlus]

@JoJoJet Wrapped that stuff in a macro so you can get a sense of that change.

[joseph-gio]

Seems to work well enough. You'll need to fix the doc links though, which refer to SystemParam even for the methods defined on FallibleSystemParam.

Also, I have a complaint about the derive macro.

If I write this, then each field gets treated like a normal SystemParam, as expected:

#[derive(SystemParam)]
struct MyParam {
    id: Entity,
    val: Res<Value>,
}

As soon as I specify an error type though, it has an implicit context switch where each field is treated as a fallible param. Fields that aren't fallible have to be explicitly marked as such:

#[derive(SystemParam)]
#[system_param(error_type = ResError<Value>]
struct MyParam {
    #[system_param(infallible)]
    id: Entity,
    val: Res<Value>,
}

---

I would prefer it if we remove the context switch entirely -- for consistency, the SystemParam macro should always treat its fields as infallible unless specified otherwise for that field. So a derived fallible param could look something like:

#[derive(SystemParam)]
#[system_param(error_type = ResError<Value>]
struct MyParam {
    id: Entity,
    #[system_param(try)]
    val: Res<Value>,
}

I prefer this since it:

1. Makes the control flow explicit. The way that fallible params are treated is essentially using the ? operator, so I think it'd make sense if the annotations reflect that.
2. Simplified dealing with the error type. This way, the only fields that have to match the error type are those that are explicitly annotated with an attribute.

[joseph-gio]

Also, I want to note that the #[system_param(error_type)] attribute is currently broken. It only accepts a single identifier, so it fails if you try to use it with a type like my_path::Error, or even if you use generics. You'd have to make the attribute parse a type instead of just an identifier.

[JonahPlusPlus]

I think the issue of opting in to fallibility instead of opting out is that if you use something that isn't fallible and forget to opt-out, you get a compile error, while if you use something that is fallible and forget to opt-in, you get a runtime error. While it would be nice to show the control flow, I think keeping the way it is prevents unintended panics.

[joseph-gio]

Ah true, that's an unfortunate complication.

[JonahPlusPlus]

Got around to fixing those issues (the doc links and error type parsing).

[ItsDoot]

This has been superceded by the recently merged #18766.