Skip to content

Commit 9e240ee

Browse files
authored
Improve error message for missing events (#18683)
# Objective Improve the parameter validation error message for `Event(Reader|Writer|Mutator)`. System parameters defined using `#[derive(SystemParam)]`, including the parameters for events, currently propagate the validation errors from their subparameters. The error includes the type of the failing parameter, so the resulting error includes the type of the failing subparameter instead of the derived parameter. In particular, `EventReader<T>` will report an error from a `Res<Events<T>>`, even though the user has no parameter of that type! This is a follow-up to #18593. ## Solution Have `#[derive]`d system parameters map errors during propagation so that they report the outer parameter type. To continue to provide context, add a field to `SystemParamValidationError` that identifies the subparameter by name, and is empty for non-`#[derive]`d parameters. Allow them to override the failure message for individual parameters. Use this to convert "Resource does not exist" to "Event not initialized" for `Event(Reader|Writer|Mutator)`. ## Showcase The validation error for a `EventReader<SomeEvent>` parameter when `add_event` has not been called changes from: Before: ``` Parameter `Res<Events<SomeEvent>>` failed validation: Resource does not exist ``` After ``` Parameter `EventReader<SomeEvent>::events` failed validation: Event not initialized ```
1 parent 746b593 commit 9e240ee

File tree

5 files changed

+98
-17
lines changed

5 files changed

+98
-17
lines changed

crates/bevy_ecs/macros/src/lib.rs

+35-10
Original file line numberDiff line numberDiff line change
@@ -229,19 +229,39 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
229229
let path = bevy_ecs_path();
230230

231231
let mut field_locals = Vec::new();
232+
let mut field_names = Vec::new();
232233
let mut fields = Vec::new();
233234
let mut field_types = Vec::new();
235+
let mut field_messages = Vec::new();
234236
for (i, field) in field_definitions.iter().enumerate() {
235237
field_locals.push(format_ident!("f{i}"));
236238
let i = Index::from(i);
237-
fields.push(
238-
field
239-
.ident
240-
.as_ref()
241-
.map(|f| quote! { #f })
242-
.unwrap_or_else(|| quote! { #i }),
243-
);
239+
let field_value = field
240+
.ident
241+
.as_ref()
242+
.map(|f| quote! { #f })
243+
.unwrap_or_else(|| quote! { #i });
244+
field_names.push(format!("::{}", field_value));
245+
fields.push(field_value);
244246
field_types.push(&field.ty);
247+
let mut field_message = None;
248+
for meta in field
249+
.attrs
250+
.iter()
251+
.filter(|a| a.path().is_ident("system_param"))
252+
{
253+
if let Err(e) = meta.parse_nested_meta(|nested| {
254+
if nested.path.is_ident("validation_message") {
255+
field_message = Some(nested.value()?.parse()?);
256+
Ok(())
257+
} else {
258+
Err(nested.error("Unsupported attribute"))
259+
}
260+
}) {
261+
return e.into_compile_error().into();
262+
}
263+
}
264+
field_messages.push(field_message.unwrap_or_else(|| quote! { err.message }));
245265
}
246266

247267
let generics = ast.generics;
@@ -427,10 +447,15 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
427447
#[inline]
428448
unsafe fn validate_param<'w, 's>(
429449
state: &'s Self::State,
430-
system_meta: &#path::system::SystemMeta,
431-
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
450+
_system_meta: &#path::system::SystemMeta,
451+
_world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
432452
) -> Result<(), #path::system::SystemParamValidationError> {
433-
<(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
453+
let #state_struct_name { state: (#(#tuple_patterns,)*) } = state;
454+
#(
455+
<#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world)
456+
.map_err(|err| #path::system::SystemParamValidationError::new::<Self>(err.skipped, #field_messages, #field_names))?;
457+
)*
458+
Ok(())
434459
}
435460

436461
#[inline]

crates/bevy_ecs/src/event/mutator.rs

+1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ use bevy_ecs::{
4444
#[derive(SystemParam, Debug)]
4545
pub struct EventMutator<'w, 's, E: Event> {
4646
pub(super) reader: Local<'s, EventCursor<E>>,
47+
#[system_param(validation_message = "Event not initialized")]
4748
events: ResMut<'w, Events<E>>,
4849
}
4950

crates/bevy_ecs/src/event/reader.rs

+1
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use bevy_ecs::{
1616
#[derive(SystemParam, Debug)]
1717
pub struct EventReader<'w, 's, E: Event> {
1818
pub(super) reader: Local<'s, EventCursor<E>>,
19+
#[system_param(validation_message = "Event not initialized")]
1920
events: Res<'w, Events<E>>,
2021
}
2122

crates/bevy_ecs/src/event/writer.rs

+1
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ use bevy_ecs::{
6060
/// [`Observer`]: crate::observer::Observer
6161
#[derive(SystemParam)]
6262
pub struct EventWriter<'w, E: Event> {
63+
#[system_param(validation_message = "Event not initialized")]
6364
events: ResMut<'w, Events<E>>,
6465
}
6566

crates/bevy_ecs/src/system/system_param.rs

+60-7
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,29 @@ use variadics_please::{all_tuples, all_tuples_enumerated};
131131
/// This will most commonly occur when working with `SystemParam`s generically, as the requirement
132132
/// has not been proven to the compiler.
133133
///
134+
/// ## Custom Validation Messages
135+
///
136+
/// When using the derive macro, any [`SystemParamValidationError`]s will be propagated from the sub-parameters.
137+
/// If you want to override the error message, add a `#[system_param(validation_message = "New message")]` attribute to the parameter.
138+
///
139+
/// ```
140+
/// # use bevy_ecs::prelude::*;
141+
/// # #[derive(Resource)]
142+
/// # struct SomeResource;
143+
/// # use bevy_ecs::system::SystemParam;
144+
/// #
145+
/// #[derive(SystemParam)]
146+
/// struct MyParam<'w> {
147+
/// #[system_param(validation_message = "Custom Message")]
148+
/// foo: Res<'w, SomeResource>,
149+
/// }
150+
///
151+
/// let mut world = World::new();
152+
/// let err = world.run_system_cached(|param: MyParam| {}).unwrap_err();
153+
/// let expected = "Parameter `MyParam::foo` failed validation: Custom Message";
154+
/// assert!(err.to_string().ends_with(expected));
155+
/// ```
156+
///
134157
/// ## Builders
135158
///
136159
/// If you want to use a [`SystemParamBuilder`](crate::system::SystemParamBuilder) with a derived [`SystemParam`] implementation,
@@ -2689,26 +2712,39 @@ pub struct SystemParamValidationError {
26892712
/// A string identifying the invalid parameter.
26902713
/// This is usually the type name of the parameter.
26912714
pub param: Cow<'static, str>,
2715+
2716+
/// A string identifying the field within a parameter using `#[derive(SystemParam)]`.
2717+
/// This will be an empty string for other parameters.
2718+
///
2719+
/// This will be printed after `param` in the `Display` impl, and should include a `::` prefix if non-empty.
2720+
pub field: Cow<'static, str>,
26922721
}
26932722

26942723
impl SystemParamValidationError {
26952724
/// Constructs a `SystemParamValidationError` that skips the system.
26962725
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
26972726
pub fn skipped<T>(message: impl Into<Cow<'static, str>>) -> Self {
2698-
Self {
2699-
skipped: true,
2700-
message: message.into(),
2701-
param: Cow::Borrowed(core::any::type_name::<T>()),
2702-
}
2727+
Self::new::<T>(true, message, Cow::Borrowed(""))
27032728
}
27042729

27052730
/// Constructs a `SystemParamValidationError` for an invalid parameter that should be treated as an error.
27062731
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
27072732
pub fn invalid<T>(message: impl Into<Cow<'static, str>>) -> Self {
2733+
Self::new::<T>(false, message, Cow::Borrowed(""))
2734+
}
2735+
2736+
/// Constructs a `SystemParamValidationError` for an invalid parameter.
2737+
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
2738+
pub fn new<T>(
2739+
skipped: bool,
2740+
message: impl Into<Cow<'static, str>>,
2741+
field: impl Into<Cow<'static, str>>,
2742+
) -> Self {
27082743
Self {
2709-
skipped: false,
2744+
skipped,
27102745
message: message.into(),
27112746
param: Cow::Borrowed(core::any::type_name::<T>()),
2747+
field: field.into(),
27122748
}
27132749
}
27142750
}
@@ -2717,8 +2753,9 @@ impl Display for SystemParamValidationError {
27172753
fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> {
27182754
write!(
27192755
fmt,
2720-
"Parameter `{}` failed validation: {}",
2756+
"Parameter `{}{}` failed validation: {}",
27212757
ShortName(&self.param),
2758+
self.field,
27222759
self.message
27232760
)
27242761
}
@@ -2971,4 +3008,20 @@ mod tests {
29713008

29723009
fn res_system(_: Res<MissingResource>) {}
29733010
}
3011+
3012+
#[test]
3013+
#[should_panic = "Encountered an error in system `bevy_ecs::system::system_param::tests::missing_event_error::event_system`: Parameter `EventReader<MissingEvent>::events` failed validation: Event not initialized"]
3014+
fn missing_event_error() {
3015+
use crate::prelude::{Event, EventReader};
3016+
3017+
#[derive(Event)]
3018+
pub struct MissingEvent;
3019+
3020+
let mut schedule = crate::schedule::Schedule::default();
3021+
schedule.add_systems(event_system);
3022+
let mut world = World::new();
3023+
schedule.run(&mut world);
3024+
3025+
fn event_system(_: EventReader<MissingEvent>) {}
3026+
}
29743027
}

0 commit comments

Comments
 (0)