Allow higher order systems (#4833)

# Objective

- Higher order system could not be created by users.
- However, a simple change to `SystemParamFunction` allows this.
- Higher order systems in this case mean functions which return systems created using other systems, such as `chain` (which is basically equivalent to map)

## Solution

- Change `SystemParamFunction` to be a safe abstraction over `FnMut([In<In>,] ...params)->Out`.
- Note that I believe `SystemParamFunction` should not have been counted as part of our public api before this PR.
    - This is because its only use was an unsafe function without an actionable safety comment.
    - The safety comment was basically 'call this within bevy code'.
    - I also believe that there are no external users in its current form. 
        - A quick search on Google and in the discord confirmed this.

## See also

- #4666, which uses this and subsumes the example here

---

## Changelog

### Added

- `SystemParamFunction`, which can be used to create higher order systems.

crates/bevy_ecs/src/system/function_system.rs

@@ -7,7 +7,7 @@ use crate::{
     schedule::SystemLabel,
     system::{
         check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch,
-        SystemParamState,
+        SystemParamItem, SystemParamState,
     },
     world::{World, WorldId},
 };
@@ -346,6 +346,16 @@ where
     }
 }
 
+impl<In, Out, Param, Marker, F> FunctionSystem<In, Out, Param, Marker, F>
+where
+    Param: SystemParam,
+{
+    /// Message shown when a system isn't initialised
+    // When lines get too long, rustfmt can sometimes refuse to format them.
+    // Work around this by storing the message separately.
+    const PARAM_MESSAGE: &'static str = "System's param_state was not found. Did you forget to initialize this system before running it?";
+}
+
 impl<In, Out, Param, Marker, F> System for FunctionSystem<In, Out, Param, Marker, F>
 where
     In: 'static,
@@ -380,20 +390,25 @@ where
     #[inline]
     unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
         let change_tick = world.increment_change_tick();
-        let out = self.func.run(
-            input,
-            self.param_state.as_mut().expect("System's param_state was not found. Did you forget to initialize this system before running it?"),
+
+        // Safety:
+        // We update the archetype component access correctly based on `Param`'s requirements
+        // in `update_archetype_component_access`.
+        // Our caller upholds the requirements.
+        let params = <Param as SystemParam>::Fetch::get_param(
+            self.param_state.as_mut().expect(Self::PARAM_MESSAGE),
             &self.system_meta,
             world,
             change_tick,
         );
+        let out = self.func.run(input, params);
         self.system_meta.last_change_tick = change_tick;
         out
     }
 
     #[inline]
     fn apply_buffers(&mut self, world: &mut World) {
-        let param_state = self.param_state.as_mut().expect("System's param_state was not found. Did you forget to initialize this system before running it?");
+        let param_state = self.param_state.as_mut().expect(Self::PARAM_MESSAGE);
         param_state.apply(world);
     }
 
@@ -474,19 +489,71 @@ impl<T> SystemLabel for SystemTypeIdLabel<T> {
 }
 
 /// A trait implemented for all functions that can be used as [`System`]s.
+///
+/// This trait can be useful for making your own systems which accept other systems,
+/// sometimes called higher order systems.
+///
+/// This should be used in combination with [`ParamSet`] when calling other systems
+/// within your system.
+/// Using [`ParamSet`] in this case avoids [`SystemParam`] collisions.
+///
+/// # Example
+///
+/// To create something like [`ChainSystem`], but in entirely safe code.
+///
+/// ```rust
+/// use std::num::ParseIntError;
+///
+/// use bevy_ecs::prelude::*;
+/// use bevy_ecs::system::{SystemParam, SystemParamItem};
+///
+/// // Unfortunately, we need all of these generics. `A` is the first system, with its
+/// // parameters and marker type required for coherence. `B` is the second system, and
+/// // the other generics are for the input/output types of A and B.
+/// /// Chain creates a new system which calls `a`, then calls `b` with the output of `a`
+/// pub fn chain<AIn, Shared, BOut, A, AParam, AMarker, B, BParam, BMarker>(
+///     mut a: A,
+///     mut b: B,
+/// ) -> impl FnMut(In<AIn>, ParamSet<(SystemParamItem<AParam>, SystemParamItem<BParam>)>) -> BOut
+/// where
+///     // We need A and B to be systems, add those bounds
+///     A: SystemParamFunction<AIn, Shared, AParam, AMarker>,
+///     B: SystemParamFunction<Shared, BOut, BParam, BMarker>,
+///     AParam: SystemParam,
+///     BParam: SystemParam,
+/// {
+///     // The type of `params` is inferred based on the return of this function above
+///     move |In(a_in), mut params| {
+///         let shared = a.run(a_in, params.p0());
+///         b.run(shared, params.p1())
+///     }
+/// }
+///
+/// // Usage example for `chain`:
+/// fn main() {
+///     let mut world = World::default();
+///     world.insert_resource(Message("42".to_string()));
+///
+///     // chain the `parse_message_system`'s output into the `filter_system`s input
+///     let mut chained_system = IntoSystem::into_system(chain(parse_message, filter));
+///     chained_system.initialize(&mut world);
+///     assert_eq!(chained_system.run((), &mut world), Some(42));
+/// }
+///
+/// struct Message(String);
+///
+/// fn parse_message(message: Res<Message>) -> Result<usize, ParseIntError> {
+///     message.0.parse::<usize>()
+/// }
+///
+/// fn filter(In(result): In<Result<usize, ParseIntError>>) -> Option<usize> {
+///     result.ok().filter(|&n| n < 100)
+/// }
+/// ```
+/// [`ChainSystem`]: crate::system::ChainSystem
+/// [`ParamSet`]: crate::system::ParamSet
 pub trait SystemParamFunction<In, Out, Param: SystemParam, Marker>: Send + Sync + 'static {
-    /// # Safety
-    ///
-    /// This call might access any of the input parameters in an unsafe way. Make sure the data
-    /// access is safe in the context of the system scheduler.
-    unsafe fn run(
-        &mut self,
-        input: In,
-        state: &mut Param::Fetch,
-        system_meta: &SystemMeta,
-        world: &World,
-        change_tick: u32,
-    ) -> Out;
+    fn run(&mut self, input: In, param_value: SystemParamItem<Param>) -> Out;
 }
 
 macro_rules! impl_system_function {
@@ -496,20 +563,21 @@ macro_rules! impl_system_function {
         where
         for <'a> &'a mut Func:
                 FnMut($($param),*) -> Out +
-                FnMut($(<<$param as SystemParam>::Fetch as SystemParamFetch>::Item),*) -> Out, Out: 'static
+                FnMut($(SystemParamItem<$param>),*) -> Out, Out: 'static
         {
             #[inline]
-            unsafe fn run(&mut self, _input: (), state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out {
-                // Yes, this is strange, but rustc fails to compile this impl
-                // without using this function.
+            fn run(&mut self, _input: (), param_value: SystemParamItem< ($($param,)*)>) -> Out {
+                // Yes, this is strange, but `rustc` fails to compile this impl
+                // without using this function. It fails to recognise that `func`
+                // is a function, potentially because of the multiple impls of `FnMut`
                 #[allow(clippy::too_many_arguments)]
                 fn call_inner<Out, $($param,)*>(
                     mut f: impl FnMut($($param,)*)->Out,
                     $($param: $param,)*
                 )->Out{
                     f($($param,)*)
                 }
-                let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick);
+                let ($($param,)*) = param_value;
                 call_inner(self, $($param),*)
             }
         }
@@ -522,7 +590,7 @@ macro_rules! impl_system_function {
                 FnMut(In<Input>, $(<<$param as SystemParam>::Fetch as SystemParamFetch>::Item),*) -> Out, Out: 'static
         {
             #[inline]
-            unsafe fn run(&mut self, input: Input, state: &mut <($($param,)*) as SystemParam>::Fetch, system_meta: &SystemMeta, world: &World, change_tick: u32) -> Out {
+            fn run(&mut self, input: Input, param_value: SystemParamItem< ($($param,)*)>) -> Out {
                 #[allow(clippy::too_many_arguments)]
                 fn call_inner<Input, Out, $($param,)*>(
                     mut f: impl FnMut(In<Input>, $($param,)*)->Out,
@@ -531,13 +599,15 @@ macro_rules! impl_system_function {
                 )->Out{
                     f(input, $($param,)*)
                 }
-                let ($($param,)*) = <<($($param,)*) as SystemParam>::Fetch as SystemParamFetch>::get_param(state, system_meta, world, change_tick);
+                let ($($param,)*) = param_value;
                 call_inner(self, In(input), $($param),*)
             }
         }
     };
 }
 
+// Note that we rely on the highest impl to be <= the highest order of the tuple impls
+// of `SystemParam` created.
 all_tuples!(impl_system_function, 0, 16, F);
 
 /// Used to implicitly convert systems to their default labels. For example, it will convert

crates/bevy_ecs/src/system/mod.rs

@@ -83,9 +83,15 @@ pub use system::*;
 pub use system_chaining::*;
 pub use system_param::*;
 
+/// Ensure that a given function is a system
+///
+/// This should be used when writing doc examples,
+/// to confirm that systems used in an example are
+/// valid systems
 pub fn assert_is_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S) {
     if false {
         // Check it can be converted into a system
+        // TODO: This should ensure that the system has no conflicting system params
         IntoSystem::into_system(sys);
     }
 }