From 465bce68e3166f2591ff9024bf69357ef4bfe7ff Mon Sep 17 00:00:00 2001 From: Boxy Date: Wed, 11 Jan 2023 10:03:50 +0000 Subject: [PATCH] squishy --- crates/bevy_ecs/src/query/state.rs | 11 +- crates/bevy_ecs/src/system/mod.rs | 13 +- crates/bevy_ecs/src/system/query.rs | 326 +++++++++++++----- .../src/system/query/query_world_borrows.rs | 175 ++++++++++ crates/bevy_ecs/src/system/system_param.rs | 7 +- crates/bevy_ecs/src/world/mod.rs | 2 + 6 files changed, 446 insertions(+), 88 deletions(-) create mode 100644 crates/bevy_ecs/src/system/query/query_world_borrows.rs diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index c96dbca3938f83..a85073b45f70b0 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -128,11 +128,18 @@ impl QueryState { } /// Checks if the query is empty for the given [`World`], where the last change and current tick are given. + /// + /// This function will only access `world` in ways that are compliant with the access of `Q::ReadOnly` and + /// `F::ReadOnly` (though may not use those `WorldQuery`'s specifically). #[inline] pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access + // SAFETY: `&World` ensures we have readonly access to the whole world. A `NopWorldQuery` does not + // access anything so it is trivially true that it cannot violate any aliasing guarantees. We convert + // the `F` world query to its `ReadOnly` which is guaranteed by the `ReadOnlyWorldQuery` trait to not + // access the world mutably. unsafe { - self.as_nop() + self.as_readonly() + .as_nop() .iter_unchecked_manual(world, last_change_tick, change_tick) .next() .is_none() diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index aaa46d1209cbd0..59eb7d1c3b5472 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1209,8 +1209,17 @@ mod tests { let mut world1 = World::new(); let world2 = World::new(); let qstate = world1.query::<()>(); - // SAFETY: doesnt access anything - let query = unsafe { Query::new(&world2, &qstate, 0, 0, false) }; + let query = unsafe { + // SAFETY: query state was made with the same QF as this query has + Query::new( + // SAFETY: doesnt have any access + super::query_world_borrows::QueryLockBorrows::new(&world2), + &qstate, + 0, + 0, + false, + ) + }; query.iter(); } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 9f822ca786eb04..4df034c618c3ff 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1,3 +1,4 @@ +use self::query_world_borrows::QueryLockBorrows; use crate::{ component::Component, entity::Entity, @@ -9,6 +10,15 @@ use crate::{ }; use std::{any::TypeId, borrow::Borrow, fmt::Debug}; +// separate file instead of inline so that it will be more obvious from looking at list of changed files +// that scary soundness important safe code has been modified. +pub mod query_world_borrows; + +const _: () = { + struct AssertSendSync(T); + let _: AssertSendSync>; +}; + /// [System parameter] that provides selective access to the [`Component`] data stored in a [`World`]. /// /// Enables access to [entity identifiers] and [components] from a system, without the need to directly access the world. @@ -274,7 +284,7 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug}; /// [`With`]: crate::query::With /// [`Without`]: crate::query::Without pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { - world: &'world World, + lock_borrows: QueryLockBorrows<'world, (Q, F)>, state: &'state QueryState, last_change_tick: u32, change_tick: u32, @@ -288,7 +298,16 @@ pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> { impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> std::fmt::Debug for Query<'w, 's, Q, F> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "Query {{ matched entities: {}, world: {:?}, state: {:?}, last_change_tick: {}, change_tick: {} }}", self.iter().count(), self.world, self.state, self.last_change_tick, self.change_tick) + write!( + f, + "Query {{ matched entities: {}, world: {:?}, state: {:?}, last_change_tick: {}, change_tick: {} }}", + self.iter().count(), + // SAFETY: `World`'s `Debug` impl only accesses world metadata and `Query` always has access to that + unsafe { self.lock_borrows.world_ref() }, + self.state, + self.last_change_tick, + self.change_tick + ) } } @@ -301,21 +320,24 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// /// # Safety /// - /// This will create a query that could violate memory safety rules. Make sure that this is only - /// called in ways that ensure the queries have unique mutable access. + /// If `force_read_only_component_access` is `false` then `state`'s access must exactly correspond to what `(Q, F)` accesses. + /// If `force_read_only_component_access` is `true` then `state`'s access with all writes replaced by + /// reads must exactly correspond to what `(Q, F)` accesses. #[inline] pub(crate) unsafe fn new( - world: &'w World, + lock_borrows: QueryLockBorrows<'w, (Q, F)>, state: &'s QueryState, last_change_tick: u32, change_tick: u32, force_read_only_component_access: bool, ) -> Self { - state.validate_world(world); + // SAFETY: `validate_world` does not access anything other than the `WorldId` which + // even a `Query<(), ()>` has access to. + state.validate_world(unsafe { lock_borrows.world_ref() }); Self { force_read_only_component_access, - world, + lock_borrows, state, last_change_tick, change_tick, @@ -329,10 +351,19 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// or reusing functionality between systems via functions that accept query types. pub fn to_readonly(&self) -> Query<'_, 's, Q::ReadOnly, F::ReadOnly> { let new_state = self.state.as_readonly(); - // SAFETY: This is memory safe because it turns the query immutable. + // SAFETY: This is maybe not sound. + // + // `WorldQuery` requires that `Q::ReadOnly` has a subset of the access that `Q` has. This permits `Q::ReadOnly` to + // outright remove access rather than downgrading a write to a read. `force_read_only_component_access` only forces + // the writes in access of `QueryState` to be reads, it does not handle the situation where you have a `Query` with + // write access to some `T` that has a `type ReadOnly = ()`. This means that it would be possible to create a + // `Query<()>` where `get_component::` succeeds. + // + // Other than that, `WorldQuery` requries `Q::ReadOnly` and `ReadOnlyWorldQuery` requires it to have no writes which + // makes this sound if the above is fixed. unsafe { Query::new( - self.world, + self.lock_borrows.reborrow(), new_state, self.last_change_tick, self.change_tick, @@ -369,11 +400,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`for_each`](Self::for_each) for the closure based alternative. #[inline] pub fn iter(&self) -> QueryIter<'_, 's, Q::ReadOnly, F::ReadOnly> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().iter_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), self.last_change_tick, self.change_tick, ) @@ -406,11 +444,15 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`for_each_mut`](Self::for_each_mut) for the closure based alternative. #[inline] pub fn iter_mut(&mut self) -> QueryIter<'_, 's, Q, F> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { - self.state - .iter_unchecked_manual(self.world, self.last_change_tick, self.change_tick) + self.state.iter_unchecked_manual( + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), + self.last_change_tick, + self.change_tick, + ) } } @@ -437,11 +479,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations( &self, ) -> QueryCombinationIter<'_, 's, Q::ReadOnly, F::ReadOnly, K> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().iter_combinations_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), self.last_change_tick, self.change_tick, ) @@ -471,11 +520,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations_mut( &mut self, ) -> QueryCombinationIter<'_, 's, Q, F, K> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.iter_combinations_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), self.last_change_tick, self.change_tick, ) @@ -526,12 +576,19 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().iter_many_unchecked_manual( entities, - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), self.last_change_tick, self.change_tick, ) @@ -579,12 +636,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.iter_many_unchecked_manual( entities, - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), self.last_change_tick, self.change_tick, ) @@ -603,10 +661,15 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter`](Self::iter) and [`iter_mut`](Self::iter_mut) for the safe versions. #[inline] pub unsafe fn iter_unsafe(&self) -> QueryIter<'_, 's, Q, F> { - // SEMI-SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict - self.state - .iter_unchecked_manual(self.world, self.last_change_tick, self.change_tick) + // SAFETY: Aliasing correctness is justified below. + self.state.iter_unchecked_manual( + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.lock_borrows.world_ref_unbounded(), + self.last_change_tick, + self.change_tick, + ) } /// Iterates over all possible combinations of `K` query items without repetition. @@ -623,10 +686,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub unsafe fn iter_combinations_unsafe( &self, ) -> QueryCombinationIter<'_, 's, Q, F, K> { - // SEMI-SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. self.state.iter_combinations_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.lock_borrows.world_ref(), self.last_change_tick, self.change_tick, ) @@ -650,9 +715,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { + // SAFETY: Aliasing correctness is justified below. self.state.iter_many_unchecked_manual( entities, - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.lock_borrows.world_ref(), self.last_change_tick, self.change_tick, ) @@ -684,11 +753,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter`](Self::iter) for the iterator based alternative. #[inline] pub fn for_each<'this>(&'this self, f: impl FnMut(ROQueryItem<'this, Q>)) { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().for_each_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), f, self.last_change_tick, self.change_tick, @@ -722,11 +798,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter_mut`](Self::iter_mut) for the iterator based alternative. #[inline] pub fn for_each_mut<'a>(&'a mut self, f: impl FnMut(Q::Item<'a>)) { - // SAFETY: system runs without conflicts with other systems. same-system queries have runtime - // borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.for_each_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), f, self.last_change_tick, self.change_tick, @@ -764,11 +841,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { batch_size: usize, f: impl Fn(ROQueryItem<'this, Q>) + Send + Sync + Clone, ) { - // SAFETY: system runs without conflicts with other systems. same-system queries have runtime - // borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().par_for_each_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), batch_size, f, self.last_change_tick, @@ -797,11 +881,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { batch_size: usize, f: impl Fn(Q::Item<'a>) + Send + Sync + Clone, ) { - // SAFETY: system runs without conflicts with other systems. same-system queries have runtime - // borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.par_for_each_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), batch_size, f, self.last_change_tick, @@ -843,11 +928,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_mut`](Self::get_mut) to get a mutable query item. #[inline] pub fn get(&self, entity: Entity) -> Result, QueryEntityError> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().get_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), entity, self.last_change_tick, self.change_tick, @@ -869,10 +961,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { &self, entities: [Entity; N], ) -> Result<[ROQueryItem<'_, Q>; N], QueryEntityError> { - // SAFETY: it is the scheduler's responsibility to ensure that `Query` is never handed out on the wrong `World`. + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.get_many_read_only_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), entities, self.last_change_tick, self.change_tick, @@ -955,11 +1055,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get`](Self::get) to get a read-only query item. #[inline] pub fn get_mut(&mut self, entity: Entity) -> Result, QueryEntityError> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.get_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), entity, self.last_change_tick, self.change_tick, @@ -980,10 +1081,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { &mut self, entities: [Entity; N], ) -> Result<[Q::Item<'_>; N], QueryEntityError> { - // SAFETY: scheduler ensures safe Query world access + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.get_many_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), entities, self.last_change_tick, self.change_tick, @@ -1058,10 +1161,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_mut`](Self::get_mut) for the safe version. #[inline] pub unsafe fn get_unchecked(&self, entity: Entity) -> Result, QueryEntityError> { - // SEMI-SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict - self.state - .get_unchecked_manual(self.world, entity, self.last_change_tick, self.change_tick) + // SAFETY: Aliasing correctness is justified below. + self.state.get_unchecked_manual( + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.lock_borrows.world_ref_unbounded(), + entity, + self.last_change_tick, + self.change_tick, + ) } /// Returns a shared reference to the component `T` of the given [`Entity`]. @@ -1097,7 +1206,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_component_mut`](Self::get_component_mut) to get a mutable reference of a component. #[inline] pub fn get_component(&self, entity: Entity) -> Result<&T, QueryComponentError> { - let world = self.world; + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s, and we only access the component of type `T` once we checked that our `Q` or `F` + // have added a read for the corresponding `ArchetypeComponentId`. + let world = unsafe { self.lock_borrows.world_ref() }; let entity_ref = world .get_entity(entity) .ok_or(QueryComponentError::NoSuchEntity)?; @@ -1180,7 +1292,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { if self.force_read_only_component_access { return Err(QueryComponentError::MissingWriteAccess); } - let world = self.world; + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s, and we only access the component of type `T` once we checked that our `Q` or `F` + // have added a write for the corresponding `ArchetypeComponentId`. + let world = &self.lock_borrows.world_ref(); let entity_ref = world .get_entity(entity) .ok_or(QueryComponentError::NoSuchEntity)?; @@ -1268,12 +1383,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`single`](Self::single) for the panicking version. #[inline] pub fn get_single(&self) -> Result, QuerySingleError> { - // SAFETY: - // the query ensures that the components it accesses are not mutably accessible somewhere else - // and the query is read only. + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().get_single_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + self.lock_borrows.world_ref(), self.last_change_tick, self.change_tick, ) @@ -1339,12 +1460,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`single_mut`](Self::single_mut) for the panicking version. #[inline] pub fn get_single_mut(&mut self) -> Result, QuerySingleError> { - // SAFETY: - // the query ensures mutable access to the components it accesses, and the query - // is uniquely borrowed + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.get_single_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + self.lock_borrows.world_mut(), self.last_change_tick, self.change_tick, ) @@ -1373,8 +1494,21 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn is_empty(&self) -> bool { - self.state - .is_empty(self.world, self.last_change_tick, self.change_tick) + self.state.is_empty( + // SAFETY: + // -`Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // - The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with `Q` and `F`(we do, see first point) then we can + // access the world using `Q::ReadOnly` and `F::ReadOnly`. + // - `ReadOnlyWorldQuery` ensures that `Q::ReadOnly` and `F::ReadOnly` do not access the world mutably + // therefore it is fine to access the world with them using a shared reference. + // - `state.is_empty()` guarantees it does not access anything in ways that are not compliant with the access + // of `Q::ReadOnly` and `F::ReadOnly`. + unsafe { self.lock_borrows.world_ref() }, + self.last_change_tick, + self.change_tick, + ) } /// Returns `true` if the given [`Entity`] matches the query. @@ -1401,11 +1535,29 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn contains(&self, entity: Entity) -> bool { - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access + // SAFETY: + // - `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. + // + // The `WorldQuery` trait guarantees that `Self::ReadOnly` has a subset of the access that `Self` has, + // therefore if we're allowed to access the world with and `F` (we do, see first point) then we can access + // the world using and `F::ReadOnly`. + // + // `ReadOnlyWorldQuery` ensures that `F::ReadOnly` does not access the world mutably therefore it is fine to + // access the world with it using a shared reference. + // - `NopWorldQuery` does not access anything so it is trivially true that `NopWorldQuery` does not break + // any aliasing guarantees. unsafe { self.state + .as_readonly() .as_nop() - .get_unchecked_manual(self.world, entity, self.last_change_tick, self.change_tick) + .get_unchecked_manual( + // SAFETY: see above + self.lock_borrows.world_ref(), + entity, + self.last_change_tick, + self.change_tick, + ) .is_ok() } } @@ -1465,6 +1617,10 @@ impl std::fmt::Display for QueryComponentError { } } +// FIXME: we should have `_inner` variants of all functions that take `self` instead of `&self` +// along with no `Q: ReadOnlyWorldQuery` bound. We should also implement copy for `WorldQuery` +// when both `Q: ReadOnlyWorldQuery` and `F: ReadOnlyWorldQuery` hold and then stop using +// `QueryLockBorrows::world_ref_unbounded` in favor of `QueryLockBorrows::into_world_ref`. impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// Returns the query item for the given [`Entity`], with the actual "inner" world lifetime. /// @@ -1500,11 +1656,14 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_inner(&self, entity: Entity) -> Result, QueryEntityError> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().get_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. The `Q` and `F` types both implement `ReadOnlyWorldQuery` so their + // `ReadOnly` assoc types are equal to `Q` and `F` respectively, this means that there is no change in access + // between us using `Q` or `Q::ReadOnly` and we already justified being able to access the world using `Q` and `F`. + self.lock_borrows.into_world_ref(), entity, self.last_change_tick, self.change_tick, @@ -1537,11 +1696,14 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn iter_inner(&self) -> QueryIter<'w, 's, Q::ReadOnly, F::ReadOnly> { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. unsafe { self.state.as_readonly().iter_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryLockBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. The `Q` and `F` types both implement `ReadOnlyWorldQuery` so their + // `ReadOnly` assoc types are equal to `Q` and `F` respectively, this means that there is no change in access + // between us using `Q` or `Q::ReadOnly` and we already justified being able to access the world using `Q` and `F`. + self.lock_borrows.world_ref_unbounded(), self.last_change_tick, self.change_tick, ) diff --git a/crates/bevy_ecs/src/system/query/query_world_borrows.rs b/crates/bevy_ecs/src/system/query/query_world_borrows.rs new file mode 100644 index 00000000000000..00a046bdebb83e --- /dev/null +++ b/crates/bevy_ecs/src/system/query/query_world_borrows.rs @@ -0,0 +1,175 @@ +//! Conceptually the world borrows for `Query` are: +//! +//! `&'lock mut HashMap>` +//! +//! or for a `(Q, F): ReadOnlyWorldQuery`: +//! +//! `&'lock HashMap>` +//! +//! Lifetime names are a bit confusing here because the `'world` lifetime on `Query` actually corresponds to +//! the `'lock` lifetime I used here and there is no lifetime on `Query` representing the `'world` in the above types. +//! For the rest of these comments I'll use `qworld` to refer to the `'world` lifetime on `Query` and `'world` to refer +//! to the one in the type above. +//! +//! --- +//! +//! Perhaps counter to intuition `Query` does not 'own' the locks for its component access, this is required in order +//! to allow [`Query::get_inner`] and [`Query::iter_inner`] to exist (these methods allow producing borrows that outlive +//! the `Query`) +//! +//! A notable thing about this is that for non-readonly `Query`'s the world borrows are an `&'lock mut ...` this +//! helps to explain why the lifetime annotations on `Query` methods do not return borrows tied to `'qworld` but instead `'_` +//! (see [#4243](https://github.com/bevyengine/bevy/pull/4243)). In our methods we either have `&Self` or `&mut Self` which +//! also means that inside of `Query::get (_mut?)` we have: +//! +//! `&'a (mut?) &'lock mut HashMap>` +//! +//! and borrow check would naturally prevent us from creating an `&'a (mut?) Column` out of this, which is what the unsound fn sig: +//! +//! `fn get_mut(&mut self, e: Entity) -> QueryItem<'qworld, Q>` +//! +//! would require doing. +//! +//! --- +//! +//! The [`QueryLockBorrows`] abstraction exists to help prevent this kind of impl footgun by never giving out a +//! `&'qworld World` without taking ownership of `QueryLockBorrows` (conceptually similar to having a `&'a mut &'b mut T` +//! and consuming the `&'a mut ..` to get a `&'a mut T`) +//! +//! This abstraction also exists to make all ways of gaining access to the `&World` that `Query` requires, unsafe. Since the +//! `&World` does not actually mean you can access the entire world immutably, a large amount of safe APIs on `World` would +//! be unsound to call which is a footgun for anyone implementing `Query`. +//! +//! In the future this type may be extended to _actually_ store a borrow of a hashmap when `debug_assertions` are enabled +//! so that we can check that all usage of the query is consistent with what access we told the scheduler we required. + +use super::*; +use std::marker::PhantomData; + +/// Modelling `Query`'s borrow on `World` as `&'qworld World` is not "correct" for many reasons. +/// This struct intends to hide the `&'qworld World` and expose an API that is more similar to what +/// the `&'qworld World` conceptually represents. +/// +/// This struct only implements [`Copy`] and [`Clone`] when `QF: ReadOnlyWorldQuery` to mimic the fact that +/// `ReadOnlyWorldQuery` borrows should act like `&'lock HashMap<..., ...>` wheras mutable world queries act +/// like `&'lock mut HashMap<..., ...>` (see module level docs). +pub struct QueryLockBorrows<'lock, QF: WorldQuery> { + world: &'lock World, + // invariance because its probably possible to have two worldquery's where one is the subtype of another + // but each have different mutabilities. i.e. `dyn for<'a> Trait<'a>: WorldQuery` and `dyn Trait<'static>: WorldQuery`. + // probably not unsound since `Q` and `F` are already invariant on `Query` but this seems like a footgun and I don't care + // to try and reason about if this could cause trouble when covariant. + // we dont use `*mut QF` because it would make this type not `Sync` and we dont use `&'static mut QF` because it + // would implicitly require `QF: 'static`. + _p: PhantomData QF>, +} + +impl<'lock, QF: WorldQuery> QueryLockBorrows<'lock, QF> { + // FIXME: this should take some kind of `InteriorMutableWorld`, see #5956. + /// # Safety + /// + /// It must be valid to access data specified by the `QF` `WorldQuery` from + /// `world` for as long as the `'lock` lifetime is live. + pub unsafe fn new(world: &'lock World) -> Self { + Self { + world, + _p: PhantomData, + } + } + + /// See module level docs for why this takes `self` // TODO + /// + /// # Safety + /// - The `World` must not be accessed in a way that the `Query`'s access does not give + /// it permission to. You should be careful when working with this `&World` as many safe functions + /// on `World` will be unsound to call. + pub unsafe fn into_world_ref(self) -> &'lock World { + self.world + } + + /// If the returned `World` is going to be accessed mutably consider using + /// [`QueryLockBorrows::world_mut`] instead. + /// + /// See module level docs for why this does not return `&'world World` + /// + /// # Safety + /// - The `World` must not be accessed in a way that the `Query`'s access does not give + /// it permission to. You should be careful when working with this `&World` as many safe functions + /// on `World` will be unsound to call. + pub unsafe fn world_ref(&self) -> &World { + self.world + } + + /// This is the same as [`QueryLockBorrows::world_mut`] except that it ties the `&World` + /// to a mutable borrow of self, in theory allowing the borrow checker to catch more mistakes. + /// `world_mut` should be used whenever mutable access of world is required if possible, otherwise use `world` instead. + /// + /// See module level docs for why this does not return `&'world World` + /// + /// # Safety + /// - The `World` must not be accessed in a way that the `Query`'s access does not give + /// it permission to. You should be careful when working with this `&World` as many safe functions + /// on `World` will be unsound to call. + pub unsafe fn world_mut(&mut self) -> &World { + self.world + } + + // FIXME ideally we remove this method its super sketchy + /// Returns the underlying `&'world World` without the lifetime tied to the borrow of self that this method makes. + /// You should almost NEVER use this method and instead opt to use `world_ref` `world_mut` or `into_world_ref`. + /// + /// # Safety + /// - The `World` must not be accessed in a way that the `Query`'s access does not give + /// it permission to. You should be careful when working with this `&World` as many safe functions + /// on `World` will be unsound to call. + /// - As the returned lifetime is not bound to `&self` you should avoid calling any methods on this struct + /// until you can be sure that the returned borrow (and any copies of it) are dead. + pub unsafe fn world_ref_unbounded(&self) -> &'lock World { + self.world + } + + /// This API mimics reborrowing `&'_ mut &'lock mut HashMap<..., ...>` + /// as `&'_ mut HashMap<..., ...>`. If `QF: ReadOnlyWorldQuery` holds then you should + /// just copy/clone `QueryLockBorrows` out from underneath the reference. + /// + /// See also [`QueryLockBorrows::reborrow`] for a version that returns readonly QF. + pub fn reborrow_mut(&mut self) -> QueryLockBorrows<'_, QF> { + QueryLockBorrows { + world: self.world, + _p: PhantomData, + } + } + + /// This API mimics reborrowing `&'_ &'lock mut HashMap<..., ...>` + /// as `&'_ HashMap<..., ...>`. If `QF: ReadOnlyWorldQuery` holds then you should + /// just copy/clone `QueryLockBorrows` out from underneath the reference. + /// + /// See also [`QueryLockBorrows::reborrow_mut`] for a version that returns QF. + pub fn reborrow(&self) -> QueryLockBorrows<'_, QF::ReadOnly> { + QueryLockBorrows { + world: self.world, + _p: PhantomData, + } + } + + pub fn to_readonly(self) -> QueryLockBorrows<'lock, QF::ReadOnly> { + QueryLockBorrows { + world: self.world, + _p: PhantomData, + } + } +} + +/// See module level docs and [`QueryLockBorrows`] docs for why this is only implemented for +/// `QF: ReadOnlyWorldQuery` instead of all `QF`. +impl Copy for QueryLockBorrows<'_, QF> {} +/// See module level docs and [`QueryLockBorrows`] docs for why this is only implemented for +/// `QF: ReadOnlyWorldQuery` instead of all `QF`. +impl Clone for QueryLockBorrows<'_, QF> { + fn clone(&self) -> Self { + Self { + world: self.world, + _p: PhantomData, + } + } +} diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index a229925eca6fc1..0049f0a87387d0 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -191,7 +191,7 @@ unsafe impl SystemPara type Item<'w, 's> = Query<'w, 's, Q, F>; fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { - let state = QueryState::new(world); + let state = QueryState::::new(world); assert_component_access_compatibility( &system_meta.name, std::any::type_name::(), @@ -223,8 +223,11 @@ unsafe impl SystemPara world: &'w World, change_tick: u32, ) -> Self::Item<'w, 's> { + // SAFETY: `state` was constructed with the same `QF` as this `Query`. Query::new( - world, + // SAFETY: caller ensures that all accesss specified in `init_state` are + // valid for us to access. `QF` is the same access as what is specified in `init_state`. + super::query_world_borrows::QueryLockBorrows::new(world), state, system_meta.last_change_tick, change_tick, diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 3146f2a97f60cd..6ffc4d4d072e9a 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1719,6 +1719,8 @@ impl World { impl fmt::Debug for World { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // SAFETY: `Query` calls this function in its `Debug` impl therefore we must be careful not to + // access any data in world that a `Query<(), ()>` would not be allowed to access. f.debug_struct("World") .field("id", &self.id) .field("entity_count", &self.entities.len())