From 30ee5ffe3bfa103c58f9df0f9375ef18f6d0507b Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sat, 29 Mar 2025 22:38:17 -0400 Subject: [PATCH] Improve error message for missing resources (#18593) # Objective Fixes #18515 After the recent changes to system param validation, the panic message for a missing resource is currently: ``` Encountered an error in system `missing_resource_error::res_system`: SystemParamValidationError { skipped: false } ``` Add the parameter type name and a descriptive message, improving the panic message to: ``` Encountered an error in system `missing_resource_error::res_system`: SystemParamValidationError { skipped: false, message: "Resource does not exist", param: "bevy_ecs::change_detection::Res" } ``` ## Solution Add fields to `SystemParamValidationError` for error context. Include the `type_name` of the param and a message. Store them as `Cow<'static, str>` and only format them into a friendly string in the `Display` impl. This lets us create errors using a `&'static str` with no allocation or formatting, while still supporting runtime `String` values if necessary. Add a unit test that verifies the panic message. ## Future Work If we change the default error handling to use `Display` instead of `Debug`, and to use `ShortName` for the system name, the panic message could be further improved to: ``` Encountered an error in system `res_system`: Parameter `Res` failed validation: Resource does not exist ``` However, `BevyError` currently includes the backtrace in `Debug` but not `Display`, and I didn't want to try to change that in this PR. --- crates/bevy_ecs/src/system/system_param.rs | 94 ++++++++++++++++++---- crates/bevy_render/src/extract_param.rs | 4 +- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index de602dbf73..126326e308 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -17,18 +17,21 @@ use crate::{ FromWorld, World, }, }; -use alloc::{borrow::ToOwned, boxed::Box, vec::Vec}; +use alloc::{ + borrow::{Cow, ToOwned}, + boxed::Box, + vec::Vec, +}; pub use bevy_ecs_macros::SystemParam; use bevy_ptr::UnsafeCellDeref; use bevy_utils::synccell::SyncCell; use core::{ any::Any, - fmt::Debug, + fmt::{Debug, Display}, marker::PhantomData, ops::{Deref, DerefMut}, panic::Location, }; -use derive_more::derive::Display; use disqualified::ShortName; use thiserror::Error; @@ -441,7 +444,12 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo }; match query.single_inner() { Ok(_) => Ok(()), - Err(_) => Err(SystemParamValidationError::skipped()), + Err(QuerySingleError::NoEntities(_)) => Err( + SystemParamValidationError::skipped::("No matching entities"), + ), + Err(QuerySingleError::MultipleEntities(_)) => Err( + SystemParamValidationError::skipped::("Multiple matching entities"), + ), } } } @@ -508,9 +516,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam }; match query.single_inner() { Ok(_) | Err(QuerySingleError::NoEntities(_)) => Ok(()), - Err(QuerySingleError::MultipleEntities(_)) => { - Err(SystemParamValidationError::skipped()) - } + Err(QuerySingleError::MultipleEntities(_)) => Err( + SystemParamValidationError::skipped::("Multiple matching entities"), + ), } } } @@ -577,7 +585,9 @@ unsafe impl SystemParam ) }; if query.is_empty() { - Err(SystemParamValidationError::skipped()) + Err(SystemParamValidationError::skipped::( + "No matching entities", + )) } else { Ok(()) } @@ -862,7 +872,9 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { { Ok(()) } else { - Err(SystemParamValidationError::invalid()) + Err(SystemParamValidationError::invalid::( + "Resource does not exist", + )) } } @@ -975,7 +987,9 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> { { Ok(()) } else { - Err(SystemParamValidationError::invalid()) + Err(SystemParamValidationError::invalid::( + "Resource does not exist", + )) } } @@ -1573,7 +1587,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { { Ok(()) } else { - Err(SystemParamValidationError::invalid()) + Err(SystemParamValidationError::invalid::( + "Non-send resource does not exist", + )) } } @@ -1683,7 +1699,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { { Ok(()) } else { - Err(SystemParamValidationError::invalid()) + Err(SystemParamValidationError::invalid::( + "Non-send resource does not exist", + )) } } @@ -2650,7 +2668,7 @@ unsafe impl SystemParam for FilteredResourcesMut<'_, '_> { /// /// Returned as an error from [`SystemParam::validate_param`], /// and handled using the unified error handling mechanisms defined in [`bevy_ecs::error`]. -#[derive(Debug, PartialEq, Eq, Clone, Display, Error)] +#[derive(Debug, PartialEq, Eq, Clone, Error)] pub struct SystemParamValidationError { /// Whether the system should be skipped. /// @@ -2664,17 +2682,45 @@ pub struct SystemParamValidationError { /// If `true`, the system should be skipped. /// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`]. pub skipped: bool, + + /// A message describing the validation error. + pub message: Cow<'static, str>, + + /// A string identifying the invalid parameter. + /// This is usually the type name of the parameter. + pub param: Cow<'static, str>, } impl SystemParamValidationError { /// Constructs a `SystemParamValidationError` that skips the system. - pub const fn skipped() -> Self { - Self { skipped: true } + /// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`. + pub fn skipped(message: impl Into>) -> Self { + Self { + skipped: true, + message: message.into(), + param: Cow::Borrowed(core::any::type_name::()), + } } /// Constructs a `SystemParamValidationError` for an invalid parameter that should be treated as an error. - pub const fn invalid() -> Self { - Self { skipped: false } + /// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`. + pub fn invalid(message: impl Into>) -> Self { + Self { + skipped: false, + message: message.into(), + param: Cow::Borrowed(core::any::type_name::()), + } + } +} + +impl Display for SystemParamValidationError { + fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> { + write!( + fmt, + "Parameter `{}` failed validation: {}", + ShortName(&self.param), + self.message + ) } } @@ -2911,4 +2957,18 @@ mod tests { let _query: Query<()> = p.downcast_mut_inner().unwrap(); let _query: Query<()> = p.downcast().unwrap(); } + + #[test] + #[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_resource_error::res_system`: SystemParamValidationError { skipped: false, message: \"Resource does not exist\", param: \"bevy_ecs::change_detection::Res\" }"] + fn missing_resource_error() { + #[derive(Resource)] + pub struct MissingResource; + + let mut schedule = crate::schedule::Schedule::default(); + schedule.add_systems(res_system); + let mut world = World::new(); + schedule.run(&mut world); + + fn res_system(_: Res) {} + } } diff --git a/crates/bevy_render/src/extract_param.rs b/crates/bevy_render/src/extract_param.rs index 918b7862a0..f543098474 100644 --- a/crates/bevy_render/src/extract_param.rs +++ b/crates/bevy_render/src/extract_param.rs @@ -88,7 +88,9 @@ where // SAFETY: Read-only access to world data registered in `init_state`. let result = unsafe { world.get_resource_by_id(state.main_world_state) }; let Some(main_world) = result else { - return Err(SystemParamValidationError::invalid()); + return Err(SystemParamValidationError::invalid::( + "`MainWorld` resource does not exist", + )); }; // SAFETY: Type is guaranteed by `SystemState`. let main_world: &World = unsafe { main_world.deref() };