From 274ace790b16b3dfdbcbc69a92a8c4bd16c8c9fc Mon Sep 17 00:00:00 2001 From: Joshua Chapman Date: Wed, 1 Dec 2021 23:28:10 +0000 Subject: [PATCH] Implement iter() for mutable Queries (#2305) A sample implementation of how to have `iter()` work on mutable queries without breaking aliasing rules. # Objective - Fixes #753 ## Solution - Added a ReadOnlyFetch to WorldQuery that is the `&T` version of `&mut T` that is used to specify the return type for read only operations like `iter()`. - ~~As the comment suggests specifying the bound doesn't work due to restrictions on defining recursive implementations (like `Or`). However bounds on the functions are fine~~ Never mind I misread how `Or` was constructed, bounds now exist. - Note that the only mutable one has a new `Fetch` for readonly as the `State` has to be the same for any of this to work Co-authored-by: Carter Anderson --- crates/bevy_ecs/src/query/fetch.rs | 150 +++++++++++ crates/bevy_ecs/src/query/filter.rs | 20 +- crates/bevy_ecs/src/query/iter.rs | 150 ++++------- crates/bevy_ecs/src/query/state.rs | 249 +++++++++++------- crates/bevy_ecs/src/system/mod.rs | 25 ++ crates/bevy_ecs/src/system/query.rs | 136 +++++----- .../system_state_iter_mut_overlap_safety.rs | 28 ++ ...ystem_state_iter_mut_overlap_safety.stderr | 11 + .../bevy_render2/src/render_component.rs | 8 +- 9 files changed, 509 insertions(+), 268 deletions(-) create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.rs create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.stderr diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 215b151790..e62d4b9057 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -43,6 +43,8 @@ use std::{ pub trait WorldQuery { type Fetch: for<'world, 'state> Fetch<'world, 'state, State = Self::State>; type State: FetchState; + type ReadOnlyFetch: for<'world, 'state> Fetch<'world, 'state, State = Self::State> + + ReadOnlyFetch; } pub type QueryItem<'w, 's, Q> = <::Fetch as Fetch<'w, 's>>::Item; @@ -136,6 +138,7 @@ pub unsafe trait ReadOnlyFetch {} impl WorldQuery for Entity { type Fetch = EntityFetch; type State = EntityState; + type ReadOnlyFetch = EntityFetch; } /// The [`Fetch`] of [`Entity`]. @@ -222,6 +225,7 @@ impl<'w, 's> Fetch<'w, 's> for EntityFetch { impl WorldQuery for &T { type Fetch = ReadFetch; type State = ReadState; + type ReadOnlyFetch = ReadFetch; } /// The [`FetchState`] of `&T`. @@ -376,6 +380,7 @@ impl<'w, 's, T: Component> Fetch<'w, 's> for ReadFetch { impl WorldQuery for &mut T { type Fetch = WriteFetch; type State = WriteState; + type ReadOnlyFetch = ReadOnlyWriteFetch; } /// The [`Fetch`] of `&mut T`. @@ -403,6 +408,28 @@ impl Clone for WriteFetch { } } +/// The [`ReadOnlyFetch`] of `&mut T`. +pub struct ReadOnlyWriteFetch { + table_components: NonNull, + entities: *const Entity, + entity_table_rows: *const usize, + sparse_set: *const ComponentSparseSet, +} + +/// SAFETY: access is read only +unsafe impl ReadOnlyFetch for ReadOnlyWriteFetch {} + +impl Clone for ReadOnlyWriteFetch { + fn clone(&self) -> Self { + Self { + table_components: self.table_components, + entities: self.entities, + entity_table_rows: self.entity_table_rows, + sparse_set: self.sparse_set, + } + } +} + /// The [`FetchState`] of `&mut T`. pub struct WriteState { component_id: ComponentId, @@ -555,9 +582,88 @@ impl<'w, 's, T: Component> Fetch<'w, 's> for WriteFetch { } } +impl<'w, 's, T: Component> Fetch<'w, 's> for ReadOnlyWriteFetch { + type Item = &'w T; + type State = WriteState; + + const IS_DENSE: bool = { + match T::Storage::STORAGE_TYPE { + StorageType::Table => true, + StorageType::SparseSet => false, + } + }; + + unsafe fn init( + world: &World, + state: &Self::State, + _last_change_tick: u32, + _change_tick: u32, + ) -> Self { + let mut value = Self { + table_components: NonNull::dangling(), + entities: ptr::null::(), + entity_table_rows: ptr::null::(), + sparse_set: ptr::null::(), + }; + if T::Storage::STORAGE_TYPE == StorageType::SparseSet { + value.sparse_set = world + .storages() + .sparse_sets + .get(state.component_id) + .unwrap(); + } + value + } + + #[inline] + unsafe fn set_archetype( + &mut self, + state: &Self::State, + archetype: &Archetype, + tables: &Tables, + ) { + match T::Storage::STORAGE_TYPE { + StorageType::Table => { + self.entity_table_rows = archetype.entity_table_rows().as_ptr(); + let column = tables[archetype.table_id()] + .get_column(state.component_id) + .unwrap(); + self.table_components = column.get_data_ptr().cast::(); + } + StorageType::SparseSet => self.entities = archetype.entities().as_ptr(), + } + } + + #[inline] + unsafe fn set_table(&mut self, state: &Self::State, table: &Table) { + let column = table.get_column(state.component_id).unwrap(); + self.table_components = column.get_data_ptr().cast::(); + } + + #[inline] + unsafe fn archetype_fetch(&mut self, archetype_index: usize) -> Self::Item { + match T::Storage::STORAGE_TYPE { + StorageType::Table => { + let table_row = *self.entity_table_rows.add(archetype_index); + &*self.table_components.as_ptr().add(table_row) + } + StorageType::SparseSet => { + let entity = *self.entities.add(archetype_index); + &*(*self.sparse_set).get(entity).unwrap().cast::() + } + } + } + + #[inline] + unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item { + &*self.table_components.as_ptr().add(table_row) + } +} + impl WorldQuery for Option { type Fetch = OptionFetch; type State = OptionState; + type ReadOnlyFetch = OptionFetch; } /// The [`Fetch`] of `Option`. @@ -733,6 +839,7 @@ impl ChangeTrackers { impl WorldQuery for ChangeTrackers { type Fetch = ChangeTrackersFetch; type State = ChangeTrackersState; + type ReadOnlyFetch = ChangeTrackersFetch; } /// The [`FetchState`] of [`ChangeTrackers`]. @@ -983,6 +1090,7 @@ macro_rules! impl_tuple_fetch { impl<$($name: WorldQuery),*> WorldQuery for ($($name,)*) { type Fetch = ($($name::Fetch,)*); type State = ($($name::State,)*); + type ReadOnlyFetch = ($($name::ReadOnlyFetch,)*); } /// SAFETY: each item in the tuple is read only @@ -992,3 +1100,45 @@ macro_rules! impl_tuple_fetch { } all_tuples!(impl_tuple_fetch, 0, 15, F, S); + +/// [`Fetch`] that does not actually fetch anything +/// +/// Mostly useful when something is generic over the Fetch and you don't want to fetch as you will discard the result +pub struct NopFetch { + state: PhantomData, +} + +impl<'w, 's, State: FetchState> Fetch<'w, 's> for NopFetch { + type Item = (); + type State = State; + + const IS_DENSE: bool = true; + + #[inline(always)] + unsafe fn init( + _world: &World, + _state: &Self::State, + _last_change_tick: u32, + _change_tick: u32, + ) -> Self { + Self { state: PhantomData } + } + + #[inline(always)] + unsafe fn set_archetype( + &mut self, + _state: &Self::State, + _archetype: &Archetype, + _tables: &Tables, + ) { + } + + #[inline(always)] + unsafe fn set_table(&mut self, _state: &Self::State, _table: &Table) {} + + #[inline(always)] + unsafe fn archetype_fetch(&mut self, _archetype_index: usize) -> Self::Item {} + + #[inline(always)] + unsafe fn table_fetch(&mut self, _table_row: usize) -> Self::Item {} +} diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index bbd8ec567a..7849f18230 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -2,7 +2,7 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId}, component::{Component, ComponentId, ComponentStorage, ComponentTicks, StorageType}, entity::Entity, - query::{Access, Fetch, FetchState, FilteredAccess, WorldQuery}, + query::{Access, Fetch, FetchState, FilteredAccess, ReadOnlyFetch, WorldQuery}, storage::{ComponentSparseSet, Table, Tables}, world::World, }; @@ -72,6 +72,7 @@ pub struct With(PhantomData); impl WorldQuery for With { type Fetch = WithFetch; type State = WithState; + type ReadOnlyFetch = WithFetch; } /// The [`Fetch`] of [`With`]. @@ -162,6 +163,9 @@ impl<'w, 's, T: Component> Fetch<'w, 's> for WithFetch { } } +// SAFETY: no component access or archetype component access +unsafe impl ReadOnlyFetch for WithFetch {} + /// Filter that selects entities without a component `T`. /// /// This is the negation of [`With`]. @@ -191,6 +195,7 @@ pub struct Without(PhantomData); impl WorldQuery for Without { type Fetch = WithoutFetch; type State = WithoutState; + type ReadOnlyFetch = WithoutFetch; } /// The [`Fetch`] of [`Without`]. @@ -281,6 +286,9 @@ impl<'w, 's, T: Component> Fetch<'w, 's> for WithoutFetch { } } +// SAFETY: no component access or archetype component access +unsafe impl ReadOnlyFetch for WithoutFetch {} + /// A filter that tests if any of the given filters apply. /// /// This is useful for example if a system with multiple components in a query only wants to run @@ -338,12 +346,15 @@ macro_rules! impl_query_filter_tuple { } impl<$($filter: WorldQuery),*> WorldQuery for Or<($($filter,)*)> - where $($filter::Fetch: FilterFetch),* + where $($filter::Fetch: FilterFetch, $filter::ReadOnlyFetch: FilterFetch),* { type Fetch = Or<($(OrFetch<$filter::Fetch>,)*)>; type State = Or<($($filter::State,)*)>; + type ReadOnlyFetch = Or<($(OrFetch<$filter::ReadOnlyFetch>,)*)>; } + /// SAFETY: this only works using the filter which doesn't write + unsafe impl<$($filter: FilterFetch + ReadOnlyFetch),*> ReadOnlyFetch for Or<($(OrFetch<$filter>,)*)> {} #[allow(unused_variables)] #[allow(non_snake_case)] @@ -464,9 +475,9 @@ macro_rules! impl_tick_filter { impl WorldQuery for $name { type Fetch = $fetch_name; type State = $state_name; + type ReadOnlyFetch = $fetch_name; } - // SAFETY: this reads the T component. archetype component access and component access are updated to reflect that unsafe impl FetchState for $state_name { fn init(world: &mut World) -> Self { @@ -572,6 +583,9 @@ macro_rules! impl_tick_filter { } } } + + /// SAFETY: read-only access + unsafe impl ReadOnlyFetch for $fetch_name {} }; } diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 5199c971a8..96c811f9ab 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -4,13 +4,13 @@ use crate::{ storage::{TableId, Tables}, world::World, }; -use std::mem::MaybeUninit; +use std::{marker::PhantomData, mem::MaybeUninit}; /// An [`Iterator`] over query results of a [`Query`](crate::system::Query). /// /// This struct is created by the [`Query::iter`](crate::system::Query::iter) and /// [`Query::iter_mut`](crate::system::Query::iter_mut) methods. -pub struct QueryIter<'w, 's, Q: WorldQuery, F: WorldQuery> +pub struct QueryIter<'w, 's, Q: WorldQuery, QF: Fetch<'w, 's, State = Q::State>, F: WorldQuery> where F::Fetch: FilterFetch, { @@ -20,15 +20,16 @@ where world: &'w World, table_id_iter: std::slice::Iter<'s, TableId>, archetype_id_iter: std::slice::Iter<'s, ArchetypeId>, - fetch: Q::Fetch, + fetch: QF, filter: F::Fetch, current_len: usize, current_index: usize, } -impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIter<'w, 's, Q, F> +impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> QueryIter<'w, 's, Q, QF, F> where F::Fetch: FilterFetch, + QF: Fetch<'w, 's, State = Q::State>, { /// # Safety /// This does not check for mutable query correctness. To be safe, make sure mutable queries @@ -41,7 +42,7 @@ where last_change_tick: u32, change_tick: u32, ) -> Self { - let fetch = ::init( + let fetch = QF::init( world, &query_state.fetch_state, last_change_tick, @@ -67,69 +68,14 @@ where current_index: 0, } } - - /// Consumes `self` and returns true if there were no elements remaining in this iterator. - #[inline(always)] - pub(crate) fn none_remaining(mut self) -> bool { - // NOTE: this mimics the behavior of `QueryIter::next()`, except that it - // never gets a `Self::Item`. - unsafe { - if Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE { - loop { - if self.current_index == self.current_len { - let table_id = match self.table_id_iter.next() { - Some(table_id) => table_id, - None => return true, - }; - let table = &self.tables[*table_id]; - self.filter.set_table(&self.query_state.filter_state, table); - self.current_len = table.len(); - self.current_index = 0; - continue; - } - - if !self.filter.table_filter_fetch(self.current_index) { - self.current_index += 1; - continue; - } - - return false; - } - } else { - loop { - if self.current_index == self.current_len { - let archetype_id = match self.archetype_id_iter.next() { - Some(archetype_id) => archetype_id, - None => return true, - }; - let archetype = &self.archetypes[*archetype_id]; - self.filter.set_archetype( - &self.query_state.filter_state, - archetype, - self.tables, - ); - self.current_len = archetype.len(); - self.current_index = 0; - continue; - } - - if !self.filter.archetype_filter_fetch(self.current_index) { - self.current_index += 1; - continue; - } - - return false; - } - } - } - } } -impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F> +impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, QF, F> where F::Fetch: FilterFetch, + QF: Fetch<'w, 's, State = Q::State>, { - type Item = >::Item; + type Item = QF::Item; // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual @@ -137,7 +83,7 @@ where #[inline(always)] fn next(&mut self) -> Option { unsafe { - if Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE { + if QF::IS_DENSE && F::Fetch::IS_DENSE { loop { if self.current_index == self.current_len { let table_id = self.table_id_iter.next()?; @@ -207,19 +153,22 @@ where } } -pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> +pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, QF, F: WorldQuery, const K: usize> where + QF: Fetch<'w, 's, State = Q::State>, F::Fetch: FilterFetch, { tables: &'w Tables, archetypes: &'w Archetypes, query_state: &'s QueryState, world: &'w World, - cursors: [QueryIterationCursor<'s, Q, F>; K], + cursors: [QueryIterationCursor<'w, 's, Q, QF, F>; K], } -impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter<'w, 's, Q, F, K> +impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery, const K: usize> + QueryCombinationIter<'w, 's, Q, QF, F, K> where + QF: Fetch<'w, 's, State = Q::State>, F::Fetch: FilterFetch, { /// # Safety @@ -237,7 +186,7 @@ where // There is no FromIterator on arrays, so instead initialize it manually with MaybeUninit // TODO: use MaybeUninit::uninit_array if it stabilizes - let mut cursors: [MaybeUninit>; K] = + let mut cursors: [MaybeUninit>; K] = MaybeUninit::uninit().assume_init(); for (i, cursor) in cursors.iter_mut().enumerate() { match i { @@ -257,8 +206,8 @@ where } // TODO: use MaybeUninit::array_assume_init if it stabilizes - let cursors: [QueryIterationCursor<'s, Q, F>; K] = - (&cursors as *const _ as *const [QueryIterationCursor<'s, Q, F>; K]).read(); + let cursors: [QueryIterationCursor<'w, 's, Q, QF, F>; K] = + (&cursors as *const _ as *const [QueryIterationCursor<'w, 's, Q, QF, F>; K]).read(); QueryCombinationIter { world, @@ -275,11 +224,9 @@ where /// references to the same component, leading to unique reference aliasing. ///. /// It is always safe for shared access. - unsafe fn fetch_next_aliased_unchecked<'a>( - &mut self, - ) -> Option<[>::Item; K]> + unsafe fn fetch_next_aliased_unchecked(&mut self) -> Option<[QF::Item; K]> where - Q::Fetch: Clone, + QF: Clone, F::Fetch: Clone, { if K == 0 { @@ -307,25 +254,23 @@ where } // TODO: use MaybeUninit::uninit_array if it stabilizes - let mut values: [MaybeUninit<>::Item>; K] = - MaybeUninit::uninit().assume_init(); + let mut values: [MaybeUninit; K] = MaybeUninit::uninit().assume_init(); for (value, cursor) in values.iter_mut().zip(&mut self.cursors) { value.as_mut_ptr().write(cursor.peek_last().unwrap()); } // TODO: use MaybeUninit::array_assume_init if it stabilizes - let values: [>::Item; K] = - (&values as *const _ as *const [>::Item; K]).read(); + let values: [QF::Item; K] = (&values as *const _ as *const [QF::Item; K]).read(); Some(values) } /// Get next combination of queried components #[inline] - pub fn fetch_next(&mut self) -> Option<[>::Item; K]> + pub fn fetch_next(&mut self) -> Option<[QF::Item; K]> where - Q::Fetch: Clone, + QF: Clone, F::Fetch: Clone, { // safety: we are limiting the returned reference to self, @@ -338,13 +283,13 @@ where // Iterator type is intentionally implemented only for read-only access. // Doing so for mutable references would be unsound, because calling `next` // multiple times would allow multiple owned references to the same data to exist. -impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> Iterator - for QueryCombinationIter<'w, 's, Q, F, K> +impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery, const K: usize> Iterator + for QueryCombinationIter<'w, 's, Q, QF, F, K> where - Q::Fetch: Clone + ReadOnlyFetch, + QF: Fetch<'w, 's, State = Q::State> + Clone + ReadOnlyFetch, F::Fetch: Clone + FilterFetch + ReadOnlyFetch, { - type Item = [>::Item; K]; + type Item = [QF::Item; K]; #[inline] fn next(&mut self) -> Option { @@ -388,7 +333,10 @@ where // (2) each archetype pre-computes length // (3) there are no per-entity filters // TODO: add an ArchetypeOnlyFilter that enables us to implement this for filters like With -impl<'w, 's, Q: WorldQuery> ExactSizeIterator for QueryIter<'w, 's, Q, ()> { +impl<'w, 's, Q: WorldQuery, QF> ExactSizeIterator for QueryIter<'w, 's, Q, QF, ()> +where + QF: Fetch<'w, 's, State = Q::State>, +{ fn len(&self) -> usize { self.query_state .matched_archetypes @@ -398,18 +346,25 @@ impl<'w, 's, Q: WorldQuery> ExactSizeIterator for QueryIter<'w, 's, Q, ()> { } } -struct QueryIterationCursor<'s, Q: WorldQuery, F: WorldQuery> { +struct QueryIterationCursor< + 'w, + 's, + Q: WorldQuery, + QF: Fetch<'w, 's, State = Q::State>, + F: WorldQuery, +> { table_id_iter: std::slice::Iter<'s, TableId>, archetype_id_iter: std::slice::Iter<'s, ArchetypeId>, - fetch: Q::Fetch, + fetch: QF, filter: F::Fetch, current_len: usize, current_index: usize, + phantom: PhantomData<&'w Q>, } -impl<'s, Q: WorldQuery, F: WorldQuery> Clone for QueryIterationCursor<'s, Q, F> +impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> Clone for QueryIterationCursor<'w, 's, Q, QF, F> where - Q::Fetch: Clone, + QF: Fetch<'w, 's, State = Q::State> + Clone, F::Fetch: Clone, { fn clone(&self) -> Self { @@ -420,12 +375,14 @@ where filter: self.filter.clone(), current_len: self.current_len, current_index: self.current_index, + phantom: PhantomData, } } } -impl<'s, Q: WorldQuery, F: WorldQuery> QueryIterationCursor<'s, Q, F> +impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> QueryIterationCursor<'w, 's, Q, QF, F> where + QF: Fetch<'w, 's, State = Q::State>, F::Fetch: FilterFetch, { unsafe fn init_empty( @@ -447,7 +404,7 @@ where last_change_tick: u32, change_tick: u32, ) -> Self { - let fetch = ::init( + let fetch = QF::init( world, &query_state.fetch_state, last_change_tick, @@ -466,14 +423,15 @@ where archetype_id_iter: query_state.matched_archetype_ids.iter(), current_len: 0, current_index: 0, + phantom: PhantomData, } } /// retrieve item returned from most recent `next` call again. #[inline] - unsafe fn peek_last<'w>(&mut self) -> Option<>::Item> { + unsafe fn peek_last(&mut self) -> Option { if self.current_index > 0 { - if Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE { + if QF::IS_DENSE && F::Fetch::IS_DENSE { Some(self.fetch.table_fetch(self.current_index - 1)) } else { Some(self.fetch.archetype_fetch(self.current_index - 1)) @@ -487,13 +445,13 @@ where // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual // We can't currently reuse QueryIterationCursor in QueryIter for performance reasons. See #1763 for context. #[inline(always)] - unsafe fn next<'w>( + unsafe fn next( &mut self, tables: &'w Tables, archetypes: &'w Archetypes, query_state: &'s QueryState, - ) -> Option<>::Item> { - if Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE { + ) -> Option { + if QF::IS_DENSE && F::Fetch::IS_DENSE { loop { if self.current_index == self.current_len { let table_id = self.table_id_iter.next()?; diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 3dd9a7c6a7..16b8ae39ba 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -3,8 +3,8 @@ use crate::{ component::ComponentId, entity::Entity, query::{ - Access, Fetch, FetchState, FilterFetch, FilteredAccess, QueryCombinationIter, QueryIter, - ReadOnlyFetch, WorldQuery, + Access, Fetch, FetchState, FilterFetch, FilteredAccess, NopFetch, QueryCombinationIter, + QueryIter, WorldQuery, }, storage::TableId, world::{World, WorldId}, @@ -73,11 +73,11 @@ where /// Checks if the query is empty for the given [`World`], where the last change and current tick are given. #[inline] pub fn is_empty(&self, world: &World, last_change_tick: u32, change_tick: u32) -> bool { - // SAFE: the iterator is instantly consumed via `none_remaining` and the implementation of - // `QueryIter::none_remaining` never creates any references to the `>::Item`. + // SAFE: NopFetch does not access any members while &self ensures no one has exclusive access unsafe { - self.iter_unchecked_manual(world, last_change_tick, change_tick) - .none_remaining() + self.iter_unchecked_manual::>(world, last_change_tick, change_tick) + .next() + .is_none() } } @@ -139,12 +139,17 @@ where &'s mut self, world: &'w World, entity: Entity, - ) -> Result<>::Item, QueryEntityError> - where - Q::Fetch: ReadOnlyFetch, - { + ) -> Result<>::Item, QueryEntityError> { + self.update_archetypes(world); // SAFETY: query is read only - unsafe { self.get_unchecked(world, entity) } + unsafe { + self.get_unchecked_manual::( + world, + entity, + world.last_change_tick(), + world.read_change_tick(), + ) + } } /// Gets the query result for the given [`World`] and [`Entity`]. @@ -154,8 +159,16 @@ where world: &'w mut World, entity: Entity, ) -> Result<>::Item, QueryEntityError> { + self.update_archetypes(world); // SAFETY: query has unique world access - unsafe { self.get_unchecked(world, entity) } + unsafe { + self.get_unchecked_manual::( + world, + entity, + world.last_change_tick(), + world.read_change_tick(), + ) + } } #[inline] @@ -163,14 +176,11 @@ where &'s self, world: &'w World, entity: Entity, - ) -> Result<>::Item, QueryEntityError> - where - Q::Fetch: ReadOnlyFetch, - { + ) -> Result<>::Item, QueryEntityError> { self.validate_world(world); // SAFETY: query is read only and world is validated unsafe { - self.get_unchecked_manual( + self.get_unchecked_manual::( world, entity, world.last_change_tick(), @@ -192,7 +202,7 @@ where entity: Entity, ) -> Result<>::Item, QueryEntityError> { self.update_archetypes(world); - self.get_unchecked_manual( + self.get_unchecked_manual::( world, entity, world.last_change_tick(), @@ -207,13 +217,13 @@ where /// /// This does not check for mutable query correctness. To be safe, make sure mutable queries /// have unique access to the components they query. - pub unsafe fn get_unchecked_manual<'w, 's>( + pub(crate) unsafe fn get_unchecked_manual<'w, 's, QF: Fetch<'w, 's, State = Q::State>>( &'s self, world: &'w World, entity: Entity, last_change_tick: u32, change_tick: u32, - ) -> Result<>::Item, QueryEntityError> { + ) -> Result { let location = world .entities .get(entity) @@ -225,8 +235,7 @@ where return Err(QueryEntityError::QueryDoesNotMatch); } let archetype = &world.archetypes[location.archetype_id]; - let mut fetch = - ::init(world, &self.fetch_state, last_change_tick, change_tick); + let mut fetch = QF::init(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = ::init(world, &self.filter_state, last_change_tick, change_tick); @@ -243,19 +252,28 @@ where /// /// This can only be called for read-only queries, see [`Self::iter_mut`] for write-queries. #[inline] - pub fn iter<'w, 's>(&'s mut self, world: &'w World) -> QueryIter<'w, 's, Q, F> - where - Q::Fetch: ReadOnlyFetch, - { + pub fn iter<'w, 's>( + &'s mut self, + world: &'w World, + ) -> QueryIter<'w, 's, Q, Q::ReadOnlyFetch, F> { // SAFETY: query is read only - unsafe { self.iter_unchecked(world) } + unsafe { + self.update_archetypes(world); + self.iter_unchecked_manual(world, world.last_change_tick(), world.read_change_tick()) + } } /// Returns an [`Iterator`] over the query results for the given [`World`]. #[inline] - pub fn iter_mut<'w, 's>(&'s mut self, world: &'w mut World) -> QueryIter<'w, 's, Q, F> { + pub fn iter_mut<'w, 's>( + &'s mut self, + world: &'w mut World, + ) -> QueryIter<'w, 's, Q, Q::Fetch, F> { // SAFETY: query has unique world access - unsafe { self.iter_unchecked(world) } + unsafe { + self.update_archetypes(world); + self.iter_unchecked_manual(world, world.last_change_tick(), world.read_change_tick()) + } } /// Returns an [`Iterator`] over all possible combinations of `K` query results without repetition. @@ -269,10 +287,10 @@ where /// This can only be called for read-only queries, see [`Self::iter_combinations_mut`] for /// write-queries. #[inline] - pub fn iter_manual<'w, 's>(&'s self, world: &'w World) -> QueryIter<'w, 's, Q, F> - where - Q::Fetch: ReadOnlyFetch, - { + pub fn iter_manual<'w, 's>( + &'s self, + world: &'w World, + ) -> QueryIter<'w, 's, Q, Q::ReadOnlyFetch, F> { self.validate_world(world); // SAFETY: query is read only and world is validated unsafe { @@ -294,12 +312,16 @@ where pub fn iter_combinations<'w, 's, const K: usize>( &'s mut self, world: &'w World, - ) -> QueryCombinationIter<'w, 's, Q, F, K> - where - Q::Fetch: ReadOnlyFetch, - { + ) -> QueryCombinationIter<'w, 's, Q, Q::ReadOnlyFetch, F, K> { // SAFE: query is read only - unsafe { self.iter_combinations_unchecked(world) } + unsafe { + self.update_archetypes(world); + self.iter_combinations_unchecked_manual( + world, + world.last_change_tick(), + world.read_change_tick(), + ) + } } /// Iterates over all possible combinations of `K` query results for the given [`World`] @@ -313,9 +335,16 @@ where pub fn iter_combinations_mut<'w, 's, const K: usize>( &'s mut self, world: &'w mut World, - ) -> QueryCombinationIter<'w, 's, Q, F, K> { + ) -> QueryCombinationIter<'w, 's, Q, Q::Fetch, F, K> { // SAFE: query has unique world access - unsafe { self.iter_combinations_unchecked(world) } + unsafe { + self.update_archetypes(world); + self.iter_combinations_unchecked_manual( + world, + world.last_change_tick(), + world.read_change_tick(), + ) + } } /// Returns an [`Iterator`] over the query results for the given [`World`]. @@ -328,7 +357,7 @@ where pub unsafe fn iter_unchecked<'w, 's>( &'s mut self, world: &'w World, - ) -> QueryIter<'w, 's, Q, F> { + ) -> QueryIter<'w, 's, Q, Q::Fetch, F> { self.update_archetypes(world); self.iter_unchecked_manual(world, world.last_change_tick(), world.read_change_tick()) } @@ -345,7 +374,7 @@ where pub unsafe fn iter_combinations_unchecked<'w, 's, const K: usize>( &'s mut self, world: &'w World, - ) -> QueryCombinationIter<'w, 's, Q, F, K> { + ) -> QueryCombinationIter<'w, 's, Q, Q::Fetch, F, K> { self.update_archetypes(world); self.iter_combinations_unchecked_manual( world, @@ -364,12 +393,12 @@ where /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` /// with a mismatched WorldId is unsound. #[inline] - pub(crate) unsafe fn iter_unchecked_manual<'w, 's>( + pub(crate) unsafe fn iter_unchecked_manual<'w, 's, QF: Fetch<'w, 's, State = Q::State>>( &'s self, world: &'w World, last_change_tick: u32, change_tick: u32, - ) -> QueryIter<'w, 's, Q, F> { + ) -> QueryIter<'w, 's, Q, QF, F> { QueryIter::new(world, self, last_change_tick, change_tick) } @@ -384,12 +413,17 @@ where /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` /// with a mismatched WorldId is unsound. #[inline] - pub(crate) unsafe fn iter_combinations_unchecked_manual<'w, 's, const K: usize>( + pub(crate) unsafe fn iter_combinations_unchecked_manual< + 'w, + 's, + QF: Fetch<'w, 's, State = Q::State>, + const K: usize, + >( &'s self, world: &'w World, last_change_tick: u32, change_tick: u32, - ) -> QueryCombinationIter<'w, 's, Q, F, K> { + ) -> QueryCombinationIter<'w, 's, Q, QF, F, K> { QueryCombinationIter::new(world, self, last_change_tick, change_tick) } @@ -398,30 +432,40 @@ where /// /// This can only be called for read-only queries, see [`Self::for_each_mut`] for write-queries. #[inline] - pub fn for_each<'w, 's>( + pub fn for_each<'w, 's, FN: FnMut(>::Item)>( &'s mut self, world: &'w World, - func: impl FnMut(>::Item), - ) where - Q::Fetch: ReadOnlyFetch, - { + func: FN, + ) { // SAFETY: query is read only unsafe { - self.for_each_unchecked(world, func); + self.update_archetypes(world); + self.for_each_unchecked_manual::( + world, + func, + world.last_change_tick(), + world.read_change_tick(), + ); } } /// Runs `func` on each query result for the given [`World`]. This is faster than the equivalent /// iter_mut() method, but cannot be chained like a normal [`Iterator`]. #[inline] - pub fn for_each_mut<'w, 's>( + pub fn for_each_mut<'w, 's, FN: FnMut(>::Item)>( &'s mut self, world: &'w mut World, - func: impl FnMut(>::Item), + func: FN, ) { // SAFETY: query has unique world access unsafe { - self.for_each_unchecked(world, func); + self.update_archetypes(world); + self.for_each_unchecked_manual::( + world, + func, + world.last_change_tick(), + world.read_change_tick(), + ); } } @@ -435,13 +479,13 @@ where /// This does not check for mutable query correctness. To be safe, make sure mutable queries /// have unique access to the components they query. #[inline] - pub unsafe fn for_each_unchecked<'w, 's>( + pub unsafe fn for_each_unchecked<'w, 's, FN: FnMut(>::Item)>( &'s mut self, world: &'w World, - func: impl FnMut(>::Item), + func: FN, ) { self.update_archetypes(world); - self.for_each_unchecked_manual( + self.for_each_unchecked_manual::( world, func, world.last_change_tick(), @@ -454,33 +498,55 @@ where /// This can only be called for read-only queries, see [`Self::par_for_each_mut`] for /// write-queries. #[inline] - pub fn par_for_each<'w, 's>( + pub fn par_for_each< + 'w, + 's, + FN: Fn(>::Item) + Send + Sync + Clone, + >( &'s mut self, world: &'w World, task_pool: &TaskPool, batch_size: usize, - func: impl Fn(>::Item) + Send + Sync + Clone, - ) where - Q::Fetch: ReadOnlyFetch, - { + func: FN, + ) { // SAFETY: query is read only unsafe { - self.par_for_each_unchecked(world, task_pool, batch_size, func); + self.update_archetypes(world); + self.par_for_each_unchecked_manual::( + world, + task_pool, + batch_size, + func, + world.last_change_tick(), + world.read_change_tick(), + ); } } /// Runs `func` on each query result in parallel using the given `task_pool`. #[inline] - pub fn par_for_each_mut<'w, 's>( + pub fn par_for_each_mut< + 'w, + 's, + FN: Fn(>::Item) + Send + Sync + Clone, + >( &'s mut self, world: &'w mut World, task_pool: &TaskPool, batch_size: usize, - func: impl Fn(>::Item) + Send + Sync + Clone, + func: FN, ) { // SAFETY: query has unique world access unsafe { - self.par_for_each_unchecked(world, task_pool, batch_size, func); + self.update_archetypes(world); + self.par_for_each_unchecked_manual::( + world, + task_pool, + batch_size, + func, + world.last_change_tick(), + world.read_change_tick(), + ); } } @@ -493,15 +559,19 @@ where /// This does not check for mutable query correctness. To be safe, make sure mutable queries /// have unique access to the components they query. #[inline] - pub unsafe fn par_for_each_unchecked<'w, 's>( + pub unsafe fn par_for_each_unchecked< + 'w, + 's, + FN: Fn(>::Item) + Send + Sync + Clone, + >( &'s mut self, world: &'w World, task_pool: &TaskPool, batch_size: usize, - func: impl Fn(>::Item) + Send + Sync + Clone, + func: FN, ) { self.update_archetypes(world); - self.par_for_each_unchecked_manual( + self.par_for_each_unchecked_manual::( world, task_pool, batch_size, @@ -521,17 +591,21 @@ where /// have unique access to the components they query. /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` /// with a mismatched WorldId is unsound. - pub(crate) unsafe fn for_each_unchecked_manual<'w, 's>( + pub(crate) unsafe fn for_each_unchecked_manual< + 'w, + 's, + QF: Fetch<'w, 's, State = Q::State>, + FN: FnMut(QF::Item), + >( &'s self, world: &'w World, - mut func: impl FnMut(>::Item), + mut func: FN, last_change_tick: u32, change_tick: u32, ) { // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual - let mut fetch = - ::init(world, &self.fetch_state, last_change_tick, change_tick); + let mut fetch = QF::init(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = ::init(world, &self.filter_state, last_change_tick, change_tick); if Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE { @@ -577,19 +651,24 @@ where /// have unique access to the components they query. /// This does not validate that `world.id()` matches `self.world_id`. Calling this on a `world` /// with a mismatched WorldId is unsound. - pub unsafe fn par_for_each_unchecked_manual<'w, 's>( + pub(crate) unsafe fn par_for_each_unchecked_manual< + 'w, + 's, + QF: Fetch<'w, 's, State = Q::State>, + FN: Fn(QF::Item) + Send + Sync + Clone, + >( &'s self, world: &'w World, task_pool: &TaskPool, batch_size: usize, - func: impl Fn(>::Item) + Send + Sync + Clone, + func: FN, last_change_tick: u32, change_tick: u32, ) { // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual task_pool.scope(|scope| { - if Q::Fetch::IS_DENSE && F::Fetch::IS_DENSE { + if QF::IS_DENSE && F::Fetch::IS_DENSE { let tables = &world.storages().tables; for table_id in self.matched_table_ids.iter() { let table = &tables[*table_id]; @@ -597,12 +676,8 @@ where while offset < table.len() { let func = func.clone(); scope.spawn(async move { - let mut fetch = ::init( - world, - &self.fetch_state, - last_change_tick, - change_tick, - ); + let mut fetch = + QF::init(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = ::init( world, &self.filter_state, @@ -633,12 +708,8 @@ where while offset < archetype.len() { let func = func.clone(); scope.spawn(async move { - let mut fetch = ::init( - world, - &self.fetch_state, - last_change_tick, - change_tick, - ); + let mut fetch = + QF::init(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = ::init( world, &self.filter_state, diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index dab9815d6d..e17a9f78e0 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -785,4 +785,29 @@ mod tests { } } } + + #[test] + fn immutable_mut_test() { + #[derive(Component, Eq, PartialEq, Debug, Clone, Copy)] + struct A(usize); + + let mut world = World::default(); + world.spawn().insert(A(1)); + world.spawn().insert(A(2)); + + let mut system_state = SystemState::>::new(&mut world); + { + let mut query = system_state.get_mut(&mut world); + assert_eq!( + query.iter_mut().map(|m| *m).collect::>(), + vec![A(1), A(2)], + "both components returned by iter_mut of &mut" + ); + assert_eq!( + query.iter().collect::>(), + vec![&A(1), &A(2)], + "both components returned by iter of &mut" + ); + } + } } diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 6aaf39bfba..ea58bb5e14 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -3,7 +3,7 @@ use crate::{ entity::Entity, query::{ Fetch, FilterFetch, QueryCombinationIter, QueryEntityError, QueryIter, QueryState, - ReadOnlyFetch, WorldQuery, + WorldQuery, }, world::{Mut, World}, }; @@ -277,8 +277,8 @@ where /// Returns an [`Iterator`] over the query results. /// - /// This can only be called for read-only queries (due to the [`ReadOnlyFetch`] trait - /// bound). See [`Self::iter_mut`] for queries that contain at least one mutable component. + /// This can only return immutable data (mutable data will be cast to an immutable form). + /// See [`Self::iter_mut`] for queries that contain at least one mutable component. /// /// # Example /// @@ -299,10 +299,7 @@ where /// # report_names_system.system(); /// ``` #[inline] - pub fn iter(&'s self) -> QueryIter<'w, 's, Q, F> - where - Q::Fetch: ReadOnlyFetch, - { + pub fn iter(&'s self) -> QueryIter<'w, 's, Q, Q::ReadOnlyFetch, F> { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { @@ -332,7 +329,7 @@ where /// # gravity_system.system(); /// ``` #[inline] - pub fn iter_mut(&mut self) -> QueryIter<'_, '_, Q, F> { + pub fn iter_mut(&mut self) -> QueryIter<'_, '_, Q, Q::Fetch, F> { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { @@ -342,17 +339,16 @@ where } /// Returns an [`Iterator`] over all possible combinations of `K` query results without repetition. - /// This can only be called for read-only queries + /// This can only return immutable data /// /// For permutations of size K of query returning N results, you will get: /// - if K == N: one permutation of all query results /// - if K < N: all possible K-sized combinations of query results, without repetition /// - if K > N: empty set (no K-sized combinations exist) #[inline] - pub fn iter_combinations(&self) -> QueryCombinationIter<'_, '_, Q, F, K> - where - Q::Fetch: ReadOnlyFetch, - { + pub fn iter_combinations( + &self, + ) -> QueryCombinationIter<'_, '_, Q, Q::ReadOnlyFetch, F, K> { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { @@ -389,7 +385,7 @@ where #[inline] pub fn iter_combinations_mut( &mut self, - ) -> QueryCombinationIter<'_, '_, Q, F, K> { + ) -> QueryCombinationIter<'_, '_, Q, Q::Fetch, F, K> { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { @@ -408,7 +404,7 @@ where /// This function makes it possible to violate Rust's aliasing guarantees. You must make sure /// this call does not result in multiple mutable references to the same component #[inline] - pub unsafe fn iter_unsafe(&self) -> QueryIter<'_, '_, Q, F> { + pub unsafe fn iter_unsafe(&'s self) -> QueryIter<'w, 's, Q, Q::Fetch, F> { // SEMI-SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict self.state @@ -424,7 +420,7 @@ where #[inline] pub unsafe fn iter_combinations_unsafe( &self, - ) -> QueryCombinationIter<'_, '_, Q, F, K> { + ) -> QueryCombinationIter<'_, '_, Q, Q::Fetch, F, K> { // SEMI-SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict self.state.iter_combinations_unchecked_manual( @@ -437,7 +433,7 @@ where /// Runs `f` on each query result. This is faster than the equivalent iter() method, but cannot /// be chained like a normal [`Iterator`]. /// - /// This can only be called for read-only queries, see [`Self::for_each_mut`] for write-queries. + /// This can only pass in immutable data, see [`Self::for_each_mut`] for mutable access. /// /// # Example /// @@ -458,19 +454,17 @@ where /// # report_names_system.system(); /// ``` #[inline] - pub fn for_each(&'s self, f: impl FnMut(>::Item)) - where - Q::Fetch: ReadOnlyFetch, - { + pub fn for_each>::Item)>(&'s self, f: FN) { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { - self.state.for_each_unchecked_manual( - self.world, - f, - self.last_change_tick, - self.change_tick, - ) + self.state + .for_each_unchecked_manual::( + self.world, + f, + self.last_change_tick, + self.change_tick, + ) }; } @@ -496,11 +490,11 @@ where /// # gravity_system.system(); /// ``` #[inline] - pub fn for_each_mut<'a>(&'a mut self, f: impl FnMut(>::Item)) { + pub fn for_each_mut<'a, FN: FnMut(>::Item)>(&'a mut self, f: FN) { // SAFE: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { - self.state.for_each_unchecked_manual( + self.state.for_each_unchecked_manual::( self.world, f, self.last_change_tick, @@ -511,43 +505,42 @@ where /// Runs `f` on each query result in parallel using the given task pool. /// - /// This can only be called for read-only queries, see [`Self::par_for_each_mut`] for - /// write-queries. + /// This can only be called for immutable data, see [`Self::par_for_each_mut`] for + /// mutable access. #[inline] - pub fn par_for_each( + pub fn par_for_each>::Item) + Send + Sync + Clone>( &'s self, task_pool: &TaskPool, batch_size: usize, - f: impl Fn(>::Item) + Send + Sync + Clone, - ) where - Q::Fetch: ReadOnlyFetch, - { + f: FN, + ) { // SAFE: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { - self.state.par_for_each_unchecked_manual( - self.world, - task_pool, - batch_size, - f, - self.last_change_tick, - self.change_tick, - ) + self.state + .par_for_each_unchecked_manual::( + self.world, + task_pool, + batch_size, + f, + self.last_change_tick, + self.change_tick, + ) }; } /// Runs `f` on each query result in parallel using the given task pool. #[inline] - pub fn par_for_each_mut<'a>( + pub fn par_for_each_mut<'a, FN: Fn(>::Item) + Send + Sync + Clone>( &'a mut self, task_pool: &TaskPool, batch_size: usize, - f: impl Fn(>::Item) + Send + Sync + Clone, + f: FN, ) { // SAFE: system runs without conflicts with other systems. same-system queries have runtime // borrow checks when they conflict unsafe { - self.state.par_for_each_unchecked_manual( + self.state.par_for_each_unchecked_manual::( self.world, task_pool, batch_size, @@ -563,8 +556,8 @@ where /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is /// returned instead. /// - /// This can only be called for read-only queries (due to the [`ReadOnlyFetch`] trait bound). - /// see [`get_mut`](Self::get_mut) for queries that contain at least one mutable component. + /// This can only return immutable data (mutable data will be cast to an immutable form). + /// See [`get_mut`](Self::get_mut) for queries that contain at least one mutable component. /// /// # Example /// @@ -593,14 +586,11 @@ where pub fn get( &'s self, entity: Entity, - ) -> Result<>::Item, QueryEntityError> - where - Q::Fetch: ReadOnlyFetch, - { + ) -> Result<>::Item, QueryEntityError> { // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { - self.state.get_unchecked_manual( + self.state.get_unchecked_manual::( self.world, entity, self.last_change_tick, @@ -641,7 +631,7 @@ where // SAFE: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict unsafe { - self.state.get_unchecked_manual( + self.state.get_unchecked_manual::( self.world, entity, self.last_change_tick, @@ -661,13 +651,17 @@ where /// this call does not result in multiple mutable references to the same component #[inline] pub unsafe fn get_unchecked( - &self, + &'s self, entity: Entity, - ) -> Result<::Item, QueryEntityError> { + ) -> Result<>::Item, QueryEntityError> { // SEMI-SAFE: 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) + self.state.get_unchecked_manual::( + self.world, + entity, + self.last_change_tick, + self.change_tick, + ) } /// Returns a reference to the [`Entity`]'s [`Component`] of the given type. @@ -800,9 +794,8 @@ where /// Returns a single immutable query result when there is exactly one entity matching /// the query. /// - /// This can only be called for read-only queries (due to the [`ReadOnlyFetch`] trait - /// bound). Use [`single_mut`](Self::single_mut) for queries that contain at least one - /// mutable component. + /// This can only return immutable data. Use [`single_mut`](Self::single_mut) for + /// queries that contain at least one mutable component. /// /// # Example /// @@ -824,19 +817,15 @@ where /// Panics if the number of query results is not exactly one. Use /// [`get_single`](Self::get_single) to return a `Result` instead of panicking. #[track_caller] - pub fn single(&'s self) -> >::Item - where - Q::Fetch: ReadOnlyFetch, - { + pub fn single(&'s self) -> >::Item { self.get_single().unwrap() } /// Returns a single immutable query result when there is exactly one entity matching /// the query. /// - /// This can only be called for read-only queries (due to the [`ReadOnlyFetch`] trait - /// bound). Use [`get_single_mut`](Self::get_single_mut) for queries that contain at least one - /// mutable component. + /// This can only return immutable data. Use [`get_single_mut`](Self::get_single_mut) + /// for queries that contain at least one mutable component. /// /// If the number of query results is not exactly one, a [`QuerySingleError`] is returned /// instead. @@ -863,10 +852,9 @@ where /// } /// # player_scoring_system.system(); /// ``` - pub fn get_single(&'s self) -> Result<>::Item, QuerySingleError> - where - Q::Fetch: ReadOnlyFetch, - { + pub fn get_single( + &'s self, + ) -> Result<>::Item, QuerySingleError> { let mut query = self.iter(); let first = query.next(); let extra = query.next().is_some(); @@ -970,8 +958,6 @@ where /// ``` #[inline] pub fn is_empty(&self) -> bool { - // TODO: This code can be replaced with `self.iter().next().is_none()` if/when - // we sort out how to convert "write" queries to "read" queries. self.state .is_empty(self.world, self.last_change_tick, self.change_tick) } diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.rs new file mode 100644 index 0000000000..2a49d5754b --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.rs @@ -0,0 +1,28 @@ +use bevy_ecs::prelude::*; +use bevy_ecs::system::SystemState; + +#[derive(Component, Eq, PartialEq, Debug, Clone, Copy)] +struct A(usize); + +fn main() { + let mut world = World::default(); + world.spawn().insert(A(1)); + world.spawn().insert(A(2)); + + let mut system_state = SystemState::>::new(&mut world); + { + let mut query = system_state.get_mut(&mut world); + let mut_vec = query.iter_mut().collect::>>(); + assert_eq!( + // this should fail to compile due to the later use of mut_vec + query.iter().collect::>(), + vec![&A(1), &A(2)], + "both components returned by iter of &mut" + ); + assert_eq!( + mut_vec.iter().map(|m| **m).collect::>(), + vec![A(1), A(2)], + "both components returned by iter_mut of &mut" + ); + } +} diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.stderr new file mode 100644 index 0000000000..77fb4c4ea6 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_state_iter_mut_overlap_safety.stderr @@ -0,0 +1,11 @@ +error[E0502]: cannot borrow `query` as immutable because it is also borrowed as mutable + --> tests/ui/system_state_iter_mut_overlap_safety.rs:18:13 + | +15 | let mut_vec = query.iter_mut().collect::>>(); + | ----- mutable borrow occurs here +... +18 | query.iter().collect::>(), + | ^^^^^ immutable borrow occurs here +... +23 | mut_vec.iter().map(|m| **m).collect::>(), + | ------- mutable borrow later used here diff --git a/pipelined/bevy_render2/src/render_component.rs b/pipelined/bevy_render2/src/render_component.rs index 25d4e3f4b7..437ae2b973 100644 --- a/pipelined/bevy_render2/src/render_component.rs +++ b/pipelined/bevy_render2/src/render_component.rs @@ -8,7 +8,7 @@ use bevy_asset::{Asset, Handle}; use bevy_ecs::{ component::Component, prelude::*, - query::{FilterFetch, QueryItem, ReadOnlyFetch, WorldQuery}, + query::{FilterFetch, QueryItem, WorldQuery}, system::{ lifetimeless::{Read, SCommands, SQuery}, RunSystem, SystemParamItem, @@ -141,7 +141,6 @@ impl Default for ExtractComponentPlugin { impl Plugin for ExtractComponentPlugin where - ::Fetch: ReadOnlyFetch, ::Fetch: FilterFetch, { fn build(&self, app: &mut App) { @@ -167,7 +166,6 @@ pub struct ExtractComponentSystem(PhantomData); impl RunSystem for ExtractComponentSystem where ::Fetch: FilterFetch, - ::Fetch: ReadOnlyFetch, { type Param = ( SCommands, @@ -176,9 +174,9 @@ where SQuery<(Entity, C::Query), C::Filter>, ); - fn run((mut commands, mut previous_len, query): SystemParamItem) { + fn run((mut commands, mut previous_len, mut query): SystemParamItem) { let mut values = Vec::with_capacity(*previous_len); - for (entity, query_item) in query.iter() { + for (entity, query_item) in query.iter_mut() { values.push((entity, (C::extract_component(query_item),))); } *previous_len = values.len();