From d09797af77b3dc1c00014ec67db9497eaf797db8 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sat, 29 Mar 2025 11:32:47 -0400 Subject: [PATCH 1/4] Unit test for `EventReader` validation error. --- crates/bevy_ecs/src/system/system_param.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 861e7f4569bbc..fd4fac10baf44 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -2971,4 +2971,20 @@ mod tests { fn res_system(_: Res) {} } + + #[test] + #[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_event_error::event_system`: Parameter `Res>` failed validation: Resource does not exist"] + fn missing_event_error() { + use crate::prelude::{Event, EventReader}; + + #[derive(Event)] + pub struct MissingEvent; + + let mut schedule = crate::schedule::Schedule::default(); + schedule.add_systems(event_system); + let mut world = World::new(); + schedule.run(&mut world); + + fn event_system(_: EventReader) {} + } } From 24117ce573df85145427e191df33b29fd22b0406 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sat, 29 Mar 2025 11:40:31 -0400 Subject: [PATCH 2/4] Write out `validate_param` for derived parameters instead of delegating to tuples. --- crates/bevy_ecs/macros/src/lib.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 6843197af2168..48a2cef97b498 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -427,10 +427,12 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { #[inline] unsafe fn validate_param<'w, 's>( state: &'s Self::State, - system_meta: &#path::system::SystemMeta, - world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, + _system_meta: &#path::system::SystemMeta, + _world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, ) -> Result<(), #path::system::SystemParamValidationError> { - <(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world) + let #state_struct_name { state: (#(#tuple_patterns,)*) } = state; + #(<#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world)?;)* + Ok(()) } #[inline] From e9567ccaf729e0cba050a3ef3833bcb264bf4710 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sat, 29 Mar 2025 11:56:54 -0400 Subject: [PATCH 3/4] Track subfield in derived params. --- crates/bevy_ecs/macros/src/lib.rs | 20 +++++++++------ crates/bevy_ecs/src/system/system_param.rs | 30 ++++++++++++++++------ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 48a2cef97b498..1dd77599af267 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -229,18 +229,19 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let path = bevy_ecs_path(); let mut field_locals = Vec::new(); + let mut field_names = Vec::new(); let mut fields = Vec::new(); let mut field_types = Vec::new(); for (i, field) in field_definitions.iter().enumerate() { field_locals.push(format_ident!("f{i}")); let i = Index::from(i); - fields.push( - field - .ident - .as_ref() - .map(|f| quote! { #f }) - .unwrap_or_else(|| quote! { #i }), - ); + let field_value = field + .ident + .as_ref() + .map(|f| quote! { #f }) + .unwrap_or_else(|| quote! { #i }); + field_names.push(format!("::{}", field_value)); + fields.push(field_value); field_types.push(&field.ty); } @@ -431,7 +432,10 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { _world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>, ) -> Result<(), #path::system::SystemParamValidationError> { let #state_struct_name { state: (#(#tuple_patterns,)*) } = state; - #(<#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world)?;)* + #( + <#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world) + .map_err(|err| #path::system::SystemParamValidationError::new::(err.skipped, err.message, #field_names))?; + )* Ok(()) } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index fd4fac10baf44..d5d9ddbd8222c 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -2689,26 +2689,39 @@ pub struct SystemParamValidationError { /// A string identifying the invalid parameter. /// This is usually the type name of the parameter. pub param: Cow<'static, str>, + + /// A string identifying the field within a parameter using `#[derive(SystemParam)]`. + /// This will be an empty string for other parameters. + /// + /// This will be printed after `param` in the `Display` impl, and should include a `::` prefix if non-empty. + pub field: Cow<'static, str>, } impl SystemParamValidationError { /// Constructs a `SystemParamValidationError` that skips the system. /// 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::()), - } + Self::new::(true, message, Cow::Borrowed("")) } /// Constructs a `SystemParamValidationError` for an invalid parameter that should be treated as an error. /// 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::new::(false, message, Cow::Borrowed("")) + } + + /// Constructs a `SystemParamValidationError` for an invalid parameter. + /// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`. + pub fn new( + skipped: bool, + message: impl Into>, + field: impl Into>, + ) -> Self { Self { - skipped: false, + skipped, message: message.into(), param: Cow::Borrowed(core::any::type_name::()), + field: field.into(), } } } @@ -2717,8 +2730,9 @@ impl Display for SystemParamValidationError { fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> { write!( fmt, - "Parameter `{}` failed validation: {}", + "Parameter `{}{}` failed validation: {}", ShortName(&self.param), + self.field, self.message ) } @@ -2973,7 +2987,7 @@ mod tests { } #[test] - #[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_event_error::event_system`: Parameter `Res>` failed validation: Resource does not exist"] + #[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_event_error::event_system`: Parameter `EventReader::events` failed validation: Resource does not exist"] fn missing_event_error() { use crate::prelude::{Event, EventReader}; From 14ca850b0dc23d9e2fe85ecba523cf1f5158787e Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sat, 29 Mar 2025 20:34:51 -0400 Subject: [PATCH 4/4] Custom messages for derived parameters. --- crates/bevy_ecs/macros/src/lib.rs | 21 +++++++++++++++++- crates/bevy_ecs/src/event/mutator.rs | 1 + crates/bevy_ecs/src/event/reader.rs | 1 + crates/bevy_ecs/src/event/writer.rs | 1 + crates/bevy_ecs/src/system/system_param.rs | 25 +++++++++++++++++++++- 5 files changed, 47 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 1dd77599af267..a657765ac23f9 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -232,6 +232,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let mut field_names = Vec::new(); let mut fields = Vec::new(); let mut field_types = Vec::new(); + let mut field_messages = Vec::new(); for (i, field) in field_definitions.iter().enumerate() { field_locals.push(format_ident!("f{i}")); let i = Index::from(i); @@ -243,6 +244,24 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { field_names.push(format!("::{}", field_value)); fields.push(field_value); field_types.push(&field.ty); + let mut field_message = None; + for meta in field + .attrs + .iter() + .filter(|a| a.path().is_ident("system_param")) + { + if let Err(e) = meta.parse_nested_meta(|nested| { + if nested.path.is_ident("validation_message") { + field_message = Some(nested.value()?.parse()?); + Ok(()) + } else { + Err(nested.error("Unsupported attribute")) + } + }) { + return e.into_compile_error().into(); + } + } + field_messages.push(field_message.unwrap_or_else(|| quote! { err.message })); } let generics = ast.generics; @@ -434,7 +453,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream { let #state_struct_name { state: (#(#tuple_patterns,)*) } = state; #( <#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world) - .map_err(|err| #path::system::SystemParamValidationError::new::(err.skipped, err.message, #field_names))?; + .map_err(|err| #path::system::SystemParamValidationError::new::(err.skipped, #field_messages, #field_names))?; )* Ok(()) } diff --git a/crates/bevy_ecs/src/event/mutator.rs b/crates/bevy_ecs/src/event/mutator.rs index 0afbeaa00d896..e95037af5ba6d 100644 --- a/crates/bevy_ecs/src/event/mutator.rs +++ b/crates/bevy_ecs/src/event/mutator.rs @@ -44,6 +44,7 @@ use bevy_ecs::{ #[derive(SystemParam, Debug)] pub struct EventMutator<'w, 's, E: Event> { pub(super) reader: Local<'s, EventCursor>, + #[system_param(validation_message = "Event not initialized")] events: ResMut<'w, Events>, } diff --git a/crates/bevy_ecs/src/event/reader.rs b/crates/bevy_ecs/src/event/reader.rs index bc0f4f86bcc99..995e2ca9e9043 100644 --- a/crates/bevy_ecs/src/event/reader.rs +++ b/crates/bevy_ecs/src/event/reader.rs @@ -16,6 +16,7 @@ use bevy_ecs::{ #[derive(SystemParam, Debug)] pub struct EventReader<'w, 's, E: Event> { pub(super) reader: Local<'s, EventCursor>, + #[system_param(validation_message = "Event not initialized")] events: Res<'w, Events>, } diff --git a/crates/bevy_ecs/src/event/writer.rs b/crates/bevy_ecs/src/event/writer.rs index 91dd13898cc30..a1c42f8b60aaf 100644 --- a/crates/bevy_ecs/src/event/writer.rs +++ b/crates/bevy_ecs/src/event/writer.rs @@ -60,6 +60,7 @@ use bevy_ecs::{ /// [`Observer`]: crate::observer::Observer #[derive(SystemParam)] pub struct EventWriter<'w, E: Event> { + #[system_param(validation_message = "Event not initialized")] events: ResMut<'w, Events>, } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index d5d9ddbd8222c..99d4c72df66e2 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -131,6 +131,29 @@ use variadics_please::{all_tuples, all_tuples_enumerated}; /// This will most commonly occur when working with `SystemParam`s generically, as the requirement /// has not been proven to the compiler. /// +/// ## Custom Validation Messages +/// +/// When using the derive macro, any [`SystemParamValidationError`]s will be propagated from the sub-parameters. +/// If you want to override the error message, add a `#[system_param(validation_message = "New message")]` attribute to the parameter. +/// +/// ``` +/// # use bevy_ecs::prelude::*; +/// # #[derive(Resource)] +/// # struct SomeResource; +/// # use bevy_ecs::system::SystemParam; +/// # +/// #[derive(SystemParam)] +/// struct MyParam<'w> { +/// #[system_param(validation_message = "Custom Message")] +/// foo: Res<'w, SomeResource>, +/// } +/// +/// let mut world = World::new(); +/// let err = world.run_system_cached(|param: MyParam| {}).unwrap_err(); +/// let expected = "Parameter `MyParam::foo` failed validation: Custom Message"; +/// assert!(err.to_string().ends_with(expected)); +/// ``` +/// /// ## Builders /// /// If you want to use a [`SystemParamBuilder`](crate::system::SystemParamBuilder) with a derived [`SystemParam`] implementation, @@ -2987,7 +3010,7 @@ mod tests { } #[test] - #[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_event_error::event_system`: Parameter `EventReader::events` failed validation: Resource does not exist"] + #[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_event_error::event_system`: Parameter `EventReader::events` failed validation: Event not initialized"] fn missing_event_error() { use crate::prelude::{Event, EventReader};