From 2d66099f3d6a169a746600a43ca66c26ec8b632f Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Mon, 3 Feb 2025 16:58:07 -0500 Subject: [PATCH] Fix access checks for DeferredWorld as SystemParam. (#17616) # 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 `::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. --- crates/bevy_ecs/src/query/access.rs | 18 ++++++++- crates/bevy_ecs/src/system/mod.rs | 45 ++++++++++++++++++++-- crates/bevy_ecs/src/system/system_param.rs | 11 +++++- 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 668b4c5c30..089d6914c6 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -309,6 +309,16 @@ impl Access { 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), @@ -1319,12 +1329,16 @@ impl FilteredAccessSet { /// 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. diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 824b178464..d5cbd5ffb1 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -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)] @@ -1599,13 +1599,22 @@ mod tests { #[test] #[should_panic( - expected = "error[B0001]: Query 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` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001" + expected = "error[B0001]: Query 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` 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) {} 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, _: &World) {} + super::assert_system_does_not_conflict(system); + } + #[test] #[should_panic( expected = "error[B0001]: Query 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` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001" @@ -1624,6 +1633,36 @@ mod tests { super::assert_system_does_not_conflict(system); } + #[test] + #[should_panic( + expected = "error[B0001]: Query 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` 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) {} + 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, _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) {} + super::assert_system_does_not_conflict(system); + } + + #[test] + fn assert_deferred_world_and_empty_query_does_not_conflict_second() { + fn system(_query: Query, _world: DeferredWorld) {} + super::assert_system_does_not_conflict(system); + } + #[test] #[should_panic] fn panic_inside_system() { diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index b475233f8c..5859b48346 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -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>(