Fix run_system for adapter systems wrapping exclusive systems (#18406)

# Objective

Fix panic in `run_system` when running an exclusive system wrapped in a
`PipeSystem` or `AdapterSystem`.

#18076 introduced a `System::run_without_applying_deferred` method. It
normally calls `System::run_unsafe`, but
`ExclusiveFunctionSystem::run_unsafe` panics, so it was overridden for
that type. Unfortunately, `PipeSystem::run_without_applying_deferred`
still calls `PipeSystem::run_unsafe`, which can then call
`ExclusiveFunctionSystem::run_unsafe` and panic.

## Solution

Make `ExclusiveFunctionSystem::run_unsafe` work instead of panicking.
Clarify the safety requirements that make this sound.

The alternative is to override `run_without_applying_deferred` in
`PipeSystem`, `CombinatorSystem`, `AdapterSystem`,
`InfallibleSystemWrapper`, and `InfallibleObserverWrapper`. That seems
like a lot of extra code just to preserve a confusing special case!

Remove some implementations of `System::run` that are no longer
necessary with this change. This slightly changes the behavior of
`PipeSystem` and `CombinatorSystem`: Currently `run` will call
`apply_deferred` on the first system before running the second, but
after this change it will only call it after *both* systems have run.
The new behavior is consistent with `run_unsafe` and
`run_without_applying_deferred`, and restores the behavior prior to
#11823.

The panic was originally necessary because [`run_unsafe` took
`&World`](https://github.com/bevyengine/bevy/pull/6083/files#diff-708dfc60ec5eef432b20a6f471357a7ea9bfb254dc2f918d5ed4a66deb0e85baR90).
Now that it takes `UnsafeWorldCell`, it is possible to make it work. See
also Cart's concerns at
#4166 (comment),
although those also predate `UnsafeWorldCell`.

And see #6698 for a previous bug caused by this panic.

Author: chescock

crates/bevy_ecs/src/observer/runner.rs

@@ -402,6 +402,7 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     // - `update_archetype_component_access` is called first
     // - there are no outstanding references to world except a private component
     // - system is an `ObserverSystem` so won't mutate world beyond the access of a `DeferredWorld`
+    //   and is never exclusive
     // - system is the same type erased system from above
     unsafe {
         (*system).update_archetype_component_access(world);

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

@@ -452,6 +452,8 @@ impl ExecutorState {
 
                 // SAFETY:
                 // - Caller ensured no other reference to this system exists.
+                // - `system_task_metadata[system_index].is_exclusive` is `false`,
+                //   so `System::is_exclusive` returned `false` when we called it.
                 // - `can_run` has been called, which calls `update_archetype_component_access` with this system.
                 // - `can_run` returned true, so no systems with conflicting world access are running.
                 unsafe {
@@ -608,6 +610,7 @@ impl ExecutorState {
 
     /// # Safety
     /// - Caller must not alias systems that are running.
+    /// - `is_exclusive` must have returned `false` for the specified system.
     /// - `world` must have permission to access the world data
     ///   used by the specified system.
     /// - `update_archetype_component_access` must have been called with `world`
@@ -625,6 +628,7 @@ impl ExecutorState {
                 // SAFETY:
                 // - The caller ensures that we have permission to
                 // access the world data used by the system.
+                // - `is_exclusive` returned false
                 // - `update_archetype_component_access` has been called.
                 unsafe {
                     if let Err(err) = __rust_begin_short_backtrace::run_unsafe(

crates/bevy_ecs/src/system/adapter_system.rs

@@ -162,12 +162,6 @@ where
         })
     }
 
-    #[inline]
-    fn run(&mut self, input: SystemIn<'_, Self>, world: &mut crate::prelude::World) -> Self::Out {
-        self.func
-            .adapt(input, |input| self.system.run(input, world))
-    }
-
     #[inline]
     fn apply_deferred(&mut self, world: &mut crate::prelude::World) {
         self.system.apply_deferred(world);

crates/bevy_ecs/src/system/combinator.rs

@@ -176,6 +176,7 @@ where
             input,
             // SAFETY: The world accesses for both underlying systems have been registered,
             // so the caller will guarantee that no other systems will conflict with `a` or `b`.
+            // If either system has `is_exclusive()`, then the combined system also has `is_exclusive`.
             // Since these closures are `!Send + !Sync + !'static`, they can never be called
             // in parallel, so their world accesses will not conflict with each other.
             // Additionally, `update_archetype_component_access` has been called,
@@ -186,19 +187,6 @@ where
         )
     }
 
-    fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
-        let world = world.as_unsafe_world_cell();
-        Func::combine(
-            input,
-            // SAFETY: Since these closures are `!Send + !Sync + !'static`, they can never
-            // be called in parallel. Since mutable access to `world` only exists within
-            // the scope of either closure, we can be sure they will never alias one another.
-            |input| self.a.run(input, unsafe { world.world_mut() }),
-            // SAFETY: See the above safety comment.
-            |input| self.b.run(input, unsafe { world.world_mut() }),
-        )
-    }
-
     #[inline]
     fn apply_deferred(&mut self, world: &mut World) {
         self.a.apply_deferred(world);
@@ -419,11 +407,6 @@ where
         self.b.run_unsafe(value, world)
     }
 
-    fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
-        let value = self.a.run(input, world);
-        self.b.run(value, world)
-    }
-
     fn apply_deferred(&mut self, world: &mut World) {
         self.a.apply_deferred(world);
         self.b.apply_deferred(world);

crates/bevy_ecs/src/system/exclusive_function_system.rs

@@ -112,18 +112,12 @@ where
 
     #[inline]
     unsafe fn run_unsafe(
-        &mut self,
-        _input: SystemIn<'_, Self>,
-        _world: UnsafeWorldCell,
-    ) -> Self::Out {
-        panic!("Cannot run exclusive systems with a shared World reference");
-    }
-
-    fn run_without_applying_deferred(
         &mut self,
         input: SystemIn<'_, Self>,
-        world: &mut World,
+        world: UnsafeWorldCell,
     ) -> Self::Out {
+        // SAFETY: The safety is upheld by the caller.
+        let world = unsafe { world.world_mut() };
         world.last_change_tick_scope(self.system_meta.last_run, |world| {
             #[cfg(feature = "trace")]
             let _span_guard = self.system_meta.system_span.enter();

crates/bevy_ecs/src/system/observer_system.rs

@@ -138,12 +138,6 @@ where
         Ok(())
     }
 
-    #[inline]
-    fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
-        self.observer.run(input, world);
-        Ok(())
-    }
-
     #[inline]
     fn apply_deferred(&mut self, world: &mut World) {
         self.observer.apply_deferred(world);

crates/bevy_ecs/src/system/schedule_system.rs

@@ -65,12 +65,6 @@ impl<S: System<In = (), Out = ()>> System for InfallibleSystemWrapper<S> {
         Ok(())
     }
 
-    #[inline]
-    fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
-        self.0.run(input, world);
-        Ok(())
-    }
-
     #[inline]
     fn apply_deferred(&mut self, world: &mut World) {
         self.0.apply_deferred(world);

crates/bevy_ecs/src/system/system.rs

@@ -69,6 +69,8 @@ pub trait System: Send + Sync + 'static {
     /// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data
     ///   registered in `archetype_component_access`. There must be no conflicting
     ///   simultaneous accesses while the system is running.
+    /// - If [`System::is_exclusive`] returns `true`, then it must be valid to call
+    ///   [`UnsafeWorldCell::world_mut`] on `world`.
     /// - The method [`System::update_archetype_component_access`] must be called at some
     ///   point before this one, with the same exact [`World`]. If [`System::update_archetype_component_access`]
     ///   panics (or otherwise does not return for any reason), this method must not be called.

crates/bevy_ecs/src/system/system_registry.rs

@@ -896,4 +896,13 @@ mod tests {
 
         assert_eq!(INVOCATIONS_LEFT.get(), 0);
     }
+
+    #[test]
+    fn run_system_exclusive_adapters() {
+        let mut world = World::new();
+        fn system(_: &mut World) {}
+        world.run_system_cached(system).unwrap();
+        world.run_system_cached(system.pipe(system)).unwrap();
+        world.run_system_cached(system.map(|()| {})).unwrap();
+    }
 }