From 02688a99b808e543987d95ac628ec76809a524da Mon Sep 17 00:00:00 2001 From: Joseph <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 25 Jul 2023 14:16:22 -0700 Subject: [PATCH] Fix safety invariants for `WorldQuery::fetch` and simplify cloning (#8246) # Objective Cloning a `WorldQuery` type's "fetch" struct was made unsafe in #5593, by adding the `unsafe fn clone_fetch` to `WorldQuery`. However, as that method's documentation explains, it is not the right place to put the safety invariant: > While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure that the returned value is not used in any way that would cause two `QueryItem` for the same `archetype_index` or `table_row` to be alive at the same time. You can clone a fetch struct all you want and it will never cause undefined behavior -- in order for something to go wrong, you need to improperly call `WorldQuery::fetch` with it (which is marked unsafe). Additionally, making it unsafe to clone a fetch struct does not even prevent undefined behavior, since there are other ways to incorrectly use a fetch struct. For example, you could just call fetch more than once for the same entity, which is not currently forbidden by any documented invariants. ## Solution Document a safety invariant on `WorldQuery::fetch` that requires the caller to not create aliased `WorldQueryItem`s for mutable types. Remove the `clone_fetch` function, and add the bound `Fetch: Clone` instead. --- ## Changelog - Removed the associated function `WorldQuery::clone_fetch`, and added a `Clone` bound to `WorldQuery::Fetch`. ## Migration Guide ### `fetch` invariants The function `WorldQuery::fetch` has had the following safety invariant added: > If this type does not implement `ReadOnlyWorldQuery`, then the caller must ensure that it is impossible for more than one `Self::Item` to exist for the same entity at any given time. This invariant was always required for soundness, but was previously undocumented. If you called this function manually anywhere, you should check to make sure that this invariant is not violated. ### Removed `clone_fetch` The function `WorldQuery::clone_fetch` has been removed. The associated type `WorldQuery::Fetch` now has the bound `Clone`. Before: ```rust struct MyFetch<'w> { ... } unsafe impl WorldQuery for MyQuery { ... type Fetch<'w> = MyFetch<'w> unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { MyFetch { field1: fetch.field1, field2: fetch.field2.clone(), ... } } } ``` After: ```rust #[derive(Clone)] struct MyFetch<'w> { ... } unsafe impl WorldQuery for MyQuery { ... type Fetch<'w> = MyFetch<'w>; } ``` --------- Co-authored-by: Alice Cecile --- crates/bevy_ecs/macros/src/fetch.rs | 21 +++--- crates/bevy_ecs/src/query/fetch.rs | 104 ++++++++++------------------ crates/bevy_ecs/src/query/filter.rs | 46 ++++-------- crates/bevy_ecs/src/query/iter.rs | 40 +++++------ 4 files changed, 77 insertions(+), 134 deletions(-) diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index c2d925a56b94d..987ee67dc4cae 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -237,6 +237,16 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { #marker_name: &'__w (), } + impl #user_impl_generics_with_world Clone for #fetch_struct_name #user_ty_generics_with_world + #user_where_clauses_with_world { + fn clone(&self) -> Self { + Self { + #(#named_field_idents: self.#named_field_idents.clone(),)* + #marker_name: &(), + } + } + } + // SAFETY: `update_component_access` and `update_archetype_component_access` are called on every field unsafe impl #user_impl_generics #path::query::WorldQuery for #struct_name #user_ty_generics #user_where_clauses { @@ -275,17 +285,6 @@ pub fn derive_world_query_impl(input: TokenStream) -> TokenStream { } } - unsafe fn clone_fetch<'__w>( - _fetch: &::Fetch<'__w> - ) -> ::Fetch<'__w> { - #fetch_struct_name { - #( - #named_field_idents: <#field_types>::clone_fetch(& _fetch. #named_field_idents), - )* - #marker_name: &(), - } - } - const IS_DENSE: bool = true #(&& <#field_types>::IS_DENSE)*; const IS_ARCHETYPAL: bool = true #(&& <#field_types>::IS_ARCHETYPAL)*; diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 0e11d42f6b6b2..240124632248d 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -318,7 +318,7 @@ pub unsafe trait WorldQuery { type Item<'a>; /// Per archetype/table state used by this [`WorldQuery`] to fetch [`Self::Item`](crate::query::WorldQuery::Item) - type Fetch<'a>; + type Fetch<'a>: Clone; /// The read-only variant of this [`WorldQuery`], which satisfies the [`ReadOnlyWorldQuery`] trait. type ReadOnly: ReadOnlyWorldQuery; @@ -345,14 +345,6 @@ pub unsafe trait WorldQuery { this_run: Tick, ) -> Self::Fetch<'w>; - /// While this function can be called for any query, it is always safe to call if `Self: ReadOnlyWorldQuery` holds. - /// - /// # Safety - /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure - /// that the returned value is not used in any way that would cause two `QueryItem` for the same - /// `archetype_row` or `table_row` to be alive at the same time. - unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w>; - /// Returns true if (and only if) every table of every archetype matched by this fetch contains /// all of the matched components. This is used to select a more efficient "table iterator" /// for "dense" queries. If this returns true, [`WorldQuery::set_table`] must be used before @@ -404,6 +396,10 @@ pub unsafe trait WorldQuery { /// /// Must always be called _after_ [`WorldQuery::set_table`] or [`WorldQuery::set_archetype`]. `entity` and /// `table_row` must be in the range of the current table and archetype. + /// + /// If `update_component_access` includes any mutable accesses, then the caller must ensure + /// that `fetch` is called no more than once for each `entity`/`table_row` in each archetype. + /// If `Self` implements [`ReadOnlyWorldQuery`], then this can safely be called multiple times. unsafe fn fetch<'w>( fetch: &mut Self::Fetch<'w>, entity: Entity, @@ -483,8 +479,6 @@ unsafe impl WorldQuery for Entity { ) -> Self::Fetch<'w> { } - unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} - #[inline] unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, @@ -554,10 +548,6 @@ unsafe impl<'a> WorldQuery for EntityRef<'a> { world.world() } - unsafe fn clone_fetch<'w>(world: &Self::Fetch<'w>) -> Self::Fetch<'w> { - world - } - #[inline] unsafe fn set_archetype<'w>( _fetch: &mut Self::Fetch<'w>, @@ -620,6 +610,13 @@ pub struct ReadFetch<'w, T> { sparse_set: Option<&'w ComponentSparseSet>, } +impl Clone for ReadFetch<'_, T> { + fn clone(&self) -> Self { + *self + } +} +impl Copy for ReadFetch<'_, T> {} + /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl WorldQuery for &T { type Fetch<'w> = ReadFetch<'w, T>; @@ -663,13 +660,6 @@ unsafe impl WorldQuery for &T { } } - unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { - ReadFetch { - table_components: fetch.table_components, - sparse_set: fetch.sparse_set, - } - } - #[inline] unsafe fn set_archetype<'w>( fetch: &mut ReadFetch<'w, T>, @@ -770,6 +760,13 @@ pub struct RefFetch<'w, T> { this_run: Tick, } +impl Clone for RefFetch<'_, T> { + fn clone(&self) -> Self { + *self + } +} +impl Copy for RefFetch<'_, T> {} + /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { type Fetch<'w> = RefFetch<'w, T>; @@ -812,15 +809,6 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { } } - unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { - RefFetch { - table_data: fetch.table_data, - sparse_set: fetch.sparse_set, - last_run: fetch.last_run, - this_run: fetch.this_run, - } - } - #[inline] unsafe fn set_archetype<'w>( fetch: &mut RefFetch<'w, T>, @@ -933,6 +921,13 @@ pub struct WriteFetch<'w, T> { this_run: Tick, } +impl Clone for WriteFetch<'_, T> { + fn clone(&self) -> Self { + *self + } +} +impl Copy for WriteFetch<'_, T> {} + /// SAFETY: access of `&T` is a subset of `&mut T` unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { type Fetch<'w> = WriteFetch<'w, T>; @@ -975,15 +970,6 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { } } - unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { - WriteFetch { - table_data: fetch.table_data, - sparse_set: fetch.sparse_set, - last_run: fetch.last_run, - this_run: fetch.this_run, - } - } - #[inline] unsafe fn set_archetype<'w>( fetch: &mut WriteFetch<'w, T>, @@ -1084,6 +1070,15 @@ pub struct OptionFetch<'w, T: WorldQuery> { matches: bool, } +impl Clone for OptionFetch<'_, T> { + fn clone(&self) -> Self { + Self { + fetch: self.fetch.clone(), + matches: self.matches, + } + } +} + // SAFETY: defers to soundness of `T: WorldQuery` impl unsafe impl WorldQuery for Option { type Fetch<'w> = OptionFetch<'w, T>; @@ -1112,13 +1107,6 @@ unsafe impl WorldQuery for Option { } } - unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { - OptionFetch { - fetch: T::clone_fetch(&fetch.fetch), - matches: fetch.matches, - } - } - #[inline] unsafe fn set_archetype<'w>( fetch: &mut OptionFetch<'w, T>, @@ -1271,10 +1259,6 @@ unsafe impl WorldQuery for Has { false } - unsafe fn clone_fetch<'w>(fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> { - *fetch - } - #[inline] unsafe fn set_archetype<'w>( fetch: &mut Self::Fetch<'w>, @@ -1351,13 +1335,6 @@ macro_rules! impl_tuple_fetch { ($($name::init_fetch(_world, $name, _last_run, _this_run),)*) } - unsafe fn clone_fetch<'w>( - fetch: &Self::Fetch<'w>, - ) -> Self::Fetch<'w> { - let ($($name,)*) = &fetch; - ($($name::clone_fetch($name),)*) - } - const IS_DENSE: bool = true $(&& $name::IS_DENSE)*; const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; @@ -1461,13 +1438,6 @@ macro_rules! impl_anytuple_fetch { ($(($name::init_fetch(_world, $name, _last_run, _this_run), false),)*) } - unsafe fn clone_fetch<'w>( - fetch: &Self::Fetch<'w>, - ) -> Self::Fetch<'w> { - let ($($name,)*) = &fetch; - ($(($name::clone_fetch(& $name.0), $name.1),)*) - } - const IS_DENSE: bool = true $(&& $name::IS_DENSE)*; const IS_ARCHETYPAL: bool = true $(&& $name::IS_ARCHETYPAL)*; @@ -1588,8 +1558,6 @@ unsafe impl WorldQuery for NopWorldQuery { ) { } - unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} - #[inline(always)] unsafe fn set_archetype( _fetch: &mut (), @@ -1651,8 +1619,6 @@ unsafe impl WorldQuery for PhantomData { ) -> Self::Fetch<'w> { } - unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} - // `PhantomData` does not match any components, so all components it matches // are stored in a Table (vacuous truth). const IS_DENSE: bool = true; diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index b3f039566eb0f..021b010021d70 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -59,8 +59,6 @@ unsafe impl WorldQuery for With { ) { } - unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} - const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { StorageType::Table => true, @@ -162,8 +160,6 @@ unsafe impl WorldQuery for Without { ) { } - unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {} - const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { StorageType::Table => true, @@ -259,6 +255,15 @@ pub struct OrFetch<'w, T: WorldQuery> { matches: bool, } +impl Clone for OrFetch<'_, T> { + fn clone(&self) -> Self { + Self { + fetch: self.fetch.clone(), + matches: self.matches, + } + } +} + macro_rules! impl_query_filter_tuple { ($(($filter: ident, $state: ident)),*) => { #[allow(unused_variables)] @@ -288,18 +293,6 @@ macro_rules! impl_query_filter_tuple { },)*) } - unsafe fn clone_fetch<'w>( - fetch: &Self::Fetch<'w>, - ) -> Self::Fetch<'w> { - let ($($filter,)*) = &fetch; - ($( - OrFetch { - fetch: $filter::clone_fetch(&$filter.fetch), - matches: $filter.matches, - }, - )*) - } - #[inline] unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) { let ($($filter,)*) = fetch; @@ -403,10 +396,10 @@ macro_rules! impl_tick_filter { pub struct $name(PhantomData); #[doc(hidden)] + #[derive(Clone)] $(#[$fetch_meta])* - pub struct $fetch_name<'w, T> { - table_ticks: Option< ThinSlicePtr<'w, UnsafeCell>>, - marker: PhantomData, + pub struct $fetch_name<'w> { + table_ticks: Option>>, sparse_set: Option<&'w ComponentSparseSet>, last_run: Tick, this_run: Tick, @@ -414,7 +407,7 @@ macro_rules! impl_tick_filter { // SAFETY: `Self::ReadOnly` is the same as `Self` unsafe impl WorldQuery for $name { - type Fetch<'w> = $fetch_name<'w, T>; + type Fetch<'w> = $fetch_name<'w>; type Item<'w> = bool; type ReadOnly = Self; type State = ComponentId; @@ -440,24 +433,11 @@ macro_rules! impl_tick_filter { .get(id) .debug_checked_unwrap() }), - marker: PhantomData, last_run, this_run, } } - unsafe fn clone_fetch<'w>( - fetch: &Self::Fetch<'w>, - ) -> Self::Fetch<'w> { - $fetch_name { - table_ticks: fetch.table_ticks, - sparse_set: fetch.sparse_set, - last_run: fetch.last_run, - this_run: fetch.this_run, - marker: PhantomData, - } - } - const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { StorageType::Table => true, diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index a148fc6ac4af4..142877efc59ca 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -6,7 +6,7 @@ use crate::{ storage::{TableId, TableRow, Tables}, world::unsafe_world_cell::UnsafeWorldCell, }; -use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit}; +use std::{borrow::Borrow, iter::FusedIterator, mem::MaybeUninit}; use super::ReadOnlyWorldQuery; @@ -163,7 +163,9 @@ where // SAFETY: set_archetype was called prior. // `location.archetype_row` is an archetype index row in range of the current archetype, because if it was not, the match above would have `continue`d if F::filter_fetch(&mut self.filter, entity, location.table_row) { - // SAFETY: set_archetype was called prior, `location.archetype_row` is an archetype index in range of the current archetype + // SAFETY: + // - set_archetype was called prior, `location.archetype_row` is an archetype index in range of the current archetype + // - fetch is only called once for each entity. return Some(Q::fetch(&mut self.fetch, entity, location.table_row)); } } @@ -344,7 +346,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery, const K: usize> match self.cursors[i].next(self.tables, self.archetypes, self.query_state) { Some(_) => { for j in (i + 1)..K { - self.cursors[j] = self.cursors[j - 1].clone_cursor(); + self.cursors[j] = self.cursors[j - 1].clone(); match self.cursors[j].next(self.tables, self.archetypes, self.query_state) { Some(_) => {} None if i > 0 => continue 'outer, @@ -453,28 +455,19 @@ struct QueryIterationCursor<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> { current_len: usize, // either table row or archetype index, depending on whether both `Q`'s and `F`'s fetches are dense current_row: usize, - phantom: PhantomData, } -impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, Q, F> { - /// This function is safe to call if `(Q, F): ReadOnlyWorldQuery` holds. - /// - /// # Safety - /// While calling this method on its own cannot cause UB it is marked `unsafe` as the caller must ensure - /// that the returned value is not used in any way that would cause two `QueryItem` for the same - /// `archetype_row` or `table_row` to be alive at the same time. - unsafe fn clone_cursor(&self) -> Self { +impl Clone for QueryIterationCursor<'_, '_, Q, F> { + fn clone(&self) -> Self { Self { table_id_iter: self.table_id_iter.clone(), archetype_id_iter: self.archetype_id_iter.clone(), table_entities: self.table_entities, archetype_entities: self.archetype_entities, - // SAFETY: upheld by caller invariants - fetch: Q::clone_fetch(&self.fetch), - filter: F::clone_fetch(&self.filter), + fetch: self.fetch.clone(), + filter: self.filter.clone(), current_len: self.current_len, current_row: self.current_row, - phantom: PhantomData, } } } @@ -515,7 +508,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, archetype_id_iter: query_state.matched_archetype_ids.iter(), current_len: 0, current_row: 0, - phantom: PhantomData, } } @@ -593,8 +585,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, continue; } - // SAFETY: set_table was called prior. - // `current_row` is a table row in range of the current table, because if it was not, then the if above would have been executed. + // SAFETY: + // - set_table was called prior. + // - `current_row` must be a table row in range of the current table, + // because if it was not, then the if above would have been executed. + // - fetch is only called once for each `entity`. let item = Q::fetch(&mut self.fetch, *entity, row); self.current_row += 1; @@ -633,8 +628,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's, continue; } - // SAFETY: set_archetype was called prior, `current_row` is an archetype index in range of the current archetype - // `current_row` is an archetype index row in range of the current archetype, because if it was not, then the if above would have been executed. + // SAFETY: + // - set_archetype was called prior. + // - `current_row` must be an archetype index row in range of the current archetype, + // because if it was not, then the if above would have been executed. + // - fetch is only called once for each `archetype_entity`. let item = Q::fetch( &mut self.fetch, archetype_entity.entity(),