use `UnsafeWorldCell` internally by jakobhellermann · Pull Request #6972 · bevyengine/bevy

jakobhellermann · 2022-12-17T12:55:07Z

builds on #6404 (and #6402)

important commits:

4f04a57 System and ParamState
91ef673 QueryState/Query

Objective

the motivation is given in [Merged by Bors] - add UnsafeWorldCell abstraction #6404 and the issue Abstraction for working with a &World and interior mutability #5956, but the TLDR is:
- &World is sometimes used internally to mean "world that is shared with others but the access is distinct"
- this means that you can accidentally write safe code like get_resource that is unsound because the &World doesn't actually give access
- introducing a separate type makes this distinction more clear and self-documenting

Solution

use UnsafeWorldCell whenever we use &World as shared mutable
- this is in the System machinery and for Query/WorldQuery

Changelog

bevy now uses the UnsafeWorldCell type in places where we previously used &World to mean "shared mutable access, but ensured to be distinct by systems".
This means that systems and queries now get a UnsafeWorldCell where every method is unsafe, so you need to reason correctly about every access.

Usually this is done like

// SAFETY: must only be called with read/write access to resource A and B, and component C
unsafe fn get_unsafe_world_cell(world: UnsafeWorldCell) {
  // SAFETY: caller promises read access, no other references
  let a = world.get_resource::<A>();
}

alice-i-cecile · 2023-01-08T22:41:32Z

@jakobhellermann can you rebase this? I'd like to prioritize this for review and merging due to its impact on ECS internals clarity.

alice-i-cecile · 2023-02-13T18:39:01Z

I'm unsure if we can get this reviewed and merged appropriately without stopping the train. Impact is mostly internal, so I've bumped it from the 0.10 milestone.

BoxyUwU · 2023-02-18T00:28:08Z

crates/bevy_ecs/src/query/state.rs

        }
    }

+    fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell<'_>) {


Suggested change

fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell<'_>) {

pub fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell<'_>) {

and also implement update_archetypes in terms of this method rather than the way it is now perhaps

BoxyUwU · 2023-02-18T00:28:17Z

crates/bevy_ecs/src/query/state.rs

+        self.validate_world_unsafe_world_cell(world.as_unsafe_world_cell_readonly())
+    }
+    #[inline]
+    pub fn validate_world_unsafe_world_cell(&self, world: UnsafeWorldCell<'_>) {


Suggested change

pub fn validate_world_unsafe_world_cell(&self, world: UnsafeWorldCell<'_>) {

pub fn validate_unsafe_world_cell(&self, world: UnsafeWorldCell<'_>) {

BoxyUwU · 2023-02-18T00:39:38Z

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

                break;
            }

+            // TODO: do this earlier?


Yes, you want to replace the cell: &'scope SyncUnsafeCell<World> with cell: UnsafeWorldCell<'scope> I believe.

james7132 · 2023-02-18T04:01:41Z

crates/bevy_ecs/src/query/fetch.rs

-                    .sparse_sets
-                    .get(component_id)
-                    .debug_checked_unwrap()
+                // SAFETY: TODO


This needs to be a non-TODO comment.

james7132 · 2023-02-18T04:02:06Z

crates/bevy_ecs/src/query/fetch.rs

-                    .sparse_sets
-                    .get(component_id)
-                    .debug_checked_unwrap()
+                // SAFETY: world has access to


This placeholder needs to be fully written.

james7132 · 2023-02-27T05:50:54Z

crates/bevy_ecs/src/query/state.rs

    #[inline]
    pub fn validate_world(&self, world: &World) {
+        self.validate_world_unsafe_world_cell(world.as_unsafe_world_cell_readonly())
+    }


Suggested change

}

}

james7132 · 2023-02-27T05:51:44Z

crates/bevy_ecs/src/query/state.rs

+        self.validate_world_unsafe_world_cell(world.as_unsafe_world_cell_readonly())
+    }
+    #[inline]
+    pub fn validate_world_unsafe_world_cell(&self, world: UnsafeWorldCell<'_>) {


If this is to be public, please document what this function does. Alternatively, we could just have validate_world take a WorldId instead, which should be accessible from both.

alice-i-cecile · 2023-06-13T00:10:49Z

Closing in favor of #8833.

# Objective Follow-up to #6404 and #8292. Mutating the world through a shared reference is surprising, and it makes the meaning of `&World` unclear: sometimes it gives read-only access to the entire world, and sometimes it gives interior mutable access to only part of it. This is an up-to-date version of #6972. ## Solution Use `UnsafeWorldCell` for all interior mutability. Now, `&World` *always* gives you read-only access to the entire world. --- ## Changelog TODO - do we still care about changelogs? ## Migration Guide Mutating any world data using `&World` is now considered unsound -- the type `UnsafeWorldCell` must be used to achieve interior mutability. The following methods now accept `UnsafeWorldCell` instead of `&World`: - `QueryState`: `get_unchecked`, `iter_unchecked`, `iter_combinations_unchecked`, `for_each_unchecked`, `get_single_unchecked`, `get_single_unchecked_manual`. - `SystemState`: `get_unchecked_manual` ```rust let mut world = World::new(); let mut query = world.query::<&mut T>(); // Before: let t1 = query.get_unchecked(&world, entity_1); let t2 = query.get_unchecked(&world, entity_2); // After: let world_cell = world.as_unsafe_world_cell(); let t1 = query.get_unchecked(world_cell, entity_1); let t2 = query.get_unchecked(world_cell, entity_2); ``` The methods `QueryState::validate_world` and `SystemState::matches_world` now take a `WorldId` instead of `&World`: ```rust // Before: query_state.validate_world(&world); // After: query_state.validate_world(world.id()); ``` The methods `QueryState::update_archetypes` and `SystemState::update_archetypes` now take `UnsafeWorldCell` instead of `&World`: ```rust // Before: query_state.update_archetypes(&world); // After: query_state.update_archetypes(world.as_unsafe_world_cell_readonly()); ```

james7132 added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Jan 2, 2023

alice-i-cecile requested a review from james7132 January 8, 2023 22:40

alice-i-cecile added this to the 0.10 milestone Jan 8, 2023

alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! P-Unsound A bug that results in undefined compiler behavior labels Jan 8, 2023

jakobhellermann force-pushed the interior-mutable-world-internal-usage branch from 5ce7cbe to 7cc5b86 Compare January 13, 2023 15:07

jakobhellermann force-pushed the interior-mutable-world-internal-usage branch from 7cc5b86 to ebc7198 Compare January 24, 2023 15:39

james7132 requested a review from BoxyUwU January 27, 2023 00:21

joseph-gio mentioned this pull request Jan 27, 2023

[Merged by Bors] - Move all logic to UnsafeWorldCell #7381

Closed

jakobhellermann force-pushed the interior-mutable-world-internal-usage branch 4 times, most recently from 8abb3cf to 93dbca5 Compare January 27, 2023 14:23

jakobhellermann changed the title ~~use InteriorMutableWorld internally~~ use UnsafeWorldCell internally Jan 27, 2023

jakobhellermann added 2 commits February 9, 2023 16:49

use UnsafeWorldCell for System and ParamState

989338a

use UnsafeWorldCell for QueryState/Query

f7606e9

jakobhellermann force-pushed the interior-mutable-world-internal-usage branch from 93dbca5 to f7606e9 Compare February 9, 2023 15:49

alice-i-cecile modified the milestones: 0.10, 0.11 Feb 13, 2023

BoxyUwU suggested changes Feb 18, 2023

View reviewed changes

james7132 requested changes Feb 27, 2023

View reviewed changes

joseph-gio mentioned this pull request Jun 12, 2023

Migrate the rest of the engine to UnsafeWorldCell #8833

Merged

alice-i-cecile closed this Jun 13, 2023

jakobhellermann deleted the interior-mutable-world-internal-usage branch June 13, 2023 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

use `UnsafeWorldCell` internally#6972

use `UnsafeWorldCell` internally#6972
jakobhellermann wants to merge 2 commits intobevyengine:mainfrom
jakobhellermann:interior-mutable-world-internal-usage

jakobhellermann commented Dec 17, 2022 •

edited

Loading

Uh oh!

alice-i-cecile commented Jan 8, 2023

Uh oh!

alice-i-cecile commented Feb 13, 2023

Uh oh!

BoxyUwU Feb 18, 2023

Uh oh!

BoxyUwU Feb 18, 2023

Uh oh!

BoxyUwU Feb 18, 2023

Uh oh!

james7132 Feb 18, 2023

Uh oh!

james7132 Feb 18, 2023

Uh oh!

james7132 Feb 27, 2023

Uh oh!

james7132 Feb 27, 2023

Uh oh!

alice-i-cecile commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell<'_>) {
	pub fn update_archetypes_unsafe_world_cell(&mut self, world: UnsafeWorldCell<'_>) {

	pub fn validate_world_unsafe_world_cell(&self, world: UnsafeWorldCell<'_>) {
	pub fn validate_unsafe_world_cell(&self, world: UnsafeWorldCell<'_>) {

Uh oh!

Conversation

jakobhellermann commented Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Uh oh!

alice-i-cecile commented Jan 8, 2023

Uh oh!

alice-i-cecile commented Feb 13, 2023

Uh oh!

BoxyUwU Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

BoxyUwU Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

james7132 Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

james7132 Feb 18, 2023

Choose a reason for hiding this comment

Uh oh!

james7132 Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

james7132 Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakobhellermann commented Dec 17, 2022 •

edited

Loading