diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index 54bb81567ae19..5567075015687 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -148,6 +148,23 @@ impl Access { /// An [`Access`] that has been filtered to include and exclude certain combinations of elements. /// /// Used internally to statically check if queries are disjoint. +/// +/// Subtle: a `read` or `write` in `access` should not be considered to imply a +/// `with` access. +/// +/// For example consider `Query>` this only has a `read` of `T` as doing +/// otherwise would allow for queriess to be considered disjoint that actually arent: +/// - `Query<(&mut T, Option<&U>)>` read/write `T`, read `U`, with `U` +/// - `Query<&mut T, Without>` read/write `T`, without `U` +/// from this we could reasonably conclude that the queries are disjoint but they aren't. +/// +/// In order to solve this the actual access that `Query<(&mut T, Option<&U>)>` has +/// is read/write `T`, read `U`. It must still have a read `U` access otherwise the following +/// queries would be incorrectly considered disjoint: +/// - `Query<&mut T>` read/write `T` +/// - `Query` accesses nothing +/// +/// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information. #[derive(Debug, Clone, Eq, PartialEq)] pub struct FilteredAccess { access: Access, @@ -210,6 +227,15 @@ impl FilteredAccess { self.without.insert(index.sparse_set_index()); } + pub fn extend_intersect_filter(&mut self, other: &FilteredAccess) { + self.without.intersect_with(&other.without); + self.with.intersect_with(&other.with); + } + + pub fn extend_access(&mut self, other: &FilteredAccess) { + self.access.extend(&other.access); + } + /// Returns `true` if this and `other` can be active at the same time. pub fn is_compatible(&self, other: &FilteredAccess) -> bool { if self.access.is_compatible(&other.access) { diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 5a89bc5280f63..d8fe776548c0a 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -1091,7 +1091,13 @@ unsafe impl FetchState for OptionState { } fn update_component_access(&self, access: &mut FilteredAccess) { - self.state.update_component_access(access); + // We don't want to add the `with`/`without` of `T` as `Option` will match things regardless of + // `T`'s filters. for example `Query<(Option<&U>, &mut V)>` will match every entity with a `V` component + // regardless of whether it has a `U` component. If we dont do this the query will not conflict with + // `Query<&mut V, Without>` which would be unsound. + let mut intermediate = access.clone(); + self.state.update_component_access(&mut intermediate); + access.extend_access(&intermediate); } fn update_archetype_component_access( @@ -1660,7 +1666,34 @@ macro_rules! impl_anytuple_fetch { fn update_component_access(&self, _access: &mut FilteredAccess) { let ($($name,)*) = &self.0; - $($name.update_component_access(_access);)* + + // We do not unconditionally add `$name`'s `with`/`without` accesses to `_access` + // as this would be unsound. For example the following two queries should conflict: + // - Query<(AnyOf<(&A, ())>, &mut B)> + // - Query<&mut B, Without> + // + // If we were to unconditionally add `$name`'s `with`/`without` accesses then `AnyOf<(&A, ())>` + // would have a `With` access which is incorrect as this `WorldQuery` will match entities that + // do not have the `A` component. This is the same logic as the `Or<...>: WorldQuery` impl. + // + // The correct thing to do here is to only add a `with`/`without` access to `_access` if all + // `$name` params have that `with`/`without` access. More jargony put- we add the intersection + // of all `with`/`without` accesses of the `$name` params to `_access`. + let mut _intersected_access = _access.clone(); + let mut _not_first = false; + $( + if _not_first { + let mut intermediate = _access.clone(); + $name.update_component_access(&mut intermediate); + _intersected_access.extend_intersect_filter(&intermediate); + _intersected_access.extend_access(&intermediate); + } else { + $name.update_component_access(&mut _intersected_access); + _not_first = true; + } + )* + + *_access = _intersected_access; } fn update_archetype_component_access(&self, _archetype: &Archetype, _access: &mut Access) { diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 63c83f6350b1a..36dc70844eff8 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -442,7 +442,34 @@ macro_rules! impl_query_filter_tuple { fn update_component_access(&self, access: &mut FilteredAccess) { let ($($filter,)*) = &self.0; - $($filter.update_component_access(access);)* + + // We do not unconditionally add `$filter`'s `with`/`without` accesses to `access` + // as this would be unsound. For example the following two queries should conflict: + // - Query<&mut B, Or<(With, ())>> + // - Query<&mut B, Without> + // + // If we were to unconditionally add `$name`'s `with`/`without` accesses then `Or<(With, ())>` + // would have a `With` access which is incorrect as this `WorldQuery` will match entities that + // do not have the `A` component. This is the same logic as the `AnyOf<...>: WorldQuery` impl. + // + // The correct thing to do here is to only add a `with`/`without` access to `_access` if all + // `$filter` params have that `with`/`without` access. More jargony put- we add the intersection + // of all `with`/`without` accesses of the `$filter` params to `access`. + let mut _intersected_access = access.clone(); + let mut _not_first = false; + $( + if _not_first { + let mut intermediate = access.clone(); + $filter.update_component_access(&mut intermediate); + _intersected_access.extend_intersect_filter(&intermediate); + _intersected_access.extend_access(&intermediate); + } else { + $filter.update_component_access(&mut _intersected_access); + _not_first = true; + } + )* + + *access = _intersected_access; } fn update_archetype_component_access(&self, archetype: &Archetype, access: &mut Access) { diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index b235467d9cc6b..254d601d56ee9 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -100,6 +100,7 @@ mod tests { bundle::Bundles, component::{Component, Components}, entity::{Entities, Entity}, + prelude::AnyOf, query::{Added, Changed, Or, With, Without}, schedule::{Schedule, Stage, SystemStage}, system::{ @@ -281,6 +282,65 @@ mod tests { assert_eq!(world.resource::().0, 2); } + #[test] + #[should_panic = "error[B0001]"] + fn option_has_no_filter_with() { + fn sys(_: Query<(Option<&A>, &mut B)>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn option_doesnt_remove_unrelated_filter_with() { + fn sys(_: Query<(Option<&A>, &mut B, &A)>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn any_of_has_no_filter_with() { + fn sys(_: Query<(AnyOf<(&A, ())>, &mut B)>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn any_of_has_filter_with_when_both_have_it() { + fn sys(_: Query<(AnyOf<(&A, &A)>, &mut B)>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn any_of_doesnt_remove_unrelated_filter_with() { + fn sys(_: Query<(AnyOf<(&A, ())>, &mut B, &A)>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn or_has_no_filter_with() { + fn sys(_: Query<&mut B, Or<(With, With)>>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn or_has_filter_with_when_both_have_it() { + fn sys(_: Query<&mut B, Or<(With, With)>>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + fn or_doesnt_remove_unrelated_filter_with() { + fn sys(_: Query<&mut B, (Or<(With, With)>, With)>, _: Query<&mut B, Without>) {} + let mut world = World::default(); + run_system(&mut world, sys); + } + #[test] #[should_panic] fn conflicting_query_mut_system() {