Skip to content

Define system param validation on a per-system parameter basis #18504

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 34 commits into from
Mar 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
acb4d73
Document that the global error handler is used to handle missing syst…
alice-i-cecile Mar 20, 2025
a2e7f3f
Move the fallible system error handling module into bevy_ecs::error
alice-i-cecile Mar 20, 2025
c00ad94
Assorted cleanup for fallible_params example
alice-i-cecile Mar 20, 2025
72a368b
Remove ParamWarnPolicy and friends completely
alice-i-cecile Mar 20, 2025
0dcc436
Explain intended fallback behavior for validate_param
alice-i-cecile Mar 20, 2025
71f93cf
Use the GLOBAL_ERROR_HANDLER for validation behavior in SystemParams …
alice-i-cecile Mar 20, 2025
098c8cd
Uncontroversial CI fixes
alice-i-cecile Mar 20, 2025
272ad85
Remove test that cannot be fixed currently
alice-i-cecile Mar 20, 2025
d2ffe4a
Use global error handler when observers fail validation
alice-i-cecile Mar 20, 2025
bd4fbaa
Add required feature to example
alice-i-cecile Mar 20, 2025
5527f08
Simplify SystemParamValidationError as type of failure is stored in E…
alice-i-cecile Mar 23, 2025
9212623
Merge branch 'main' into globally-fallible-system-params
alice-i-cecile Mar 23, 2025
8b983cb
Remove misleading example comments for now
alice-i-cecile Mar 23, 2025
ba3b2f2
Define and use a ValidationBehavior enum
alice-i-cecile Mar 23, 2025
0349503
Clippy
alice-i-cecile Mar 23, 2025
a6e3df1
Merge branch 'main' into globally-fallible-system-params
alice-i-cecile Mar 23, 2025
3ae2f34
Remove another outdated comment
alice-i-cecile Mar 24, 2025
28c809a
Fix macro todo thanks to @chescock <3
alice-i-cecile Mar 24, 2025
fdae8ae
Properly qualify NonSendMarker import
alice-i-cecile Mar 24, 2025
a9d7db2
Merge branch 'globally-fallible-system-params' into per-paramlity
alice-i-cecile Mar 24, 2025
47135ee
Merge remote-tracking branch 'upstream/main' into per-paramlity
alice-i-cecile Mar 24, 2025
ca12af8
Revert broken doc comment
alice-i-cecile Mar 24, 2025
9c5e3ec
Fix accidentally removed skipped_system insertion (thanks @chescock)
alice-i-cecile Mar 24, 2025
27ed3f9
Typo
alice-i-cecile Mar 24, 2025
42f6e7e
Polish comments for the example
alice-i-cecile Mar 24, 2025
a241587
Fix docs for Single and Populated
alice-i-cecile Mar 24, 2025
ffa31ca
Missed if statement
alice-i-cecile Mar 24, 2025
d438897
Fix broken doc link
alice-i-cecile Mar 24, 2025
8c3a645
Clippy
alice-i-cecile Mar 24, 2025
5b7a553
Remove unneeded unsafe
alice-i-cecile Mar 24, 2025
ad6a567
Merge remote-tracking branch 'origin/per-paramlity' into per-paramlity
alice-i-cecile Mar 24, 2025
1ae7646
Add tests
alice-i-cecile Mar 24, 2025
9ac7652
Clippy please...
alice-i-cecile Mar 24, 2025
9d25684
Encourage short-circuiting
alice-i-cecile Mar 24, 2025
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
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
state: &'s Self::State,
system_meta: &#path::system::SystemMeta,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
) -> bool {
) -> #path::system::ValidationOutcome {
<(#(#tuple_types,)*) as #path::system::SystemParam>::validate_param(&state.state, system_meta, world)
}

Expand Down
32 changes: 17 additions & 15 deletions 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, SystemParamValidationError},
system::{IntoObserverSystem, ObserverSystem, SystemParamValidationError, ValidationOutcome},
world::DeferredWorld,
};
use bevy_ptr::PtrMut;
Expand Down Expand Up @@ -405,25 +405,27 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
// - system is the same type erased system from above
unsafe {
(*system).update_archetype_component_access(world);
if (*system).validate_param_unsafe(world) {
if let Err(err) = (*system).run_unsafe(trigger, world) {
error_handler(
err,
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
};
(*system).queue_deferred(world.into_deferred());
} else {
error_handler(
match (*system).validate_param_unsafe(world) {
ValidationOutcome::Valid => {
if let Err(err) = (*system).run_unsafe(trigger, world) {
error_handler(
err,
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
};
(*system).queue_deferred(world.into_deferred());
}
ValidationOutcome::Invalid => error_handler(
SystemParamValidationError.into(),
ErrorContext::Observer {
name: (*system).name(),
last_run: (*system).get_last_run(),
},
);
),
ValidationOutcome::Skipped => (),
}
}
}
Expand Down
102 changes: 75 additions & 27 deletions crates/bevy_ecs/src/schedule/executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
prelude::{IntoSystemSet, SystemSet},
query::Access,
schedule::{BoxedCondition, InternedSystemSet, NodeId, SystemTypeSet},
system::{ScheduleSystem, System, SystemIn},
system::{ScheduleSystem, System, SystemIn, ValidationOutcome},
world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
};

Expand Down Expand Up @@ -221,10 +221,10 @@ impl System for ApplyDeferred {

fn queue_deferred(&mut self, _world: DeferredWorld) {}

unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> bool {
unsafe fn validate_param_unsafe(&mut self, _world: UnsafeWorldCell) -> ValidationOutcome {
// This system is always valid to run because it doesn't do anything,
// and only used as a marker for the executor.
true
ValidationOutcome::Valid
}

fn initialize(&mut self, _world: &mut World) {}
Expand Down Expand Up @@ -312,49 +312,97 @@ mod __rust_begin_short_backtrace {
#[cfg(test)]
mod tests {
use crate::{
prelude::{IntoScheduleConfigs, Resource, Schedule, SystemSet},
prelude::{Component, Resource, Schedule},
schedule::ExecutorKind,
system::Commands,
system::{Populated, Res, ResMut, Single},
world::World,
};

#[derive(Resource)]
struct R1;

#[derive(Resource)]
struct R2;
#[derive(Component)]
struct TestComponent;

const EXECUTORS: [ExecutorKind; 3] = [
ExecutorKind::Simple,
ExecutorKind::SingleThreaded,
ExecutorKind::MultiThreaded,
];

#[derive(Resource, Default)]
struct TestState {
populated_ran: bool,
single_ran: bool,
}

fn set_single_state(mut _single: Single<&TestComponent>, mut state: ResMut<TestState>) {
state.single_ran = true;
}

fn set_populated_state(
mut _populated: Populated<&TestComponent>,
mut state: ResMut<TestState>,
) {
state.populated_ran = true;
}

#[test]
fn invalid_system_param_skips() {
#[expect(clippy::print_stdout, reason = "std and println are allowed in tests")]
fn single_and_populated_skipped_and_run() {
for executor in EXECUTORS {
invalid_system_param_skips_core(executor);
std::println!("Testing executor: {:?}", executor);

let mut world = World::new();
world.init_resource::<TestState>();

let mut schedule = Schedule::default();
schedule.set_executor_kind(executor);
schedule.add_systems((set_single_state, set_populated_state));
schedule.run(&mut world);

let state = world.get_resource::<TestState>().unwrap();
assert!(!state.single_ran);
assert!(!state.populated_ran);

world.spawn(TestComponent);

schedule.run(&mut world);
let state = world.get_resource::<TestState>().unwrap();
assert!(state.single_ran);
assert!(state.populated_ran);
}
}

fn invalid_system_param_skips_core(executor: ExecutorKind) {
fn look_for_missing_resource(_res: Res<TestState>) {}

#[test]
#[should_panic]
fn missing_resource_panics_simple() {
let mut world = World::new();
let mut schedule = Schedule::default();
schedule.set_executor_kind(executor);
schedule.add_systems(
(
// This system depends on a system that is always skipped.
(|mut commands: Commands| {
commands.insert_resource(R2);
}),
)
.chain(),
);

schedule.set_executor_kind(ExecutorKind::Simple);
schedule.add_systems(look_for_missing_resource);
schedule.run(&mut world);
}

#[test]
#[should_panic]
fn missing_resource_panics_single_threaded() {
let mut world = World::new();
let mut schedule = Schedule::default();

schedule.set_executor_kind(ExecutorKind::SingleThreaded);
schedule.add_systems(look_for_missing_resource);
schedule.run(&mut world);
assert!(world.get_resource::<R1>().is_none());
assert!(world.get_resource::<R2>().is_some());
}

#[derive(SystemSet, Hash, Debug, PartialEq, Eq, Clone)]
struct S1;
#[test]
#[should_panic]
fn missing_resource_panics_multi_threaded() {
let mut world = World::new();
let mut schedule = Schedule::default();

schedule.set_executor_kind(ExecutorKind::MultiThreaded);
schedule.add_systems(look_for_missing_resource);
schedule.run(&mut world);
}
}
49 changes: 29 additions & 20 deletions crates/bevy_ecs/src/schedule/executor/multi_threaded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::{
prelude::Resource,
query::Access,
schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
system::{ScheduleSystem, SystemParamValidationError},
system::{ScheduleSystem, SystemParamValidationError, ValidationOutcome},
world::{unsafe_world_cell::UnsafeWorldCell, World},
};

Expand Down Expand Up @@ -581,18 +581,24 @@ impl ExecutorState {
// - The caller ensures that `world` has permission to read any data
// required by the system.
// - `update_archetype_component_access` has been called for system.
let valid_params = unsafe { system.validate_param_unsafe(world) };
let valid_params = match unsafe { system.validate_param_unsafe(world) } {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This match is repeated in quite a few places. Would it make sense to put it in a shared method somewhere, maybe on System? Then the only change at the call sites would be changing the method name and passing an extra error_handler parameter (unless we want to use default_error_handler() in observers).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the problem outlined in #18453, but yeah, maybe we can extract it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried extracting this out, but I'm not pleased with how the straightforward approach requires eagerly constructing an ErrorContext in the happy path. IMO this is better suited to a proper overhaul of our executor stack.

ValidationOutcome::Valid => true,
ValidationOutcome::Invalid => {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: system.name(),
last_run: system.get_last_run(),
},
);
false
}
ValidationOutcome::Skipped => false,
};
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 @@ -789,16 +795,19 @@ unsafe fn evaluate_and_fold_conditions(
// - The caller ensures that `world` has permission to read any data
// 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;
match unsafe { condition.validate_param_unsafe(world) } {
ValidationOutcome::Valid => (),
ValidationOutcome::Invalid => {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: condition.name(),
last_run: condition.get_last_run(),
},
);
return false;
}
ValidationOutcome::Skipped => return false,
}
// SAFETY:
// - The caller ensures that `world` has permission to read any data
Expand Down
49 changes: 28 additions & 21 deletions crates/bevy_ecs/src/schedule/executor/simple.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{
schedule::{
executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
},
system::SystemParamValidationError,
system::{SystemParamValidationError, ValidationOutcome},
world::World,
};

Expand Down Expand Up @@ -88,17 +88,20 @@ 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(),
},
);
}

let valid_params = match system.validate_param(world) {
ValidationOutcome::Valid => true,
ValidationOutcome::Invalid => {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: system.name(),
last_run: system.get_last_run(),
},
);
false
}
ValidationOutcome::Skipped => false,
};
should_run &= valid_params;
}

Expand Down Expand Up @@ -173,15 +176,19 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut W
conditions
.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;
match condition.validate_param(world) {
ValidationOutcome::Valid => (),
ValidationOutcome::Invalid => {
error_handler(
SystemParamValidationError.into(),
ErrorContext::System {
name: condition.name(),
last_run: condition.get_last_run(),
},
);
return false;
}
ValidationOutcome::Skipped => return false,
}
__rust_begin_short_backtrace::readonly_run(&mut **condition, world)
})
Expand Down
Loading