System param validation for observers, system registry and run once (#…

…15526)

# Objective

Fixes #15394

## Solution

Observers now validate params.

System registry has a new error variant for when system running fails
due to invalid parameters.

Run once now returns a `Result<Out, RunOnceError>` instead of `Out`.
This is more inline with system registry, which also returns a result.

I'll address warning messages in #15500.

## Testing

Added one test for each case.

---

## Migration Guide

- `RunSystemOnce::run_system_once` and
`RunSystemOnce::run_system_once_with` now return a `Result<Out>` instead
of just `Out`

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Zachary Harrold <[email protected]>

crates/bevy_ecs/src/observer/mod.rs

@@ -1190,4 +1190,25 @@ mod tests {
         // after the observer's spawn_empty.
         world.despawn(ent);
     }
+
+    #[test]
+    fn observer_invalid_params() {
+        #[derive(Event)]
+        struct EventA;
+
+        #[derive(Resource)]
+        struct ResA;
+
+        #[derive(Resource)]
+        struct ResB;
+
+        let mut world = World::new();
+        // This fails because `ResA` is not present in the world
+        world.observe(|_: Trigger<EventA>, _: Res<ResA>, mut commands: Commands| {
+            commands.insert_resource(ResB);
+        });
+        world.trigger(EventA);
+
+        assert!(world.get_resource::<ResB>().is_none());
+    }
 }

crates/bevy_ecs/src/observer/runner.rs

@@ -374,8 +374,10 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     // - system is the same type erased system from above
     unsafe {
         (*system).update_archetype_component_access(world);
-        (*system).run_unsafe(trigger, world);
-        (*system).queue_deferred(world.into_deferred());
+        if (*system).validate_param_unsafe(world) {
+            (*system).run_unsafe(trigger, world);
+            (*system).queue_deferred(world.into_deferred());
+        }
     }
 }
 

crates/bevy_ecs/src/system/builder.rs

@@ -386,8 +386,8 @@ mod tests {
             .build_state(&mut world)
             .build_system(local_system);
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 10);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 10);
     }
 
     #[test]
@@ -403,8 +403,8 @@ mod tests {
             .build_state(&mut world)
             .build_system(query_system);
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 1);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 1);
     }
 
     #[test]
@@ -418,8 +418,8 @@ mod tests {
 
         let system = (state,).build_state(&mut world).build_system(query_system);
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 1);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 1);
     }
 
     #[test]
@@ -433,8 +433,8 @@ mod tests {
             .build_state(&mut world)
             .build_system(multi_param_system);
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 1);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 1);
     }
 
     #[test]
@@ -464,8 +464,8 @@ mod tests {
                 count
             });
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 3);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 3);
     }
 
     #[test]
@@ -479,8 +479,8 @@ mod tests {
             .build_state(&mut world)
             .build_system(|a, b| *a + *b + 1);
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 1);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 1);
     }
 
     #[test]
@@ -506,8 +506,8 @@ mod tests {
                 params.p0().iter().count() + params.p1().iter().count()
             });
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 5);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 5);
     }
 
     #[test]
@@ -535,8 +535,8 @@ mod tests {
                 count
             });
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 5);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 5);
     }
 
     #[test]
@@ -564,8 +564,8 @@ mod tests {
                 },
             );
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 4);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 4);
     }
 
     #[derive(SystemParam)]
@@ -591,7 +591,7 @@ mod tests {
             .build_state(&mut world)
             .build_system(|param: CustomParam| *param.local + param.query.iter().count());
 
-        let result = world.run_system_once(system);
-        assert_eq!(result, 101);
+        let output = world.run_system_once(system).unwrap();
+        assert_eq!(output, 101);
     }
 }

crates/bevy_ecs/src/system/system.rs

@@ -1,5 +1,6 @@
 use bevy_utils::tracing::warn;
 use core::fmt::Debug;
+use thiserror::Error;
 
 use crate::{
     archetype::ArchetypeComponentId,
@@ -269,7 +270,7 @@ where
 /// let mut world = World::default();
 /// let entity = world.run_system_once(|mut commands: Commands| {
 ///     commands.spawn_empty().id()
-/// });
+/// }).unwrap();
 /// # assert!(world.get_entity(entity).is_some());
 /// ```
 ///
@@ -289,7 +290,7 @@ where
 /// world.spawn(T(1));
 /// let count = world.run_system_once(|query: Query<&T>| {
 ///     query.iter().filter(|t| t.0 == 1).count()
-/// });
+/// }).unwrap();
 ///
 /// # assert_eq!(count, 2);
 /// ```
@@ -311,25 +312,25 @@ where
 /// world.spawn(T(0));
 /// world.spawn(T(1));
 /// world.spawn(T(1));
-/// let count = world.run_system_once(count);
+/// let count = world.run_system_once(count).unwrap();
 ///
 /// # assert_eq!(count, 2);
 /// ```
 pub trait RunSystemOnce: Sized {
-    /// Runs a system and applies its deferred parameters.
-    fn run_system_once<T, Out, Marker>(self, system: T) -> Out
+    /// Tries to run a system and apply its deferred parameters.
+    fn run_system_once<T, Out, Marker>(self, system: T) -> Result<Out, RunSystemError>
     where
         T: IntoSystem<(), Out, Marker>,
     {
         self.run_system_once_with((), system)
     }
 
-    /// Runs a system with given input and applies its deferred parameters.
+    /// Tries to run a system with given input and apply deferred parameters.
     fn run_system_once_with<T, In, Out, Marker>(
         self,
         input: SystemIn<'_, T::System>,
         system: T,
-    ) -> Out
+    ) -> Result<Out, RunSystemError>
     where
         T: IntoSystem<In, Out, Marker>,
         In: SystemInput;
@@ -340,14 +341,36 @@ impl RunSystemOnce for &mut World {
         self,
         input: SystemIn<'_, T::System>,
         system: T,
-    ) -> Out
+    ) -> Result<Out, RunSystemError>
     where
         T: IntoSystem<In, Out, Marker>,
         In: SystemInput,
     {
         let mut system: T::System = IntoSystem::into_system(system);
         system.initialize(self);
-        system.run(input, self)
+        if system.validate_param(self) {
+            Ok(system.run(input, self))
+        } else {
+            Err(RunSystemError::InvalidParams(system.name()))
+        }
+    }
+}
+
+/// Running system failed.
+#[derive(Error)]
+pub enum RunSystemError {
+    /// System could not be run due to parameters that failed validation.
+    ///
+    /// This can occur because the data required by the system was not present in the world.
+    #[error("The data required by the system {0:?} was not found in the world and the system did not run due to failed parameter validation.")]
+    InvalidParams(Cow<'static, str>),
+}
+
+impl Debug for RunSystemError {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        match self {
+            Self::InvalidParams(arg0) => f.debug_tuple("InvalidParams").field(arg0).finish(),
+        }
     }
 }
 
@@ -369,7 +392,7 @@ mod tests {
         }
 
         let mut world = World::default();
-        let n = world.run_system_once_with(1, system);
+        let n = world.run_system_once_with(1, system).unwrap();
         assert_eq!(n, 2);
         assert_eq!(world.resource::<T>().0, 1);
     }
@@ -387,9 +410,9 @@ mod tests {
         let mut world = World::new();
         world.init_resource::<Counter>();
         assert_eq!(*world.resource::<Counter>(), Counter(0));
-        world.run_system_once(count_up);
+        world.run_system_once(count_up).unwrap();
         assert_eq!(*world.resource::<Counter>(), Counter(1));
-        world.run_system_once(count_up);
+        world.run_system_once(count_up).unwrap();
         assert_eq!(*world.resource::<Counter>(), Counter(2));
     }
 
@@ -402,7 +425,7 @@ mod tests {
     fn command_processing() {
         let mut world = World::new();
         assert_eq!(world.entities.len(), 0);
-        world.run_system_once(spawn_entity);
+        world.run_system_once(spawn_entity).unwrap();
         assert_eq!(world.entities.len(), 1);
     }
 
@@ -415,7 +438,20 @@ mod tests {
         let mut world = World::new();
         world.insert_non_send_resource(Counter(10));
         assert_eq!(*world.non_send_resource::<Counter>(), Counter(10));
-        world.run_system_once(non_send_count_down);
+        world.run_system_once(non_send_count_down).unwrap();
         assert_eq!(*world.non_send_resource::<Counter>(), Counter(9));
     }
+
+    #[test]
+    fn run_system_once_invalid_params() {
+        struct T;
+        impl Resource for T {}
+        fn system(_: Res<T>) {}
+
+        let mut world = World::default();
+        // This fails because `T` has not been added to the world yet.
+        let result = world.run_system_once(system);
+
+        assert!(matches!(result, Err(RunSystemError::InvalidParams(_))));
+    }
 }

crates/bevy_ecs/src/system/system_name.rs

@@ -141,7 +141,7 @@ mod tests {
         let mut world = World::default();
         let system =
             IntoSystem::into_system(|name: SystemName| name.name().to_owned()).with_name("testing");
-        let name = world.run_system_once(system);
+        let name = world.run_system_once(system).unwrap();
         assert_eq!(name, "testing");
     }
 
@@ -151,7 +151,7 @@ mod tests {
         let system =
             IntoSystem::into_system(|_world: &mut World, name: SystemName| name.name().to_owned())
                 .with_name("testing");
-        let name = world.run_system_once(system);
+        let name = world.run_system_once(system).unwrap();
         assert_eq!(name, "testing");
     }
 }