port old ambiguity tests over (#9617)

# Objective

- Some of the old ambiguity tests didn't get ported over during schedule
v3.

## Solution

- Port over tests from
https://github.com/bevyengine/bevy/blob/15ee98db8d1c6705111e0f11a8fc240ceaf9f2db/crates/bevy_ecs/src/schedule/ambiguity_detection.rs#L279-L612
with minimal changes
- Make a method to convert the ambiguity conflicts to a string for
easier verification of correct results.

Author: hymm

crates/bevy_ecs/src/schedule/mod.rs

@@ -716,4 +716,334 @@ mod tests {
             assert!(matches!(result, Err(ScheduleBuildError::Ambiguity(_))));
         }
     }
+
+    mod system_ambiguity {
+        // 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
+        #[derive(Event)]
+        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(A);
+            world.init_resource::<Events<E>>();
+
+            let mut schedule = Schedule::default();
+            schedule
+                // nonsendmut system deliberately conflicts with resmut system
+                .add_systems((resmut_system, write_component_system, event_writer_system));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        #[test]
+        fn read_only() {
+            let mut world = World::new();
+            world.insert_resource(R);
+            world.spawn(A);
+            world.init_resource::<Events<E>>();
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                empty_system,
+                empty_system,
+                res_system,
+                res_system,
+                nonsend_system,
+                nonsend_system,
+                read_component_system,
+                read_component_system,
+                event_reader_system,
+                event_reader_system,
+                read_world_system,
+                read_world_system,
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        #[test]
+        fn read_world() {
+            let mut world = World::new();
+            world.insert_resource(R);
+            world.spawn(A);
+            world.init_resource::<Events<E>>();
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                resmut_system,
+                write_component_system,
+                event_writer_system,
+                read_world_system,
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 3);
+        }
+
+        #[test]
+        fn resources() {
+            let mut world = World::new();
+            world.insert_resource(R);
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((resmut_system, res_system));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 1);
+        }
+
+        #[test]
+        fn nonsend() {
+            let mut world = World::new();
+            world.insert_resource(R);
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((nonsendmut_system, nonsend_system));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 1);
+        }
+
+        #[test]
+        fn components() {
+            let mut world = World::new();
+            world.spawn(A);
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((read_component_system, write_component_system));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 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(A);
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                with_filtered_component_system,
+                without_filtered_component_system,
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        #[test]
+        fn events() {
+            let mut world = World::new();
+            world.init_resource::<Events<E>>();
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                // All of these systems clash
+                event_reader_system,
+                event_writer_system,
+                event_resource_system,
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 3);
+        }
+
+        #[test]
+        fn exclusive() {
+            let mut world = World::new();
+            world.insert_resource(R);
+            world.spawn(A);
+            world.init_resource::<Events<E>>();
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                // All 3 of these conflict with each other
+                write_world_system,
+                write_world_system,
+                res_system,
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 3);
+        }
+
+        // Tests for silencing and resolving ambiguities
+        #[test]
+        fn before_and_after() {
+            let mut world = World::new();
+            world.init_resource::<Events<E>>();
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                event_reader_system.before(event_writer_system),
+                event_writer_system,
+                event_resource_system.after(event_writer_system),
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        #[test]
+        fn ignore_all_ambiguities() {
+            let mut world = World::new();
+            world.insert_resource(R);
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                resmut_system.ambiguous_with_all(),
+                res_system,
+                nonsend_system,
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        #[test]
+        fn ambiguous_with_label() {
+            let mut world = World::new();
+            world.insert_resource(R);
+
+            #[derive(SystemSet, Hash, PartialEq, Eq, Debug, Clone)]
+            struct IgnoreMe;
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                resmut_system.ambiguous_with(IgnoreMe),
+                res_system.in_set(IgnoreMe),
+                nonsend_system.in_set(IgnoreMe),
+            ));
+
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        #[test]
+        fn ambiguous_with_system() {
+            let mut world = World::new();
+
+            let mut schedule = Schedule::default();
+            schedule.add_systems((
+                write_component_system.ambiguous_with(read_component_system),
+                read_component_system,
+            ));
+            let _ = schedule.initialize(&mut world);
+
+            assert_eq!(schedule.graph().conflicting_systems().len(), 0);
+        }
+
+        // Tests that the correct ambiguities were reported in the correct order.
+        #[test]
+        fn correct_ambiguities() {
+            use super::*;
+
+            #[derive(ScheduleLabel, Hash, PartialEq, Eq, Debug, Clone)]
+            struct TestSchedule;
+
+            fn system_a(_res: ResMut<R>) {}
+            fn system_b(_res: ResMut<R>) {}
+            fn system_c(_res: ResMut<R>) {}
+            fn system_d(_res: ResMut<R>) {}
+            fn system_e(_res: ResMut<R>) {}
+
+            let mut world = World::new();
+            world.insert_resource(R);
+
+            let mut schedule = Schedule::new(TestSchedule);
+            schedule.add_systems((
+                system_a,
+                system_b,
+                system_c.ambiguous_with_all(),
+                system_d.ambiguous_with(system_b),
+                system_e.after(system_a),
+            ));
+
+            schedule.graph_mut().initialize(&mut world);
+            let _ = schedule
+                .graph_mut()
+                .build_schedule(world.components(), &TestSchedule.dyn_clone());
+
+            let ambiguities: Vec<_> = schedule
+                .graph()
+                .conflicts_to_string(world.components())
+                .collect();
+
+            let expected = &[
+                (
+                    "system_d".to_string(),
+                    "system_a".to_string(),
+                    vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
+                ),
+                (
+                    "system_d".to_string(),
+                    "system_e".to_string(),
+                    vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
+                ),
+                (
+                    "system_b".to_string(),
+                    "system_a".to_string(),
+                    vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
+                ),
+                (
+                    "system_b".to_string(),
+                    "system_e".to_string(),
+                    vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
+                ),
+            ];
+
+            // ordering isn't stable so do this
+            for entry in expected {
+                assert!(ambiguities.contains(entry));
+            }
+        }
+    }
 }

crates/bevy_ecs/src/schedule/schedule.rs

@@ -1553,21 +1553,11 @@ impl ScheduleGraph {
             Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n",
         );
 
-        for (system_a, system_b, conflicts) in ambiguities {
-            let name_a = self.get_node_name(system_a);
-            let name_b = self.get_node_name(system_b);
-
-            debug_assert!(system_a.is_system(), "{name_a} is not a system.");
-            debug_assert!(system_b.is_system(), "{name_b} is not a system.");
-
+        for (name_a, name_b, conflicts) in self.conflicts_to_string(components) {
             writeln!(message, " -- {name_a} and {name_b}").unwrap();
-            if !conflicts.is_empty() {
-                let conflict_names: Vec<_> = conflicts
-                    .iter()
-                    .map(|id| components.get_name(*id).unwrap())
-                    .collect();
 
-                writeln!(message, "    conflict on: {conflict_names:?}").unwrap();
+            if !conflicts.is_empty() {
+                writeln!(message, "    conflict on: {conflicts:?}").unwrap();
             } else {
                 // one or both systems must be exclusive
                 let world = std::any::type_name::<World>();
@@ -1578,6 +1568,29 @@ impl ScheduleGraph {
         message
     }
 
+    /// convert conflics to human readable format
+    pub fn conflicts_to_string<'a>(
+        &'a self,
+        components: &'a Components,
+    ) -> impl Iterator<Item = (String, String, Vec<&str>)> + 'a {
+        self.conflicting_systems
+            .iter()
+            .map(move |(system_a, system_b, conflicts)| {
+                let name_a = self.get_node_name(system_a);
+                let name_b = self.get_node_name(system_b);
+
+                debug_assert!(system_a.is_system(), "{name_a} is not a system.");
+                debug_assert!(system_b.is_system(), "{name_b} is not a system.");
+
+                let conflict_names: Vec<_> = conflicts
+                    .iter()
+                    .map(|id| components.get_name(*id).unwrap())
+                    .collect();
+
+                (name_a, name_b, conflict_names)
+            })
+    }
+
     fn traverse_sets_containing_node(&self, id: NodeId, f: &mut impl FnMut(NodeId) -> bool) {
         for (set_id, _, _) in self.hierarchy.graph.edges_directed(id, Direction::Incoming) {
             if f(set_id) {