Skip to content

Commit

Permalink
Migrate the rest of the engine to UnsafeWorldCell (#8833)
Browse files Browse the repository at this point in the history
# 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());
```
  • Loading branch information
JoJoJet authored Jun 15, 2023
1 parent f7aa83a commit db8d365
Show file tree
Hide file tree
Showing 13 changed files with 350 additions and 217 deletions.
3 changes: 2 additions & 1 deletion benches/benches/bevy_ecs/world/world_get.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,10 @@ pub fn query_get_component_simple(criterion: &mut Criterion) {
let entity = world.spawn(A(0.0)).id();
let mut query = world.query::<&mut A>();

let world_cell = world.as_unsafe_world_cell();
bencher.iter(|| {
for _x in 0..100000 {
let mut a = unsafe { query.get_unchecked(&world, entity).unwrap() };
let mut a = unsafe { query.get_unchecked(world_cell, entity).unwrap() };
a.0 += 1.0;
}
});
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream {
}

unsafe fn init_fetch<'__w>(
_world: &'__w #path::world::World,
_world: #path::world::unsafe_world_cell::UnsafeWorldCell<'__w>,
state: &Self::State,
_last_run: #path::component::Tick,
_this_run: #path::component::Tick,
Expand Down
54 changes: 37 additions & 17 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
entity::Entity,
query::{Access, DebugCheckedUnwrap, FilteredAccess},
storage::{ComponentSparseSet, Table, TableRow},
world::{Mut, Ref, World},
world::{unsafe_world_cell::UnsafeWorldCell, Mut, Ref, World},
};
pub use bevy_ecs_macros::WorldQuery;
use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref};
Expand Down Expand Up @@ -335,10 +335,11 @@ pub unsafe trait WorldQuery {
///
/// # Safety
///
/// `state` must have been initialized (via [`WorldQuery::init_state`]) using the same `world` passed
/// in to this function.
/// - `world` must have permission to access any of the components specified in `Self::update_archetype_component_access`.
/// - `state` must have been initialized (via [`WorldQuery::init_state`]) using the same `world` passed
/// in to this function.
unsafe fn init_fetch<'w>(
world: &'w World,
world: UnsafeWorldCell<'w>,
state: &Self::State,
last_run: Tick,
this_run: Tick,
Expand Down Expand Up @@ -372,8 +373,10 @@ pub unsafe trait WorldQuery {
///
/// # Safety
///
/// `archetype` and `tables` must be from the [`World`] [`WorldQuery::init_state`] was called on. `state` must
/// be the [`Self::State`] this was initialized with.
/// - `archetype` and `tables` must be from the same [`World`] that [`WorldQuery::init_state`] was called on.
/// - [`Self::update_archetype_component_access`] must have been previously called with `archetype`.
/// - `table` must correspond to `archetype`.
/// - `state` must be the [`State`](Self::State) that `fetch` was initialized with.
unsafe fn set_archetype<'w>(
fetch: &mut Self::Fetch<'w>,
state: &Self::State,
Expand All @@ -386,8 +389,10 @@ pub unsafe trait WorldQuery {
///
/// # Safety
///
/// `table` must be from the [`World`] [`WorldQuery::init_state`] was called on. `state` must be the
/// [`Self::State`] this was initialized with.
/// - `table` must be from the same [`World`] that [`WorldQuery::init_state`] was called on.
/// - `table` must belong to an archetype that was previously registered with
/// [`Self::update_archetype_component_access`].
/// - `state` must be the [`State`](Self::State) that `fetch` was initialized with.
unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table);

/// Fetch [`Self::Item`](`WorldQuery::Item`) for either the given `entity` in the current [`Table`],
Expand Down Expand Up @@ -475,7 +480,7 @@ unsafe impl WorldQuery for Entity {
const IS_ARCHETYPAL: bool = true;

unsafe fn init_fetch<'w>(
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_state: &Self::State,
_last_run: Tick,
_this_run: Tick,
Expand Down Expand Up @@ -558,7 +563,7 @@ unsafe impl<T: Component> WorldQuery for &T {

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
world: UnsafeWorldCell<'w>,
&component_id: &ComponentId,
_last_run: Tick,
_this_run: Tick,
Expand All @@ -567,6 +572,11 @@ unsafe impl<T: Component> WorldQuery for &T {
table_components: None,
sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| {
world
// SAFETY: The underlying type associated with `component_id` is `T`,
// which we are allowed to access since we registered it in `update_archetype_component_access`.
// Note that we do not actually access any components in this function, we just get a shared
// reference to the sparse set, which is used to access the components in `Self::fetch`.
.unsafe_world()
.storages()
.sparse_sets
.get(component_id)
Expand Down Expand Up @@ -704,7 +714,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
world: UnsafeWorldCell<'w>,
&component_id: &ComponentId,
last_run: Tick,
this_run: Tick,
Expand All @@ -713,6 +723,8 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
table_data: None,
sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| {
world
// SAFETY: See &T::init_fetch.
.unsafe_world()
.storages()
.sparse_sets
.get(component_id)
Expand Down Expand Up @@ -866,7 +878,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
world: UnsafeWorldCell<'w>,
&component_id: &ComponentId,
last_run: Tick,
this_run: Tick,
Expand All @@ -875,6 +887,8 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
table_data: None,
sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| {
world
// SAFETY: See &T::init_fetch.
.unsafe_world()
.storages()
.sparse_sets
.get(component_id)
Expand Down Expand Up @@ -1011,7 +1025,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
world: UnsafeWorldCell<'w>,
state: &T::State,
last_run: Tick,
this_run: Tick,
Expand Down Expand Up @@ -1116,7 +1130,7 @@ macro_rules! impl_tuple_fetch {

#[inline]
#[allow(clippy::unused_unit)]
unsafe fn init_fetch<'w>(_world: &'w World, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
unsafe fn init_fetch<'w>(_world: UnsafeWorldCell<'w>, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
let ($($name,)*) = state;
($($name::init_fetch(_world, $name, _last_run, _this_run),)*)
}
Expand Down Expand Up @@ -1226,7 +1240,7 @@ macro_rules! impl_anytuple_fetch {

#[inline]
#[allow(clippy::unused_unit)]
unsafe fn init_fetch<'w>(_world: &'w World, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
unsafe fn init_fetch<'w>(_world: UnsafeWorldCell<'w>, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
let ($($name,)*) = state;
($(($name::init_fetch(_world, $name, _last_run, _this_run), false),)*)
}
Expand Down Expand Up @@ -1350,7 +1364,13 @@ unsafe impl<Q: WorldQuery> WorldQuery for NopWorldQuery<Q> {
const IS_ARCHETYPAL: bool = true;

#[inline(always)]
unsafe fn init_fetch(_world: &World, _state: &Q::State, _last_run: Tick, _this_run: Tick) {}
unsafe fn init_fetch(
_world: UnsafeWorldCell,
_state: &Q::State,
_last_run: Tick,
_this_run: Tick,
) {
}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}

Expand Down Expand Up @@ -1408,7 +1428,7 @@ unsafe impl<T: ?Sized> WorldQuery for PhantomData<T> {
fn shrink<'wlong: 'wshort, 'wshort>(_item: Self::Item<'wlong>) -> Self::Item<'wshort> {}

unsafe fn init_fetch<'w>(
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_state: &Self::State,
_last_run: Tick,
_this_run: Tick,
Expand Down
30 changes: 24 additions & 6 deletions crates/bevy_ecs/src/query/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
entity::Entity,
query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery},
storage::{Column, ComponentSparseSet, Table, TableRow},
world::World,
world::{unsafe_world_cell::UnsafeWorldCell, World},
};
use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref};
use bevy_utils::all_tuples;
Expand Down Expand Up @@ -51,7 +51,13 @@ unsafe impl<T: Component> WorldQuery for With<T> {
fn shrink<'wlong: 'wshort, 'wshort>(_: Self::Item<'wlong>) -> Self::Item<'wshort> {}

#[inline]
unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {}
unsafe fn init_fetch(
_world: UnsafeWorldCell,
_state: &ComponentId,
_last_run: Tick,
_this_run: Tick,
) {
}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}

Expand Down Expand Up @@ -148,7 +154,13 @@ unsafe impl<T: Component> WorldQuery for Without<T> {
fn shrink<'wlong: 'wshort, 'wshort>(_: Self::Item<'wlong>) -> Self::Item<'wshort> {}

#[inline]
unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {}
unsafe fn init_fetch(
_world: UnsafeWorldCell,
_state: &ComponentId,
_last_run: Tick,
_this_run: Tick,
) {
}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}

Expand Down Expand Up @@ -268,7 +280,7 @@ macro_rules! impl_query_filter_tuple {
const IS_ARCHETYPAL: bool = true $(&& $filter::IS_ARCHETYPAL)*;

#[inline]
unsafe fn init_fetch<'w>(world: &'w World, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
unsafe fn init_fetch<'w>(world: UnsafeWorldCell<'w>, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
let ($($filter,)*) = state;
($(OrFetch {
fetch: $filter::init_fetch(world, $filter, last_run, this_run),
Expand Down Expand Up @@ -412,12 +424,18 @@ macro_rules! impl_tick_filter {
}

#[inline]
unsafe fn init_fetch<'w>(world: &'w World, &id: &ComponentId, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
unsafe fn init_fetch<'w>(
world: UnsafeWorldCell<'w>,
&id: &ComponentId,
last_run: Tick,
this_run: Tick
) -> Self::Fetch<'w> {
Self::Fetch::<'w> {
table_ticks: None,
sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet)
.then(|| {
world.storages()
world.unsafe_world()
.storages()
.sparse_sets
.get(id)
.debug_checked_unwrap()
Expand Down
51 changes: 26 additions & 25 deletions crates/bevy_ecs/src/query/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ use crate::{
archetype::{ArchetypeEntity, ArchetypeId, Archetypes},
component::Tick,
entity::{Entities, Entity},
prelude::World,
query::{ArchetypeFilter, DebugCheckedUnwrap, QueryState, WorldQuery},
storage::{TableId, TableRow, Tables},
world::unsafe_world_cell::UnsafeWorldCell,
};
use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit};

Expand All @@ -23,20 +23,19 @@ pub struct QueryIter<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> {

impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIter<'w, 's, Q, F> {
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
/// This does not validate that `world.id()` matches `query_state.world_id`. Calling this on a `world`
/// with a mismatched [`WorldId`](crate::world::WorldId) is unsound.
/// - `world` must have permission to access any of the components registered in `query_state`.
/// - `world` must be the same one used to initialize `query_state`.
pub(crate) unsafe fn new(
world: &'w World,
world: UnsafeWorldCell<'w>,
query_state: &'s QueryState<Q, F>,
last_run: Tick,
this_run: Tick,
) -> Self {
QueryIter {
query_state,
tables: &world.storages().tables,
archetypes: &world.archetypes,
// SAFETY: We only access table data that has been registered in `query_state`.
tables: &world.unsafe_world().storages().tables,
archetypes: world.archetypes(),
cursor: QueryIterationCursor::init(world, query_state, last_run, this_run),
}
}
Expand Down Expand Up @@ -91,12 +90,10 @@ where
I::Item: Borrow<Entity>,
{
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
/// This does not validate that `world.id()` matches `query_state.world_id`. Calling this on a `world`
/// with a mismatched [`WorldId`](crate::world::WorldId) is unsound.
/// - `world` must have permission to access any of the components registered in `query_state`.
/// - `world` must be the same one used to initialize `query_state`.
pub(crate) unsafe fn new<EntityList: IntoIterator<IntoIter = I>>(
world: &'w World,
world: UnsafeWorldCell<'w>,
query_state: &'s QueryState<Q, F>,
entity_list: EntityList,
last_run: Tick,
Expand All @@ -106,9 +103,11 @@ where
let filter = F::init_fetch(world, &query_state.filter_state, last_run, this_run);
QueryManyIter {
query_state,
entities: &world.entities,
archetypes: &world.archetypes,
tables: &world.storages.tables,
entities: world.entities(),
archetypes: world.archetypes(),
// SAFETY: We only access table data that has been registered in `query_state`.
// This means `world` has permission to access the data we use.
tables: &world.unsafe_world().storages.tables,
fetch,
filter,
entity_iter: entity_list.into_iter(),
Expand Down Expand Up @@ -282,12 +281,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>
QueryCombinationIter<'w, 's, Q, F, K>
{
/// # Safety
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
/// have unique access to the components they query.
/// This does not validate that `world.id()` matches `query_state.world_id`. Calling this on a
/// `world` with a mismatched [`WorldId`](crate::world::WorldId) is unsound.
/// - `world` must have permission to access any of the components registered in `query_state`.
/// - `world` must be the same one used to initialize `query_state`.
pub(crate) unsafe fn new(
world: &'w World,
world: UnsafeWorldCell<'w>,
query_state: &'s QueryState<Q, F>,
last_run: Tick,
this_run: Tick,
Expand Down Expand Up @@ -318,8 +315,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize>

QueryCombinationIter {
query_state,
tables: &world.storages().tables,
archetypes: &world.archetypes,
// SAFETY: We only access table data that has been registered in `query_state`.
tables: &world.unsafe_world().storages().tables,
archetypes: world.archetypes(),
cursors: array.assume_init(),
}
}
Expand Down Expand Up @@ -485,7 +483,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
const IS_DENSE: bool = Q::IS_DENSE && F::IS_DENSE;

unsafe fn init_empty(
world: &'w World,
world: UnsafeWorldCell<'w>,
query_state: &'s QueryState<Q, F>,
last_run: Tick,
this_run: Tick,
Expand All @@ -497,8 +495,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
}
}

/// # Safety
/// - `world` must have permission to access any of the components registered in `query_state`.
/// - `world` must be the same one used to initialize `query_state`.
unsafe fn init(
world: &'w World,
world: UnsafeWorldCell<'w>,
query_state: &'s QueryState<Q, F>,
last_run: Tick,
this_run: Tick,
Expand Down
Loading

0 comments on commit db8d365

Please sign in to comment.