Fix FilteredAccessSet get_conflicts inconsistency (bevyengine#5105)

# Objective

* Enable `Res` and `Query` parameter mutual exclusion
* Required for bevyengine#5080

The `FilteredAccessSet::get_conflicts` methods didn't work properly with
`Res` and `ResMut` parameters. Because those added their access by using
the `combined_access_mut` method and directly modifying the global
access state of the FilteredAccessSet. This caused an inconsistency,
because get_conflicts assumes that ALL added access have a corresponding
`FilteredAccess` added to the `filtered_accesses` field.

In practice, that means that SystemParam that adds their access through
the `Access` returned by `combined_access_mut` and the ones that add
their access using the `add` method lived in two different universes. As
a result, they could never be mutually exclusive.

## Solution

This commit fixes it by removing the `combined_access_mut` method. This
ensures that the `combined_access` field of FilteredAccessSet is always
updated consistently with the addition of a filter. When checking for
filtered access, it is now possible to account for `Res` and `ResMut`
invalid access. This is currently not needed, but might be in the
future.

We add the `add_unfiltered_{read,write}` methods to replace previous
usages of `combined_access_mut`.

We also add improved Debug implementations on FixedBitSet so that their
meaning is much clearer in debug output.


---

## Changelog

* Fix `Res` and `Query` parameter never being mutually exclusive.

## Migration Guide

Note: this mostly changes ECS internals, but since the API is public, it is technically breaking:
* Removed `FilteredAccessSet::combined_access_mut`
  * Replace _immutable_ usage of those by `combined_access`
  * For _mutable_ usages, use the new `add_unfiltered_{read,write}` methods instead of `combined_access_mut` followed by `add_{read,write}`

crates/bevy_ecs/src/query/access.rs

@@ -1,13 +1,51 @@
 use crate::storage::SparseSetIndex;
 use bevy_utils::HashSet;
+use core::fmt;
 use fixedbitset::FixedBitSet;
 use std::marker::PhantomData;
 
+/// A wrapper struct to make Debug representations of [`FixedBitSet`] easier
+/// to read, when used to store [`SparseSetIndex`].
+///
+/// Instead of the raw integer representation of the `FixedBitSet`, the list of
+/// `T` valid for [`SparseSetIndex`] is shown.
+///
+/// Normal `FixedBitSet` `Debug` output:
+/// ```text
+/// read_and_writes: FixedBitSet { data: [ 160 ], length: 8 }
+/// ```
+///
+/// Which, unless you are a computer, doesn't help much understand what's in
+/// the set. With `FormattedBitSet`, we convert the present set entries into
+/// what they stand for, it is much clearer what is going on:
+/// ```text
+/// read_and_writes: [ ComponentId(5), ComponentId(7) ]
+/// ```
+struct FormattedBitSet<'a, T: SparseSetIndex> {
+    bit_set: &'a FixedBitSet,
+    _marker: PhantomData<T>,
+}
+impl<'a, T: SparseSetIndex> FormattedBitSet<'a, T> {
+    fn new(bit_set: &'a FixedBitSet) -> Self {
+        Self {
+            bit_set,
+            _marker: PhantomData,
+        }
+    }
+}
+impl<'a, T: SparseSetIndex + fmt::Debug> fmt::Debug for FormattedBitSet<'a, T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_list()
+            .entries(self.bit_set.ones().map(T::get_sparse_set_index))
+            .finish()
+    }
+}
+
 /// Tracks read and write access to specific elements in a collection.
 ///
 /// Used internally to ensure soundness during system initialization and execution.
 /// See the [`is_compatible`](Access::is_compatible) and [`get_conflicts`](Access::get_conflicts) functions.
-#[derive(Debug, Clone, Eq, PartialEq)]
+#[derive(Clone, Eq, PartialEq)]
 pub struct Access<T: SparseSetIndex> {
     /// All accessed elements.
     reads_and_writes: FixedBitSet,
@@ -19,6 +57,18 @@ pub struct Access<T: SparseSetIndex> {
     marker: PhantomData<T>,
 }
 
+impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for Access<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("Access")
+            .field(
+                "read_and_writes",
+                &FormattedBitSet::<T>::new(&self.reads_and_writes),
+            )
+            .field("writes", &FormattedBitSet::<T>::new(&self.writes))
+            .field("reads_all", &self.reads_all)
+            .finish()
+    }
+}
 impl<T: SparseSetIndex> Default for Access<T> {
     fn default() -> Self {
         Self {
@@ -55,11 +105,7 @@ impl<T: SparseSetIndex> Access<T> {
 
     /// Returns `true` if this can access the element given by `index`.
     pub fn has_read(&self, index: T) -> bool {
-        if self.reads_all {
-            true
-        } else {
-            self.reads_and_writes.contains(index.sparse_set_index())
-        }
+        self.reads_all || self.reads_and_writes.contains(index.sparse_set_index())
     }
 
     /// Returns `true` if this can exclusively access the element given by `index`.
@@ -106,7 +152,7 @@ impl<T: SparseSetIndex> Access<T> {
         }
 
         self.writes.is_disjoint(&other.reads_and_writes)
-            && self.reads_and_writes.is_disjoint(&other.writes)
+            && other.writes.is_disjoint(&self.reads_and_writes)
     }
 
     /// Returns a vector of elements that the access and `other` cannot access at the same time.
@@ -153,7 +199,7 @@ impl<T: SparseSetIndex> Access<T> {
 /// `with` access.
 ///
 /// For example consider `Query<Option<&T>>` this only has a `read` of `T` as doing
-/// otherwise would allow for queries to be considered disjoint that actually aren't:
+/// otherwise would allow for queries to be considered disjoint when they shouldn't:
 /// - `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.
@@ -165,12 +211,21 @@ impl<T: SparseSetIndex> Access<T> {
 /// - `Query<Option<&T>` accesses nothing
 ///
 /// See comments the `WorldQuery` impls of `AnyOf`/`Option`/`Or` for more information.
-#[derive(Debug, Clone, Eq, PartialEq)]
+#[derive(Clone, Eq, PartialEq)]
 pub struct FilteredAccess<T: SparseSetIndex> {
     access: Access<T>,
     with: FixedBitSet,
     without: FixedBitSet,
 }
+impl<T: SparseSetIndex + fmt::Debug> fmt::Debug for FilteredAccess<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("FilteredAccess")
+            .field("access", &self.access)
+            .field("with", &FormattedBitSet::<T>::new(&self.with))
+            .field("without", &FormattedBitSet::<T>::new(&self.without))
+            .finish()
+    }
+}
 
 impl<T: SparseSetIndex> Default for FilteredAccess<T> {
     fn default() -> Self {
@@ -238,12 +293,9 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
 
     /// 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) {
-            true
-        } else {
-            self.with.intersection(&other.without).next().is_some()
-                || self.without.intersection(&other.with).next().is_some()
-        }
+        self.access.is_compatible(&other.access)
+            || !self.with.is_disjoint(&other.without)
+            || !other.with.is_disjoint(&self.without)
     }
 
     /// Returns a vector of elements that this and `other` cannot access at the same time.
@@ -271,6 +323,10 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
 /// A collection of [`FilteredAccess`] instances.
 ///
 /// Used internally to statically check if systems have conflicting access.
+///
+/// It stores multiple sets of accesses.
+/// - A "combined" set, which is the access of all filters in this set combined.
+/// - The set of access of each individual filters in this set.
 #[derive(Debug, Clone)]
 pub struct FilteredAccessSet<T: SparseSetIndex> {
     combined_access: Access<T>,
@@ -284,13 +340,18 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
         &self.combined_access
     }
 
-    /// Returns a mutable reference to the unfiltered access of the entire set.
-    #[inline]
-    pub fn combined_access_mut(&mut self) -> &mut Access<T> {
-        &mut self.combined_access
-    }
-
     /// Returns `true` if this and `other` can be active at the same time.
+    ///
+    /// Access conflict resolution happen in two steps:
+    /// 1. A "coarse" check, if there is no mutual unfiltered conflict between
+    ///    `self` and `other`, we already know that the two access sets are
+    ///    compatible.
+    /// 2. A "fine grained" check, it kicks in when the "coarse" check fails.
+    ///    the two access sets might still be compatible if some of the accesses
+    ///    are restricted with the `With` or `Without` filters so that access is
+    ///    mutually exclusive. The fine grained phase iterates over all filters in
+    ///    the `self` set and compares it to all the filters in the `other` set,
+    ///    making sure they are all mutually compatible.
     pub fn is_compatible(&self, other: &FilteredAccessSet<T>) -> bool {
         if self.combined_access.is_compatible(other.combined_access()) {
             return true;
@@ -302,7 +363,6 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
                 }
             }
         }
-
         true
     }
 
@@ -338,6 +398,20 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
         self.filtered_accesses.push(filtered_access);
     }
 
+    /// Adds a read access without filters to the set.
+    pub(crate) fn add_unfiltered_read(&mut self, index: T) {
+        let mut filter = FilteredAccess::default();
+        filter.add_read(index);
+        self.add(filter);
+    }
+
+    /// Adds a write access without filters to the set.
+    pub(crate) fn add_unfiltered_write(&mut self, index: T) {
+        let mut filter = FilteredAccess::default();
+        filter.add_write(index);
+        self.add(filter);
+    }
+
     pub fn extend(&mut self, filtered_access_set: FilteredAccessSet<T>) {
         self.combined_access
             .extend(&filtered_access_set.combined_access);
@@ -362,7 +436,30 @@ impl<T: SparseSetIndex> Default for FilteredAccessSet<T> {
 
 #[cfg(test)]
 mod tests {
-    use crate::query::{Access, FilteredAccess};
+    use crate::query::{Access, FilteredAccess, FilteredAccessSet};
+
+    #[test]
+    fn read_all_access_conflicts() {
+        // read_all / single write
+        let mut access_a = Access::<usize>::default();
+        access_a.grow(10);
+        access_a.add_write(0);
+
+        let mut access_b = Access::<usize>::default();
+        access_b.read_all();
+
+        assert!(!access_b.is_compatible(&access_a));
+
+        // read_all / read_all
+        let mut access_a = Access::<usize>::default();
+        access_a.grow(10);
+        access_a.read_all();
+
+        let mut access_b = Access::<usize>::default();
+        access_b.read_all();
+
+        assert!(access_b.is_compatible(&access_a));
+    }
 
     #[test]
     fn access_get_conflicts() {
@@ -391,6 +488,22 @@ mod tests {
         assert_eq!(access_d.get_conflicts(&access_c), vec![0]);
     }
 
+    #[test]
+    fn filtered_combined_access() {
+        let mut access_a = FilteredAccessSet::<usize>::default();
+        access_a.add_unfiltered_read(1);
+
+        let mut filter_b = FilteredAccess::<usize>::default();
+        filter_b.add_write(1);
+
+        let conflicts = access_a.get_conflicts_single(&filter_b);
+        assert_eq!(
+            &conflicts,
+            &[1_usize],
+            "access_a: {access_a:?}, filter_b: {filter_b:?}"
+        );
+    }
+
     #[test]
     fn filtered_access_extend() {
         let mut access_a = FilteredAccess::<usize>::default();

crates/bevy_ecs/src/storage/sparse_set.rs

@@ -421,6 +421,11 @@ impl<I: SparseSetIndex, V> SparseSet<I, V> {
     }
 }
 
+/// Represents something that can be stored in a [`SparseSet`] as an integer.
+///
+/// Ideally, the `usize` values should be very small (ie: incremented starting from
+/// zero), as the number of bits needed to represent a `SparseSetIndex` in a `FixedBitSet`
+/// is proportional to the **value** of those `usize`.
 pub trait SparseSetIndex: Clone + PartialEq + Eq + Hash {
     fn sparse_set_index(&self) -> usize;
     fn get_sparse_set_index(value: usize) -> Self;

crates/bevy_ecs/src/system/system_param.rs

@@ -372,14 +372,16 @@ impl<'a, T: Resource> SystemParam for Res<'a, T> {
 unsafe impl<T: Resource> SystemParamState for ResState<T> {
     fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
         let component_id = world.initialize_resource::<T>();
-        let combined_access = system_meta.component_access_set.combined_access_mut();
+        let combined_access = system_meta.component_access_set.combined_access();
         assert!(
             !combined_access.has_write(component_id),
             "error[B0002]: Res<{}> in system {} conflicts with a previous ResMut<{0}> access. Consider removing the duplicate access.",
             std::any::type_name::<T>(),
             system_meta.name,
         );
-        combined_access.add_read(component_id);
+        system_meta
+            .component_access_set
+            .add_unfiltered_read(component_id);
 
         let archetype_component_id = world
             .get_resource_archetype_component_id(component_id)
@@ -479,7 +481,7 @@ impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
 unsafe impl<T: Resource> SystemParamState for ResMutState<T> {
     fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self {
         let component_id = world.initialize_resource::<T>();
-        let combined_access = system_meta.component_access_set.combined_access_mut();
+        let combined_access = system_meta.component_access_set.combined_access();
         if combined_access.has_write(component_id) {
             panic!(
                 "error[B0002]: ResMut<{}> in system {} conflicts with a previous ResMut<{0}> access. Consider removing the duplicate access.",
@@ -489,7 +491,9 @@ unsafe impl<T: Resource> SystemParamState for ResMutState<T> {
                 "error[B0002]: ResMut<{}> in system {} conflicts with a previous Res<{0}> access. Consider removing the duplicate access.",
                 std::any::type_name::<T>(), system_meta.name);
         }
-        combined_access.add_write(component_id);
+        system_meta
+            .component_access_set
+            .add_unfiltered_write(component_id);
 
         let archetype_component_id = world
             .get_resource_archetype_component_id(component_id)
@@ -963,14 +967,16 @@ unsafe impl<T: 'static> SystemParamState for NonSendState<T> {
         system_meta.set_non_send();
 
         let component_id = world.initialize_non_send_resource::<T>();
-        let combined_access = system_meta.component_access_set.combined_access_mut();
+        let combined_access = system_meta.component_access_set.combined_access();
         assert!(
             !combined_access.has_write(component_id),
             "error[B0002]: NonSend<{}> in system {} conflicts with a previous mutable resource access ({0}). Consider removing the duplicate access.",
             std::any::type_name::<T>(),
             system_meta.name,
         );
-        combined_access.add_read(component_id);
+        system_meta
+            .component_access_set
+            .add_unfiltered_read(component_id);
 
         let archetype_component_id = world
             .get_resource_archetype_component_id(component_id)
@@ -1075,7 +1081,7 @@ unsafe impl<T: 'static> SystemParamState for NonSendMutState<T> {
         system_meta.set_non_send();
 
         let component_id = world.initialize_non_send_resource::<T>();
-        let combined_access = system_meta.component_access_set.combined_access_mut();
+        let combined_access = system_meta.component_access_set.combined_access();
         if combined_access.has_write(component_id) {
             panic!(
                 "error[B0002]: NonSendMut<{}> in system {} conflicts with a previous mutable resource access ({0}). Consider removing the duplicate access.",
@@ -1085,7 +1091,9 @@ unsafe impl<T: 'static> SystemParamState for NonSendMutState<T> {
                 "error[B0002]: NonSendMut<{}> in system {} conflicts with a previous immutable resource access ({0}). Consider removing the duplicate access.",
                 std::any::type_name::<T>(), system_meta.name);
         }
-        combined_access.add_write(component_id);
+        system_meta
+            .component_access_set
+            .add_unfiltered_write(component_id);
 
         let archetype_component_id = world
             .get_resource_archetype_component_id(component_id)