Skip to content

Merge SystemParam::validate_param into SystemParam::get_param #23225

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.

Sign up for GitHub

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
alice-i-cecile merged 24 commits into from
Mar 11, 2026
Merged

Merge SystemParam::validate_param into SystemParam::get_param #23225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9d3a8ed
Describe strategy in the migration guide
alice-i-cecile Mar 4, 2026
b92e3d9
Note `SystemState` changes that will be needed
alice-i-cecile Mar 4, 2026
1b739a6
Initial migration
alice-i-cecile Mar 4, 2026
7c0c045
Beef up migration guide to account for changes required
alice-i-cecile Mar 4, 2026
8303d4f
Add tests for validation occurring when systems are run
alice-i-cecile Mar 4, 2026
fbcbbb9
Perform the same changes for ExclusiveSystemParam
alice-i-cecile Mar 4, 2026
026c974
Cargo fmt
alice-i-cecile Mar 4, 2026
fbff8d2
Note panic in `ParamSet::get_mut`
alice-i-cecile Mar 4, 2026
e604357
Improve migration guide for Multithreaded executor
alice-i-cecile Mar 4, 2026
0a280d6
h2 headers in migration guide
alice-i-cecile Mar 4, 2026
b2782dd
Clippy
alice-i-cecile Mar 4, 2026
eeab64a
Fill in PR number
alice-i-cecile Mar 4, 2026
dddb08e
Clippy
alice-i-cecile Mar 4, 2026
3c9560f
Fix failing tests
alice-i-cecile Mar 4, 2026
40ef199
Revert incidental behavior change to `Single`
alice-i-cecile Mar 5, 2026
8bcbf40
Add pipe_system_validation_skip_in_second test
alice-i-cecile Mar 5, 2026
fdaaeb5
Refactor gizmos::get_config
alice-i-cecile Mar 5, 2026
d151037
Don't format twice for no reason
alice-i-cecile Mar 5, 2026
69cb5c0
Validate `ParamSet` sub-params eagerly, and skip if any fail
alice-i-cecile Mar 6, 2026
4958e80
Remove panics comment
alice-i-cecile Mar 9, 2026
3f700e1
Merge branch 'main' into param-validation-merging
alice-i-cecile Mar 9, 2026
90ac6a1
Fix failing test by making sure error message does not change
alice-i-cecile Mar 10, 2026
5c5674f
Update docs on get_param to match chescock's suggestion
alice-i-cecile Mar 10, 2026
34fc43b
Fix missing unwrap...
alice-i-cecile Mar 11, 2026
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
4 changes: 2 additions & 2 deletions benches/benches/bevy_ecs/fragmentation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn iter_frag_empty(c: &mut Criterion) {
spawn_empty_frag_archetype::<Table>(&mut world);
let mut q: SystemState<Query<(Entity, &Table)>> =
SystemState::<Query<(Entity, &Table<0>)>>::new(&mut world);
let query = q.get(&world);
let query = q.get(&world).unwrap();
b.iter(move || {
let mut res = 0;
query.iter().for_each(|(e, t)| {
Expand All @@ -39,7 +39,7 @@ fn iter_frag_empty(c: &mut Criterion) {
spawn_empty_frag_archetype::<Sparse>(&mut world);
let mut q: SystemState<Query<(Entity, &Sparse)>> =
SystemState::<Query<(Entity, &Sparse<0>)>>::new(&mut world);
let query = q.get(&world);
let query = q.get(&world).unwrap();
b.iter(move || {
let mut res = 0;
query.iter().for_each(|(e, t)| {
Expand Down
8 changes: 4 additions & 4 deletions benches/benches/bevy_ecs/world/world_get.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ pub fn query_get(criterion: &mut Criterion) {
.collect();
entities.shuffle(&mut deterministic_rand());
let mut query = SystemState::<Query<&Table>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

@alice-i-cecile alice-i-cecile Mar 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TBH this is a pretty weird pattern. Who uses SystemState to query on the World?

@chescock chescock Mar 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TBH this is a pretty weird pattern. Who uses SystemState to query on the World?

It's silly when there is only one Query since they could just have used QueryState, but using SystemState<(Query<A>, Query<B>)> to split the world seems reasonable. And it's going to feel bad to have to unwrap() there even though Query parameters never fail.

I think we're eventually going to want an InfallibleSystemParam subtrait like

trait InfallibleSystemParam: SystemParam {
    fn get_param(/* same params as `SystemParam::get_param` */) -> Self::Item<'w, 's>;
}

And then have fallible and infallible versions of SystemState::get.

That doesn't need to be in this PR, of course, although it will feel a little silly to add all these unwrap()s if we then get to remove them again soon.

@alice-i-cecile alice-i-cecile Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that we should instead be pushing users to just call World::run_system more for these sorts of world-splitting use cases. The ergonomics are ultimately always going to be much better, and it's more consistent with other usages.

Then: no more internal complexity!

@chescock chescock Mar 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that we should instead be pushing users to just call World::run_system more for these sorts of world-splitting use cases. The ergonomics are ultimately always going to be much better, and it's more consistent with other usages.

Then: no more internal complexity!

Yeah, run_system_cached has really nice ergonomics when it works! I don't think it's a complete replacement for SystemState yet, though. It's a little less flexible, since needing a function prevents certain kinds of control flow, and it's a little less efficient, since you need to look up the system state in the world.

@alice-i-cecile alice-i-cecile Mar 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mhmm, but for tests and benchmarks it's a much better pattern! It just didn't exist when these were written.


bencher.iter(|| {
let mut count = 0;
Expand All @@ -288,7 +288,7 @@ pub fn query_get(criterion: &mut Criterion) {
.collect();
entities.shuffle(&mut deterministic_rand());
let mut query = SystemState::<Query<&Sparse>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand Down Expand Up @@ -319,7 +319,7 @@ pub fn query_get_many<const N: usize>(criterion: &mut Criterion) {
entity_groups.shuffle(&mut deterministic_rand());

let mut query = SystemState::<Query<&Table>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand All @@ -342,7 +342,7 @@ pub fn query_get_many<const N: usize>(criterion: &mut Criterion) {
entity_groups.shuffle(&mut deterministic_rand());

let mut query = SystemState::<Query<&Sparse>>::new(&mut world);
let query = query.get(&world);
let query = query.get(&world).unwrap();

bencher.iter(|| {
let mut count = 0;
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/compile_fail/tests/ui/query_lens_lifetime_safety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ fn main() {

let mut system_state = SystemState::<(Query<&mut Foo>, Query<&mut Bar>)>::new(&mut world);
{
let (mut foo_query, mut bar_query) = system_state.get_mut(&mut world);
let (mut foo_query, mut bar_query) = system_state.get_mut(&mut world).unwrap();
dbg!("hi");
{
let mut lens = foo_query.as_query_lens();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/compile_fail/tests/ui/query_lifetime_safety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fn main() {

let mut system_state = SystemState::<Query<&mut Foo>>::new(&mut world);
{
let mut query = system_state.get_mut(&mut world);
let mut query = system_state.get_mut(&mut world).unwrap();
dbg!("hi");
{
let data: &Foo = query.get(e).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/compile_fail/tests/ui/query_transmute_safety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn main() {
world.spawn(Foo(10));

let mut system_state = SystemState::<Query<(&mut Foo, &Bar)>>::new(&mut world);
let mut query = system_state.get_mut(&mut world);
let mut query = system_state.get_mut(&mut world).unwrap();

{
let mut lens_a = query.transmute_lens::<&mut Foo>();
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/compile_fail/tests/ui/system_state_get_lifetime_safety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ struct State {

impl State {
fn get_component(&mut self, world: &mut World, entity: Entity) {
let q1 = self.state_r.get(&world);
let q1 = self.state_r.get(&world).unwrap();
let a1 = q1.get(entity).unwrap();

let mut q2 = self.state_w.get_mut(world);
let mut q2 = self.state_w.get_mut(world).unwrap();
//~^ E0502
let _ = q2.get_mut(entity).unwrap();

Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_lifetime_safety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ struct State {

impl State {
fn get_components(&mut self, world: &mut World) {
let q1 = self.state_r.get(&world);
let q1 = self.state_r.get(&world).unwrap();
let a1 = q1.iter().next().unwrap();

let mut q2 = self.state_w.get_mut(world);
let mut q2 = self.state_w.get_mut(world).unwrap();
//~^ E0502
let _ = q2.iter_mut().next().unwrap();

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/compile_fail/tests/ui/system_state_iter_mut_overlap_safety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ fn main() {

let mut system_state = SystemState::<Query<&mut A>>::new(&mut world);
{
let mut query = system_state.get_mut(&mut world);
let mut query = system_state.get_mut(&mut world).unwrap();
let mut_vec = query.iter_mut().collect::<Vec<bevy_ecs::prelude::Mut<A>>>();
assert_eq!(
// this should fail to compile due to the later use of mut_vec
Expand Down
29 changes: 9 additions & 20 deletions crates/bevy_ecs/macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,33 +449,22 @@ fn derive_system_param_impl(
<#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::queue(&mut state.state, system_meta, world);
}

#[inline]
unsafe fn validate_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &#path::system::SystemMeta,
_world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
) -> Result<(), #path::system::SystemParamValidationError> {
let #state_struct_name { state: (#(#tuple_patterns,)*) } = state;
#(
<#field_types as #path::system::SystemParam>::validate_param(#field_locals, _system_meta, _world)
.map_err(|err| #path::system::SystemParamValidationError::new::<Self>(err.skipped, #field_validation_messages, #field_validation_names))?;
)*
Result::Ok(())
}

#[inline]
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
change_tick: #path::change_detection::Tick,
) -> Self::Item<'w, 's> {
let (#(#tuple_patterns,)*) = <
(#(#tuple_types,)*) as #path::system::SystemParam
>::get_param(&mut state.state, system_meta, world, change_tick);
#struct_name {
) -> Result<Self::Item<'w, 's>, #path::system::SystemParamValidationError> {
let (#(#tuple_patterns,)*) = &mut state.state;
#(
let #field_locals = unsafe {
<#field_types as #path::system::SystemParam>::get_param(#field_locals, system_meta, world, change_tick)
}.map_err(|err| #path::system::SystemParamValidationError::new::<Self>(err.skipped, #field_validation_messages, #field_validation_names))?;
)*
Result::Ok(#struct_name {
#(#field_members: #field_locals,)*
}
})
}
}

Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/lifecycle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ use crate::{
query::FilteredAccessSet,
relationship::RelationshipHookMode,
storage::SparseSet,
system::{Local, ReadOnlySystemParam, SystemMeta, SystemParam},
system::{Local, ReadOnlySystemParam, SystemMeta, SystemParam, SystemParamValidationError},
world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
};

Expand Down Expand Up @@ -639,7 +639,7 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentMessages {
_system_meta: &SystemMeta,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.removed_components()
) -> Result<Self::Item<'w, 's>, SystemParamValidationError> {
Ok(world.removed_components())
}
}
26 changes: 3 additions & 23 deletions crates/bevy_ecs/src/message/message_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,35 +162,15 @@ unsafe impl<'w, 's, M: Message> SystemParam for PopulatedMessageReader<'w, 's, M
system_meta: &crate::system::SystemMeta,
world: crate::world::unsafe_world_cell::UnsafeWorldCell<'world>,
change_tick: crate::change_detection::Tick,
) -> Self::Item<'world, 'state> {
) -> Result<Self::Item<'world, 'state>, SystemParamValidationError> {
// SAFETY: requirements are upheld by MessageReader's implementation
unsafe {
PopulatedMessageReader(MessageReader::get_param(
state,
system_meta,
world,
change_tick,
))
}
}

unsafe fn validate_param(
state: &mut Self::State,
system_meta: &crate::system::SystemMeta,
world: crate::world::unsafe_world_cell::UnsafeWorldCell,
) -> Result<(), SystemParamValidationError> {
// SAFETY: requirements are upheld by MessageReader's implementation
unsafe { MessageReader::<M>::validate_param(state, system_meta, world) }?;

// SAFETY: requirements are upheld by MessageReader's implementation
let reader =
unsafe { MessageReader::get_param(state, system_meta, world, world.change_tick()) };
let reader = unsafe { MessageReader::get_param(state, system_meta, world, change_tick)? };
if reader.is_empty() {
Err(SystemParamValidationError::skipped::<Self>(
"message queue is empty",
))
} else {
Ok(())
Ok(PopulatedMessageReader(reader))
}
}
}
Expand Down
6 changes: 1 addition & 5 deletions crates/bevy_ecs/src/observer/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,7 @@ pub(super) unsafe fn observer_system_runner<E: Event, B: Bundle, S: ObserverSyst
(*system).refresh_hotpatch();
};

if let Err(RunSystemError::Failed(err)) = (*system)
.validate_param_unsafe(world)
.map_err(From::from)
.and_then(|()| (*system).run_unsafe(on, world))
{
if let Err(RunSystemError::Failed(err)) = (*system).run_unsafe(on, world) {
let handler = state
.error_handler
.unwrap_or_else(|| world.default_error_handler());
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,7 +774,7 @@ mod tests {

// system param
let mut q = SystemState::<Query<&mut Foo>>::new(&mut world);
let q = q.get_mut(&mut world);
let q = q.get_mut(&mut world).unwrap();
let _: Option<&Foo> = q.iter().next();
let _: Option<[&Foo; 2]> = q.iter_combinations::<2>().next();
let _: Option<&Foo> = q.iter_many([e]).next();
Expand Down
16 changes: 8 additions & 8 deletions crates/bevy_ecs/src/reflect/entity_commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands.spawn_empty().id();
let entity2 = commands.spawn_empty().id();
Expand Down Expand Up @@ -508,7 +508,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands.spawn_empty().id();

Expand Down Expand Up @@ -538,7 +538,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands.spawn(ComponentA(0)).id();

Expand Down Expand Up @@ -567,7 +567,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands.spawn(ComponentA(0)).id();

Expand Down Expand Up @@ -596,7 +596,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands.spawn_empty().id();
let bundle = Box::new(BundleA {
Expand Down Expand Up @@ -626,7 +626,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands.spawn_empty().id();
let bundle = Box::new(BundleA {
Expand Down Expand Up @@ -656,7 +656,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands
.spawn(BundleA {
Expand Down Expand Up @@ -694,7 +694,7 @@ mod tests {
world.insert_resource(type_registry);

let mut system_state: SystemState<Commands> = SystemState::new(&mut world);
let mut commands = system_state.get_mut(&mut world);
let mut commands = system_state.get_mut(&mut world).unwrap();

let entity = commands
.spawn(BundleA {
Expand Down
Loading