Skip to content

Commit ac04ec0

Browse files
chescockmockersf
authored andcommitted
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<missing_resource_error::MissingResource>" } ``` ## 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<MissingResource>` 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.
1 parent 03e299b commit ac04ec0

File tree

2 files changed

+80
-18
lines changed

2 files changed

+80
-18
lines changed

crates/bevy_ecs/src/system/system_param.rs

+77-17
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,21 @@ use crate::{
1717
FromWorld, World,
1818
},
1919
};
20-
use alloc::{borrow::ToOwned, boxed::Box, vec::Vec};
20+
use alloc::{
21+
borrow::{Cow, ToOwned},
22+
boxed::Box,
23+
vec::Vec,
24+
};
2125
pub use bevy_ecs_macros::SystemParam;
2226
use bevy_ptr::UnsafeCellDeref;
2327
use bevy_utils::synccell::SyncCell;
2428
use core::{
2529
any::Any,
26-
fmt::Debug,
30+
fmt::{Debug, Display},
2731
marker::PhantomData,
2832
ops::{Deref, DerefMut},
2933
panic::Location,
3034
};
31-
use derive_more::derive::Display;
3235
use disqualified::ShortName;
3336
use thiserror::Error;
3437

@@ -441,7 +444,12 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo
441444
};
442445
match query.single_inner() {
443446
Ok(_) => Ok(()),
444-
Err(_) => Err(SystemParamValidationError::skipped()),
447+
Err(QuerySingleError::NoEntities(_)) => Err(
448+
SystemParamValidationError::skipped::<Self>("No matching entities"),
449+
),
450+
Err(QuerySingleError::MultipleEntities(_)) => Err(
451+
SystemParamValidationError::skipped::<Self>("Multiple matching entities"),
452+
),
445453
}
446454
}
447455
}
@@ -508,9 +516,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
508516
};
509517
match query.single_inner() {
510518
Ok(_) | Err(QuerySingleError::NoEntities(_)) => Ok(()),
511-
Err(QuerySingleError::MultipleEntities(_)) => {
512-
Err(SystemParamValidationError::skipped())
513-
}
519+
Err(QuerySingleError::MultipleEntities(_)) => Err(
520+
SystemParamValidationError::skipped::<Self>("Multiple matching entities"),
521+
),
514522
}
515523
}
516524
}
@@ -577,7 +585,9 @@ unsafe impl<D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
577585
)
578586
};
579587
if query.is_empty() {
580-
Err(SystemParamValidationError::skipped())
588+
Err(SystemParamValidationError::skipped::<Self>(
589+
"No matching entities",
590+
))
581591
} else {
582592
Ok(())
583593
}
@@ -862,7 +872,9 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> {
862872
{
863873
Ok(())
864874
} else {
865-
Err(SystemParamValidationError::invalid())
875+
Err(SystemParamValidationError::invalid::<Self>(
876+
"Resource does not exist",
877+
))
866878
}
867879
}
868880

@@ -975,7 +987,9 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
975987
{
976988
Ok(())
977989
} else {
978-
Err(SystemParamValidationError::invalid())
990+
Err(SystemParamValidationError::invalid::<Self>(
991+
"Resource does not exist",
992+
))
979993
}
980994
}
981995

@@ -1546,7 +1560,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> {
15461560
{
15471561
Ok(())
15481562
} else {
1549-
Err(SystemParamValidationError::invalid())
1563+
Err(SystemParamValidationError::invalid::<Self>(
1564+
"Non-send resource does not exist",
1565+
))
15501566
}
15511567
}
15521568

@@ -1656,7 +1672,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> {
16561672
{
16571673
Ok(())
16581674
} else {
1659-
Err(SystemParamValidationError::invalid())
1675+
Err(SystemParamValidationError::invalid::<Self>(
1676+
"Non-send resource does not exist",
1677+
))
16601678
}
16611679
}
16621680

@@ -2623,7 +2641,7 @@ unsafe impl SystemParam for FilteredResourcesMut<'_, '_> {
26232641
///
26242642
/// Returned as an error from [`SystemParam::validate_param`],
26252643
/// and handled using the unified error handling mechanisms defined in [`bevy_ecs::error`].
2626-
#[derive(Debug, PartialEq, Eq, Clone, Display, Error)]
2644+
#[derive(Debug, PartialEq, Eq, Clone, Error)]
26272645
pub struct SystemParamValidationError {
26282646
/// Whether the system should be skipped.
26292647
///
@@ -2637,17 +2655,45 @@ pub struct SystemParamValidationError {
26372655
/// If `true`, the system should be skipped.
26382656
/// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
26392657
pub skipped: bool,
2658+
2659+
/// A message describing the validation error.
2660+
pub message: Cow<'static, str>,
2661+
2662+
/// A string identifying the invalid parameter.
2663+
/// This is usually the type name of the parameter.
2664+
pub param: Cow<'static, str>,
26402665
}
26412666

26422667
impl SystemParamValidationError {
26432668
/// Constructs a `SystemParamValidationError` that skips the system.
2644-
pub const fn skipped() -> Self {
2645-
Self { skipped: true }
2669+
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
2670+
pub fn skipped<T>(message: impl Into<Cow<'static, str>>) -> Self {
2671+
Self {
2672+
skipped: true,
2673+
message: message.into(),
2674+
param: Cow::Borrowed(core::any::type_name::<T>()),
2675+
}
26462676
}
26472677

26482678
/// Constructs a `SystemParamValidationError` for an invalid parameter that should be treated as an error.
2649-
pub const fn invalid() -> Self {
2650-
Self { skipped: false }
2679+
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
2680+
pub fn invalid<T>(message: impl Into<Cow<'static, str>>) -> Self {
2681+
Self {
2682+
skipped: false,
2683+
message: message.into(),
2684+
param: Cow::Borrowed(core::any::type_name::<T>()),
2685+
}
2686+
}
2687+
}
2688+
2689+
impl Display for SystemParamValidationError {
2690+
fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> Result<(), core::fmt::Error> {
2691+
write!(
2692+
fmt,
2693+
"Parameter `{}` failed validation: {}",
2694+
ShortName(&self.param),
2695+
self.message
2696+
)
26512697
}
26522698
}
26532699

@@ -2884,4 +2930,18 @@ mod tests {
28842930
let _query: Query<()> = p.downcast_mut_inner().unwrap();
28852931
let _query: Query<()> = p.downcast().unwrap();
28862932
}
2933+
2934+
#[test]
2935+
#[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<bevy_ecs::system::system_param::tests::missing_resource_error::MissingResource>\" }"]
2936+
fn missing_resource_error() {
2937+
#[derive(Resource)]
2938+
pub struct MissingResource;
2939+
2940+
let mut schedule = crate::schedule::Schedule::default();
2941+
schedule.add_systems(res_system);
2942+
let mut world = World::new();
2943+
schedule.run(&mut world);
2944+
2945+
fn res_system(_: Res<MissingResource>) {}
2946+
}
28872947
}

crates/bevy_render/src/extract_param.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,9 @@ where
8888
// SAFETY: Read-only access to world data registered in `init_state`.
8989
let result = unsafe { world.get_resource_by_id(state.main_world_state) };
9090
let Some(main_world) = result else {
91-
return Err(SystemParamValidationError::invalid());
91+
return Err(SystemParamValidationError::invalid::<Self>(
92+
"`MainWorld` resource does not exist",
93+
));
9294
};
9395
// SAFETY: Type is guaranteed by `SystemState`.
9496
let main_world: &World = unsafe { main_world.deref() };

0 commit comments

Comments
 (0)