Fix soudness issue with Conflicts involving read_all and write_all (#14579)

# Objective

- Fixes https://github.com/bevyengine/bevy/issues/14575
- There is a soundness issue because we use `conflicts()` to check for
system ambiguities + soundness issues. However since the current
conflicts is a `Vec<T>`, we cannot express conflicts where there is no
specific `ComponentId` at fault. For example `q1: Query<EntityMut>, q2:
Query<EntityMut>`
There was a TODO to handle the `write_all` case but it was never
resolved


## Solution

- Introduce an `AccessConflict` enum that is either a list of specific
ids that are conflicting or `All` if all component ids are conflicting

## Testing

- Introduced a new unit test to check for the `EntityMut` case

## Migration guide

The `get_conflicts` method of `Access` now returns an `AccessConflict`
enum instead of simply a `Vec` of `ComponentId`s that are causing the
access conflict. This can be useful in cases where there are no
particular `ComponentId`s conflicting, but instead **all** of them are;
for example `fn system(q1: Query<EntityMut>, q2: Query<EntityRef>)`

crates/bevy_ecs/src/lib.rs

@@ -74,6 +74,7 @@ pub mod prelude {
 mod tests {
     use crate as bevy_ecs;
     use crate::prelude::Or;
+    use crate::world::EntityMut;
     use crate::{
         bundle::Bundle,
         change_detection::Ref,
@@ -1389,6 +1390,26 @@ mod tests {
         world.query::<(&mut A, EntityRef)>();
     }
 
+    #[test]
+    #[should_panic]
+    fn entity_ref_and_entity_mut_query_panic() {
+        let mut world = World::new();
+        world.query::<(EntityRef, EntityMut)>();
+    }
+
+    #[test]
+    #[should_panic]
+    fn entity_mut_and_entity_mut_query_panic() {
+        let mut world = World::new();
+        world.query::<(EntityMut, EntityMut)>();
+    }
+
+    #[test]
+    fn entity_ref_and_entity_ref_query_no_panic() {
+        let mut world = World::new();
+        world.query::<(EntityRef, EntityRef)>();
+    }
+
     #[test]
     #[should_panic]
     fn mut_and_mut_query_panic() {

crates/bevy_ecs/src/query/access.rs

@@ -1,7 +1,7 @@
 use crate::storage::SparseSetIndex;
-use bevy_utils::HashSet;
 use core::fmt;
 use fixedbitset::FixedBitSet;
+use std::fmt::Debug;
 use std::marker::PhantomData;
 
 /// A wrapper struct to make Debug representations of [`FixedBitSet`] easier
@@ -484,15 +484,19 @@ impl<T: SparseSetIndex> Access<T> {
     }
 
     /// Returns a vector of elements that the access and `other` cannot access at the same time.
-    pub fn get_conflicts(&self, other: &Access<T>) -> Vec<T> {
-        let mut conflicts = FixedBitSet::default();
+    pub fn get_conflicts(&self, other: &Access<T>) -> AccessConflicts {
+        let mut conflicts = FixedBitSet::new();
         if self.reads_all_components {
-            // QUESTION: How to handle `other.writes_all`?
+            if other.writes_all_components {
+                return AccessConflicts::All;
+            }
             conflicts.extend(other.component_writes.ones());
         }
 
         if other.reads_all_components {
-            // QUESTION: How to handle `self.writes_all`.
+            if self.writes_all_components {
+                return AccessConflicts::All;
+            }
             conflicts.extend(self.component_writes.ones());
         }
 
@@ -504,15 +508,18 @@ impl<T: SparseSetIndex> Access<T> {
             conflicts.extend(self.component_read_and_writes.ones());
         }
         if self.reads_all_resources {
-            // QUESTION: How to handle `other.writes_all`?
+            if other.reads_all_resources {
+                return AccessConflicts::All;
+            }
             conflicts.extend(other.resource_writes.ones());
         }
 
         if other.reads_all_resources {
-            // QUESTION: How to handle `self.writes_all`.
+            if self.reads_all_resources {
+                return AccessConflicts::All;
+            }
             conflicts.extend(self.resource_writes.ones());
         }
-
         if self.writes_all_resources {
             conflicts.extend(other.resource_read_and_writes.ones());
         }
@@ -537,10 +544,7 @@ impl<T: SparseSetIndex> Access<T> {
             self.resource_read_and_writes
                 .intersection(&other.resource_writes),
         );
-        conflicts
-            .ones()
-            .map(SparseSetIndex::get_sparse_set_index)
-            .collect()
+        AccessConflicts::Individual(conflicts)
     }
 
     /// Returns the indices of the components this has access to.
@@ -635,6 +639,53 @@ impl<T: SparseSetIndex> From<FilteredAccess<T>> for FilteredAccessSet<T> {
     }
 }
 
+/// Records how two accesses conflict with each other
+#[derive(Debug, PartialEq)]
+pub enum AccessConflicts {
+    /// Conflict is for all indices
+    All,
+    /// There is a conflict for a subset of indices
+    Individual(FixedBitSet),
+}
+
+impl AccessConflicts {
+    fn add(&mut self, other: &Self) {
+        match (self, other) {
+            (s, AccessConflicts::All) => {
+                *s = AccessConflicts::All;
+            }
+            (AccessConflicts::Individual(this), AccessConflicts::Individual(other)) => {
+                this.extend(other.ones());
+            }
+            _ => {}
+        }
+    }
+
+    pub(crate) fn is_empty(&self) -> bool {
+        match self {
+            Self::All => false,
+            Self::Individual(set) => set.is_empty(),
+        }
+    }
+
+    /// An [`AccessConflicts`] which represents the absence of any conflict
+    pub(crate) fn empty() -> Self {
+        Self::Individual(FixedBitSet::new())
+    }
+}
+
+impl From<FixedBitSet> for AccessConflicts {
+    fn from(value: FixedBitSet) -> Self {
+        Self::Individual(value)
+    }
+}
+
+impl<T: SparseSetIndex> From<Vec<T>> for AccessConflicts {
+    fn from(value: Vec<T>) -> Self {
+        Self::Individual(value.iter().map(T::sparse_set_index).collect())
+    }
+}
+
 impl<T: SparseSetIndex> FilteredAccess<T> {
     /// Returns a `FilteredAccess` which has no access and matches everything.
     /// This is the equivalent of a `TRUE` logic atom.
@@ -752,12 +803,12 @@ impl<T: SparseSetIndex> FilteredAccess<T> {
     }
 
     /// Returns a vector of elements that this and `other` cannot access at the same time.
-    pub fn get_conflicts(&self, other: &FilteredAccess<T>) -> Vec<T> {
+    pub fn get_conflicts(&self, other: &FilteredAccess<T>) -> AccessConflicts {
         if !self.is_compatible(other) {
             // filters are disjoint, so we can just look at the unfiltered intersection
             return self.access.get_conflicts(&other.access);
         }
-        Vec::new()
+        AccessConflicts::empty()
     }
 
     /// Adds all access and filters from `other`.
@@ -948,29 +999,29 @@ impl<T: SparseSetIndex> FilteredAccessSet<T> {
     }
 
     /// Returns a vector of elements that this set and `other` cannot access at the same time.
-    pub fn get_conflicts(&self, other: &FilteredAccessSet<T>) -> Vec<T> {
+    pub fn get_conflicts(&self, other: &FilteredAccessSet<T>) -> AccessConflicts {
         // if the unfiltered access is incompatible, must check each pair
-        let mut conflicts = HashSet::new();
+        let mut conflicts = AccessConflicts::empty();
         if !self.combined_access.is_compatible(other.combined_access()) {
             for filtered in &self.filtered_accesses {
                 for other_filtered in &other.filtered_accesses {
-                    conflicts.extend(filtered.get_conflicts(other_filtered).into_iter());
+                    conflicts.add(&filtered.get_conflicts(other_filtered));
                 }
             }
         }
-        conflicts.into_iter().collect()
+        conflicts
     }
 
     /// Returns a vector of elements that this set and `other` cannot access at the same time.
-    pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess<T>) -> Vec<T> {
+    pub fn get_conflicts_single(&self, filtered_access: &FilteredAccess<T>) -> AccessConflicts {
         // if the unfiltered access is incompatible, must check each pair
-        let mut conflicts = HashSet::new();
+        let mut conflicts = AccessConflicts::empty();
         if !self.combined_access.is_compatible(filtered_access.access()) {
             for filtered in &self.filtered_accesses {
-                conflicts.extend(filtered.get_conflicts(filtered_access).into_iter());
+                conflicts.add(&filtered.get_conflicts(filtered_access));
             }
         }
-        conflicts.into_iter().collect()
+        conflicts
     }
 
     /// Adds the filtered access to the set.
@@ -1030,7 +1081,7 @@ impl<T: SparseSetIndex> Default for FilteredAccessSet<T> {
 #[cfg(test)]
 mod tests {
     use crate::query::access::AccessFilters;
-    use crate::query::{Access, FilteredAccess, FilteredAccessSet};
+    use crate::query::{Access, AccessConflicts, FilteredAccess, FilteredAccessSet};
     use fixedbitset::FixedBitSet;
     use std::marker::PhantomData;
 
@@ -1195,21 +1246,27 @@ mod tests {
         access_b.add_component_read(0);
         access_b.add_component_write(1);
 
-        assert_eq!(access_a.get_conflicts(&access_b), vec![1]);
+        assert_eq!(access_a.get_conflicts(&access_b), vec![1_usize].into());
 
         let mut access_c = Access::<usize>::default();
         access_c.add_component_write(0);
         access_c.add_component_write(1);
 
-        assert_eq!(access_a.get_conflicts(&access_c), vec![0, 1]);
-        assert_eq!(access_b.get_conflicts(&access_c), vec![0, 1]);
+        assert_eq!(
+            access_a.get_conflicts(&access_c),
+            vec![0_usize, 1_usize].into()
+        );
+        assert_eq!(
+            access_b.get_conflicts(&access_c),
+            vec![0_usize, 1_usize].into()
+        );
 
         let mut access_d = Access::<usize>::default();
         access_d.add_component_read(0);
 
-        assert_eq!(access_d.get_conflicts(&access_a), vec![]);
-        assert_eq!(access_d.get_conflicts(&access_b), vec![]);
-        assert_eq!(access_d.get_conflicts(&access_c), vec![0]);
+        assert_eq!(access_d.get_conflicts(&access_a), AccessConflicts::empty());
+        assert_eq!(access_d.get_conflicts(&access_b), AccessConflicts::empty());
+        assert_eq!(access_d.get_conflicts(&access_c), vec![0_usize].into());
     }
 
     #[test]
@@ -1223,7 +1280,7 @@ mod tests {
         let conflicts = access_a.get_conflicts_single(&filter_b);
         assert_eq!(
             &conflicts,
-            &[1_usize],
+            &AccessConflicts::from(vec![1_usize]),
             "access_a: {access_a:?}, filter_b: {filter_b:?}"
         );
     }

crates/bevy_ecs/src/schedule/schedule.rs

@@ -23,6 +23,8 @@ use crate::{
     world::World,
 };
 
+use crate::query::AccessConflicts;
+use crate::storage::SparseSetIndex;
 pub use stepping::Stepping;
 
 /// Resource that stores [`Schedule`]s mapped to [`ScheduleLabel`]s excluding the current running [`Schedule`].
@@ -1363,14 +1365,22 @@ impl ScheduleGraph {
                 let access_a = system_a.component_access();
                 let access_b = system_b.component_access();
                 if !access_a.is_compatible(access_b) {
-                    let conflicts: Vec<_> = access_a
-                        .get_conflicts(access_b)
-                        .into_iter()
-                        .filter(|id| !ignored_ambiguities.contains(id))
-                        .collect();
-
-                    if !conflicts.is_empty() {
-                        conflicting_systems.push((a, b, conflicts));
+                    match access_a.get_conflicts(access_b) {
+                        AccessConflicts::Individual(conflicts) => {
+                            let conflicts: Vec<_> = conflicts
+                                .ones()
+                                .map(ComponentId::get_sparse_set_index)
+                                .filter(|id| !ignored_ambiguities.contains(id))
+                                .collect();
+                            if !conflicts.is_empty() {
+                                conflicting_systems.push((a, b, conflicts));
+                            }
+                        }
+                        AccessConflicts::All => {
+                            // there is no specific component conflicting, but the systems are overall incompatible
+                            // for example 2 systems with `Query<EntityMut>`
+                            conflicting_systems.push((a, b, Vec::new()));
+                        }
                     }
                 }
             }

crates/bevy_ecs/src/system/mod.rs

@@ -312,9 +312,9 @@ where
     assert_is_system(system);
 }
 
-/// Ensures that the provided system doesn't with itself.
+/// Ensures that the provided system doesn't conflict with itself.
 ///
-/// This function will  panic if the provided system conflict with itself.
+/// This function will panic if the provided system conflict with itself.
 ///
 /// Note: this will run the system on an empty world.
 pub fn assert_system_does_not_conflict<Out, Params, S: IntoSystem<(), Out, Params>>(sys: S) {
@@ -340,10 +340,11 @@ impl<T> std::ops::DerefMut for In<T> {
 
 #[cfg(test)]
 mod tests {
+    use bevy_utils::default;
     use std::any::TypeId;
 
-    use bevy_utils::default;
-
+    use crate::prelude::EntityRef;
+    use crate::world::EntityMut;
     use crate::{
         self as bevy_ecs,
         archetype::{ArchetypeComponentId, Archetypes},
@@ -1102,7 +1103,7 @@ mod tests {
             .get_resource_id(TypeId::of::<B>())
             .unwrap();
         let d_id = world.components().get_id(TypeId::of::<D>()).unwrap();
-        assert_eq!(conflicts, vec![b_id, d_id]);
+        assert_eq!(conflicts, vec![b_id, d_id].into());
     }
 
     #[test]
@@ -1608,6 +1609,33 @@ mod tests {
         super::assert_system_does_not_conflict(system);
     }
 
+    #[test]
+    #[should_panic(
+        expected = "error[B0001]: Query<bevy_ecs::world::entity_ref::EntityMut, ()> 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<T>` 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 system(_query: &World, _q2: Query<EntityMut>) {}
+        super::assert_system_does_not_conflict(system);
+    }
+
+    #[test]
+    #[should_panic(
+        expected = "error[B0001]: Query<bevy_ecs::world::entity_ref::EntityMut, ()> 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<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"
+    )]
+    fn assert_entity_ref_and_entity_mut_system_does_conflict() {
+        fn system(_query: Query<EntityRef>, _q2: Query<EntityMut>) {}
+        super::assert_system_does_not_conflict(system);
+    }
+
+    #[test]
+    #[should_panic(
+        expected = "error[B0001]: Query<bevy_ecs::world::entity_ref::EntityMut, ()> in system bevy_ecs::system::tests::assert_entity_mut_system_does_conflict::system accesses component(s) in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001"
+    )]
+    fn assert_entity_mut_system_does_conflict() {
+        fn system(_query: Query<EntityMut>, _q2: Query<EntityMut>) {}
+        super::assert_system_does_not_conflict(system);
+    }
+
     #[test]
     #[should_panic]
     fn panic_inside_system() {

crates/bevy_ecs/src/system/system_param.rs

@@ -1,4 +1,6 @@
 pub use crate::change_detection::{NonSendMut, Res, ResMut};
+use crate::query::AccessConflicts;
+use crate::storage::SparseSetIndex;
 use crate::{
     archetype::{Archetype, Archetypes},
     bundle::Bundles,
@@ -296,12 +298,22 @@ fn assert_component_access_compatibility(
     if conflicts.is_empty() {
         return;
     }
-    let conflicting_components = conflicts
-        .into_iter()
-        .map(|component_id| world.components.get_info(component_id).unwrap().name())
-        .collect::<Vec<&str>>();
-    let accesses = conflicting_components.join(", ");
-    panic!("error[B0001]: Query<{query_type}, {filter_type}> in system {system_name} accesses component(s) {accesses} in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001");
+    let accesses = match conflicts {
+        AccessConflicts::All => "",
+        AccessConflicts::Individual(indices) => &format!(
+            " {}",
+            indices
+                .ones()
+                .map(|index| world
+                    .components
+                    .get_info(ComponentId::get_sparse_set_index(index))
+                    .unwrap()
+                    .name())
+                .collect::<Vec<&str>>()
+                .join(", ")
+        ),
+    };
+    panic!("error[B0001]: Query<{query_type}, {filter_type}> in system {system_name} accesses component(s){accesses} in a way that conflicts with a previous system parameter. Consider using `Without<T>` to create disjoint Queries or merging conflicting Queries into a `ParamSet`. See: https://bevyengine.org/learn/errors/b0001");
 }
 
 /// A collection of potentially conflicting [`SystemParam`]s allowed by disjoint access.