From 9d4d1107047d9dc99c5fd898da64324704485a30 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Tue, 1 Apr 2025 15:27:08 -0400 Subject: [PATCH] Include SystemParamValidationError in RunSystemError and RegisteredSystemError (#18666) # Objective Provide more useful errors when `World::run_system` and related methods fail parameter validation. Let callers determine whether the validation failure would have skipped or failed the system. Follow-up to #18541. ## Solution Add a `SystemParamValidationError` value to the `RunSystemError::InvalidParams` and `RegisteredSystemError::InvalidParams` variants. That includes the complete context of the parameter validation error, including the `skipped` flag. --- crates/bevy_ecs/src/system/system.rs | 40 ++++++++-------- crates/bevy_ecs/src/system/system_registry.rs | 46 +++++++++++-------- 2 files changed, 48 insertions(+), 38 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 4eff0b85cd..69f5ae980a 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -368,37 +368,35 @@ impl RunSystemOnce for &mut World { { let mut system: T::System = IntoSystem::into_system(system); system.initialize(self); - match system.validate_param(self) { - Ok(()) => Ok(system.run(input, self)), - // TODO: should we expse the fact that the system was skipped to the user? - // Should we somehow unify this better with system error handling? - Err(_) => Err(RunSystemError::InvalidParams(system.name())), - } + system + .validate_param(self) + .map_err(|err| RunSystemError::InvalidParams { + system: system.name(), + err, + })?; + Ok(system.run(input, self)) } } /// Running system failed. -#[derive(Error)] +#[derive(Error, Debug)] 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(), - } - } + /// This should not be considered an error if [`field@SystemParamValidationError::skipped`] is `true`. + #[error("System {system} did not run due to failed parameter validation: {err}")] + InvalidParams { + /// The identifier of the system that was run. + system: Cow<'static, str>, + /// The returned parameter validation error. + err: SystemParamValidationError, + }, } #[cfg(test)] mod tests { use super::*; use crate::prelude::*; + use alloc::string::ToString; #[test] fn run_system_once() { @@ -470,6 +468,8 @@ mod tests { // 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(_)))); + assert!(matches!(result, Err(RunSystemError::InvalidParams { .. }))); + let expected = "System bevy_ecs::system::system::tests::run_system_once_invalid_params::system did not run due to failed parameter validation: Parameter `Res` failed validation: Resource does not exist"; + assert_eq!(expected, result.unwrap_err().to_string()); } } diff --git a/crates/bevy_ecs/src/system/system_registry.rs b/crates/bevy_ecs/src/system/system_registry.rs index 56ade62958..cf53b35be5 100644 --- a/crates/bevy_ecs/src/system/system_registry.rs +++ b/crates/bevy_ecs/src/system/system_registry.rs @@ -3,7 +3,7 @@ use crate::reflect::ReflectComponent; use crate::{ change_detection::Mut, entity::Entity, - system::{input::SystemInput, BoxedSystem, IntoSystem}, + system::{input::SystemInput, BoxedSystem, IntoSystem, SystemParamValidationError}, world::World, }; use alloc::boxed::Box; @@ -351,17 +351,16 @@ impl World { initialized = true; } - let result = if system.validate_param(self).is_ok() { - // Wait to run the commands until the system is available again. - // This is needed so the systems can recursively run themselves. - let ret = system.run_without_applying_deferred(input, self); - system.queue_deferred(self.into()); - Ok(ret) - } else { - // TODO: do we want to differentiate between failed validation and skipped systems? - // Do we want to better unify this with system error handling? - Err(RegisteredSystemError::InvalidParams(id)) - }; + let result = system + .validate_param(self) + .map_err(|err| RegisteredSystemError::InvalidParams { system: id, err }) + .map(|()| { + // Wait to run the commands until the system is available again. + // This is needed so the systems can recursively run themselves. + let ret = system.run_without_applying_deferred(input, self); + system.queue_deferred(self.into()); + ret + }); // Return ownership of system trait object (if entity still exists) if let Ok(mut entity) = self.get_entity_mut(id.entity) { @@ -494,10 +493,14 @@ pub enum RegisteredSystemError { #[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), + /// This should not be considered an error if [`field@SystemParamValidationError::skipped`] is `true`. + #[error("System {system:?} did not run due to failed parameter validation: {err}")] + InvalidParams { + /// The identifier of the system that was run. + system: SystemId, + /// The returned parameter validation error. + err: SystemParamValidationError, + }, } impl core::fmt::Debug for RegisteredSystemError { @@ -509,7 +512,11 @@ 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(), + Self::InvalidParams { system, err } => f + .debug_struct("InvalidParams") + .field("system", system) + .field("err", err) + .finish(), } } } @@ -858,6 +865,7 @@ mod tests { #[test] fn run_system_invalid_params() { use crate::system::RegisteredSystemError; + use alloc::{format, string::ToString}; struct T; impl Resource for T {} @@ -870,8 +878,10 @@ mod tests { assert!(matches!( result, - Err(RegisteredSystemError::InvalidParams(_)) + Err(RegisteredSystemError::InvalidParams { .. }) )); + let expected = format!("System {id:?} did not run due to failed parameter validation: Parameter `Res` failed validation: Resource does not exist"); + assert_eq!(expected, result.unwrap_err().to_string()); } #[test]