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.

Author: chescock

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<T>` failed validation: Resource does not exist";
+        assert_eq!(expected, result.unwrap_err().to_string());
     }
 }

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<I: SystemInput = (), O = ()> {
     #[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>),
+    /// 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<I, O>,
+        /// The returned parameter validation error.
+        err: SystemParamValidationError,
+    },
 }
 
 impl<I: SystemInput, O> core::fmt::Debug for RegisteredSystemError<I, O> {
@@ -509,7 +512,11 @@ 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(),
+            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<T>` failed validation: Resource does not exist");
+        assert_eq!(expected, result.unwrap_err().to_string());
     }
 
     #[test]