From 5a08b7d31ae152e4d333e1ffb45fd29fc297c316 Mon Sep 17 00:00:00 2001 From: Ellen Date: Sat, 6 Aug 2022 11:51:40 +0100 Subject: [PATCH] blahblah --- crates/bevy_ecs/src/query/state.rs | 12 +- crates/bevy_ecs/src/system/query.rs | 531 ++++++++++++++++++--- crates/bevy_ecs/src/system/system_param.rs | 7 +- 3 files changed, 470 insertions(+), 80 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 9a1f08cde42d37..028a1e38f4f212 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -115,11 +115,19 @@ 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. (FIXME) `WorldId` being correct is not currently upheld. The user can just + // pass in any `World` they like. 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/query.rs b/crates/bevy_ecs/src/system/query.rs index 262d12436a5aa3..c65326e565e2da 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -9,6 +9,8 @@ use crate::{ }; use std::{any::TypeId, borrow::Borrow, fmt::Debug}; +use self::query_world_borrows::QueryWorldBorrows; + /// Provides scoped access to components in a [`World`]. /// /// Queries enable iteration over entities and their components as well as filtering them @@ -250,12 +252,184 @@ use std::{any::TypeId, borrow::Borrow, fmt::Debug}; /// number of query results differ from being exactly one. If that's the case, use `iter.next()` /// (or `iter_mut.next()`) to only get the first query result. pub struct Query<'world, 'state, Q: WorldQuery, F: WorldQuery = ()> { - pub(crate) world: &'world World, + pub(crate) world: QueryWorldBorrows<'world, (Q, F)>, pub(crate) state: &'state QueryState, pub(crate) last_change_tick: u32, pub(crate) change_tick: u32, } +pub mod query_world_borrows { + //! 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 [`QueryWorldBorrows`] abstraction exists to help prevent this kind of impl footgun by never giving out a + //! `&'qworld World` without taking ownership of `QueryWorldBorrows` (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 QueryWorldBorrows<'world, QF: WorldQuery> { + world: &'world 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. + _p: PhantomData<*mut QF>, + } + + impl<'world, QF: WorldQuery> QueryWorldBorrows<'world, QF> { + // FIXME: should this be unsafe? + pub fn new(world: &'world 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) -> &'world World { + self.world + } + + /// If the returned `World` is going to be accessed mutably consider using + /// [`QueryWorldBorrows::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(&self) -> &World { + self.world + } + + /// This is the same as [`QueryWorldBorrows::world`] 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` `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_unbounded(&self) -> &'world World { + self.world + } + + /// This API mimics reborrowing `&'_ mut &'lock mut HashMap<..., ...>` + /// as `&'_ mut HashMap<..., ...>`. If `QF: ReadOnlyWorldQuery` holds then you should + /// just copy/clone `QueryWorldBorrows` out from underneath the reference. + /// + /// See also [`QueryWorldBorrows::reborrow`] for a version that returns readonly QF. + pub fn reborrow_mut(&mut self) -> QueryWorldBorrows<'_, QF> { + QueryWorldBorrows { + world: self.world, + _p: PhantomData, + } + } + + /// This API mimics reborrowing `&'_ &'lock mut HashMap<..., ...>` + /// as `&'_ HashMap<..., ...>`. If `QF: ReadOnlyWorldQuery` holds then you should + /// just copy/clone `QueryWorldBorrows` out from underneath the reference. + /// + /// See also [`QueryWorldBorrows::reborrow_mut`] for a version that returns QF. + pub fn reborrow(&self) -> QueryWorldBorrows<'_, QF::ReadOnly> { + QueryWorldBorrows { + world: self.world, + _p: PhantomData, + } + } + + pub fn to_readonly(self) -> QueryWorldBorrows<'world, QF::ReadOnly> { + QueryWorldBorrows { + world: self.world, + _p: PhantomData, + } + } + } + + /// See module level docs and [`QueryWorldBorrows`] docs for why this is only implemented for + /// `QF: ReadOnlyWorldQuery` instead of all `QF`. + impl Copy for QueryWorldBorrows<'_, QF> {} + /// See module level docs and [`QueryWorldBorrows`] docs for why this is only implemented for + /// `QF: ReadOnlyWorldQuery` instead of all `QF`. + impl Clone for QueryWorldBorrows<'_, QF> { + fn clone(&self) -> Self { + Self { + world: self.world, + _p: PhantomData, + } + } + } +} + impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// Creates a new query. /// @@ -265,7 +439,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// called in ways that ensure the queries have unique mutable access. #[inline] pub(crate) unsafe fn new( - world: &'w World, + world: QueryWorldBorrows<'w, (Q, F)>, state: &'s QueryState, last_change_tick: u32, change_tick: u32, @@ -288,7 +462,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { // SAFETY: This is memory safe because it turns the query immutable. unsafe { Query::new( - self.world, + self.world.reborrow(), new_state, self.last_change_tick, self.change_tick, @@ -321,11 +495,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().iter_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), self.last_change_tick, self.change_tick, ) @@ -354,11 +538,18 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. 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 `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), + self.last_change_tick, + self.change_tick, + ) } } @@ -373,11 +564,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations( &self, ) -> QueryCombinationIter<'_, '_, 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().iter_combinations_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), self.last_change_tick, self.change_tick, ) @@ -410,11 +611,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations_mut( &mut self, ) -> QueryCombinationIter<'_, '_, 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.iter_combinations_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), self.last_change_tick, self.change_tick, ) @@ -459,12 +664,22 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().iter_many_unchecked_manual( entities, - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), self.last_change_tick, self.change_tick, ) @@ -508,12 +723,16 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.iter_many_unchecked_manual( entities, - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), self.last_change_tick, self.change_tick, ) @@ -528,10 +747,18 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// this call does not result in multiple mutable references to the same component #[inline] pub unsafe fn iter_unsafe(&'s self) -> QueryIter<'w, '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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. + self.state.iter_unchecked_manual( + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.world.world_unbounded(), + self.last_change_tick, + self.change_tick, + ) } /// Iterates over all possible combinations of `K` query results without repetition. @@ -544,10 +771,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { pub unsafe fn iter_combinations_unsafe( &self, ) -> QueryCombinationIter<'_, '_, 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. self.state.iter_combinations_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.world.world_unbounded(), self.last_change_tick, self.change_tick, ) @@ -568,9 +800,16 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { + // SAFETY: Aliasing correctness is justified below. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. self.state.iter_many_unchecked_manual( entities, - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.world.world(), self.last_change_tick, self.change_tick, ) @@ -601,11 +840,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().for_each_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), f, self.last_change_tick, self.change_tick, @@ -636,11 +885,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn for_each_mut<'a, FN: FnMut(QueryItem<'a, Q>)>(&'a mut self, f: FN) { - // SAFETY: system runs without conflicts with other systems. same-system queries have runtime - // borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.for_each_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), f, self.last_change_tick, self.change_tick, @@ -679,11 +932,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().par_for_each_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), batch_size, f, self.last_change_tick, @@ -706,11 +969,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { batch_size: usize, f: FN, ) { - // SAFETY: system runs without conflicts with other systems. same-system queries have runtime - // borrow checks when they conflict + // SAFETY: Aliasing correctness is justified below. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.par_for_each_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), batch_size, f, self.last_change_tick, @@ -752,11 +1019,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().get_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), entity, self.last_change_tick, self.change_tick, @@ -777,10 +1054,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`.. unsafe { self.state.get_many_read_only_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), entities, self.last_change_tick, self.change_tick, @@ -854,11 +1142,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.get_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), entity, self.last_change_tick, self.change_tick, @@ -877,10 +1169,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &mut self, entities: [Entity; N], ) -> Result<[QueryItem<'_, Q>; N], QueryEntityError> { - // SAFETY: scheduler ensures safe Query world access + // SAFETY: Aliasing correctness is justified below. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.get_many_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), entities, self.last_change_tick, self.change_tick, @@ -948,10 +1245,19 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &'s 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. + self.state.get_unchecked_manual( + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. Additionally the caller is responsible for any aliasing issues caused + // by the lifetime on the returned data being not correctly constrained. + self.world.world_unbounded(), + entity, + self.last_change_tick, + self.change_tick, + ) } /// Returns a reference to the [`Entity`]'s [`Component`] of the given type. @@ -984,7 +1290,11 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_component(&self, entity: Entity) -> Result<&T, QueryComponentError> { - let world = self.world; + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` 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`. (FIXME) This is partially wishful + // thinking as `Query` is constructable (and fields mutateable) via safe code from anywhere in `bevy_ecs`. + let world = unsafe { self.world.world() }; let entity_ref = world .get_entity(entity) .ok_or(QueryComponentError::NoSuchEntity)?; @@ -1056,7 +1366,11 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { &self, entity: Entity, ) -> Result, QueryComponentError> { - let world = self.world; + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` 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`. (FIXME) This is partially wishful + // thinking as `Query` is constructable (and fields mutateable) via safe code from anywhere in `bevy_ecs`. + let world = &self.world.world(); let entity_ref = world .get_entity(entity) .ok_or(QueryComponentError::NoSuchEntity)?; @@ -1144,12 +1458,21 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().get_single_unchecked_manual( - self.world, + // SAFETY: + // -`Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world(), self.last_change_tick, self.change_tick, ) @@ -1209,12 +1532,15 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.get_single_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + self.world.world_mut(), self.last_change_tick, self.change_tick, ) @@ -1244,8 +1570,22 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> 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 `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // - 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.world.world() }, + self.last_change_tick, + self.change_tick, + ) } /// Returns `true` if the given [`Entity`] matches the query. @@ -1271,11 +1611,32 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> 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 `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. + // + // 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. + // - (FIXME) `WorldId` being correct is not currently upheld. `Query::new` does not require the `WorldId` to be + // correct and even if it did `Query` is constructable (and fields mutateable) via safe code from anywhere in `bevy_ecs`. 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.world.world(), + entity, + self.last_change_tick, + self.change_tick, + ) .is_ok() } } @@ -1335,7 +1696,11 @@ impl std::fmt::Display for QueryComponentError { } } -impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { +// 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 +// `QueryWorldBorrows::world_unbounded` in favor of `QueryWorldBorrows::into_world_ref`. +impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// Returns the query result for the given [`Entity`], with the actual "inner" world lifetime. /// /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is @@ -1369,11 +1734,17 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn get_inner(&'s 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().get_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. 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.world.into_world_ref(), entity, self.last_change_tick, self.change_tick, @@ -1406,11 +1777,17 @@ impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn iter_inner(&'s 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. (FIXME) `WorldId` being correct is not currently upheld. + // `Query::new` does not require the `WorldId` to be correct and even if it did `Query` is constructable + // (and fields mutateable) via safe code from anywhere in `bevy_ecs`. unsafe { self.state.as_readonly().iter_unchecked_manual( - self.world, + // SAFETY: `Query::new` upholds that `QueryWorldBorrows` has adequate access for our `Q` and `F` + // `WorldQuery`'s. (FIXME) This is partially wishful thinking as `Query` is constructable (and fields mutateable) + // via safe code from anywhere in `bevy_ecs`. 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.world.world_unbounded(), self.last_change_tick, self.change_tick, ) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 0472e45c367c66..aeca9fded3e67a 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -181,7 +181,12 @@ impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParamFetch< world: &'w World, change_tick: u32, ) -> Self::Item { - Query::new(world, state, system_meta.last_change_tick, change_tick) + Query::new( + super::query_world_borrows::QueryWorldBorrows::new(world), + state, + system_meta.last_change_tick, + change_tick, + ) } }