From b6c63c4c997d05862ab56470f6cbbaadcb6156d1 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Wed, 20 Jul 2022 17:43:25 +0200 Subject: [PATCH 1/5] Remove `many_for_each_mut`. --- crates/bevy_ecs/src/query/mod.rs | 5 +- crates/bevy_ecs/src/query/state.rs | 85 +------------------ crates/bevy_ecs/src/system/query.rs | 53 +----------- ...query_many_for_each_mut_lifetime_safety.rs | 14 --- ...y_many_for_each_mut_lifetime_safety.stderr | 10 --- 5 files changed, 8 insertions(+), 159 deletions(-) delete mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.rs delete mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.stderr diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 16dc86db4062e..3efb6a5dc1017 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -633,9 +633,10 @@ count(): {count}"# } { fn system(has_a: Query>, mut b_query: Query<&mut B>) { - b_query.many_for_each_mut(&has_a, |mut b| { + let mut iter = b_query.iter_many_mut(&has_a); + while let Some(mut b) = iter.fetch_next() { b.0 = 1; - }); + } } let mut system = IntoSystem::into_system(system); system.initialize(&mut world); diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 95b058b76f70f..2069a9dd4a885 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -602,7 +602,7 @@ impl QueryState { /// Returns an [`Iterator`] over the query results of a list of [`Entity`]'s. /// /// This can only return immutable data (mutable data will be cast to an immutable form). - /// See [`Self::many_for_each_mut`] for queries that contain at least one mutable component. + /// See [`Self::iter_many_mut`] for queries that contain at least one mutable component. /// #[inline] pub fn iter_many<'w, 's, EntityList: IntoIterator>( @@ -868,29 +868,6 @@ impl QueryState { ); } - /// Runs `func` on each query result where the entities match. - #[inline] - pub fn many_for_each_mut( - &mut self, - world: &mut World, - entities: EntityList, - func: impl FnMut(QueryItem<'_, Q>), - ) where - EntityList::Item: Borrow, - { - // SAFETY: query has unique world access - unsafe { - self.update_archetypes(world); - self.many_for_each_unchecked_manual( - world, - entities, - func, - world.last_change_tick(), - world.read_change_tick(), - ); - }; - } - /// Runs `func` on each query result for the given [`World`], where the last change and /// the current change tick are given. This is faster than the equivalent /// iter() method, but cannot be chained like a normal [`Iterator`]. @@ -909,7 +886,7 @@ impl QueryState { 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::many_for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual + // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual let mut fetch = as Fetch>::init(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = as Fetch>::init( @@ -978,7 +955,7 @@ impl QueryState { 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::many_for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual + // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual ComputeTaskPool::get().scope(|scope| { if >::IS_DENSE && >::IS_DENSE { let tables = &world.storages().tables; @@ -1078,62 +1055,6 @@ impl QueryState { }); } - /// Runs `func` on each query result for the given [`World`] and list of [`Entity`]'s, where the last change and - /// the current change tick are given. This is faster than the equivalent - /// iter() method, but cannot be chained like a normal [`Iterator`]. - /// - /// # Safety - /// - /// This does not check for mutable query correctness. To be safe, make sure mutable queries - /// 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 many_for_each_unchecked_manual( - &self, - world: &World, - entity_list: EntityList, - mut func: impl FnMut(QueryItem<'_, Q>), - last_change_tick: u32, - change_tick: u32, - ) where - EntityList::Item: Borrow, - { - // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: - // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::many_for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual - let mut fetch = - as Fetch>::init(world, &self.fetch_state, last_change_tick, change_tick); - let mut filter = as Fetch>::init( - world, - &self.filter_state, - last_change_tick, - change_tick, - ); - - let tables = &world.storages.tables; - - for entity in entity_list { - let location = match world.entities.get(*entity.borrow()) { - Some(location) => location, - None => continue, - }; - - if !self - .matched_archetypes - .contains(location.archetype_id.index()) - { - continue; - } - - let archetype = &world.archetypes[location.archetype_id]; - - fetch.set_archetype(&self.fetch_state, archetype, tables); - filter.set_archetype(&self.filter_state, archetype, tables); - if filter.archetype_filter_fetch(location.index) { - func(fetch.archetype_fetch(location.index)); - } - } - } - /// Returns a single immutable query result when there is exactly one entity matching /// the query. /// diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 5a9fe5adc0aa3..086a8a0ec8038 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -413,7 +413,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// Returns an [`Iterator`] over the query results of a list of [`Entity`]'s. /// /// This can only return immutable data (mutable data will be cast to an immutable form). - /// See [`Self::many_for_each_mut`] for queries that contain at least one mutable component. + /// See [`Self::iter_many_mut`] for queries that contain at least one mutable component. /// /// # Examples /// ``` @@ -495,7 +495,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// Returns an [`Iterator`] over the query results of a list of [`Entity`]'s. /// - /// If you want safe mutable access to query results of a list of [`Entity`]'s. See [`Self::many_for_each_mut`]. + /// If you want safe mutable access to query results of a list of [`Entity`]'s. See [`Self::iter_many_mut`]. /// /// # Safety /// This allows aliased mutability and does not check for entity uniqueness. @@ -659,55 +659,6 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { }; } - /// Calls a closure on each result of [`Query`] where the entities match. - /// # Examples - /// - /// ``` - /// # use bevy_ecs::prelude::*; - /// #[derive(Component)] - /// struct Counter { - /// value: i32 - /// } - /// - /// #[derive(Component)] - /// struct Friends { - /// list: Vec, - /// } - /// - /// fn system( - /// friends_query: Query<&Friends>, - /// mut counter_query: Query<&mut Counter>, - /// ) { - /// for friends in &friends_query { - /// counter_query.many_for_each_mut(&friends.list, |mut counter| { - /// println!("Friend's counter: {:?}", counter.value); - /// counter.value += 1; - /// }); - /// } - /// } - /// # bevy_ecs::system::assert_is_system(system); - /// ``` - #[inline] - pub fn many_for_each_mut( - &mut self, - entities: EntityList, - f: impl FnMut(QueryItem<'_, Q>), - ) where - EntityList::Item: Borrow, - { - // SAFETY: system runs without conflicts with other systems. - // same-system queries have runtime borrow checks when they conflict - unsafe { - self.state.many_for_each_unchecked_manual( - self.world, - entities, - f, - self.last_change_tick, - self.change_tick, - ); - }; - } - /// Returns the query result for the given [`Entity`]. /// /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.rs deleted file mode 100644 index e35ba6dbc57d2..0000000000000 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.rs +++ /dev/null @@ -1,14 +0,0 @@ -use bevy_ecs::prelude::*; - -#[derive(Component)] -struct A(usize); - -fn system(mut query: Query<&mut A>, e: Res) { - let mut results = Vec::new(); - query.many_for_each_mut(vec![*e, *e], |a| { - // this should fail to compile - results.push(a); - }); -} - -fn main() {} diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.stderr deleted file mode 100644 index b37e88e932473..0000000000000 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_many_for_each_mut_lifetime_safety.stderr +++ /dev/null @@ -1,10 +0,0 @@ -error[E0521]: borrowed data escapes outside of closure - --> tests/ui/system_query_many_for_each_mut_lifetime_safety.rs:10:9 - | -7 | let mut results = Vec::new(); - | ----------- `results` declared here, outside of the closure body -8 | query.many_for_each_mut(vec![*e, *e], |a| { - | - `a` is a reference that is only valid in the closure body -9 | // this should fail to compile -10 | results.push(a); - | ^^^^^^^^^^^^^^^ `a` escapes the closure body here From e90a679291356573a89d536ab463cd46e91e0d3c Mon Sep 17 00:00:00 2001 From: devil-ira Date: Wed, 20 Jul 2022 18:23:12 +0200 Subject: [PATCH 2/5] Add `iter_many_mut`. --- crates/bevy_ecs/src/query/iter.rs | 60 +++++++++++++++++++---------- crates/bevy_ecs/src/query/state.rs | 24 +++++++++++- crates/bevy_ecs/src/system/query.rs | 48 +++++++++++++++++++++++ 3 files changed, 111 insertions(+), 21 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index e5bdc4770a9e8..cd0715fe31dc4 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -72,9 +72,9 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Iterator for QueryIter<'w, 's, Q, F> // This is correct as [`QueryIter`] always returns `None` once exhausted. impl<'w, 's, Q: WorldQuery, F: WorldQuery> FusedIterator for QueryIter<'w, 's, Q, F> {} -/// An [`Iterator`] over query results of a [`Query`](crate::system::Query). +/// An [`Iterator`] over [`Query`](crate::system::Query) results of a list of [`Entity`]s. /// -/// This struct is created by the [`Query::iter_many`](crate::system::Query::iter_many) method. +/// This struct is created by the [`Query::iter_many`](crate::system::Query::iter_many) and [`Query::iter_many_mut`](crate::system::Query::iter_many_mut) methods. pub struct QueryManyIter<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator> where I::Item: Borrow, @@ -126,16 +126,15 @@ where entity_iter: entity_list.into_iter(), } } -} - -impl<'w, 's, Q: WorldQuery, F: WorldQuery, I: Iterator> Iterator for QueryManyIter<'w, 's, Q, F, I> -where - I::Item: Borrow, -{ - type Item = QueryItem<'w, Q>; + /// Safety: + /// The lifetime here is not restrictive enough for Fetch with &mut access, + /// as calling `fetch_next_aliased_unchecked` multiple times can produce multiple + /// references to the same component, leading to unique reference aliasing. + /// + /// It is always safe for shared access. #[inline(always)] - fn next(&mut self) -> Option { + unsafe fn fetch_next_aliased_unchecked(&mut self) -> Option> { for entity in self.entity_iter.by_ref() { let location = match self.entities.get(*entity.borrow()) { Some(location) => location, @@ -154,26 +153,47 @@ where // SAFETY: `archetype` is from the world that `fetch/filter` were created for, // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with - unsafe { - self.fetch - .set_archetype(&self.query_state.fetch_state, archetype, self.tables); - } + self.fetch + .set_archetype(&self.query_state.fetch_state, archetype, self.tables); + // SAFETY: `table` is from the world that `fetch/filter` were created for, // `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with - unsafe { - self.filter - .set_archetype(&self.query_state.filter_state, archetype, self.tables); - } + self.filter + .set_archetype(&self.query_state.filter_state, archetype, self.tables); + // SAFETY: set_archetype was called prior. // `location.index` is an archetype index row in range of the current archetype, because if it was not, the match above would have `continue`d - if unsafe { self.filter.archetype_filter_fetch(location.index) } { + if self.filter.archetype_filter_fetch(location.index) { // SAFETY: set_archetype was called prior, `location.index` is an archetype index in range of the current archetype - return Some(unsafe { self.fetch.archetype_fetch(location.index) }); + return Some(self.fetch.archetype_fetch(location.index)); } } None } + /// Get next result from the query + #[inline(always)] + pub fn fetch_next(&mut self) -> Option> { + // SAFETY: we are limiting the returned reference to self, + // making sure this method cannot be called multiple times without getting rid + // of any previously returned unique references first, thus preventing aliasing. + unsafe { self.fetch_next_aliased_unchecked().map(Q::shrink) } + } +} + +impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, I: Iterator> Iterator + for QueryManyIter<'w, 's, Q, F, I> +where + I::Item: Borrow, +{ + type Item = QueryItem<'w, Q>; + + #[inline(always)] + fn next(&mut self) -> Option { + // SAFETY: It is safe to alias for ReadOnlyWorldQuery. + unsafe { self.fetch_next_aliased_unchecked() } + } + fn size_hint(&self) -> (usize, Option) { let (_, max_size) = self.entity_iter.size_hint(); (0, max_size) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 2069a9dd4a885..3b3a76f58c970 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -613,9 +613,9 @@ impl QueryState { where EntityList::Item: Borrow, { + self.update_archetypes(world); // SAFETY: query is read only unsafe { - self.update_archetypes(world); self.as_readonly().iter_many_unchecked_manual( entities, world, @@ -625,6 +625,28 @@ impl QueryState { } } + /// Returns an iterator over the query results of a list of [`Entity`]'s. + #[inline] + pub fn iter_many_mut<'w, 's, EntityList: IntoIterator>( + &'s mut self, + world: &'w mut World, + entities: EntityList, + ) -> QueryManyIter<'w, 's, Q, F, EntityList::IntoIter> + where + EntityList::Item: Borrow, + { + self.update_archetypes(world); + // SAFETY: Query has unique world access. + unsafe { + self.iter_many_unchecked_manual( + entities, + world, + world.last_change_tick(), + world.read_change_tick(), + ) + } + } + /// Returns an [`Iterator`] over the query results for the given [`World`]. /// /// # Safety diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 086a8a0ec8038..566fd4aeb5e07 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -459,6 +459,54 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { ) } } + /// Calls a closure on each result of [`Query`] where the entities match. + /// # Examples + /// + /// ``` + /// # use bevy_ecs::prelude::*; + /// #[derive(Component)] + /// struct Counter { + /// value: i32 + /// } + /// + /// #[derive(Component)] + /// struct Friends { + /// list: Vec, + /// } + /// + /// fn system( + /// friends_query: Query<&Friends>, + /// mut counter_query: Query<&mut Counter>, + /// ) { + /// for friends in &friends_query { + /// let mut iter = counter_query.iter_many_mut(&friends.list); + /// while let Some(mut counter) = iter.fetch_next() { + /// println!("Friend's counter: {:?}", counter.value); + /// counter.value += 1; + /// } + /// } + /// } + /// # bevy_ecs::system::assert_is_system(system); + /// ``` + #[inline] + pub fn iter_many_mut( + &mut self, + entities: EntityList, + ) -> QueryManyIter<'_, '_, Q, F, EntityList::IntoIter> + where + EntityList::Item: Borrow, + { + // SAFETY: system runs without conflicts with other systems. + // same-system queries have runtime borrow checks when they conflict + unsafe { + self.state.iter_many_unchecked_manual( + entities, + self.world, + self.last_change_tick, + self.change_tick, + ) + } + } /// Returns an [`Iterator`] over the query results. /// From e125153af45a05790f051e2af2880aeef8c1bcc4 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Wed, 20 Jul 2022 18:23:31 +0200 Subject: [PATCH 3/5] Add trybuild tests. --- ...y_iter_combinations_mut_iterator_safety.rs | 15 +++++++++++ ...er_combinations_mut_iterator_safety.stderr | 25 +++++++++++++++++++ .../ui/query_iter_many_mut_iterator_safety.rs | 15 +++++++++++ ...query_iter_many_mut_iterator_safety.stderr | 25 +++++++++++++++++++ ...tem_query_iter_many_mut_lifetime_safety.rs | 15 +++++++++++ ...query_iter_many_mut_lifetime_safety.stderr | 5 ++++ 6 files changed, 100 insertions(+) create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.stderr create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.stderr create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.rs create mode 100644 crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.stderr diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs new file mode 100644 index 0000000000000..e7d844183f54f --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs @@ -0,0 +1,15 @@ +use bevy_ecs::prelude::*; + +#[derive(Component)] +struct A(usize); + +fn system(mut query: Query<&mut A>) { + let iter = query.iter_combinations_mut(); + + // This should fail to compile. + is_iterator(iter) +} + +fn is_iterator(_iter: T) {} + +fn main() {} \ No newline at end of file diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.stderr new file mode 100644 index 0000000000000..7cd36eadcff93 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.stderr @@ -0,0 +1,25 @@ +error[E0277]: the trait bound `&mut A: ReadOnlyWorldQuery` is not satisfied + --> tests/ui/query_iter_combinations_mut_iterator_safety.rs:10:17 + | +10 | is_iterator(iter) + | ----------- ^^^^ the trait `ReadOnlyWorldQuery` is not implemented for `&mut A` + | | + | required by a bound introduced by this call + | + = help: the following other types implement trait `ReadOnlyWorldQuery`: + &T + () + (F0, F1) + (F0, F1, F2) + (F0, F1, F2, F3) + (F0, F1, F2, F3, F4) + (F0, F1, F2, F3, F4, F5) + (F0, F1, F2, F3, F4, F5, F6) + and 49 others + = note: `ReadOnlyWorldQuery` is implemented for `&A`, but not for `&mut A` + = note: required because of the requirements on the impl of `Iterator` for `QueryCombinationIter<'_, '_, &mut A, (), {_: usize}>` +note: required by a bound in `is_iterator` + --> tests/ui/query_iter_combinations_mut_iterator_safety.rs:13:19 + | +13 | fn is_iterator(_iter: T) {} + | ^^^^^^^^ required by this bound in `is_iterator` diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs new file mode 100644 index 0000000000000..f84b851168f96 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs @@ -0,0 +1,15 @@ +use bevy_ecs::prelude::*; + +#[derive(Component)] +struct A(usize); + +fn system(mut query: Query<&mut A>, e: Res) { + let iter = query.iter_many_mut([*e]); + + // This should fail to compile. + is_iterator(iter) +} + +fn is_iterator(_iter: T) {} + +fn main() {} \ No newline at end of file diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.stderr new file mode 100644 index 0000000000000..45840aa3230be --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.stderr @@ -0,0 +1,25 @@ +error[E0277]: the trait bound `&mut A: ReadOnlyWorldQuery` is not satisfied + --> tests/ui/query_iter_many_mut_iterator_safety.rs:10:17 + | +10 | is_iterator(iter) + | ----------- ^^^^ the trait `ReadOnlyWorldQuery` is not implemented for `&mut A` + | | + | required by a bound introduced by this call + | + = help: the following other types implement trait `ReadOnlyWorldQuery`: + &T + () + (F0, F1) + (F0, F1, F2) + (F0, F1, F2, F3) + (F0, F1, F2, F3, F4) + (F0, F1, F2, F3, F4, F5) + (F0, F1, F2, F3, F4, F5, F6) + and 49 others + = note: `ReadOnlyWorldQuery` is implemented for `&A`, but not for `&mut A` + = note: required because of the requirements on the impl of `Iterator` for `QueryManyIter<'_, '_, &mut A, (), std::array::IntoIter>` +note: required by a bound in `is_iterator` + --> tests/ui/query_iter_many_mut_iterator_safety.rs:13:19 + | +13 | fn is_iterator(_iter: T) {} + | ^^^^^^^^ required by this bound in `is_iterator` diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.rs new file mode 100644 index 0000000000000..7a1cb107fb40c --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.rs @@ -0,0 +1,15 @@ +use bevy_ecs::prelude::*; + +#[derive(Component)] +struct A(usize); + +fn system(mut query: Query<&mut A>, e: Res) { + let mut results = Vec::new(); + let mut iter = query.iter_many_mut([*e, *e]); + while let Some(a) = iter.fetch_next() { + // this should fail to compile + results.push(a); + } +} + +fn main() {} diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.stderr new file mode 100644 index 0000000000000..a78f5a85ec849 --- /dev/null +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_query_iter_many_mut_lifetime_safety.stderr @@ -0,0 +1,5 @@ +error[E0499]: cannot borrow `iter` as mutable more than once at a time + --> tests/ui/system_query_iter_many_mut_lifetime_safety.rs:9:25 + | +9 | while let Some(a) = iter.fetch_next() { + | ^^^^^^^^^^^^^^^^^ `iter` was mutably borrowed here in the previous iteration of the loop From 91b2f61892c07157b81e810fee0a96d4b1ae131d Mon Sep 17 00:00:00 2001 From: devil-ira Date: Wed, 20 Jul 2022 19:47:09 +0200 Subject: [PATCH 4/5] Nits. Added `FusedIterator` impl. --- crates/bevy_ecs/src/query/iter.rs | 8 ++++++++ crates/bevy_ecs/src/system/query.rs | 1 + .../ui/query_iter_combinations_mut_iterator_safety.rs | 2 +- .../tests/ui/query_iter_many_mut_iterator_safety.rs | 2 +- 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index cd0715fe31dc4..c39d7a9034e88 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -200,6 +200,14 @@ where } } +// This is correct as [`QueryManyIter`] always returns `None` once exhausted. +impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, I: Iterator> FusedIterator + for QueryManyIter<'w, 's, Q, F, I> +where + I::Item: Borrow, +{ +} + pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> { tables: &'w Tables, archetypes: &'w Archetypes, diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 566fd4aeb5e07..8b464ea65adc4 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -459,6 +459,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { ) } } + /// Calls a closure on each result of [`Query`] where the entities match. /// # Examples /// diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs index e7d844183f54f..e88bc34cb4906 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_combinations_mut_iterator_safety.rs @@ -12,4 +12,4 @@ fn system(mut query: Query<&mut A>) { fn is_iterator(_iter: T) {} -fn main() {} \ No newline at end of file +fn main() {} diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs index f84b851168f96..988e267de5e2f 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/query_iter_many_mut_iterator_safety.rs @@ -12,4 +12,4 @@ fn system(mut query: Query<&mut A>, e: Res) { fn is_iterator(_iter: T) {} -fn main() {} \ No newline at end of file +fn main() {} From 4bae6ee539df0a8a7c43a3445acb4bf00c4d3152 Mon Sep 17 00:00:00 2001 From: devil-ira Date: Mon, 25 Jul 2022 12:14:47 +0200 Subject: [PATCH 5/5] Update note. --- crates/bevy_ecs/src/query/iter.rs | 2 +- crates/bevy_ecs/src/query/state.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index c39d7a9034e88..c8f62663cfdb9 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -504,7 +504,7 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery> QueryIterationCursor<'w, 's, Q, F> { } // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: - // QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual + // QueryIter, QueryIterationCursor, QueryManyIter, QueryCombinationIter, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual /// # Safety /// `tables` and `archetypes` must belong to the same world that the [`QueryIterationCursor`] /// was initialized for. diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 3b3a76f58c970..400ea4a5e45d9 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -908,7 +908,7 @@ impl QueryState { 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 + // QueryIter, QueryIterationCursor, QueryManyIter, QueryCombinationIter, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual let mut fetch = as Fetch>::init(world, &self.fetch_state, last_change_tick, change_tick); let mut filter = as Fetch>::init( @@ -977,7 +977,7 @@ impl QueryState { 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 + // QueryIter, QueryIterationCursor, QueryManyIter, QueryCombinationIter, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual ComputeTaskPool::get().scope(|scope| { if >::IS_DENSE && >::IS_DENSE { let tables = &world.storages().tables;