diff --git a/crates/bevy_ecs/src/query/access.rs b/crates/bevy_ecs/src/query/access.rs index b324792460ad82..99abaa77003cd9 100644 --- a/crates/bevy_ecs/src/query/access.rs +++ b/crates/bevy_ecs/src/query/access.rs @@ -181,6 +181,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); + } + pub fn is_compatible(&self, other: &FilteredAccess) -> bool { if self.access.is_compatible(&other.access) { true diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 9a6ad81fb02e95..2dfaed224d1a41 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 ab0aa8dd03adad..fd4d47b2c5dab8 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 52d8b0a12d6f93..4b21d59bae2e4a 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() {