Support StaticSystemInput in more places

[chescock]

Objective

Let functions that take StaticSystemInput<I> be used in APIs that require IntoSystem<I, O, M>.

Higher-order systems that are generic in their input type need to wrap their input in StaticSystemInput. Unfortunately, that means they are not usable with World::run_system_cached or Schedule::add_systems, since those require In = () and not In = StaticSystemInput<()>.

Solution

Add an associated type SystemInput::Underlying that is equal to Self (up to lifetimes) for all input types other than StaticSystemInput<I>, and equal to I::Underlying for StaticSystemInput<I>. Use that associated type for <FunctionSystem as System>::In.

That means a function taking StaticSystemInput<()> will be turned into a system with In = () instead of In = StaticSystemInput<()>, making it usable for World::run_system_cached and Schedule::add_systems.

Testing

Updated the doc example of a safe PipeSystem to call the resulting system with World::run_system_cached instead of needing to call System::run explicitly.

Added unit tests passing a function taking StaticSystemInput<()> to World::run_system_cached and Schedule::add_systems.

[cBournhonesque]

Would this work instead?
https://github.com/cBournhonesque/bevy/pull/4/files
It seems easier to follow to me than the adding an Underlying associated type

[chescock]

Would this work instead? https://github.com/cBournhonesque/bevy/pull/4/files It seems easier to follow to me than the adding an Underlying associated type

Yeah, that would make run_system_cached work!

We'd need to make the same change in a lot of places to make it work seamlessly everywhere, though, including IntoScheduleConfigs, IntoObserverSystem, run_if, and (un)register_system_cached. And it would be easy to forget it we added a new place. Doing it centrally like this means we know it works everywhere, and makes the method signatures simpler.

[cBournhonesque]

I do think the approach of changing the call sites is better, but I won't block on this. I think this is a useful change!

[ItsDoot]

Looks good, only thing I'd add is a test for a tuple of StaticSystemInputs and one for a mixed tuple of StaticSystemInput + a normal one.

[hymm]

I haven't done a full review yet, but if we go with this approach I'd like to bikeshed Underlying I don't think it's very descriptive to what it actually is.

[chescock]

I haven't done a full review yet, but if we go with this approach I'd like to bikeshed Underlying I don't think it's very descriptive to what it actually is.

Yeah, if anyone has a good name, I'd be happy to update it!

I usually use Inner for something that removes a layer of type like this, but that was already taken by the In<T> -> T mapping, so I just reached for a thesaurus :).

For a more brute-force approach, we could do something like type WithoutStaticSystemInput;.

Mostly I'm hoping that nobody needs to interact with this type very often, and that the doc comment mentioning StaticSystemInput will clarify things when they do.

Looks good, only thing I'd add is a test for a tuple of StaticSystemInputs and one for a mixed tuple of StaticSystemInput + a normal one.

I added one test with a tuple of two SSIs and one normal one, since that should cover both cases. They do work, although I'm not actually sure what they're useful for. And you can always use StaticSystemInput<(In<T>, I1, I2)> in place of (In<T>, StaticSystemInput<I1>, StaticSystemInput<I2>).

[alice-i-cecile]

Can you say more about the underlying user-facing motivation here? I want to be careful that we're not piling on complexity without good reason.

[chescock]

Can you say more about the underlying user-facing motivation here? I want to be careful that we're not piling on complexity without good reason.

I think @ecoskey already answered this on Discord, but the goal is being able to write higher-order systems like PipeSystem as ordinary function systems instead of manual implementations of trait System. That is mostly unlocked by #21811. The trouble is that you often wind up with an impl System<In=StaticSystemInput<()>> instead of an impl System<In = ()>, which you can run manually but can't add to a schedule.

In particular, I've always liked the "PipeSystem, but in entirely safe code" example in the SystemParamFunction docs, but been annoyed that it doesn't work with run_system_cached.

[hymm]

impl System<In=StaticSystemInput<()>> instead of an impl System<In = ()>

Could we just write a method to cast between the types? Seems like it should be something that is safe.

[chescock]

impl System<In=StaticSystemInput<()>> instead of an impl System<In = ()>

Could we just write a method to cast between the types? Seems like it should be something that is safe.

Yeah, that should be possible! We could make a wrapper type with a manual System impl and use that.

That approach just feels awkward to me because you need to remember to call that method nearly every time you use StaticSystemInput. (And I'm sort of hoping we'll be able to get to a place where all systems are FunctionSystems, but there's no way to implement the wrapper that way.)

[alice-i-cecile]

I'm currently leaning towards #21917 over this, as it doesn't add an associated type. I plan to merge that and close this, unless one of y'all have a strong argument otherwise.

[chescock]

I'm currently leaning towards #21917 over this, as it doesn't add an associated type. I plan to merge that and close this, unless one of y'all have a strong argument otherwise.

Sounds good to me!

I personally think associated types are simpler here, since they often don't need to be written out, but I acknowledge that it's entirely a matter of taste :).