Skip to content

Commit 5095481

Browse files
Add get_multiple and get_multiple_mut APIs for Query and QueryState (#4298)
# Objective - The inability to have multiple active mutable borrows into a query is a common source of borrow-checker pain for users. - This is a pointless restriction if and only if we can guarantee that the entities they are accessing are unique. - This could already by bypassed with get_unchecked, but that is an extremely unsafe API. - Closes #2042. ## Solution - Add `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to `Query` and `QueryState`. - Improve the `QueryEntityError` type to provide more useful error information. ## Changelog - Added `get_multiple`, `get_multiple_mut` and their unchecked equivalents (`multiple` and `multiple_mut`) to Query and QueryState. ## Migration Guide - The `QueryEntityError` enum now has a `AliasedMutability variant, and returns the offending entity. ## Context This is a fresh attempt at #3333; rebasing was behaving very badly and it was important to rebase on top of the recent query soundness fixes. Many thanks to all the reviewers in that thread, especially @BoxyUwU for the help with lifetimes. ## To-do - [x] Add compile fail tests - [x] Successfully deduplicate code - [x] Decide what to do about failing doc tests - [x] Get some reviews for lifetime soundness
1 parent 63fee25 commit 5095481

6 files changed

+489
-6
lines changed

crates/bevy_ecs/src/query/state.rs

Lines changed: 305 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,57 @@ where
153153
}
154154
}
155155

156+
/// Returns the read-only query results for the given array of [`Entity`].
157+
///
158+
/// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is
159+
/// returned instead.
160+
///
161+
/// Note that the unlike [`QueryState::get_multiple_mut`], the entities passed in do not need to be unique.
162+
///
163+
/// # Examples
164+
///
165+
/// ```rust
166+
/// use bevy_ecs::prelude::*;
167+
/// use bevy_ecs::query::QueryEntityError;
168+
///
169+
/// #[derive(Component, PartialEq, Debug)]
170+
/// struct A(usize);
171+
///
172+
/// let mut world = World::new();
173+
/// let entity_vec: Vec<Entity> = (0..3).map(|i|world.spawn().insert(A(i)).id()).collect();
174+
/// let entities: [Entity; 3] = entity_vec.try_into().unwrap();
175+
///
176+
/// world.spawn().insert(A(73));
177+
///
178+
/// let mut query_state = world.query::<&A>();
179+
///
180+
/// let component_values = query_state.get_multiple(&world, entities).unwrap();
181+
///
182+
/// assert_eq!(component_values, [&A(0), &A(1), &A(2)]);
183+
///
184+
/// let wrong_entity = Entity::from_raw(365);
185+
///
186+
/// assert_eq!(query_state.get_multiple(&world, [wrong_entity]), Err(QueryEntityError::NoSuchEntity(wrong_entity)));
187+
/// ```
188+
#[inline]
189+
pub fn get_multiple<'w, 's, const N: usize>(
190+
&'s mut self,
191+
world: &'w World,
192+
entities: [Entity; N],
193+
) -> Result<[<Q::ReadOnlyFetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
194+
self.update_archetypes(world);
195+
196+
// SAFE: update_archetypes validates the `World` matches
197+
unsafe {
198+
self.get_multiple_read_only_manual(
199+
world,
200+
entities,
201+
world.last_change_tick(),
202+
world.read_change_tick(),
203+
)
204+
}
205+
}
206+
156207
/// Gets the query result for the given [`World`] and [`Entity`].
157208
#[inline]
158209
pub fn get_mut<'w, 's>(
@@ -172,6 +223,64 @@ where
172223
}
173224
}
174225

226+
/// Returns the query results for the given array of [`Entity`].
227+
///
228+
/// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is
229+
/// returned instead.
230+
///
231+
/// ```rust
232+
/// use bevy_ecs::prelude::*;
233+
/// use bevy_ecs::query::QueryEntityError;
234+
///
235+
/// #[derive(Component, PartialEq, Debug)]
236+
/// struct A(usize);
237+
///
238+
/// let mut world = World::new();
239+
///
240+
/// let entities: Vec<Entity> = (0..3).map(|i|world.spawn().insert(A(i)).id()).collect();
241+
/// let entities: [Entity; 3] = entities.try_into().unwrap();
242+
///
243+
/// world.spawn().insert(A(73));
244+
///
245+
/// let mut query_state = world.query::<&mut A>();
246+
///
247+
/// let mut mutable_component_values = query_state.get_multiple_mut(&mut world, entities).unwrap();
248+
///
249+
/// for mut a in mutable_component_values.iter_mut(){
250+
/// a.0 += 5;
251+
/// }
252+
///
253+
/// let component_values = query_state.get_multiple(&world, entities).unwrap();
254+
///
255+
/// assert_eq!(component_values, [&A(5), &A(6), &A(7)]);
256+
///
257+
/// let wrong_entity = Entity::from_raw(57);
258+
/// let invalid_entity = world.spawn().id();
259+
///
260+
/// assert_eq!(query_state.get_multiple_mut(&mut world, [wrong_entity]).unwrap_err(), QueryEntityError::NoSuchEntity(wrong_entity));
261+
/// assert_eq!(query_state.get_multiple_mut(&mut world, [invalid_entity]).unwrap_err(), QueryEntityError::QueryDoesNotMatch(invalid_entity));
262+
/// assert_eq!(query_state.get_multiple_mut(&mut world, [entities[0], entities[0]]).unwrap_err(), QueryEntityError::AliasedMutability(entities[0]));
263+
/// ```
264+
#[inline]
265+
pub fn get_multiple_mut<'w, 's, const N: usize>(
266+
&'s mut self,
267+
world: &'w mut World,
268+
entities: [Entity; N],
269+
) -> Result<[<Q::Fetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
270+
self.update_archetypes(world);
271+
272+
// SAFE: method requires exclusive world access
273+
// and world has been validated via update_archetypes
274+
unsafe {
275+
self.get_multiple_unchecked_manual(
276+
world,
277+
entities,
278+
world.last_change_tick(),
279+
world.read_change_tick(),
280+
)
281+
}
282+
}
283+
175284
#[inline]
176285
pub fn get_manual<'w, 's>(
177286
&'s self,
@@ -218,6 +327,9 @@ where
218327
///
219328
/// This does not check for mutable query correctness. To be safe, make sure mutable queries
220329
/// have unique access to the components they query.
330+
///
331+
/// This must be called on the same `World` that the `Query` was generated from:
332+
/// use `QueryState::validate_world` to verify this.
221333
pub(crate) unsafe fn get_unchecked_manual<'w, 's, QF: Fetch<'w, 's, State = Q::State>>(
222334
&'s self,
223335
world: &'w World,
@@ -228,12 +340,12 @@ where
228340
let location = world
229341
.entities
230342
.get(entity)
231-
.ok_or(QueryEntityError::NoSuchEntity)?;
343+
.ok_or(QueryEntityError::NoSuchEntity(entity))?;
232344
if !self
233345
.matched_archetypes
234346
.contains(location.archetype_id.index())
235347
{
236-
return Err(QueryEntityError::QueryDoesNotMatch);
348+
return Err(QueryEntityError::QueryDoesNotMatch(entity));
237349
}
238350
let archetype = &world.archetypes[location.archetype_id];
239351
let mut fetch = QF::init(world, &self.fetch_state, last_change_tick, change_tick);
@@ -245,10 +357,90 @@ where
245357
if filter.archetype_filter_fetch(location.index) {
246358
Ok(fetch.archetype_fetch(location.index))
247359
} else {
248-
Err(QueryEntityError::QueryDoesNotMatch)
360+
Err(QueryEntityError::QueryDoesNotMatch(entity))
249361
}
250362
}
251363

364+
/// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and
365+
/// the current change tick are given.
366+
///
367+
/// # Safety
368+
///
369+
/// This must be called on the same `World` that the `Query` was generated from:
370+
/// use `QueryState::validate_world` to verify this.
371+
pub(crate) unsafe fn get_multiple_read_only_manual<'s, 'w, const N: usize>(
372+
&'s self,
373+
world: &'w World,
374+
entities: [Entity; N],
375+
last_change_tick: u32,
376+
change_tick: u32,
377+
) -> Result<[<Q::ReadOnlyFetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
378+
// SAFE: fetch is read-only
379+
// and world must be validated
380+
let array_of_results = entities.map(|entity| {
381+
self.get_unchecked_manual::<Q::ReadOnlyFetch>(
382+
world,
383+
entity,
384+
last_change_tick,
385+
change_tick,
386+
)
387+
});
388+
389+
// TODO: Replace with TryMap once https://github.com/rust-lang/rust/issues/79711 is stabilized
390+
// If any of the get calls failed, bubble up the error
391+
for result in &array_of_results {
392+
match result {
393+
Ok(_) => (),
394+
Err(error) => return Err(*error),
395+
}
396+
}
397+
398+
// Since we have verified that all entities are present, we can safely unwrap
399+
Ok(array_of_results.map(|result| result.unwrap()))
400+
}
401+
402+
/// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and
403+
/// the current change tick are given.
404+
///
405+
/// # Safety
406+
///
407+
/// This does not check for unique access to subsets of the entity-component data.
408+
/// To be safe, make sure mutable queries have unique access to the components they query.
409+
///
410+
/// This must be called on the same `World` that the `Query` was generated from:
411+
/// use `QueryState::validate_world` to verify this.
412+
pub(crate) unsafe fn get_multiple_unchecked_manual<'s, 'w, const N: usize>(
413+
&'s self,
414+
world: &'w World,
415+
entities: [Entity; N],
416+
last_change_tick: u32,
417+
change_tick: u32,
418+
) -> Result<[<Q::Fetch as Fetch<'w, 's>>::Item; N], QueryEntityError> {
419+
// Verify that all entities are unique
420+
for i in 0..N {
421+
for j in 0..i {
422+
if entities[i] == entities[j] {
423+
return Err(QueryEntityError::AliasedMutability(entities[i]));
424+
}
425+
}
426+
}
427+
428+
let array_of_results = entities.map(|entity| {
429+
self.get_unchecked_manual::<Q::Fetch>(world, entity, last_change_tick, change_tick)
430+
});
431+
432+
// If any of the get calls failed, bubble up the error
433+
for result in &array_of_results {
434+
match result {
435+
Ok(_) => (),
436+
Err(error) => return Err(*error),
437+
}
438+
}
439+
440+
// Since we have verified that all entities are present, we can safely unwrap
441+
Ok(array_of_results.map(|result| result.unwrap()))
442+
}
443+
252444
/// Returns an [`Iterator`] over the query results for the given [`World`].
253445
///
254446
/// This can only be called for read-only queries, see [`Self::iter_mut`] for write-queries.
@@ -733,10 +925,117 @@ where
733925
}
734926

735927
/// An error that occurs when retrieving a specific [`Entity`]'s query result.
736-
#[derive(Error, Debug)]
928+
// TODO: return the type_name as part of this error
929+
#[derive(Error, Debug, PartialEq, Clone, Copy)]
737930
pub enum QueryEntityError {
738931
#[error("The given entity does not have the requested component.")]
739-
QueryDoesNotMatch,
932+
QueryDoesNotMatch(Entity),
740933
#[error("The requested entity does not exist.")]
741-
NoSuchEntity,
934+
NoSuchEntity(Entity),
935+
#[error("The entity was requested mutably more than once.")]
936+
AliasedMutability(Entity),
937+
}
938+
939+
#[cfg(test)]
940+
mod tests {
941+
use crate::{prelude::*, query::QueryEntityError};
942+
943+
#[test]
944+
fn get_multiple_unchecked_manual_uniqueness() {
945+
let mut world = World::new();
946+
947+
let entities: Vec<Entity> = (0..10).map(|_| world.spawn().id()).collect();
948+
949+
let query_state = world.query::<Entity>();
950+
951+
// These don't matter for the test
952+
let last_change_tick = world.last_change_tick();
953+
let change_tick = world.read_change_tick();
954+
955+
// It's best to test get_multiple_unchecked_manual directly,
956+
// as it is shared and unsafe
957+
// We don't care about aliased mutabilty for the read-only equivalent
958+
assert!(unsafe {
959+
query_state
960+
.get_multiple_unchecked_manual::<10>(
961+
&world,
962+
entities.clone().try_into().unwrap(),
963+
last_change_tick,
964+
change_tick,
965+
)
966+
.is_ok()
967+
});
968+
969+
assert_eq!(
970+
unsafe {
971+
query_state
972+
.get_multiple_unchecked_manual(
973+
&world,
974+
[entities[0], entities[0]],
975+
last_change_tick,
976+
change_tick,
977+
)
978+
.unwrap_err()
979+
},
980+
QueryEntityError::AliasedMutability(entities[0])
981+
);
982+
983+
assert_eq!(
984+
unsafe {
985+
query_state
986+
.get_multiple_unchecked_manual(
987+
&world,
988+
[entities[0], entities[1], entities[0]],
989+
last_change_tick,
990+
change_tick,
991+
)
992+
.unwrap_err()
993+
},
994+
QueryEntityError::AliasedMutability(entities[0])
995+
);
996+
997+
assert_eq!(
998+
unsafe {
999+
query_state
1000+
.get_multiple_unchecked_manual(
1001+
&world,
1002+
[entities[9], entities[9]],
1003+
last_change_tick,
1004+
change_tick,
1005+
)
1006+
.unwrap_err()
1007+
},
1008+
QueryEntityError::AliasedMutability(entities[9])
1009+
);
1010+
}
1011+
1012+
#[test]
1013+
#[should_panic]
1014+
fn right_world_get() {
1015+
let mut world_1 = World::new();
1016+
let world_2 = World::new();
1017+
1018+
let mut query_state = world_1.query::<Entity>();
1019+
let _panics = query_state.get(&world_2, Entity::from_raw(0));
1020+
}
1021+
1022+
#[test]
1023+
#[should_panic]
1024+
fn right_world_get_multiple() {
1025+
let mut world_1 = World::new();
1026+
let world_2 = World::new();
1027+
1028+
let mut query_state = world_1.query::<Entity>();
1029+
let _panics = query_state.get_multiple(&world_2, []);
1030+
}
1031+
1032+
#[test]
1033+
#[should_panic]
1034+
fn right_world_get_multiple_mut() {
1035+
let mut world_1 = World::new();
1036+
let mut world_2 = World::new();
1037+
1038+
let mut query_state = world_1.query::<Entity>();
1039+
let _panics = query_state.get_multiple_mut(&mut world_2, []);
1040+
}
7421041
}

0 commit comments

Comments
 (0)