Added Fallible System Params

[JonahPlusPlus]

Objective

Due to orphan rules, it is not possible for downstream crates to implement SystemParam for Option<T> and Result<T, E>.

Solution

Create OptionalSystemParam and ResultfulSystemParam traits that provide blanket implementations of SystemParam for Option<T> and Result<T, E>.

It goes back to the *SystemParam traits design from #6854, rather than the generic SystemParam<T> in #6923, since the separate traits allows for ResultfulSystemParam to constrain the error type.

The SystemParam derive macro has been altered to have the struct and field attribute system_param, which can have the value of infallible, optional, resultful(ErrorType) (ErrorType is only on the struct-level), or ignore (on the field-level).

To handle OptionalSystemParams used in a ResultfulSystemParam, there is SystemParamError<T> which wraps the type, allowing for users to implement handlers.

Future Work

With this PR, implementing a fallible system param provides blanket implementations automatically. However, there may be unknown use cases where getting a blanket implementation is undesirable. If the need arises, it would be possible to require marker traits to apply the blanket implementations. I am not including them in this PR, because I believe they add extra boilerplate that only serves to take away control from users of the system param.

---

Changelog

• Added OptionalSystemParam
• Added ResultfulSystemParam
• Added SystemParamError

[JonahPlusPlus]

Example of resultful system param

use bevy::{
  ecs::system::*,
  prelude::*,
};

#[derive(Debug, Resource)]
struct Foo {
  foo: i32,
}

#[derive(Debug, Resource)]
struct Bar {
  bar: u32,
}

#[derive(SystemParam, Debug)]
#[system_param(resultful(MyParamErr))]
struct MyParam<'w> {
  #[system_param(optional)]
  foo: Res<'w, Foo>,
  #[system_param(optional)]
  bar: Res<'w, Bar>,
}

#[derive(Debug)]
enum MyParamErr {
  Foo,
  Bar,
}

impl std::fmt::Display for MyParamErr {
  fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
    let field = match self {
      MyParamErr::Foo => "Res<Foo>",
      MyParamErr::Bar => "Res<Bar>"
    };
    write!(f, "MyParam was missing {field}")
  }
}

impl std::error::Error for MyParamErr {}

impl From<SystemParamError<Res<'_, Foo>>> for MyParamErr {
  fn from(_: SystemParamError<Res<'_, Foo>>) -> Self {
    Self::Foo
  }
}

impl From<SystemParamError<Res<'_, Bar>>> for MyParamErr {
  fn from(_: SystemParamError<Res<'_, Bar>>) -> Self {
    Self::Bar
  }
}

fn main() {
  App::new()
    .insert_resource(Foo { foo: 653 })
    .insert_resource(Bar { bar: 534 })
    .add_system(foo)
    .run();
}

fn foo(foo: Result<MyParam, MyParamErr>) {
  match foo {
    Ok(o) => println!("{o:?}"),
    Err(e) => println!("{e}"),
  }
}

[JonahPlusPlus]

Still need to clean up the derive macro and fix some bugs with it (like it still requiring an error type in the field-level attribute).

[alice-i-cecile]

Handy! This is uncontroversial but fairly complex IMO. Ping me if I'm slow to review this once it's ready.

[JonahPlusPlus]

The only thing I'm still undecided about is whether SystemParamError should be named something shorter. I'm thinking about renaming it as bevy::utils::Ty, since there is nothing ECS specific about it; it's just a wrapper around the type.

Edit:Oh yeah, I forgot it had Error implemented...
Maybe it can be a general error type? Or maybe it is niche enough that verbosity doesn't matter...

[alice-i-cecile]

Yeah don't worry about verbosity here on a niche error type. Clarity is more important.

[JonahPlusPlus]

@alice-i-cecile Okay, I'll clean up the docs. It's still not ready though: I keep on having a bug with the derive macro with long system params (more than 16 fields), where it panics (I was trying to clean it up, but I clearly messed something up in the process). I was just fixing some merge issues with the hopes it might resolve itself.

[alice-i-cecile]

No worries, I just saw this PR and was in the mood to help out with some docs as I tried to understand what was going on a bit better <3

[JonahPlusPlus]

@alice-i-cecile Hey, I've added an example for optional system params, and I was hoping to get some feedback. I just wanted to make sure that my example isn't too niche or doesn't focus on the subject enough. The pattern it demonstrates is something I'm using in bevy_atmosphere that I felt could be useful for other libraries as well.

[alice-i-cecile]

Oh cool, I really like that example. More advanced, but it's an advanced feature and it's nice to show reasonable patterns.

Can you add a note in the example module level docs explaining that this is a relatively advanced usage, and that much of the time you can just use the derive macro?

[JonahPlusPlus]

Will do, thanks! :)

[JonahPlusPlus]

Okay, branch is now up-to-date. @maniwani Could you review this?

[Diddykonga]

Idea, Docs, and Code LGTM

[JonahPlusPlus]

Thank you @Diddykonga for your review. @alice-i-cecile I think this PR is ready to be merged, unless you want to take another look at it (there were only a few minor changes to make it up-to-date and some small documentation edits to clarify how the macro works).

[joseph-gio]

Sorry it's taken me so long to review this. I agree with the motivation, but I think this PR still needs work. Here's some points in no particular order.

• I don't love the name Resultful -- it doesn't really make sense as a word to me, and it clashes with system_param(infallible). FallibleSystemParam just makes more sense IMO.
• I don't think the derive macro is in a very good state right now. Not to be harsh, but most of the examples kind of look like attribute soup.
  • I think the best approach is to include derive macro support in a follow-up PR, where hopefully we can settle on a nice and usable design. For now, I think manual implementations are acceptable since optional and fallible params are sorta niche, and at the end of the day this feature just needs to let users bypass the orphan rule.
• This PR causes the panic messages for types like Res<T> to regress. Instead of including a specialized message with a suggestion for how to fix it, it just spits out a generic message for all optional system params.
• I'm still struggling to imagine a case where fallible system params would be useful. Optional system params are clearly useful, both within the engine and without (your optional_system_param.rs example very nicely illustrates a nontrivial usecase). But for fallible system params, I can't imagine a case where you would do anything except immediately unwrap or log the error case (this is also what your example does). And at that point, why not just do the error handling in SystemParam::get_param?
  • I would be a lot more comfortable with this PR if fallible system params were factored out to a different PR. Even if we do decide that they're worth adding, I don't think there's any need to bundle them together with optional system params.

[JonahPlusPlus]

@JoJoJet Thank you for taking the time to review this. In response to your points:

• I understand where you are coming from: the term Resultful is indeed a strange word to get used to, however it is a word, unlike alternatives like Resultable. I'm against Fallible, because it's not indicative of Result and Optional is also a level of fallibility. I could change the names to SystemParamOption and SystemParamResult, if you prefer those.
• I'm a little confused on what you mean by most of the examples looking like "attribute soup", since there are only two examples that use the macros, one using a single attribute and the other using two. The Resultful trait would be the biggest offender, since all library types use lower levels of fallibilty, which means all fields would have to be marked. I suppose the solution to this would be to add a struct-level attribute for overriding the default field-level attribute. Something like #[system_param(default_field = optional)].
• Good point. Edit: I took another look and realized that what I was thinking wouldn't work without specialization. The solution would be to make the blanket implementations opt-in (by requiring marker traits DefaultOptional and DefaultInfallible) and make custom implementations that provide custom message. The alternative (and what I prefer) is to make the default messages better so that they cover all the information provided before the regression.
• The benefit of Resultful system params is in cases where one needs to know what caused a failure. Could this be done today without Resultful system params? Of course; all the parts in the system param can be used directly from a system. However, this is the case for most third-party system params. The reason for writing it as a system param is for convenience/reusability. In my example, the Resultful system param is being used print the average score, but in a game the score would be displayed in a UI, so putting this behavior directly inside of the system param would be bad idea (since you probably want to display other things inside the UI from your system).
  • I think it was good to work on them together, since the addition of ResultfulSystemParam prevented some other alternative designs.

[joseph-gio]

• Good point. The solution here would be to convert the system params to use Resultful, then give them an error type that can hold the error message/relevant details and in the implementation of SystemParam for ResultfulSystemParam, print that message. Some other things to do would be to improve the default error messages by saying what systems the originate in and provide overrides in the macro for custom messages.

That would be better. How about we take it a step further, and combine OptionalSystemParam and ResultfulSystemParam into one trait? I can't imagine a case in which you'd want an optional system param that doesn't have a custom panic message.

Currently, the traits are defined like this:

pub trait OptionalSystemParam { ... }

impl<T: OptionalSystemParam> SystemParam for T { ... }

pub trait ResultfulSystemParam { ... }

impl<T: ResultfulSystemParam> OptionalSystemParam for T { ... }

This means that every resultful param is also an optional param, is also a regular param. I suggest we just cut out OptionalSystemParam entirely, and make Option<T> work with T: ResultfulSystemParam. Also with this refactor, it would make even more sense to use FallibleSystemParam as a name, since this would encompass both Option and Result params.

[joseph-gio]

If I understand this correctly, this means that if a custom SystemParam has a field with incorrect fallibility, it will panic at runtime instead of failing at compile time. I don't think this is an acceptable failure mode.

[JonahPlusPlus]

Here is an example of that:

#[derive(SystemParam)] // By default, it's infallible, which means it must return or panic
struct MyParam<'w> {
  #[system_param(optional)]
  foo: Res<'w, SomeResource>,
}

What it is doing is getting the param as Option<Res<SomeResource>> instead of Res<SomeResource> and unwrapping it. The only difference from not getting it as Res<SomeResource> is that if it panics, it will panic here instead of from inside the SystemParam implementation for Res<SomeResource>.

Essentially, you would never want to write this, because it's pointless. However, I kept it in for consistency and for usability in macros (it makes it easier to write macros when there are less edge cases to handle). Otherwise, there aren't any downsides.

[joseph-gio]

Hmm, I see. That's a lot better than what I thought it was doing, but I'd still prefer we just have a compiler error. If there's never any reason to write this, then I don't see any point in supporting it. If we want consistency, then there should be only one right way of doing this IMO.

[joseph-gio]

This can also go back to using parse_quote! I believe.

[JonahPlusPlus]

@JoJoJet That could work (it would solve the problem of ResultfulSystemParam not being able to implement SystemParam for T and Option<T> directly due to conflicting implementation with OptionalSystemParam), here is a MVP of what it would look like: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0072fff233317e2bdba60da806e2dfd2

The downside would be that you would be forced to have an error type and return Result<T, E>, but maybe this isn't a big deal.

Otherwise, I could just change the panic message to include the system name and type.

[joseph-gio]

Yes, that's what I was picturing :). That would also cut out on a lot of boilerplate, since now there's only two near-identical traits instead of three.

The downside would be that you would be forced to have an error type and return Result<T, E>

True, but you could always use &'static str for a "quick and dirty" error type. This way, you're encouraged to write better code since you have to provide some kind of custom error message (unless the user just uses () for the error, but if someone really wants to make things harder for themselves, that's up to them).

[JonahPlusPlus]

Okay, I guess I'll spin up another PR with those changes. I'll leave this one up just in case it turns out to have unexpected issues.

[joseph-gio]

Otherwise, I could just change the panic message to include the system name and type.

Also: I think this is a good idea either way.

[bas-ie]

Backlog cleanup: closing due to inactivity, and in favour of its successor #8196.