Skip to content

Make system param validation rely on the unified ECS error handling via the GLOBAL_ERROR_HANDLER #18454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2215,6 +2215,7 @@ wasm = false
name = "fallible_params"
path = "examples/ecs/fallible_params.rs"
doc-scrape-examples = true
required-features = ["configurable_error_handler"]

[package.metadata.example.fallible_params]
name = "Fallible System Parameters"
Expand Down
14 changes: 13 additions & 1 deletion crates/bevy_ecs/src/error/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ pub enum ErrorContext {
/// The last tick that the system was run.
last_run: Tick,
},
/// The error occurred in a run condition.
RunCondition {
/// The name of the run condition that failed.
name: Cow<'static, str>,
/// The last tick that the run condition was evaluated.
last_run: Tick,
},
/// The error occurred in a command.
Command {
/// The name of the command that failed.
Expand All @@ -39,6 +46,9 @@ impl Display for ErrorContext {
Self::Observer { name, .. } => {
write!(f, "Observer `{}` failed", name)
}
Self::RunCondition { name, .. } => {
write!(f, "Run condition `{}` failed", name)
}
}
}
}
Expand All @@ -49,7 +59,8 @@ impl ErrorContext {
match self {
Self::System { name, .. }
| Self::Command { name, .. }
| Self::Observer { name, .. } => name,
| Self::Observer { name, .. }
| Self::RunCondition { name, .. } => name,
}
}

Expand All @@ -61,6 +72,7 @@ impl ErrorContext {
Self::System { .. } => "system",
Self::Command { .. } => "command",
Self::Observer { .. } => "observer",
Self::RunCondition { .. } => "run condition",
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,9 @@ pub mod prelude {
spawn::{Spawn, SpawnRelated},
system::{
Command, Commands, Deferred, EntityCommand, EntityCommands, In, InMut, InRef,
IntoSystem, Local, NonSend, NonSendMarker, NonSendMut, ParamSet, Populated, Query,
ReadOnlySystem, Res, ResMut, Single, System, SystemIn, SystemInput, SystemParamBuilder,
SystemParamFunction, WithParamWarnPolicy,
IntoSystem, Local, NonSend, NonSendMut, ParamSet, Populated, Query, ReadOnlySystem,
Res, ResMut, Single, System, SystemIn, SystemInput, SystemParamBuilder,
SystemParamFunction,
},
world::{
EntityMut, EntityRef, EntityWorldMut, FilteredResources, FilteredResourcesMut,
Expand Down
10 changes: 9 additions & 1 deletion crates/bevy_ecs/src/observer/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
observer::{ObserverDescriptor, ObserverTrigger},
prelude::*,
query::DebugCheckedUnwrap,
system::{IntoObserverSystem, ObserverSystem},
system::{IntoObserverSystem, ObserverSystem, SystemParamValidationError},
world::DeferredWorld,
};
use bevy_ptr::PtrMut;
Expand Down Expand Up @@ -416,6 +416,14 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
);
};
(*system).queue_deferred(world.into_deferred());
} else {
error_handler(
SystemParamValidationError.into(),
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
}
}
}
Expand Down
36 changes: 2 additions & 34 deletions crates/bevy_ecs/src/schedule/executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ mod tests {
use crate::{
prelude::{IntoScheduleConfigs, Resource, Schedule, SystemSet},
schedule::ExecutorKind,
system::{Commands, Res, WithParamWarnPolicy},
system::Commands,
world::World,
};

Expand Down Expand Up @@ -346,8 +346,7 @@ mod tests {
// This system depends on a system that is always skipped.
(|mut commands: Commands| {
commands.insert_resource(R2);
})
.warn_param_missing(),
}),
)
.chain(),
);
Expand All @@ -358,35 +357,4 @@ mod tests {

#[derive(SystemSet, Hash, Debug, PartialEq, Eq, Clone)]
struct S1;

#[test]
fn invalid_condition_param_skips_system() {
for executor in EXECUTORS {
invalid_condition_param_skips_system_core(executor);
}
}

fn invalid_condition_param_skips_system_core(executor: ExecutorKind) {
let mut world = World::new();
let mut schedule = Schedule::default();
schedule.set_executor_kind(executor);
schedule.configure_sets(S1.run_if((|_: Res<R1>| true).warn_param_missing()));
schedule.add_systems((
// System gets skipped if system set run conditions fail validation.
(|mut commands: Commands| {
commands.insert_resource(R1);
})
.warn_param_missing()
.in_set(S1),
// System gets skipped if run conditions fail validation.
(|mut commands: Commands| {
commands.insert_resource(R2);
})
.warn_param_missing()
.run_if((|_: Res<R2>| true).warn_param_missing()),
));
schedule.run(&mut world);
assert!(world.get_resource::<R1>().is_none());
assert!(world.get_resource::<R2>().is_none());
}
}
23 changes: 21 additions & 2 deletions crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ use tracing::{info_span, Span};

use crate::{
archetype::ArchetypeComponentId,
error::{BevyError, ErrorContext, Result},
error::{default_error_handler, BevyError, ErrorContext, Result},
prelude::Resource,
query::Access,
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
system::ScheduleSystem,
system::{ScheduleSystem, SystemParamValidationError},
world::{unsafe_world_cell::UnsafeWorldCell, World},
};

Expand Down Expand Up @@ -536,6 +536,7 @@ impl ExecutorState {
world: UnsafeWorldCell,
) -> bool {
let mut should_run = !self.skipped_systems.contains(system_index);
let error_handler = default_error_handler();

for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() {
if self.evaluated_sets.contains(set_idx) {
Expand Down Expand Up @@ -582,6 +583,14 @@ impl ExecutorState {
// - `update_archetype_component_access` has been called for system.
let valid_params = unsafe { system.validate_param_unsafe(world) };
if !valid_params {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: system.name(),
last_run: system.get_last_run(),
},
);

self.skipped_systems.insert(system_index);
}
should_run &= valid_params;
Expand Down Expand Up @@ -767,6 +776,8 @@ unsafe fn evaluate_and_fold_conditions(
conditions: &mut [BoxedCondition],
world: UnsafeWorldCell,
) -> bool {
let error_handler = default_error_handler();

#[expect(
clippy::unnecessary_fold,
reason = "Short-circuiting here would prevent conditions from mutating their own state as needed."
Expand All @@ -779,6 +790,14 @@ unsafe fn evaluate_and_fold_conditions(
// required by the condition.
// - `update_archetype_component_access` has been called for condition.
if !unsafe { condition.validate_param_unsafe(world) } {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: condition.name(),
last_run: condition.get_last_run(),
},
);

return false;
}
// SAFETY:
Expand Down
22 changes: 21 additions & 1 deletion crates/bevy_ecs/src/schedule/executor/simple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ use tracing::info_span;
use std::eprintln;

use crate::{
error::{BevyError, ErrorContext},
error::{default_error_handler, BevyError, ErrorContext},
schedule::{
executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
},
system::SystemParamValidationError,
world::World,
};

Expand Down Expand Up @@ -88,6 +89,16 @@ impl SystemExecutor for SimpleExecutor {
let system = &mut schedule.systems[system_index];
if should_run {
let valid_params = system.validate_param(world);
if !valid_params {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: system.name(),
last_run: system.get_last_run(),
},
);
}

should_run &= valid_params;
}

Expand Down Expand Up @@ -153,6 +164,8 @@ impl SimpleExecutor {
}

fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool {
let error_handler = default_error_handler();

#[expect(
clippy::unnecessary_fold,
reason = "Short-circuiting here would prevent conditions from mutating their own state as needed."
Expand All @@ -161,6 +174,13 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
.iter_mut()
.map(|condition| {
if !condition.validate_param(world) {
error_handler(
SystemParamValidationError.into(),
ErrorContext::RunCondition {
name: condition.name(),
last_run: condition.get_last_run(),
},
);
return false;
}
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
Expand Down
21 changes: 20 additions & 1 deletion crates/bevy_ecs/src/schedule/executor/single_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ use tracing::info_span;
use std::eprintln;

use crate::{
error::{BevyError, ErrorContext},
error::{default_error_handler, BevyError, ErrorContext},
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
system::SystemParamValidationError,
world::World,
};

Expand Down Expand Up @@ -94,6 +95,15 @@ impl SystemExecutor for SingleThreadedExecutor {
let system = &mut schedule.systems[system_index];
if should_run {
let valid_params = system.validate_param(world);
if !valid_params {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: system.name(),
last_run: system.get_last_run(),
},
);
}
should_run &= valid_params;
}

Expand Down Expand Up @@ -196,6 +206,8 @@ impl SingleThreadedExecutor {
}

fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool {
let error_handler: fn(BevyError, ErrorContext) = default_error_handler();

#[expect(
clippy::unnecessary_fold,
reason = "Short-circuiting here would prevent conditions from mutating their own state as needed."
Expand All @@ -204,6 +216,13 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
.iter_mut()
.map(|condition| {
if !condition.validate_param(world) {
error_handler(
SystemParamValidationError.into(),
ErrorContext::RunCondition {
name: condition.name(),
last_run: condition.get_last_run(),
},
);
return false;
}
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
Expand Down
Loading