Skip to content

Commit

Permalink
Fix access checks for DeferredWorld as SystemParam. (#17616)
Browse files Browse the repository at this point in the history
# Objective

Prevent unsound uses of `DeferredWorld` as a `SystemParam`. It is
currently unsound because it does not check for existing access, and
because it incorrectly registers filtered access.

## Solution

Have `DeferredWorld` panic if a previous parameter has conflicting
access.

Have `DeferredWorld` update `archetype_component_access` so that the
multi-threaded executor sees the access.

Fix `FilteredAccessSet::read_all()` and `write_all()` to correctly add a
`FilteredAccess` with no filter so that `Query` is able to detect the
conflicts.

Remove redundant `read_all()` call, since `write_all()` already declares
read access.

Remove unnecessary `set_has_deferred()` call, since `<DeferredWorld as
SystemParam>::apply_deferred()` does nothing. Previously we were
inserting unnecessary `apply_deferred` systems in the schedule.

## Testing

Added unit tests for systems where `DeferredWorld` conflicts with a
`Query` in the same system.
  • Loading branch information
chescock authored Feb 3, 2025
1 parent b978b13 commit 2d66099
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 7 deletions.
18 changes: 16 additions & 2 deletions crates/bevy_ecs/src/query/access.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,16 @@ impl<T: SparseSetIndex> Access<T> {
self.writes_all_resources || !self.resource_writes.is_clear()
}

/// Returns `true` if this accesses any resources or components.
pub fn has_any_read(&self) -> bool {
self.has_any_component_read() || self.has_any_resource_read()
}

/// Returns `true` if this accesses any resources or components mutably.
pub fn has_any_write(&self) -> bool {
self.has_any_component_write() || self.has_any_resource_write()
}

/// Returns true if this has an archetypal (indirect) access to the component given by `index`.
///
/// This is a component whose value is not accessed (and thus will never cause conflicts),
Expand Down Expand Up @@ -1319,12 +1329,16 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {

/// Marks the set as reading all possible indices of type T.
pub fn read_all(&mut self) {
self.combined_access.read_all();
let mut filter = FilteredAccess::matches_everything();
filter.read_all();
self.add(filter);
}

/// Marks the set as writing all T.
pub fn write_all(&mut self) {
self.combined_access.write_all();
let mut filter = FilteredAccess::matches_everything();
filter.write_all();
self.add(filter);
}

/// Removes all accesses stored in this set.
Expand Down
45 changes: 42 additions & 3 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ mod tests {
Commands, In, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, Res, ResMut,
Single, StaticSystemParam, System, SystemState,
},
world::{EntityMut, FromWorld, World},
world::{DeferredWorld, EntityMut, FromWorld, World},
};

#[derive(Resource, PartialEq, Debug)]
Expand Down Expand Up @@ -1599,13 +1599,22 @@ mod tests {

#[test]
#[should_panic(
expected = "error[B0001]: Query<EntityMut, ()> in system bevy_ecs::system::tests::assert_world_and_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"
expected = "error[B0001]: Query<EntityMut, ()> in system bevy_ecs::system::tests::assert_world_and_entity_mut_system_does_conflict_first::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"
)]
fn assert_world_and_entity_mut_system_does_conflict() {
fn assert_world_and_entity_mut_system_does_conflict_first() {
fn system(_query: &World, _q2: Query<EntityMut>) {}
super::assert_system_does_not_conflict(system);
}

#[test]
#[should_panic(
expected = "&World conflicts with a previous mutable system parameter. Allowing this would break Rust's mutability rules"
)]
fn assert_world_and_entity_mut_system_does_conflict_second() {
fn system(_: Query<EntityMut>, _: &World) {}
super::assert_system_does_not_conflict(system);
}

#[test]
#[should_panic(
expected = "error[B0001]: Query<EntityMut, ()> in system bevy_ecs::system::tests::assert_entity_ref_and_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"
Expand All @@ -1624,6 +1633,36 @@ mod tests {
super::assert_system_does_not_conflict(system);
}

#[test]
#[should_panic(
expected = "error[B0001]: Query<EntityRef, ()> in system bevy_ecs::system::tests::assert_deferred_world_and_entity_ref_system_does_conflict_first::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"
)]
fn assert_deferred_world_and_entity_ref_system_does_conflict_first() {
fn system(_world: DeferredWorld, _query: Query<EntityRef>) {}
super::assert_system_does_not_conflict(system);
}

#[test]
#[should_panic(
expected = "DeferredWorld in system bevy_ecs::system::tests::assert_deferred_world_and_entity_ref_system_does_conflict_second::system conflicts with a previous access."
)]
fn assert_deferred_world_and_entity_ref_system_does_conflict_second() {
fn system(_query: Query<EntityRef>, _world: DeferredWorld) {}
super::assert_system_does_not_conflict(system);
}

#[test]
fn assert_deferred_world_and_empty_query_does_not_conflict_first() {
fn system(_world: DeferredWorld, _query: Query<Entity>) {}
super::assert_system_does_not_conflict(system);
}

#[test]
fn assert_deferred_world_and_empty_query_does_not_conflict_second() {
fn system(_query: Query<Entity>, _world: DeferredWorld) {}
super::assert_system_does_not_conflict(system);
}

#[test]
#[should_panic]
fn panic_inside_system() {
Expand Down
11 changes: 9 additions & 2 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1072,9 +1072,16 @@ unsafe impl<'w> SystemParam for DeferredWorld<'w> {
type Item<'world, 'state> = DeferredWorld<'world>;

fn init_state(_world: &mut World, system_meta: &mut SystemMeta) -> Self::State {
system_meta.component_access_set.read_all();
assert!(
!system_meta
.component_access_set
.combined_access()
.has_any_read(),
"DeferredWorld in system {} conflicts with a previous access.",
system_meta.name,
);
system_meta.component_access_set.write_all();
system_meta.set_has_deferred();
system_meta.archetype_component_access.write_all();
}

unsafe fn get_param<'world, 'state>(
Expand Down

0 comments on commit 2d66099

Please sign in to comment.