From 218f78157dc7ac6dfac2a9191a8927728f58bd08 Mon Sep 17 00:00:00 2001
From: Joseph <21144246+JoJoJet@users.noreply.github.com>
Date: Wed, 24 Jul 2024 05:42:28 -0700
Subject: [PATCH] Require `&mut self` for `World::increment_change_tick`
 (#14459)

# Objective

The method `World::increment_change_tick` currently takes `&self` as the
method receiver, which is semantically strange. Even though the interior
mutability is sound, the existence of this method is strange since we
tend to think of `&World` as being a read-only snapshot of a world, not
an aliasable reference to a world with mutability. For those purposes,
we have `UnsafeWorldCell`.

## Solution

Change the method signature to take `&mut self`. Use exclusive access to
remove the need for atomic adds, which makes the method slightly more
efficient. Redirect users to [`UnsafeWorldCell::increment_change_tick`]
if they need to increment the world's change tick from an aliased
context.

In practice I don't think there will be many breakages, if any. In cases
where you need to call `increment_change_tick`, you usually already have
either `&mut World` or `UnsafeWorldCell`.

---

## Migration Guide

The method `World::increment_change_tick` now requires `&mut self`
instead of `&self`. If you need to call this method but do not have
mutable access to the world, consider using
`world.as_unsafe_world_cell_readonly().increment_change_tick()`, which
does the same thing, but is less efficient than the method on `World`
due to requiring atomic synchronization.

```rust
fn my_system(world: &World) {
    // Before
    world.increment_change_tick();

    // After
    world.as_unsafe_world_cell_readonly().increment_change_tick();
}
```
---
 .../bevy_ecs/src/system/exclusive_function_system.rs  |  4 +---
 crates/bevy_ecs/src/world/mod.rs                      | 11 +++++++++--
 crates/bevy_ecs/src/world/unsafe_world_cell.rs        |  5 ++++-
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs
index 349f48af2e..08b4b678bd 100644
--- a/crates/bevy_ecs/src/system/exclusive_function_system.rs
+++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs
@@ -124,9 +124,7 @@ where
             let out = self.func.run(world, input, params);
 
             world.flush();
-            let change_tick = world.change_tick.get_mut();
-            self.system_meta.last_run.set(*change_tick);
-            *change_tick = change_tick.wrapping_add(1);
+            self.system_meta.last_run = world.increment_change_tick();
 
             out
         })
diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs
index f47aa55ae5..c7a66c86a7 100644
--- a/crates/bevy_ecs/src/world/mod.rs
+++ b/crates/bevy_ecs/src/world/mod.rs
@@ -2063,9 +2063,16 @@ impl World {
     }
 
     /// Increments the world's current change tick and returns the old value.
+    ///
+    /// If you need to call this method, but do not have `&mut` access to the world,
+    /// consider using [`as_unsafe_world_cell_readonly`](Self::as_unsafe_world_cell_readonly)
+    /// to obtain an [`UnsafeWorldCell`] and calling [`increment_change_tick`](UnsafeWorldCell::increment_change_tick) on that.
+    /// Note that this *can* be done in safe code, despite the name of the type.
     #[inline]
-    pub fn increment_change_tick(&self) -> Tick {
-        let prev_tick = self.change_tick.fetch_add(1, Ordering::AcqRel);
+    pub fn increment_change_tick(&mut self) -> Tick {
+        let change_tick = self.change_tick.get_mut();
+        let prev_tick = *change_tick;
+        *change_tick = change_tick.wrapping_add(1);
         Tick::new(prev_tick)
     }
 
diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs
index 8de3d3af3e..bbcc4b58ba 100644
--- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs
+++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs
@@ -276,7 +276,10 @@ impl<'w> UnsafeWorldCell<'w> {
     pub fn increment_change_tick(self) -> Tick {
         // SAFETY:
         // - we only access world metadata
-        unsafe { self.world_metadata() }.increment_change_tick()
+        let change_tick = unsafe { &self.world_metadata().change_tick };
+        // NOTE: We can used a relaxed memory ordering here, since nothing
+        // other than the atomic value itself is relying on atomic synchronization
+        Tick::new(change_tick.fetch_add(1, std::sync::atomic::Ordering::Relaxed))
     }
 
     /// Provides unchecked access to the internal data stores of the [`World`].