From 5289e18e0bdbc4fe2643893ae767437e3781f26c Mon Sep 17 00:00:00 2001 From: MiniaczQ Date: Mon, 30 Sep 2024 03:00:39 +0200 Subject: [PATCH] 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` 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` instead of just `Out` --------- Co-authored-by: Alice Cecile Co-authored-by: Zachary Harrold --- crates/bevy_ecs/src/observer/mod.rs | 21 ++++ crates/bevy_ecs/src/observer/runner.rs | 6 +- crates/bevy_ecs/src/system/builder.rs | 40 +++---- crates/bevy_ecs/src/system/system.rs | 64 ++++++++--- crates/bevy_ecs/src/system/system_name.rs | 4 +- crates/bevy_ecs/src/system/system_registry.rs | 41 +++++-- crates/bevy_ecs/src/world/entity_ref.rs | 12 +-- crates/bevy_scene/src/scene_spawner.rs | 100 ++++++++++-------- crates/bevy_ui/src/layout/mod.rs | 4 +- examples/ecs/one_shot_systems.rs | 2 +- 10 files changed, 199 insertions(+), 95 deletions(-) diff --git a/crates/bevy_ecs/src/observer/mod.rs b/crates/bevy_ecs/src/observer/mod.rs index fe0154b7a4289..41ba7f5289235 100644 --- a/crates/bevy_ecs/src/observer/mod.rs +++ b/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, _: Res, mut commands: Commands| { + commands.insert_resource(ResB); + }); + world.trigger(EventA); + + assert!(world.get_resource::().is_none()); + } } diff --git a/crates/bevy_ecs/src/observer/runner.rs b/crates/bevy_ecs/src/observer/runner.rs index e45eb763146c1..f85725d392525 100644 --- a/crates/bevy_ecs/src/observer/runner.rs +++ b/crates/bevy_ecs/src/observer/runner.rs @@ -374,8 +374,10 @@ fn observer_system_runner>( // - 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()); + } } } diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index 279cc41381af5..6dc37ab1ab82b 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/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); } } diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 7741f3cb80746..689c9f725f208 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/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(self, system: T) -> Out + /// Tries to run a system and apply its deferred parameters. + fn run_system_once(self, system: T) -> Result 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( self, input: SystemIn<'_, T::System>, system: T, - ) -> Out + ) -> Result where T: IntoSystem, In: SystemInput; @@ -340,14 +341,36 @@ impl RunSystemOnce for &mut World { self, input: SystemIn<'_, T::System>, system: T, - ) -> Out + ) -> Result where T: IntoSystem, 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::().0, 1); } @@ -387,9 +410,9 @@ mod tests { let mut world = World::new(); world.init_resource::(); assert_eq!(*world.resource::(), Counter(0)); - world.run_system_once(count_up); + world.run_system_once(count_up).unwrap(); assert_eq!(*world.resource::(), Counter(1)); - world.run_system_once(count_up); + world.run_system_once(count_up).unwrap(); assert_eq!(*world.resource::(), 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(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(9)); } + + #[test] + fn run_system_once_invalid_params() { + struct T; + impl Resource for T {} + fn system(_: Res) {} + + 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(_)))); + } } diff --git a/crates/bevy_ecs/src/system/system_name.rs b/crates/bevy_ecs/src/system/system_name.rs index 4759f8cb5ca65..5a3763465cd8a 100644 --- a/crates/bevy_ecs/src/system/system_name.rs +++ b/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"); } } diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index a459383ebc53a..480f16868bcfc 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/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 { /// A system tried to remove itself. #[error("System {0:?} tried to remove itself")] SelfRemove(SystemId), + /// 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), } impl core::fmt::Debug for RegisteredSystemError { @@ -606,13 +615,14 @@ impl core::fmt::Debug for RegisteredSystemError { 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) {} + + 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(_)) + )); + } } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index 6851356f5c3a2..f207ee2d3bc36 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/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)>) {} } @@ -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>) {} } @@ -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>) { 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)>) {} } @@ -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>) { 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>) { for mut entity_mut in query.iter_mut() { diff --git a/crates/bevy_scene/src/scene_spawner.rs b/crates/bevy_scene/src/scene_spawner.rs index 7341d9a250d21..cd138502251bf 100644 --- a/crates/bevy_scene/src/scene_spawner.rs +++ b/crates/bevy_scene/src/scene_spawner.rs @@ -557,16 +557,18 @@ mod tests { } fn build_scene(app: &mut App) -> Handle { - app.world_mut().run_system_once( - |world: &World, - type_registry: Res<'_, AppTypeRegistry>, - asset_server: Res<'_, AssetServer>| { - asset_server.add( - Scene::from_dynamic_scene(&DynamicScene::from_world(world), &type_registry) - .unwrap(), - ) - }, - ) + app.world_mut() + .run_system_once( + |world: &World, + type_registry: Res<'_, AppTypeRegistry>, + asset_server: Res<'_, AssetServer>| { + asset_server.add( + Scene::from_dynamic_scene(&DynamicScene::from_world(world), &type_registry) + .unwrap(), + ) + }, + ) + .expect("Failed to run scene builder system.") } fn build_dynamic_scene(app: &mut App) -> Handle { @@ -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() - .run_system_once(move |mut scene_spawner: ResMut<'_, SceneSpawner>| { - scene_spawner.spawn(scene.clone()) - }); + 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() - .run_system_once(move |mut scene_spawner: ResMut<'_, SceneSpawner>| { - scene_spawner.spawn_dynamic(scene.clone()) - }); + 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 entity = commands.spawn_empty().id(); - let id = scene_spawner.spawn_as_child(scene.clone(), entity); - (id, entity) - }, - ); + 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 entity = commands.spawn_empty().id(); - let id = scene_spawner.spawn_dynamic_as_child(scene.clone(), entity); - (id, entity) - }, - ); + 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( - |mut commands: Commands, query: Query>| { - for entity in query.iter() { - commands.entity(entity).despawn_recursive(); - } - }, - ); + app.world_mut() + .run_system_once( + |mut commands: Commands, query: Query>| { + for entity in query.iter() { + commands.entity(entity).despawn_recursive(); + } + }, + ) + .unwrap(); app.update(); check(app.world_mut(), 0); diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs index ef3f0e24733b4..f9ee04ea7c5a2 100644 --- a/crates/bevy_ui/src/layout/mod.rs +++ b/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>() @@ -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); diff --git a/examples/ecs/one_shot_systems.rs b/examples/ecs/one_shot_systems.rs index cc082828cb5b4..e26d77460596e 100644 --- a/examples/ecs/one_shot_systems.rs +++ b/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));