Fix unsoundness with Or/AnyOf/Option component access' (bevyeng…

…ine#4659)

# Objective

Fixes bevyengine#4657

Example code that wasnt panic'ing before this PR (and so was unsound):
```rust
    #[test]
    #[should_panic = "error[B0001]"]
    fn option_has_no_filter_with() {
        fn sys(_1: Query<(Option<&A>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn any_of_has_no_filter_with() {
        fn sys(_1: Query<(AnyOf<(&A, ())>, &mut B)>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }

    #[test]
    #[should_panic = "error[B0001]"]
    fn or_has_no_filter_with() {
        fn sys(_1: Query<&mut B, Or<(With<A>, With<B>)>>, _2: Query<&mut B, Without<A>>) {}
        let mut world = World::default();
        run_system(&mut world, sys);
    }
```
## Solution

- Only add the intersection of `with`/`without` accesses of all the elements in `Or/AnyOf` to the world query's `FilteredAccess<ComponentId>` instead of the union.
- `Option`'s fix can be thought of the same way since its basically `AnyOf<T, ()>` but its impl is just simpler as `()` has no `with`/`without` accesses
---

## Changelog

- `Or`/`AnyOf`/`Option` will now report more query conflicts in order to fix unsoundness

## Migration Guide

- If you are now getting query conflicts from `Or`/`AnyOf`/`Option` rip to you and ur welcome for it now being caught

crates/bevy_ecs/src/query/access.rs

@@ -148,6 +148,23 @@ impl<T: SparseSetIndex> Access<T> {
 /// 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<Option<&T>>` 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<U>>` 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<Option<&T>` accesses nothing
+///
+/// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct FilteredAccess<T: SparseSetIndex> {
  access: Access<T>,
@@ -210,6 +227,15 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
  self.without.insert(index.sparse_set_index());
  }
 
+ pub fn extend_intersect_filter(&mut self, other: &FilteredAccess<T>) {
+ self.without.intersect_with(&other.without);
+ self.with.intersect_with(&other.with);
+ }
+
+ pub fn extend_access(&mut self, other: &FilteredAccess<T>) {
+ 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<T>) -> bool {
  if self.access.is_compatible(&other.access) {

crates/bevy_ecs/src/query/fetch.rs

@@ -1091,7 +1091,13 @@ unsafe impl<T: FetchState> FetchState for OptionState<T> {
  }
 
  fn update_component_access(&self, access: &mut FilteredAccess<ComponentId>) {
- self.state.update_component_access(access);
+ // We don't want to add the `with`/`without` of `T` as `Option<T>` 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<U>>` 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<ComponentId>) {
  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<A>>
+ //
+ // If we were to unconditionally add `$name`'s `with`/`without` accesses then `AnyOf<(&A, ())>`
+ // would have a `With<A>` 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<ArchetypeComponentId>) {

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<ComponentId>) {
  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<A>, ())>>
+ // - Query<&mut B, Without<A>>
+ //
+ // If we were to unconditionally add `$name`'s `with`/`without` accesses then `Or<(With<A>, ())>`
+ // would have a `With<A>` 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<ArchetypeComponentId>) {

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::<Changed>().0, 2);
  }
 
+ #[test]
+ #[should_panic = "error[B0001]"]
+ fn option_has_no_filter_with() {
+ fn sys(_: Query<(Option<&A>, &mut B)>, _: Query<&mut B, Without<A>>) {}
+ 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<A>>) {}
+ 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<A>>) {}
+ 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<A>>) {}
+ 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<A>>) {}
+ 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<A>, With<B>)>>, _: Query<&mut B, Without<A>>) {}
+ 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<A>, With<A>)>>, _: Query<&mut B, Without<A>>) {}
+ 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<A>, With<B>)>, With<A>)>, _: Query<&mut B, Without<A>>) {}
+ let mut world = World::default();
+ run_system(&mut world, sys);
+ }
+
  #[test]
  #[should_panic]
  fn conflicting_query_mut_system() {