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 <alice.i.cecile@gmail.com>
Co-authored-by: Zachary Harrold <zac@harrold.com.au>

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,10 +374,12 @@ 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);
+        if (*system).validate_param_unsafe(world) {
             (*system).run_unsafe(trigger, world);
             (*system).queue_deferred(world.into_deferred());
         }
     }
+}
 
 /// A [`ComponentHook`] used by [`Observer`] to handle its [`on-add`](`ComponentHooks::on_add`).
 ///

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");
     }
 }

crates/bevy_ecs/src/system/system_registry.rs

@@ -3,8 +3,7 @@ use crate::{
     bundle::Bundle,
     change_detection::Mut,
     entity::Entity,
-    system::input::SystemInput,
-    system::{BoxedSystem, IntoSystem, System},
+    system::{input::SystemInput, BoxedSystem, IntoSystem, System},
     world::{Command, World},
 };
 use bevy_ecs_macros::{Component, Resource};
@@ -337,7 +336,12 @@ impl World {
             system.initialize(self);
             initialized = true;
         }
-        let result = system.run(input, self);
+
+        let result = if system.validate_param(self) {
+            Ok(system.run(input, self))
+        } else {
+            Err(RegisteredSystemError::InvalidParams(id))
+        };
 
         // return ownership of system trait object (if entity still exists)
         if let Some(mut entity) = self.get_entity_mut(id.entity) {
@@ -346,7 +350,7 @@ impl World {
                 system,
             });
         }
-        Ok(result)
+        result
     }
 
     /// Registers a system or returns its cached [`SystemId`].
@@ -496,7 +500,7 @@ where
 {
     #[inline]
     fn apply(self, world: &mut World) {
-        let _ = world.run_system_with_input(self.system_id, self.input);
+        _ = world.run_system_with_input(self.system_id, self.input);
     }
 }
 
@@ -595,6 +599,11 @@ pub enum RegisteredSystemError<I: SystemInput = (), O = ()> {
     /// A system tried to remove itself.
     #[error("System {0:?} tried to remove itself")]
     SelfRemove(SystemId<I, O>),
+    /// 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(SystemId<I, O>),
 }
 
 impl<I: SystemInput, O> core::fmt::Debug for RegisteredSystemError<I, O> {
@@ -606,13 +615,14 @@ impl<I: SystemInput, O> core::fmt::Debug for RegisteredSystemError<I, O> {
             Self::SystemNotCached => write!(f, "SystemNotCached"),
             Self::Recursive(arg0) => f.debug_tuple("Recursive").field(arg0).finish(),
             Self::SelfRemove(arg0) => f.debug_tuple("SelfRemove").field(arg0).finish(),
+            Self::InvalidParams(arg0) => f.debug_tuple("InvalidParams").field(arg0).finish(),
         }
     }
 }
 
 mod tests {
-    use crate as bevy_ecs;
     use crate::prelude::*;
+    use crate::{self as bevy_ecs};
 
     #[derive(Resource, Default, PartialEq, Debug)]
     struct Counter(u8);
@@ -913,4 +923,23 @@ mod tests {
             .unwrap();
         assert!(event.cancelled);
     }
+
+    #[test]
+    fn run_system_invalid_params() {
+        use crate::system::RegisteredSystemError;
+
+        struct T;
+        impl Resource for T {}
+        fn system(_: Res<T>) {}
+
+        let mut world = World::new();
+        let id = world.register_system_cached(system);
+        // This fails because `T` has not been added to the world yet.
+        let result = world.run_system(id);
+
+        assert!(matches!(
+            result,
+            Err(RegisteredSystemError::InvalidParams(_))
+        ));
+    }
 }

crates/bevy_ecs/src/world/entity_ref.rs

@@ -3225,7 +3225,7 @@ mod tests {
         // This should panic, because we have a mutable borrow on
         // `TestComponent` but have a simultaneous indirect immutable borrow on
         // that component via `EntityRefExcept`.
-        world.run_system_once(system);
+        world.run_system_once(system).unwrap();
 
         fn system(_: Query<(&mut TestComponent, EntityRefExcept<TestComponent2>)>) {}
     }
@@ -3241,7 +3241,7 @@ mod tests {
         // This should panic, because we have a mutable borrow on
         // `TestComponent` but have a simultaneous indirect immutable borrow on
         // that component via `EntityRefExcept`.
-        world.run_system_once(system);
+        world.run_system_once(system).unwrap();
 
         fn system(_: Query<&mut TestComponent>, _: Query<EntityRefExcept<TestComponent2>>) {}
     }
@@ -3253,7 +3253,7 @@ mod tests {
         let mut world = World::new();
         world.spawn(TestComponent(0)).insert(TestComponent2(0));
 
-        world.run_system_once(system);
+        world.run_system_once(system).unwrap();
 
         fn system(_: Query<&mut TestComponent>, query: Query<EntityRefExcept<TestComponent>>) {
             for entity_ref in query.iter() {
@@ -3301,7 +3301,7 @@ mod tests {
         // This should panic, because we have a mutable borrow on
         // `TestComponent` but have a simultaneous indirect immutable borrow on
         // that component via `EntityRefExcept`.
-        world.run_system_once(system);
+        world.run_system_once(system).unwrap();
 
         fn system(_: Query<(&mut TestComponent, EntityMutExcept<TestComponent2>)>) {}
     }
@@ -3317,7 +3317,7 @@ mod tests {
         // This should panic, because we have a mutable borrow on
         // `TestComponent` but have a simultaneous indirect immutable borrow on
         // that component via `EntityRefExcept`.
-        world.run_system_once(system);
+        world.run_system_once(system).unwrap();
 
         fn system(_: Query<&mut TestComponent>, mut query: Query<EntityMutExcept<TestComponent2>>) {
             for mut entity_mut in query.iter_mut() {
@@ -3335,7 +3335,7 @@ mod tests {
         let mut world = World::new();
         world.spawn(TestComponent(0)).insert(TestComponent2(0));
 
-        world.run_system_once(system);
+        world.run_system_once(system).unwrap();
 
         fn system(_: Query<&mut TestComponent>, mut query: Query<EntityMutExcept<TestComponent>>) {
             for mut entity_mut in query.iter_mut() {

crates/bevy_scene/src/scene_spawner.rs

@@ -557,7 +557,8 @@ mod tests {
     }
 
     fn build_scene(app: &mut App) -> Handle<Scene> {
-        app.world_mut().run_system_once(
+        app.world_mut()
+            .run_system_once(
                 |world: &World,
                  type_registry: Res<'_, AppTypeRegistry>,
                  asset_server: Res<'_, AssetServer>| {
@@ -567,6 +568,7 @@ mod tests {
                     )
                 },
             )
+            .expect("Failed to run scene builder system.")
     }
 
     fn build_dynamic_scene(app: &mut App) -> Handle<DynamicScene> {
@@ -574,6 +576,7 @@ mod tests {
             .run_system_once(|world: &World, asset_server: Res<'_, AssetServer>| {
                 asset_server.add(DynamicScene::from_world(world))
             })
+            .expect("Failed to run dynamic scene builder system.")
     }
 
     fn observe_trigger(app: &mut App, scene_id: InstanceId, scene_entity: Entity) {
@@ -608,7 +611,8 @@ mod tests {
                     trigger_count.0, 1,
                     "wrong number of `SceneInstanceReady` triggers"
                 );
-            });
+            })
+            .unwrap();
     }
 
     #[test]
@@ -619,11 +623,12 @@ mod tests {
         let scene = build_scene(&mut app);
 
         // Spawn scene.
-        let scene_id =
-            app.world_mut()
+        let scene_id = app
+            .world_mut()
             .run_system_once(move |mut scene_spawner: ResMut<'_, SceneSpawner>| {
                 scene_spawner.spawn(scene.clone())
-                });
+            })
+            .unwrap();
 
         // Check trigger.
         observe_trigger(&mut app, scene_id, Entity::PLACEHOLDER);
@@ -637,11 +642,12 @@ mod tests {
         let scene = build_dynamic_scene(&mut app);
 
         // Spawn scene.
-        let scene_id =
-            app.world_mut()
+        let scene_id = app
+            .world_mut()
             .run_system_once(move |mut scene_spawner: ResMut<'_, SceneSpawner>| {
                 scene_spawner.spawn_dynamic(scene.clone())
-                });
+            })
+            .unwrap();
 
         // Check trigger.
         observe_trigger(&mut app, scene_id, Entity::PLACEHOLDER);
@@ -655,13 +661,17 @@ mod tests {
         let scene = build_scene(&mut app);
 
         // Spawn scene as child.
-        let (scene_id, scene_entity) = app.world_mut().run_system_once(
-            move |mut commands: Commands<'_, '_>, mut scene_spawner: ResMut<'_, SceneSpawner>| {
+        let (scene_id, scene_entity) = app
+            .world_mut()
+            .run_system_once(
+                move |mut commands: Commands<'_, '_>,
+                      mut scene_spawner: ResMut<'_, SceneSpawner>| {
                     let entity = commands.spawn_empty().id();
                     let id = scene_spawner.spawn_as_child(scene.clone(), entity);
                     (id, entity)
                 },
-        );
+            )
+            .unwrap();
 
         // Check trigger.
         observe_trigger(&mut app, scene_id, scene_entity);
@@ -675,13 +685,17 @@ mod tests {
         let scene = build_dynamic_scene(&mut app);
 
         // Spawn scene as child.
-        let (scene_id, scene_entity) = app.world_mut().run_system_once(
-            move |mut commands: Commands<'_, '_>, mut scene_spawner: ResMut<'_, SceneSpawner>| {
+        let (scene_id, scene_entity) = app
+            .world_mut()
+            .run_system_once(
+                move |mut commands: Commands<'_, '_>,
+                      mut scene_spawner: ResMut<'_, SceneSpawner>| {
                     let entity = commands.spawn_empty().id();
                     let id = scene_spawner.spawn_dynamic_as_child(scene.clone(), entity);
                     (id, entity)
                 },
-        );
+            )
+            .unwrap();
 
         // Check trigger.
         observe_trigger(&mut app, scene_id, scene_entity);
@@ -718,13 +732,15 @@ mod tests {
         check(app.world_mut(), count);
 
         // Despawn scene.
-        app.world_mut().run_system_once(
+        app.world_mut()
+            .run_system_once(
                 |mut commands: Commands, query: Query<Entity, With<ComponentA>>| {
                     for entity in query.iter() {
                         commands.entity(entity).despawn_recursive();
                     }
                 },
-        );
+            )
+            .unwrap();
 
         app.update();
         check(app.world_mut(), 0);

crates/bevy_ui/src/layout/mod.rs

@@ -944,7 +944,7 @@ mod tests {
             new_pos: Vec2,
             expected_camera_entity: &Entity,
         ) {
-            world.run_system_once_with(new_pos, move_ui_node);
+            world.run_system_once_with(new_pos, move_ui_node).unwrap();
             ui_schedule.run(world);
             let (ui_node_entity, TargetCamera(target_camera_entity)) = world
                 .query_filtered::<(Entity, &TargetCamera), With<MovingUiNode>>()
@@ -995,7 +995,7 @@ mod tests {
         // add total cameras - 1 (the assumed default) to get an idea for how many nodes we should expect
         let expected_max_taffy_node_count = get_taffy_node_count(&world) + total_cameras - 1;
 
-        world.run_system_once(update_camera_viewports);
+        world.run_system_once(update_camera_viewports).unwrap();
 
         ui_schedule.run(&mut world);
 

examples/ecs/one_shot_systems.rs

@@ -46,7 +46,7 @@ fn setup_with_commands(mut commands: Commands) {
 
 fn setup_with_world(world: &mut World) {
     // We can run it once manually
-    world.run_system_once(system_b);
+    world.run_system_once(system_b).unwrap();
     // Or with a Callback
     let system_id = world.register_system(system_b);
     world.spawn((Callback(system_id), B));