Add ambiguity detection tests (#6053)

# Objective

- Add unit tests for ambiguity detection reporting.
- Incremental implementation of #4299.

## Solution

- Refactor ambiguity detection internals to make it testable. As a bonus, this should make it easier to extend in the future.

## Notes

* This code was copy-pasted from #4299 and modified. Credit goes to @alice-i-cecile and @afonsolage, though I'm not sure who wrote what at this point.

crates/bevy_ecs/src/schedule/ambiguity_detection.rs

@@ -5,75 +5,216 @@ use crate::component::ComponentId;
 use crate::schedule::{SystemContainer, SystemStage};
 use crate::world::World;
 
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+struct SystemOrderAmbiguity {
+    segment: SystemStageSegment,
+    // Note: In order for comparisons to work correctly,
+    // `system_names` and `conflicts` must be sorted at all times.
+    system_names: [String; 2],
+    conflicts: Vec<String>,
+}
+
+/// Which part of a [`SystemStage`] was a [`SystemOrderAmbiguity`] detected in?
+#[derive(Debug, PartialEq, Eq, Clone, Copy, PartialOrd, Ord, Hash)]
+enum SystemStageSegment {
+    Parallel,
+    ExclusiveAtStart,
+    ExclusiveBeforeCommands,
+    ExclusiveAtEnd,
+}
+
+impl SystemStageSegment {
+    pub fn desc(&self) -> &'static str {
+        match self {
+            SystemStageSegment::Parallel => "Parallel systems",
+            SystemStageSegment::ExclusiveAtStart => "Exclusive systems at start of stage",
+            SystemStageSegment::ExclusiveBeforeCommands => {
+                "Exclusive systems before commands of stage"
+            }
+            SystemStageSegment::ExclusiveAtEnd => "Exclusive systems at end of stage",
+        }
+    }
+}
+
+impl SystemOrderAmbiguity {
+    fn from_raw(
+        system_a_index: usize,
+        system_b_index: usize,
+        component_ids: Vec<ComponentId>,
+        segment: SystemStageSegment,
+        stage: &SystemStage,
+        world: &World,
+    ) -> Self {
+        use crate::schedule::graph_utils::GraphNode;
+        use SystemStageSegment::*;
+
+        // TODO: blocked on https://github.com/bevyengine/bevy/pull/4166
+        // We can't grab the system container generically, because .parallel_systems()
+        // and the exclusive equivalent return a different type,
+        // and SystemContainer is not object-safe
+        let (system_a_name, system_b_name) = match segment {
+            Parallel => {
+                let system_container = stage.parallel_systems();
+                (
+                    system_container[system_a_index].name(),
+                    system_container[system_b_index].name(),
+                )
+            }
+            ExclusiveAtStart => {
+                let system_container = stage.exclusive_at_start_systems();
+                (
+                    system_container[system_a_index].name(),
+                    system_container[system_b_index].name(),
+                )
+            }
+            ExclusiveBeforeCommands => {
+                let system_container = stage.exclusive_before_commands_systems();
+                (
+                    system_container[system_a_index].name(),
+                    system_container[system_b_index].name(),
+                )
+            }
+            ExclusiveAtEnd => {
+                let system_container = stage.exclusive_at_end_systems();
+                (
+                    system_container[system_a_index].name(),
+                    system_container[system_b_index].name(),
+                )
+            }
+        };
+
+        let mut system_names = [system_a_name.to_string(), system_b_name.to_string()];
+        system_names.sort();
+
+        let mut conflicts: Vec<_> = component_ids
+            .iter()
+            .map(|id| world.components().get_info(*id).unwrap().name().to_owned())
+            .collect();
+        conflicts.sort();
+
+        Self {
+            system_names,
+            conflicts,
+            segment,
+        }
+    }
+}
+
 impl SystemStage {
-    /// Logs execution order ambiguities between systems. System orders must be fresh.
+    /// Logs execution order ambiguities between systems.
+    ///
+    /// The output may be incorrect if this stage has not been initialized with `world`.
     pub fn report_ambiguities(&self, world: &World) {
         debug_assert!(!self.systems_modified);
         use std::fmt::Write;
-        fn write_display_names_of_pairs(
-            string: &mut String,
-            systems: &[impl SystemContainer],
-            mut ambiguities: Vec<(usize, usize, Vec<ComponentId>)>,
-            world: &World,
-        ) {
-            for (index_a, index_b, conflicts) in ambiguities.drain(..) {
-                writeln!(
-                    string,
-                    " -- {:?} and {:?}",
-                    systems[index_a].name(),
-                    systems[index_b].name()
-                )
-                .unwrap();
-                if !conflicts.is_empty() {
-                    let names = conflicts
-                        .iter()
-                        .map(|id| world.components().get_info(*id).unwrap().name())
-                        .collect::<Vec<_>>();
-                    writeln!(string, "    conflicts: {:?}", names).unwrap();
-                }
-            }
-        }
-        let parallel = find_ambiguities(&self.parallel);
-        let at_start = find_ambiguities(&self.exclusive_at_start);
-        let before_commands = find_ambiguities(&self.exclusive_before_commands);
-        let at_end = find_ambiguities(&self.exclusive_at_end);
-        if !(parallel.is_empty()
-            && at_start.is_empty()
-            && before_commands.is_empty()
-            && at_end.is_empty())
-        {
+        let ambiguities = self.ambiguities(world);
+        if !ambiguities.is_empty() {
             let mut string = "Execution order ambiguities detected, you might want to \
 						add an explicit dependency relation between some of these systems:\n"
                 .to_owned();
-            if !parallel.is_empty() {
-                writeln!(string, " * Parallel systems:").unwrap();
-                write_display_names_of_pairs(&mut string, &self.parallel, parallel, world);
-            }
-            if !at_start.is_empty() {
-                writeln!(string, " * Exclusive systems at start of stage:").unwrap();
-                write_display_names_of_pairs(
-                    &mut string,
-                    &self.exclusive_at_start,
-                    at_start,
-                    world,
-                );
-            }
-            if !before_commands.is_empty() {
-                writeln!(string, " * Exclusive systems before commands of stage:").unwrap();
-                write_display_names_of_pairs(
-                    &mut string,
-                    &self.exclusive_before_commands,
-                    before_commands,
-                    world,
-                );
-            }
-            if !at_end.is_empty() {
-                writeln!(string, " * Exclusive systems at end of stage:").unwrap();
-                write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end, world);
+
+            let mut last_segment_kind = None;
+            for SystemOrderAmbiguity {
+                system_names: [system_a, system_b],
+                conflicts,
+                segment,
+            } in &ambiguities
+            {
+                // If the ambiguity occurred in a different segment than the previous one, write a header for the segment.
+                if last_segment_kind != Some(segment) {
+                    writeln!(string, " * {}:", segment.desc()).unwrap();
+                    last_segment_kind = Some(segment);
+                }
+
+                writeln!(string, " -- {:?} and {:?}", system_a, system_b).unwrap();
+
+                if !conflicts.is_empty() {
+                    writeln!(string, "    conflicts: {conflicts:?}").unwrap();
+                }
             }
+
             info!("{}", string);
         }
     }
+
+    /// Returns all execution order ambiguities between systems.
+    ///
+    /// Returns 4 vectors of ambiguities for each stage, in the following order:
+    /// - parallel
+    /// - exclusive at start,
+    /// - exclusive before commands
+    /// - exclusive at end
+    ///
+    /// The result may be incorrect if this stage has not been initialized with `world`.
+    fn ambiguities(&self, world: &World) -> Vec<SystemOrderAmbiguity> {
+        let parallel = find_ambiguities(&self.parallel).into_iter().map(
+            |(system_a_index, system_b_index, component_ids)| {
+                SystemOrderAmbiguity::from_raw(
+                    system_a_index,
+                    system_b_index,
+                    component_ids.to_vec(),
+                    SystemStageSegment::Parallel,
+                    self,
+                    world,
+                )
+            },
+        );
+
+        let at_start = find_ambiguities(&self.exclusive_at_start).into_iter().map(
+            |(system_a_index, system_b_index, component_ids)| {
+                SystemOrderAmbiguity::from_raw(
+                    system_a_index,
+                    system_b_index,
+                    component_ids,
+                    SystemStageSegment::ExclusiveAtStart,
+                    self,
+                    world,
+                )
+            },
+        );
+
+        let before_commands = find_ambiguities(&self.exclusive_before_commands)
+            .into_iter()
+            .map(|(system_a_index, system_b_index, component_ids)| {
+                SystemOrderAmbiguity::from_raw(
+                    system_a_index,
+                    system_b_index,
+                    component_ids,
+                    SystemStageSegment::ExclusiveBeforeCommands,
+                    self,
+                    world,
+                )
+            });
+
+        let at_end = find_ambiguities(&self.exclusive_at_end).into_iter().map(
+            |(system_a_index, system_b_index, component_ids)| {
+                SystemOrderAmbiguity::from_raw(
+                    system_a_index,
+                    system_b_index,
+                    component_ids,
+                    SystemStageSegment::ExclusiveAtEnd,
+                    self,
+                    world,
+                )
+            },
+        );
+
+        let mut ambiguities: Vec<_> = at_start
+            .chain(parallel)
+            .chain(before_commands)
+            .chain(at_end)
+            .collect();
+        ambiguities.sort();
+        ambiguities
+    }
+
+    /// Returns the number of system order ambiguities between systems in this stage.
+    ///
+    /// The result may be incorrect if this stage has not been initialized with `world`.
+    #[cfg(test)]
+    fn ambiguity_count(&self, world: &World) -> usize {
+        self.ambiguities(world).len()
+    }
 }
 
 /// Returns vector containing all pairs of indices of systems with ambiguous execution order,
@@ -138,3 +279,221 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<
     }
     ambiguities
 }
+
+#[cfg(test)]
+mod tests {
+    // Required to make the derive macro behave
+    use crate as bevy_ecs;
+    use crate::event::Events;
+    use crate::prelude::*;
+
+    #[derive(Resource)]
+    struct R;
+
+    #[derive(Component)]
+    struct A;
+
+    #[derive(Component)]
+    struct B;
+
+    // An event type
+    struct E;
+
+    fn empty_system() {}
+    fn res_system(_res: Res<R>) {}
+    fn resmut_system(_res: ResMut<R>) {}
+    fn nonsend_system(_ns: NonSend<R>) {}
+    fn nonsendmut_system(_ns: NonSendMut<R>) {}
+    fn read_component_system(_query: Query<&A>) {}
+    fn write_component_system(_query: Query<&mut A>) {}
+    fn with_filtered_component_system(_query: Query<&mut A, With<B>>) {}
+    fn without_filtered_component_system(_query: Query<&mut A, Without<B>>) {}
+    fn event_reader_system(_reader: EventReader<E>) {}
+    fn event_writer_system(_writer: EventWriter<E>) {}
+    fn event_resource_system(_events: ResMut<Events<E>>) {}
+    fn read_world_system(_world: &World) {}
+    fn write_world_system(_world: &mut World) {}
+
+    // Tests for conflict detection
+
+    #[test]
+    fn one_of_everything() {
+        let mut world = World::new();
+        world.insert_resource(R);
+        world.spawn().insert(A);
+        world.init_resource::<Events<E>>();
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            // nonsendmut system deliberately conflicts with resmut system
+            .add_system(resmut_system)
+            .add_system(write_component_system)
+            .add_system(event_writer_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 0);
+    }
+
+    #[test]
+    fn read_only() {
+        let mut world = World::new();
+        world.insert_resource(R);
+        world.spawn().insert(A);
+        world.init_resource::<Events<E>>();
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            .add_system(empty_system)
+            .add_system(empty_system)
+            .add_system(res_system)
+            .add_system(res_system)
+            .add_system(nonsend_system)
+            .add_system(nonsend_system)
+            .add_system(read_component_system)
+            .add_system(read_component_system)
+            .add_system(event_reader_system)
+            .add_system(event_reader_system)
+            .add_system(read_world_system)
+            .add_system(read_world_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 0);
+    }
+
+    #[test]
+    fn read_world() {
+        let mut world = World::new();
+        world.insert_resource(R);
+        world.spawn().insert(A);
+        world.init_resource::<Events<E>>();
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            .add_system(resmut_system)
+            .add_system(write_component_system)
+            .add_system(event_writer_system)
+            .add_system(read_world_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 3);
+    }
+
+    #[test]
+    fn resources() {
+        let mut world = World::new();
+        world.insert_resource(R);
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage.add_system(resmut_system).add_system(res_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 1);
+    }
+
+    #[test]
+    fn nonsend() {
+        let mut world = World::new();
+        world.insert_resource(R);
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            .add_system(nonsendmut_system)
+            .add_system(nonsend_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 1);
+    }
+
+    #[test]
+    fn components() {
+        let mut world = World::new();
+        world.spawn().insert(A);
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            .add_system(read_component_system)
+            .add_system(write_component_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 1);
+    }
+
+    #[test]
+    #[ignore = "Known failing but fix is non-trivial: https://github.com/bevyengine/bevy/issues/4381"]
+    fn filtered_components() {
+        let mut world = World::new();
+        world.spawn().insert(A);
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            .add_system(with_filtered_component_system)
+            .add_system(without_filtered_component_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 0);
+    }
+
+    #[test]
+    fn events() {
+        let mut world = World::new();
+        world.init_resource::<Events<E>>();
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            // All of these systems clash
+            .add_system(event_reader_system)
+            .add_system(event_writer_system)
+            .add_system(event_resource_system);
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 3);
+    }
+
+    #[test]
+    fn exclusive() {
+        let mut world = World::new();
+        world.insert_resource(R);
+        world.spawn().insert(A);
+        world.init_resource::<Events<E>>();
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            // All 3 of these conflict with each other
+            .add_system(write_world_system.exclusive_system())
+            .add_system(write_world_system.exclusive_system().at_end())
+            .add_system(res_system.exclusive_system())
+            // These do not, as they're in different segments of the stage
+            .add_system(write_world_system.exclusive_system().at_start())
+            .add_system(write_world_system.exclusive_system().before_commands());
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 3);
+    }
+
+    // Tests for silencing and resolving ambiguities
+
+    #[test]
+    fn before_and_after() {
+        let mut world = World::new();
+        world.init_resource::<Events<E>>();
+
+        let mut test_stage = SystemStage::parallel();
+        test_stage
+            .add_system(event_reader_system.before(event_writer_system))
+            .add_system(event_writer_system)
+            .add_system(event_resource_system.after(event_writer_system));
+
+        test_stage.run(&mut world);
+
+        assert_eq!(test_stage.ambiguity_count(&world), 0);
+    }
+}