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.
This commit is contained in:
parent
03e299b455
commit
ac04ec0075
@ -17,18 +17,21 @@ use crate::{
|
|||||||
FromWorld, World,
|
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;
|
pub use bevy_ecs_macros::SystemParam;
|
||||||
use bevy_ptr::UnsafeCellDeref;
|
use bevy_ptr::UnsafeCellDeref;
|
||||||
use bevy_utils::synccell::SyncCell;
|
use bevy_utils::synccell::SyncCell;
|
||||||
use core::{
|
use core::{
|
||||||
any::Any,
|
any::Any,
|
||||||
fmt::Debug,
|
fmt::{Debug, Display},
|
||||||
marker::PhantomData,
|
marker::PhantomData,
|
||||||
ops::{Deref, DerefMut},
|
ops::{Deref, DerefMut},
|
||||||
panic::Location,
|
panic::Location,
|
||||||
};
|
};
|
||||||
use derive_more::derive::Display;
|
|
||||||
use disqualified::ShortName;
|
use disqualified::ShortName;
|
||||||
use thiserror::Error;
|
use thiserror::Error;
|
||||||
|
|
||||||
@ -441,7 +444,12 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam fo
|
|||||||
};
|
};
|
||||||
match query.single_inner() {
|
match query.single_inner() {
|
||||||
Ok(_) => Ok(()),
|
Ok(_) => Ok(()),
|
||||||
Err(_) => Err(SystemParamValidationError::skipped()),
|
Err(QuerySingleError::NoEntities(_)) => Err(
|
||||||
|
SystemParamValidationError::skipped::<Self>("No matching entities"),
|
||||||
|
),
|
||||||
|
Err(QuerySingleError::MultipleEntities(_)) => Err(
|
||||||
|
SystemParamValidationError::skipped::<Self>("Multiple matching entities"),
|
||||||
|
),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@ -508,9 +516,9 @@ unsafe impl<'a, D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
|
|||||||
};
|
};
|
||||||
match query.single_inner() {
|
match query.single_inner() {
|
||||||
Ok(_) | Err(QuerySingleError::NoEntities(_)) => Ok(()),
|
Ok(_) | Err(QuerySingleError::NoEntities(_)) => Ok(()),
|
||||||
Err(QuerySingleError::MultipleEntities(_)) => {
|
Err(QuerySingleError::MultipleEntities(_)) => Err(
|
||||||
Err(SystemParamValidationError::skipped())
|
SystemParamValidationError::skipped::<Self>("Multiple matching entities"),
|
||||||
}
|
),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@ -577,7 +585,9 @@ unsafe impl<D: QueryData + 'static, F: QueryFilter + 'static> SystemParam
|
|||||||
)
|
)
|
||||||
};
|
};
|
||||||
if query.is_empty() {
|
if query.is_empty() {
|
||||||
Err(SystemParamValidationError::skipped())
|
Err(SystemParamValidationError::skipped::<Self>(
|
||||||
|
"No matching entities",
|
||||||
|
))
|
||||||
} else {
|
} else {
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
@ -862,7 +872,9 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> {
|
|||||||
{
|
{
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
Err(SystemParamValidationError::invalid())
|
Err(SystemParamValidationError::invalid::<Self>(
|
||||||
|
"Resource does not exist",
|
||||||
|
))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -975,7 +987,9 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
|
|||||||
{
|
{
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
Err(SystemParamValidationError::invalid())
|
Err(SystemParamValidationError::invalid::<Self>(
|
||||||
|
"Resource does not exist",
|
||||||
|
))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -1546,7 +1560,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> {
|
|||||||
{
|
{
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
Err(SystemParamValidationError::invalid())
|
Err(SystemParamValidationError::invalid::<Self>(
|
||||||
|
"Non-send resource does not exist",
|
||||||
|
))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -1656,7 +1672,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> {
|
|||||||
{
|
{
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
Err(SystemParamValidationError::invalid())
|
Err(SystemParamValidationError::invalid::<Self>(
|
||||||
|
"Non-send resource does not exist",
|
||||||
|
))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -2623,7 +2641,7 @@ unsafe impl SystemParam for FilteredResourcesMut<'_, '_> {
|
|||||||
///
|
///
|
||||||
/// Returned as an error from [`SystemParam::validate_param`],
|
/// Returned as an error from [`SystemParam::validate_param`],
|
||||||
/// and handled using the unified error handling mechanisms defined in [`bevy_ecs::error`].
|
/// 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 {
|
pub struct SystemParamValidationError {
|
||||||
/// Whether the system should be skipped.
|
/// Whether the system should be skipped.
|
||||||
///
|
///
|
||||||
@ -2637,17 +2655,45 @@ pub struct SystemParamValidationError {
|
|||||||
/// If `true`, the system should be skipped.
|
/// 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`].
|
/// This is suitable for system params that are intended to only operate in certain application states, such as [`Single`].
|
||||||
pub skipped: bool,
|
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 {
|
impl SystemParamValidationError {
|
||||||
/// Constructs a `SystemParamValidationError` that skips the system.
|
/// Constructs a `SystemParamValidationError` that skips the system.
|
||||||
pub const fn skipped() -> Self {
|
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
|
||||||
Self { skipped: true }
|
pub fn skipped<T>(message: impl Into<Cow<'static, str>>) -> Self {
|
||||||
|
Self {
|
||||||
|
skipped: true,
|
||||||
|
message: message.into(),
|
||||||
|
param: Cow::Borrowed(core::any::type_name::<T>()),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Constructs a `SystemParamValidationError` for an invalid parameter that should be treated as an error.
|
/// Constructs a `SystemParamValidationError` for an invalid parameter that should be treated as an error.
|
||||||
pub const fn invalid() -> Self {
|
/// The parameter name is initialized to the type name of `T`, so a `SystemParam` should usually pass `Self`.
|
||||||
Self { skipped: false }
|
pub fn invalid<T>(message: impl Into<Cow<'static, str>>) -> Self {
|
||||||
|
Self {
|
||||||
|
skipped: false,
|
||||||
|
message: message.into(),
|
||||||
|
param: Cow::Borrowed(core::any::type_name::<T>()),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
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
|
||||||
|
)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -2884,4 +2930,18 @@ mod tests {
|
|||||||
let _query: Query<()> = p.downcast_mut_inner().unwrap();
|
let _query: Query<()> = p.downcast_mut_inner().unwrap();
|
||||||
let _query: Query<()> = p.downcast().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<bevy_ecs::system::system_param::tests::missing_resource_error::MissingResource>\" }"]
|
||||||
|
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<MissingResource>) {}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -88,7 +88,9 @@ where
|
|||||||
// SAFETY: Read-only access to world data registered in `init_state`.
|
// SAFETY: Read-only access to world data registered in `init_state`.
|
||||||
let result = unsafe { world.get_resource_by_id(state.main_world_state) };
|
let result = unsafe { world.get_resource_by_id(state.main_world_state) };
|
||||||
let Some(main_world) = result else {
|
let Some(main_world) = result else {
|
||||||
return Err(SystemParamValidationError::invalid());
|
return Err(SystemParamValidationError::invalid::<Self>(
|
||||||
|
"`MainWorld` resource does not exist",
|
||||||
|
));
|
||||||
};
|
};
|
||||||
// SAFETY: Type is guaranteed by `SystemState`.
|
// SAFETY: Type is guaranteed by `SystemState`.
|
||||||
let main_world: &World = unsafe { main_world.deref() };
|
let main_world: &World = unsafe { main_world.deref() };
|
||||||
|
Loading…
Reference in New Issue
Block a user