Add a missing safety invariant to System::run_unsafe (#7778)

# Objective

The implementation of `System::run_unsafe` for `FunctionSystem` requires
that the world is the same one used to initialize the system. However,
the `System` trait has no requirements that the world actually matches,
which makes this implementation unsound.

This was previously mentioned in
https://github.com/bevyengine/bevy/pull/7605#issuecomment-1426491871

Fixes part of #7833.

## Solution

Add the safety invariant that
`System::update_archetype_component_access` must be called prior to
`System::run_unsafe`. Since
`FunctionSystem::update_archetype_component_access` properly validates
the world, this ensures that `run_unsafe` is not called with a
mismatched world.

Most exclusive systems are not required to be run on the same world that
they are initialized with, so this is not a concern for them. Systems
formed by combining an exclusive system with a regular system *do*
require the world to match, however the validation is done inside of
`System::run` when needed.

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

@@ -320,6 +320,8 @@ impl MultiThreadedExecutor {
 
             self.ready_systems.set(system_index, false);
 
+            // SAFETY: Since `self.can_run` returned true earlier, it must have called
+            // `update_archetype_component_access` for each run condition.
             if !self.should_run(system_index, system, conditions, world) {
                 self.skip_system_and_signal_dependents(system_index);
                 continue;
@@ -338,7 +340,9 @@ impl MultiThreadedExecutor {
                 break;
             }
 
-            // SAFETY: No other reference to this system exists.
+            // SAFETY:
+            // - No other reference to this system exists.
+            // - `self.can_run` has been called, which calls `update_archetype_component_access` with this system.
             unsafe {
                 self.spawn_system_task(scope, system_index, systems, world);
             }
@@ -408,7 +412,11 @@ impl MultiThreadedExecutor {
         true
     }
 
-    fn should_run(
+    /// # Safety
+    ///
+    /// `update_archetype_component` must have been called with `world`
+    /// for each run condition in `conditions`.
+    unsafe fn should_run(
         &mut self,
         system_index: usize,
         _system: &BoxedSystem,
@@ -421,7 +429,8 @@ impl MultiThreadedExecutor {
                 continue;
             }
 
-            // evaluate system set's conditions
+            // Evaluate the system set's conditions.
+            // SAFETY: `update_archetype_component_access` has been called for each run condition.
             let set_conditions_met =
                 evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world);
 
@@ -434,7 +443,8 @@ impl MultiThreadedExecutor {
             self.evaluated_sets.insert(set_idx);
         }
 
-        // evaluate system's conditions
+        // Evaluate the system's conditions.
+        // SAFETY: `update_archetype_component_access` has been called for each run condition.
         let system_conditions_met =
             evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world);
 
@@ -448,7 +458,9 @@ impl MultiThreadedExecutor {
     }
 
     /// # Safety
-    /// Caller must not alias systems that are running.
+    /// - Caller must not alias systems that are running.
+    /// - `update_archetype_component_access` must have been called with `world`
+    ///   on the system assocaited with `system_index`.
     unsafe fn spawn_system_task<'scope>(
         &mut self,
         scope: &Scope<'_, 'scope, ()>,
@@ -470,7 +482,9 @@ impl MultiThreadedExecutor {
             #[cfg(feature = "trace")]
             let system_guard = system_span.enter();
             let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
-                // SAFETY: access is compatible
+                // SAFETY:
+                // - Access: TODO.
+                // - `update_archetype_component_access` has been called.
                 unsafe { system.run_unsafe((), world) };
             }));
             #[cfg(feature = "trace")]
@@ -673,7 +687,11 @@ fn apply_system_buffers(
     Ok(())
 }
 
-fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool {
+/// # Safety
+///
+/// `update_archetype_component_access` must have been called
+/// with `world` for each condition in `conditions`.
+unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool {
     // not short-circuiting is intentional
     #[allow(clippy::unnecessary_fold)]
     conditions

crates/bevy_ecs/src/system/combinator.rs

@@ -164,6 +164,8 @@ where
             // so the caller will guarantee that no other systems will conflict with `a` or `b`.
             // 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,
+            // which forwards to the implementations for `self.a` and `self.b`.
             |input| self.a.run_unsafe(input, world),
             |input| self.b.run_unsafe(input, world),
         )

crates/bevy_ecs/src/system/exclusive_function_system.rs

@@ -6,7 +6,7 @@ use crate::{
         check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, In, IntoSystem,
         System, SystemMeta,
     },
-    world::{World, WorldId},
+    world::World,
 };
 
 use bevy_utils::all_tuples;
@@ -25,7 +25,6 @@ where
     func: F,
     param_state: Option<<F::Param as ExclusiveSystemParam>::State>,
     system_meta: SystemMeta,
-    world_id: Option<WorldId>,
     // NOTE: PhantomData<fn()-> T> gives this safe Send/Sync impls
     marker: PhantomData<fn() -> Marker>,
 }
@@ -43,7 +42,6 @@ where
             func,
             param_state: None,
             system_meta: SystemMeta::new::<F>(),
-            world_id: None,
             marker: PhantomData,
         }
     }
@@ -132,7 +130,6 @@ where
 
     #[inline]
     fn initialize(&mut self, world: &mut World) {
-        self.world_id = Some(world.id());
         self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX);
         self.param_state = Some(F::Param::init(world, &mut self.system_meta));
     }

crates/bevy_ecs/src/system/function_system.rs

@@ -479,10 +479,11 @@ where
     unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out {
         let change_tick = world.increment_change_tick();
 
-        // Safety:
-        // We update the archetype component access correctly based on `Param`'s requirements
-        // in `update_archetype_component_access`.
-        // Our caller upholds the requirements.
+        // SAFETY:
+        // - The caller has invoked `update_archetype_component_access`, which will panic
+        //   if the world does not match.
+        // - All world accesses used by `F::Param` have been registered, so the caller
+        //   will ensure that there are no data access conflicts.
         let params = F::Param::get_param(
             self.param_state.as_mut().expect(Self::PARAM_MESSAGE),
             &self.system_meta,

crates/bevy_ecs/src/system/system.rs

@@ -49,11 +49,17 @@ pub trait System: Send + Sync + 'static {
     ///     1. This system is the only system running on the given world across all threads.
     ///     2. This system only runs in parallel with other systems that do not conflict with the
     ///        [`System::archetype_component_access()`].
+    ///
+    /// Additionally, the method [`Self::update_archetype_component_access`] must be called at some
+    /// point before this one, with the same exact [`World`]. If `update_archetype_component_access`
+    /// panics (or otherwise does not return for any reason), this method must not be called.
     unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out;
     /// Runs the system with the given input in the world.
     fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out {
         self.update_archetype_component_access(world);
-        // SAFETY: world and resources are exclusively borrowed
+        // SAFETY:
+        // - World and resources are exclusively borrowed, which ensures no data access conflicts.
+        // - `update_archetype_component_access` has been called.
         unsafe { self.run_unsafe(input, world) }
     }
     fn apply_buffers(&mut self, world: &mut World);