Use UnsafeWorldCell to increase code quality for SystemParam (#8174)

# Objective

The type `&World` is currently in an awkward place, since it has two
meanings:
1. Read-only access to the entire world.
2. Interior mutable access to the world; immutable and/or mutable access
to certain portions of world data.

This makes `&World` difficult to reason about, and surprising to see in
function signatures if one does not know about the interior mutable
property.

The type `UnsafeWorldCell` was added in #6404, which is meant to
alleviate this confusion by adding a dedicated type for interior mutable
world access. However, much of the engine still treats `&World` as an
interior mutable-ish type. One of those places is `SystemParam`.

## Solution

Modify `SystemParam::get_param` to accept `UnsafeWorldCell` instead of
`&World`. Simplify the safety invariants, since the `UnsafeWorldCell`
type encapsulates the concept of constrained world access.

---

## Changelog

`SystemParam::get_param` now accepts an `UnsafeWorldCell` instead of
`&World`. This type provides a high-level API for unsafe interior
mutable world access.

## Migration Guide

For manual implementers of `SystemParam`: the function `get_item` now
takes `UnsafeWorldCell` instead of `&World`. To access world data, use:

* `.get_entity()`, which returns an `UnsafeEntityCell` which can be used
to access component data.
* `get_resource()` and its variants, to access resource data.
This commit is contained in:
JoJoJet 2023-04-01 11:45:07 -04:00 committed by GitHub
parent 253db50c63
commit 3442a13d2c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 57 additions and 48 deletions

View file

@ -229,7 +229,7 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
ParamSet {
@ -407,7 +407,7 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &#path::system::SystemMeta,
world: &'w #path::world::World,
world: #path::world::unsafe_world_cell::UnsafeWorldCell<'w>,
change_tick: #path::component::Tick,
) -> Self::Item<'w, 's> {
let (#(#tuple_patterns,)*) = <

View file

@ -8,7 +8,7 @@ use crate::{
prelude::Local,
storage::SparseSet,
system::{ReadOnlySystemParam, SystemMeta, SystemParam},
world::World,
world::{unsafe_world_cell::UnsafeWorldCell, World},
};
use std::{
@ -264,9 +264,9 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentEvents {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.removed_components()
world.world_metadata().removed_components()
}
}

View file

@ -291,7 +291,12 @@ impl<Param: SystemParam> SystemState<Param> {
world: &'w World,
change_tick: Tick,
) -> SystemParamItem<'w, 's, Param> {
let param = Param::get_param(&mut self.param_state, &self.meta, world, change_tick);
let param = Param::get_param(
&mut self.param_state,
&self.meta,
world.as_unsafe_world_cell_migration_internal(),
change_tick,
);
self.meta.last_run = change_tick;
param
}
@ -481,7 +486,7 @@ where
let params = F::Param::get_param(
self.param_state.as_mut().expect(Self::PARAM_MESSAGE),
&self.system_meta,
world,
world.as_unsafe_world_cell_migration_internal(),
change_tick,
);
let out = self.func.run(input, params);

View file

@ -9,7 +9,7 @@ use crate::{
Access, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery,
},
system::{Query, SystemMeta},
world::{FromWorld, World},
world::{unsafe_world_cell::UnsafeWorldCell, FromWorld, World},
};
use bevy_ecs_macros::impl_param_set;
pub use bevy_ecs_macros::Resource;
@ -127,14 +127,13 @@ pub unsafe trait SystemParam: Sized {
/// # Safety
///
/// This call might use any of the [`World`] accesses that were registered in [`Self::init_state`].
/// - None of those accesses may conflict with any other [`SystemParam`]s
/// that exist at the same time, including those on other threads.
/// - The passed [`UnsafeWorldCell`] must have access to any world data
/// registered in [`init_state`](SystemParam::init_state).
/// - `world` must be the same `World` that was used to initialize [`state`](SystemParam::init_state).
unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State,
system_meta: &SystemMeta,
world: &'world World,
world: UnsafeWorldCell<'world>,
change_tick: Tick,
) -> Self::Item<'world, 'state>;
}
@ -192,10 +191,19 @@ unsafe impl<Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemPara
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
Query::new(world, state, system_meta.last_run, change_tick, false)
Query::new(
// SAFETY: We have registered all of the query's world accesses,
// so the caller ensures that `world` has permission to access any
// world data that the query needs.
world.unsafe_world(),
state,
system_meta.last_run,
change_tick,
false,
)
}
}
@ -329,7 +337,7 @@ fn assert_component_access_compatibility(
/// ```
pub struct ParamSet<'w, 's, T: SystemParam> {
param_states: &'s mut T::State,
world: &'w World,
world: UnsafeWorldCell<'w>,
system_meta: SystemMeta,
change_tick: Tick,
}
@ -435,11 +443,10 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let (ptr, ticks) = world
.as_unsafe_world_cell_migration_internal()
.get_resource_with_ticks(component_id)
.unwrap_or_else(|| {
panic!(
@ -476,11 +483,10 @@ unsafe impl<'a, T: Resource> SystemParam for Option<Res<'a, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_resource_with_ticks(component_id)
.map(|(ptr, ticks)| Res {
value: ptr.deref(),
@ -530,11 +536,10 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let value = world
.as_unsafe_world_cell_migration_internal()
.get_resource_mut_by_id(component_id)
.unwrap_or_else(|| {
panic!(
@ -568,11 +573,10 @@ unsafe impl<'a, T: Resource> SystemParam for Option<ResMut<'a, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_resource_mut_by_id(component_id)
.map(|value| ResMut {
value: value.value.deref_mut::<T>(),
@ -621,10 +625,11 @@ unsafe impl SystemParam for &'_ World {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world
// SAFETY: Read-only access to the entire world was registerd in `init_state`.
world.world()
}
}
@ -742,7 +747,7 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
Local(state.get())
@ -916,7 +921,7 @@ unsafe impl<T: SystemBuffer> SystemParam for Deferred<'_, T> {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
Deferred(state.get())
@ -1022,11 +1027,10 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let (ptr, ticks) = world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.unwrap_or_else(|| {
panic!(
@ -1061,11 +1065,10 @@ unsafe impl<T: 'static> SystemParam for Option<NonSend<'_, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.map(|(ptr, ticks)| NonSend {
value: ptr.deref(),
@ -1114,11 +1117,10 @@ unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
let (ptr, ticks) = world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.unwrap_or_else(|| {
panic!(
@ -1147,11 +1149,10 @@ unsafe impl<'a, T: 'static> SystemParam for Option<NonSendMut<'a, T>> {
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
world
.as_unsafe_world_cell_migration_internal()
.get_non_send_with_ticks(component_id)
.map(|(ptr, ticks)| NonSendMut {
value: ptr.assert_unique().deref_mut(),
@ -1174,7 +1175,7 @@ unsafe impl<'a> SystemParam for &'a Archetypes {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.archetypes()
@ -1195,7 +1196,7 @@ unsafe impl<'a> SystemParam for &'a Components {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.components()
@ -1216,7 +1217,7 @@ unsafe impl<'a> SystemParam for &'a Entities {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.entities()
@ -1237,7 +1238,7 @@ unsafe impl<'a> SystemParam for &'a Bundles {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
world.bundles()
@ -1286,7 +1287,7 @@ unsafe impl SystemParam for SystemChangeTick {
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
SystemChangeTick {
@ -1356,7 +1357,7 @@ unsafe impl SystemParam for SystemName<'_> {
unsafe fn get_param<'w, 's>(
name: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
SystemName { name }
@ -1398,7 +1399,7 @@ macro_rules! impl_system_param_tuple {
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
_system_meta: &SystemMeta,
_world: &'w World,
_world: UnsafeWorldCell<'w>,
_change_tick: Tick,
) -> Self::Item<'w, 's> {
@ -1521,7 +1522,7 @@ unsafe impl<P: SystemParam + 'static> SystemParam for StaticSystemParam<'_, '_,
unsafe fn get_param<'world, 'state>(
state: &'state mut Self::State,
system_meta: &SystemMeta,
world: &'world World,
world: UnsafeWorldCell<'world>,
change_tick: Tick,
) -> Self::Item<'world, 'state> {
// SAFETY: Defer to the safety of P::SystemParam
@ -1539,7 +1540,7 @@ unsafe impl<T: ?Sized> SystemParam for PhantomData<T> {
unsafe fn get_param<'world, 'state>(
_state: &'state mut Self::State,
_system_meta: &SystemMeta,
_world: &'world World,
_world: UnsafeWorldCell<'world>,
_change_tick: Tick,
) -> Self::Item<'world, 'state> {
PhantomData

View file

@ -6,6 +6,8 @@ use crate::{
};
use std::sync::atomic::{AtomicUsize, Ordering};
use super::unsafe_world_cell::UnsafeWorldCell;
#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
// We use usize here because that is the largest `Atomic` we want to require
/// A unique identifier for a [`World`].
@ -56,10 +58,10 @@ unsafe impl SystemParam for WorldId {
unsafe fn get_param<'world, 'state>(
_: &'state mut Self::State,
_: &crate::system::SystemMeta,
world: &'world super::World,
world: UnsafeWorldCell<'world>,
_: Tick,
) -> Self::Item<'world, 'state> {
world.id
world.world_metadata().id()
}
}

View file

@ -144,7 +144,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// # Safety
/// - must not be used in a way that would conflict with any
/// live exclusive borrows on world data
unsafe fn unsafe_world(self) -> &'w World {
pub(crate) unsafe fn unsafe_world(self) -> &'w World {
// SAFETY:
// - caller ensures that the returned `&World` is not used in a way that would conflict
// with any existing mutable borrows of world data

View file

@ -3,6 +3,7 @@ use bevy_ecs::{
component::Tick,
prelude::*,
system::{ReadOnlySystemParam, SystemMeta, SystemParam, SystemParamItem, SystemState},
world::unsafe_world_cell::UnsafeWorldCell,
};
use std::ops::{Deref, DerefMut};
@ -76,7 +77,7 @@ where
unsafe fn get_param<'w, 's>(
state: &'s mut Self::State,
system_meta: &SystemMeta,
world: &'w World,
world: UnsafeWorldCell<'w>,
change_tick: Tick,
) -> Self::Item<'w, 's> {
// SAFETY: