From 394c5061574108e8cd724223c8a2024624937bd6 Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Mon, 4 Sep 2023 13:25:09 +1000 Subject: [PATCH 01/15] Moved `get_component(_unchecked_mut)` from `Query` to `QueryState` Also had to move `QueryComponentError` to match the new location of its methods. Also performed a QA pass to narrow the scope of `unsafe` blocks within `Query` methods. --- crates/bevy_ecs/src/query/state.rs | 177 +++++++++++++++++++++- crates/bevy_ecs/src/system/mod.rs | 6 +- crates/bevy_ecs/src/system/query.rs | 223 +++++----------------------- 3 files changed, 215 insertions(+), 191 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index c1214c55585db..793b872b5fd7f 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -1,8 +1,9 @@ use crate::{ archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, + change_detection::Mut, component::{ComponentId, Tick}, entity::Entity, - prelude::FromWorld, + prelude::{Component, FromWorld}, query::{ Access, BatchingStrategy, DebugCheckedUnwrap, FilteredAccess, QueryCombinationIter, QueryIter, QueryParIter, WorldQuery, @@ -13,7 +14,7 @@ use crate::{ #[cfg(feature = "trace")] use bevy_utils::tracing::Instrument; use fixedbitset::FixedBitSet; -use std::{borrow::Borrow, fmt, mem::MaybeUninit}; +use std::{any::TypeId, borrow::Borrow, fmt, mem::MaybeUninit}; use super::{NopWorldQuery, QueryManyIter, ROQueryItem, ReadOnlyWorldQuery}; @@ -506,6 +507,85 @@ impl QueryState { } } + /// Returns a shared reference to the component `T` of the given [`Entity`]. + /// + /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is returned instead. + #[inline] + pub(crate) fn get_component<'w, 's, 'r, T: Component>( + &'s self, + world: UnsafeWorldCell<'w>, + entity: Entity, + ) -> Result<&'r T, QueryComponentError> + where + 'w: 'r, + { + let entity_ref = world + .get_entity(entity) + .ok_or(QueryComponentError::NoSuchEntity)?; + let component_id = world + .components() + .get_id(TypeId::of::()) + .ok_or(QueryComponentError::MissingComponent)?; + let archetype_component = entity_ref + .archetype() + .get_archetype_component_id(component_id) + .ok_or(QueryComponentError::MissingComponent)?; + if self + .archetype_component_access + .has_read(archetype_component) + { + // SAFETY: `world` must have access to the component `T` for this entity, + // since it was registered in `self`'s archetype component access set. + unsafe { entity_ref.get::() }.ok_or(QueryComponentError::MissingComponent) + } else { + Err(QueryComponentError::MissingReadAccess) + } + } + + /// Returns a mutable reference to the component `T` of the given entity. + /// + /// In case of a nonexisting entity or mismatched component, a [`QueryComponentError`] is returned instead. + /// + /// # Safety + /// + /// 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 get_component_unchecked_mut<'w, 's, 'r, T: Component>( + &'s self, + world: UnsafeWorldCell<'w>, + entity: Entity, + last_run: Tick, + this_run: Tick, + ) -> Result, QueryComponentError> + where + 'w: 'r, + { + let entity_ref = world + .get_entity(entity) + .ok_or(QueryComponentError::NoSuchEntity)?; + let component_id = world + .components() + .get_id(TypeId::of::()) + .ok_or(QueryComponentError::MissingComponent)?; + let archetype_component = entity_ref + .archetype() + .get_archetype_component_id(component_id) + .ok_or(QueryComponentError::MissingComponent)?; + if self + .archetype_component_access + .has_write(archetype_component) + { + // SEMI-SAFETY: It is the responsibility of the caller to ensure it is sound to get a + // mutable reference to this entity's component `T`. + let result = unsafe { entity_ref.get_mut_using_ticks::(last_run, this_run) }; + + result.ok_or(QueryComponentError::MissingComponent) + } else { + Err(QueryComponentError::MissingWriteAccess) + } + } + /// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and /// the current change tick are given. /// @@ -1343,6 +1423,99 @@ impl fmt::Display for QueryEntityError { } } +/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`](crate::system::Query). +#[derive(Debug, PartialEq, Eq)] +pub enum QueryComponentError { + /// The [`Query`](crate::system::Query) does not have read access to the requested component. + /// + /// This error occurs when the requested component is not included in the original query. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::{prelude::*, query::QueryComponentError}; + /// # + /// # #[derive(Component)] + /// # struct OtherComponent; + /// # + /// # #[derive(Component, PartialEq, Debug)] + /// # struct RequestedComponent; + /// # + /// # #[derive(Resource)] + /// # struct SpecificEntity { + /// # entity: Entity, + /// # } + /// # + /// fn get_missing_read_access_error(query: Query<&OtherComponent>, res: Res) { + /// assert_eq!( + /// query.get_component::(res.entity), + /// Err(QueryComponentError::MissingReadAccess), + /// ); + /// println!("query doesn't have read access to RequestedComponent because it does not appear in Query<&OtherComponent>"); + /// } + /// # bevy_ecs::system::assert_is_system(get_missing_read_access_error); + /// ``` + MissingReadAccess, + /// The [`Query`](crate::system::Query) does not have write access to the requested component. + /// + /// This error occurs when the requested component is not included in the original query, or the mutability of the requested component is mismatched with the original query. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::{prelude::*, query::QueryComponentError}; + /// # + /// # #[derive(Component, PartialEq, Debug)] + /// # struct RequestedComponent; + /// # + /// # #[derive(Resource)] + /// # struct SpecificEntity { + /// # entity: Entity, + /// # } + /// # + /// fn get_missing_write_access_error(mut query: Query<&RequestedComponent>, res: Res) { + /// assert_eq!( + /// query.get_component::(res.entity), + /// Err(QueryComponentError::MissingWriteAccess), + /// ); + /// println!("query doesn't have write access to RequestedComponent because it doesn't have &mut in Query<&RequestedComponent>"); + /// } + /// # bevy_ecs::system::assert_is_system(get_missing_write_access_error); + /// ``` + MissingWriteAccess, + /// The given [`Entity`] does not have the requested component. + MissingComponent, + /// The requested [`Entity`] does not exist. + NoSuchEntity, +} + +impl std::error::Error for QueryComponentError {} + +impl std::fmt::Display for QueryComponentError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + QueryComponentError::MissingReadAccess => { + write!( + f, + "This query does not have read access to the requested component." + ) + } + QueryComponentError::MissingWriteAccess => { + write!( + f, + "This query does not have write access to the requested component." + ) + } + QueryComponentError::MissingComponent => { + write!(f, "The given entity does not have the requested component.") + } + QueryComponentError::NoSuchEntity => { + write!(f, "The requested entity does not exist.") + } + } + } +} + #[cfg(test)] mod tests { use crate::{prelude::*, query::QueryEntityError}; diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index fd4b3027c4b90..ac2887c36ee3d 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -548,8 +548,8 @@ mod tests { Schedule, }, system::{ - Commands, In, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, - QueryComponentError, Res, ResMut, Resource, System, SystemState, + Commands, In, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, Res, ResMut, + Resource, System, SystemState, }, world::{FromWorld, World}, }; @@ -1759,6 +1759,8 @@ mod tests { #[test] fn readonly_query_get_mut_component_fails() { + use crate::query::QueryComponentError; + let mut world = World::new(); let entity = world.spawn(W(42u32)).id(); run_system(&mut world, move |q: Query<&mut W>| { diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 9fbad21a291ce..a718cd8a68f9c 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -2,12 +2,13 @@ use crate::{ component::{Component, Tick}, entity::Entity, query::{ - BatchingStrategy, QueryCombinationIter, QueryEntityError, QueryIter, QueryManyIter, - QueryParIter, QuerySingleError, QueryState, ROQueryItem, ReadOnlyWorldQuery, WorldQuery, + BatchingStrategy, QueryCombinationIter, QueryComponentError, QueryEntityError, QueryIter, + QueryManyIter, QueryParIter, QuerySingleError, QueryState, ROQueryItem, ReadOnlyWorldQuery, + WorldQuery, }, world::{unsafe_world_cell::UnsafeWorldCell, Mut}, }; -use std::{any::TypeId, borrow::Borrow, fmt::Debug}; +use std::borrow::Borrow; /// [System parameter] that provides selective access to the [`Component`] data stored in a [`World`]. /// @@ -425,14 +426,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`for_each`](Self::for_each) for the closure based alternative. #[inline] pub fn iter(&self) -> QueryIter<'_, 's, Q::ReadOnly, F::ReadOnly> { + let state = self.state.as_readonly(); + // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. - unsafe { - self.state - .as_readonly() - .iter_unchecked_manual(self.world, self.last_run, self.this_run) - } + unsafe { state.iter_unchecked_manual(self.world, self.last_run, self.this_run) } } /// Returns an [`Iterator`] over the query items. @@ -491,15 +490,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations( &self, ) -> QueryCombinationIter<'_, 's, Q::ReadOnly, F::ReadOnly, K> { + let state = self.state.as_readonly(); + // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. unsafe { - self.state.as_readonly().iter_combinations_unchecked_manual( - self.world, - self.last_run, - self.this_run, - ) + state.iter_combinations_unchecked_manual(self.world, self.last_run, self.this_run) } } @@ -577,16 +574,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { + let state = self.state.as_readonly(); + // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. unsafe { - self.state.as_readonly().iter_many_unchecked_manual( - entities, - self.world, - self.last_run, - self.this_run, - ) + state.iter_many_unchecked_manual(entities, self.world, self.last_run, self.this_run) } } @@ -733,16 +727,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter`](Self::iter) for the iterator based alternative. #[inline] pub fn for_each<'this>(&'this self, f: impl FnMut(ROQueryItem<'this, Q>)) { + let state = self.state.as_readonly(); + // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. unsafe { - self.state.as_readonly().for_each_unchecked_manual( - self.world, - f, - self.last_run, - self.this_run, - ); + state.for_each_unchecked_manual(self.world, f, self.last_run, self.this_run); }; } @@ -846,16 +837,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_mut`](Self::get_mut) to get a mutable query item. #[inline] pub fn get(&self, entity: Entity) -> Result, QueryEntityError> { + let state = self.state.as_readonly(); + // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict - unsafe { - self.state.as_readonly().get_unchecked_manual( - self.world, - entity, - self.last_run, - self.this_run, - ) - } + unsafe { state.get_unchecked_manual(self.world, entity, self.last_run, self.this_run) } } /// Returns the read-only query items for the given array of [`Entity`]. @@ -1094,29 +1080,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_component_mut`](Self::get_component_mut) to get a mutable reference of a component. #[inline] pub fn get_component(&self, entity: Entity) -> Result<&T, QueryComponentError> { - let world = self.world; - let entity_ref = world - .get_entity(entity) - .ok_or(QueryComponentError::NoSuchEntity)?; - let component_id = world - .components() - .get_id(TypeId::of::()) - .ok_or(QueryComponentError::MissingComponent)?; - let archetype_component = entity_ref - .archetype() - .get_archetype_component_id(component_id) - .ok_or(QueryComponentError::MissingComponent)?; - if self - .state - .archetype_component_access - .has_read(archetype_component) - { - // SAFETY: `self.world` must have access to the component `T` for this entity, - // since it was registered in `self.state`'s archetype component access set. - unsafe { entity_ref.get::() }.ok_or(QueryComponentError::MissingComponent) - } else { - Err(QueryComponentError::MissingReadAccess) - } + self.state.get_component(self.world, entity) } /// Returns a mutable reference to the component `T` of the given entity. @@ -1172,33 +1136,15 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { &self, entity: Entity, ) -> Result, QueryComponentError> { - // SAFETY: this check is required to ensure soundness in the case of `to_readonly().get_component_mut()` - // See the comments on the `force_read_only_component_access` field for more info. if self.force_read_only_component_access { return Err(QueryComponentError::MissingWriteAccess); } - let world = self.world; - let entity_ref = world - .get_entity(entity) - .ok_or(QueryComponentError::NoSuchEntity)?; - let component_id = world - .components() - .get_id(TypeId::of::()) - .ok_or(QueryComponentError::MissingComponent)?; - let archetype_component = entity_ref - .archetype() - .get_archetype_component_id(component_id) - .ok_or(QueryComponentError::MissingComponent)?; - if self - .state - .archetype_component_access - .has_write(archetype_component) - { - entity_ref - .get_mut_using_ticks::(self.last_run, self.this_run) - .ok_or(QueryComponentError::MissingComponent) - } else { - Err(QueryComponentError::MissingWriteAccess) + + // SAFETY: this check is required to ensure soundness in the case of `to_readonly().get_component_mut()` + // See the comments on the `force_read_only_component_access` field for more info. + unsafe { + self.state + .get_component_unchecked_mut(self.world, entity, self.last_run, self.this_run) } } @@ -1265,16 +1211,12 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`single`](Self::single) for the panicking version. #[inline] pub fn get_single(&self) -> Result, QuerySingleError> { + let state = self.state.as_readonly(); + // SAFETY: // the query ensures that the components it accesses are not mutably accessible somewhere else // and the query is read only. - unsafe { - self.state.as_readonly().get_single_unchecked_manual( - self.world, - self.last_run, - self.this_run, - ) - } + unsafe { state.get_single_unchecked_manual(self.world, self.last_run, self.this_run) } } /// Returns a single query item when there is exactly one entity matching the query. @@ -1401,13 +1343,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn contains(&self, entity: Entity) -> bool { - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access - unsafe { - self.state - .as_nop() - .get_unchecked_manual(self.world, entity, self.last_run, self.this_run) - .is_ok() - } + let state = self.state.as_nop(); + + let result = + // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access + unsafe { state.get_unchecked_manual(self.world, entity, self.last_run, self.this_run) }; + + result.is_ok() } } @@ -1429,99 +1371,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> IntoIterator for &'w mut Quer } } -/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`]. -#[derive(Debug, PartialEq, Eq)] -pub enum QueryComponentError { - /// The [`Query`] does not have read access to the requested component. - /// - /// This error occurs when the requested component is not included in the original query. - /// - /// # Example - /// - /// ``` - /// # use bevy_ecs::{prelude::*, system::QueryComponentError}; - /// # - /// # #[derive(Component)] - /// # struct OtherComponent; - /// # - /// # #[derive(Component, PartialEq, Debug)] - /// # struct RequestedComponent; - /// # - /// # #[derive(Resource)] - /// # struct SpecificEntity { - /// # entity: Entity, - /// # } - /// # - /// fn get_missing_read_access_error(query: Query<&OtherComponent>, res: Res) { - /// assert_eq!( - /// query.get_component::(res.entity), - /// Err(QueryComponentError::MissingReadAccess), - /// ); - /// println!("query doesn't have read access to RequestedComponent because it does not appear in Query<&OtherComponent>"); - /// } - /// # bevy_ecs::system::assert_is_system(get_missing_read_access_error); - /// ``` - MissingReadAccess, - /// The [`Query`] does not have write access to the requested component. - /// - /// This error occurs when the requested component is not included in the original query, or the mutability of the requested component is mismatched with the original query. - /// - /// # Example - /// - /// ``` - /// # use bevy_ecs::{prelude::*, system::QueryComponentError}; - /// # - /// # #[derive(Component, PartialEq, Debug)] - /// # struct RequestedComponent; - /// # - /// # #[derive(Resource)] - /// # struct SpecificEntity { - /// # entity: Entity, - /// # } - /// # - /// fn get_missing_write_access_error(mut query: Query<&RequestedComponent>, res: Res) { - /// assert_eq!( - /// query.get_component::(res.entity), - /// Err(QueryComponentError::MissingWriteAccess), - /// ); - /// println!("query doesn't have write access to RequestedComponent because it doesn't have &mut in Query<&RequestedComponent>"); - /// } - /// # bevy_ecs::system::assert_is_system(get_missing_write_access_error); - /// ``` - MissingWriteAccess, - /// The given [`Entity`] does not have the requested component. - MissingComponent, - /// The requested [`Entity`] does not exist. - NoSuchEntity, -} - -impl std::error::Error for QueryComponentError {} - -impl std::fmt::Display for QueryComponentError { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - match self { - QueryComponentError::MissingReadAccess => { - write!( - f, - "This query does not have read access to the requested component." - ) - } - QueryComponentError::MissingWriteAccess => { - write!( - f, - "This query does not have write access to the requested component." - ) - } - QueryComponentError::MissingComponent => { - write!(f, "The given entity does not have the requested component.") - } - QueryComponentError::NoSuchEntity => { - write!(f, "The requested entity does not exist.") - } - } - } -} - impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// Returns the query item for the given [`Entity`], with the actual "inner" world lifetime. /// From 7b3ebb7b37f3a704cec9fd7a6809c219c88101db Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Tue, 5 Sep 2023 09:10:01 +1000 Subject: [PATCH 02/15] Fixed Safety Comment on `get_component_unchecked_mut` --- crates/bevy_ecs/src/system/query.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index a718cd8a68f9c..fdc42e21e95c5 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1136,12 +1136,14 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { &self, entity: Entity, ) -> Result, QueryComponentError> { + // This check is required to ensure soundness in the case of `to_readonly().get_component_mut()` + // See the comments on the `force_read_only_component_access` field for more info. if self.force_read_only_component_access { return Err(QueryComponentError::MissingWriteAccess); } - // SAFETY: this check is required to ensure soundness in the case of `to_readonly().get_component_mut()` - // See the comments on the `force_read_only_component_access` field for more info. + // SAFETY: The above check ensures we are not a readonly query. + // It is the callers responsibility to ensure multiple mutable access is not provided. unsafe { self.state .get_component_unchecked_mut(self.world, entity, self.last_run, self.this_run) From dd0a5eaf008446ce10b954fae6b554c67010090a Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Tue, 5 Sep 2023 09:39:24 +1000 Subject: [PATCH 03/15] Formatting Changes --- crates/bevy_ecs/src/system/query.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 8386f6567c8a7..e92fd8c99046c 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -8,7 +8,7 @@ use crate::{ }, world::{unsafe_world_cell::UnsafeWorldCell, Mut}, }; -use std::borrow::Borrow; +use std::{any::TypeId, borrow::Borrow}; /// [System parameter] that provides selective access to the [`Component`] data stored in a [`World`]. /// @@ -1395,11 +1395,18 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn contains(&self, entity: Entity) -> bool { - let state = self.state.as_nop(); + let Self { + world, + last_run, + this_run, + state, + .. + } = self; + + let state = state.as_nop(); - let result = - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access - unsafe { state.get_unchecked_manual(self.world, entity, self.last_run, self.this_run) }; + // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access + let result = unsafe { state.get_unchecked_manual(*world, entity, *last_run, *this_run) }; result.is_ok() } From 15f2a613fa02fe71cb3932f66f4201a530e3c4ac Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Tue, 5 Sep 2023 10:21:58 +1000 Subject: [PATCH 04/15] Method Parity Between `Query` and `QueryState` Added to `QueryState`: * `component` * `component_unchecked_mut` * `many_read_only_manual` * `many_unchecked_manual` --- crates/bevy_ecs/src/query/state.rs | 104 ++++++++++++++++++++++++++++ crates/bevy_ecs/src/system/query.rs | 69 ++++++++++++------ 2 files changed, 152 insertions(+), 21 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 793b872b5fd7f..73c1956b705ea 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -542,6 +542,31 @@ impl QueryState { } } + /// Returns a shared reference to the component `T` of the given [`Entity`]. + /// + /// # Panics + /// + /// If given a nonexisting entity or mismatched component, this will panic. + #[inline] + pub(crate) fn component<'w, 's, 'r, T: Component>( + &'s self, + world: UnsafeWorldCell<'w>, + entity: Entity, + ) -> &'r T + where + 'w: 'r, + { + match self.get_component(world, entity) { + Ok(component) => component, + Err(error) => { + panic!( + "Cannot get component `{:?}` from {entity:?}: {error}", + TypeId::of::() + ) + } + } + } + /// Returns a mutable reference to the component `T` of the given entity. /// /// In case of a nonexisting entity or mismatched component, a [`QueryComponentError`] is returned instead. @@ -586,6 +611,38 @@ impl QueryState { } } + /// Returns a mutable reference to the component `T` of the given entity. + /// + /// # Panics + /// + /// Panics in case of a nonexisting entity or mismatched component. + /// + /// # Safety + /// + /// 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(crate) unsafe fn component_unchecked_mut<'w, 's, 'r, T: Component>( + &'s self, + world: UnsafeWorldCell<'w>, + entity: Entity, + last_run: Tick, + this_run: Tick, + ) -> Mut<'r, T> + where + 'w: 'r, + { + match self.get_component_unchecked_mut(world, entity, last_run, this_run) { + Ok(component) => component, + Err(error) => { + panic!( + "Cannot get component `{:?}` from {entity:?}: {error}", + TypeId::of::() + ) + } + } + } + /// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and /// the current change tick are given. /// @@ -618,6 +675,28 @@ impl QueryState { Ok(values.map(|x| x.assume_init())) } + /// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and + /// the current change tick are given. + /// + /// # Panics + /// + /// Panics if any item cannot be retrieved. + /// + /// # Safety + /// + /// This must be called on the same `World` that the `Query` was generated from: + /// use `QueryState::validate_world` to verify this. + pub(crate) unsafe fn many_read_only_manual<'w, const N: usize>( + &self, + world: &'w World, + entities: [Entity; N], + last_run: Tick, + this_run: Tick, + ) -> [ROQueryItem<'w, Q>; N] { + self.get_many_read_only_manual(world, entities, last_run, this_run) + .unwrap() + } + /// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and /// the current change tick are given. /// @@ -655,6 +734,31 @@ impl QueryState { Ok(values.map(|x| x.assume_init())) } + /// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and + /// the current change tick are given. + /// + /// # Panics + /// + /// Panics if any item cannot be retrieved. + /// + /// # Safety + /// + /// This does not check for unique access to subsets of the entity-component data. + /// To be safe, make sure mutable queries have unique access to the components they query. + /// + /// This must be called on the same `World` that the `Query` was generated from: + /// use `QueryState::validate_world` to verify this. + pub(crate) unsafe fn many_unchecked_manual<'w, const N: usize>( + &self, + world: UnsafeWorldCell<'w>, + entities: [Entity; N], + last_run: Tick, + this_run: Tick, + ) -> [Q::Item<'w>; N] { + self.get_many_unchecked_manual(world, entities, last_run, this_run) + .unwrap() + } + /// Returns an [`Iterator`] over the query results for the given [`World`]. /// /// This can only be called for read-only queries, see [`Self::iter_mut`] for write-queries. diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index e92fd8c99046c..7154fe93eb1ff 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -8,7 +8,7 @@ use crate::{ }, world::{unsafe_world_cell::UnsafeWorldCell, Mut}, }; -use std::{any::TypeId, borrow::Borrow}; +use std::borrow::Borrow; /// [System parameter] that provides selective access to the [`Component`] data stored in a [`World`]. /// @@ -913,7 +913,15 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_many`](Self::get_many) for the non-panicking version. #[inline] pub fn many(&self, entities: [Entity; N]) -> [ROQueryItem<'_, Q>; N] { - self.get_many(entities).unwrap() + // SAFETY: it is the scheduler's responsibility to ensure that `Query` is never handed out on the wrong `World`. + unsafe { + self.state.many_read_only_manual( + self.world.unsafe_world(), + entities, + self.last_run, + self.this_run, + ) + } } /// Returns the query item for the given [`Entity`]. @@ -1024,7 +1032,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`many`](Self::many) to get read-only query items. #[inline] pub fn many_mut(&mut self, entities: [Entity; N]) -> [Q::Item<'_>; N] { - self.get_many_mut(entities).unwrap() + // SAFETY: scheduler ensures safe Query world access + unsafe { + self.state + .many_unchecked_manual(self.world, entities, self.last_run, self.this_run) + } } /// Returns the query item for the given [`Entity`]. @@ -1134,15 +1146,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { #[inline] #[track_caller] pub fn component(&self, entity: Entity) -> &T { - match self.get_component(entity) { - Ok(component) => component, - Err(error) => { - panic!( - "Cannot get component `{:?}` from {entity:?}: {error}", - TypeId::of::() - ) - } - } + self.state.component(self.world, entity) } /// Returns a mutable reference to the component `T` of the given entity. @@ -1158,15 +1162,8 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { #[inline] #[track_caller] pub fn component_mut(&mut self, entity: Entity) -> Mut<'_, T> { - match self.get_component_mut(entity) { - Ok(component) => component, - Err(error) => { - panic!( - "Cannot get component `{:?}` from {entity:?}: {error}", - TypeId::of::() - ) - } - } + // SAFETY: unique access to query (preventing aliased access) + unsafe { self.component_unchecked_mut(entity) } } /// Returns a mutable reference to the component `T` of the given entity. @@ -1200,6 +1197,36 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } + /// Returns a mutable reference to the component `T` of the given entity. + /// + /// # Panics + /// + /// Panics in case of a nonexisting entity or mismatched component. + /// + /// # Safety + /// + /// 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. + /// + /// # See also + /// + /// - [`get_component_mut`](Self::get_component_mut) for the safe version. + #[inline] + pub unsafe fn component_unchecked_mut(&self, entity: Entity) -> Mut<'_, T> { + // This check is required to ensure soundness in the case of `to_readonly().get_component_mut()` + // See the comments on the `force_read_only_component_access` field for more info. + if self.force_read_only_component_access { + panic!("Missing write access"); + } + + // SAFETY: The above check ensures we are not a readonly query. + // It is the callers responsibility to ensure multiple mutable access is not provided. + unsafe { + self.state + .component_unchecked_mut(self.world, entity, self.last_run, self.this_run) + } + } + /// Returns a single read-only query item when there is exactly one entity matching the query. /// /// # Panics From cc476b5ba0cd0e66f7a54af1d747485ea4f0f90d Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Wed, 6 Sep 2023 08:45:51 +1000 Subject: [PATCH 05/15] Revert Noisy Changes --- crates/bevy_ecs/src/system/query.rs | 63 +++++++++++++++++------------ 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 7154fe93eb1ff..f01c77a1d6284 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -431,7 +431,11 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. - unsafe { state.iter_unchecked_manual(self.world, self.last_run, self.this_run) } + unsafe { + self.state + .as_readonly() + .iter_unchecked_manual(self.world, self.last_run, self.this_run) + } } /// Returns an [`Iterator`] over the query items. @@ -490,13 +494,15 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { pub fn iter_combinations( &self, ) -> QueryCombinationIter<'_, 's, Q::ReadOnly, F::ReadOnly, K> { - let state = self.state.as_readonly(); - // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. unsafe { - state.iter_combinations_unchecked_manual(self.world, self.last_run, self.this_run) + self.state.as_readonly().iter_combinations_unchecked_manual( + self.world, + self.last_run, + self.this_run, + ) } } @@ -574,13 +580,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { where EntityList::Item: Borrow, { - let state = self.state.as_readonly(); - // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. unsafe { - state.iter_many_unchecked_manual(entities, self.world, self.last_run, self.this_run) + self.state.as_readonly().iter_many_unchecked_manual( + entities, + self.world, + self.last_run, + self.this_run, + ) } } @@ -837,11 +846,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_mut`](Self::get_mut) to get a mutable query item. #[inline] pub fn get(&self, entity: Entity) -> Result, QueryEntityError> { - let state = self.state.as_readonly(); - // SAFETY: system runs without conflicts with other systems. // same-system queries have runtime borrow checks when they conflict - unsafe { state.get_unchecked_manual(self.world, entity, self.last_run, self.this_run) } + unsafe { + self.state.as_readonly().get_unchecked_manual( + self.world, + entity, + self.last_run, + self.this_run, + ) + } } /// Returns the read-only query items for the given array of [`Entity`]. @@ -1290,12 +1304,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`single`](Self::single) for the panicking version. #[inline] pub fn get_single(&self) -> Result, QuerySingleError> { - let state = self.state.as_readonly(); - // SAFETY: // the query ensures that the components it accesses are not mutably accessible somewhere else // and the query is read only. - unsafe { state.get_single_unchecked_manual(self.world, self.last_run, self.this_run) } + unsafe { + self.state.as_readonly().get_single_unchecked_manual( + self.world, + self.last_run, + self.this_run, + ) + } } /// Returns a single query item when there is exactly one entity matching the query. @@ -1422,20 +1440,13 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// ``` #[inline] pub fn contains(&self, entity: Entity) -> bool { - let Self { - world, - last_run, - this_run, - state, - .. - } = self; - - let state = state.as_nop(); - // SAFETY: NopFetch does not access any members while &self ensures no one has exclusive access - let result = unsafe { state.get_unchecked_manual(*world, entity, *last_run, *this_run) }; - - result.is_ok() + unsafe { + self.state + .as_nop() + .get_unchecked_manual(self.world, entity, self.last_run, self.this_run) + .is_ok() + } } } From 4e16b0b248598b1cea27e392b17daae10d4553de Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Wed, 6 Sep 2023 08:46:29 +1000 Subject: [PATCH 06/15] Fix Missed Revert --- crates/bevy_ecs/src/system/query.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index f01c77a1d6284..0690e222e6066 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -426,8 +426,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`for_each`](Self::for_each) for the closure based alternative. #[inline] pub fn iter(&self) -> QueryIter<'_, 's, Q::ReadOnly, F::ReadOnly> { - let state = self.state.as_readonly(); - // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. From 492f20908657552d14ead13b00053c9a5b3f16bf Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Wed, 6 Sep 2023 09:39:07 +1000 Subject: [PATCH 07/15] Reverted Noisy Change --- crates/bevy_ecs/src/system/query.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 0690e222e6066..58ee03a156f05 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -734,13 +734,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`iter`](Self::iter) for the iterator based alternative. #[inline] pub fn for_each<'this>(&'this self, f: impl FnMut(ROQueryItem<'this, Q>)) { - let state = self.state.as_readonly(); - // SAFETY: // - `self.world` has permission to access the required components. // - The query is read-only, so it can be aliased even if it was originally mutable. unsafe { - state.for_each_unchecked_manual(self.world, f, self.last_run, self.this_run); + self.state.as_readonly().for_each_unchecked_manual( + self.world, + f, + self.last_run, + self.this_run, + ); }; } From 36bbfa2d122c59b3e5935a72a494d44638c07f5e Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Wed, 6 Sep 2023 10:17:23 +1000 Subject: [PATCH 08/15] Moved `QueryComponentError` and `QueryEntityError` into their own Module --- crates/bevy_ecs/src/query/error.rs | 128 ++++++++++++++++++++++++++++ crates/bevy_ecs/src/query/mod.rs | 2 + crates/bevy_ecs/src/query/state.rs | 130 +---------------------------- 3 files changed, 134 insertions(+), 126 deletions(-) create mode 100644 crates/bevy_ecs/src/query/error.rs diff --git a/crates/bevy_ecs/src/query/error.rs b/crates/bevy_ecs/src/query/error.rs new file mode 100644 index 0000000000000..9788f4c3d91c2 --- /dev/null +++ b/crates/bevy_ecs/src/query/error.rs @@ -0,0 +1,128 @@ +use std::fmt; + +use crate::entity::Entity; + +/// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`]. +// TODO: return the type_name as part of this error +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub enum QueryEntityError { + /// The given [`Entity`]'s components do not match the query. + /// + /// Either it does not have a requested component, or it has a component which the query filters out. + QueryDoesNotMatch(Entity), + /// The given [`Entity`] does not exist. + NoSuchEntity(Entity), + /// The [`Entity`] was requested mutably more than once. + /// + /// See [`QueryState::get_many_mut`] for an example. + AliasedMutability(Entity), +} + +impl std::error::Error for QueryEntityError {} + +impl fmt::Display for QueryEntityError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + QueryEntityError::QueryDoesNotMatch(_) => { + write!(f, "The given entity's components do not match the query.") + } + QueryEntityError::NoSuchEntity(_) => write!(f, "The requested entity does not exist."), + QueryEntityError::AliasedMutability(_) => { + write!(f, "The entity was requested mutably more than once.") + } + } + } +} + +/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`](crate::system::Query). +#[derive(Debug, PartialEq, Eq)] +pub enum QueryComponentError { + /// The [`Query`](crate::system::Query) does not have read access to the requested component. + /// + /// This error occurs when the requested component is not included in the original query. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::{prelude::*, query::QueryComponentError}; + /// # + /// # #[derive(Component)] + /// # struct OtherComponent; + /// # + /// # #[derive(Component, PartialEq, Debug)] + /// # struct RequestedComponent; + /// # + /// # #[derive(Resource)] + /// # struct SpecificEntity { + /// # entity: Entity, + /// # } + /// # + /// fn get_missing_read_access_error(query: Query<&OtherComponent>, res: Res) { + /// assert_eq!( + /// query.get_component::(res.entity), + /// Err(QueryComponentError::MissingReadAccess), + /// ); + /// println!("query doesn't have read access to RequestedComponent because it does not appear in Query<&OtherComponent>"); + /// } + /// # bevy_ecs::system::assert_is_system(get_missing_read_access_error); + /// ``` + MissingReadAccess, + /// The [`Query`](crate::system::Query) does not have write access to the requested component. + /// + /// This error occurs when the requested component is not included in the original query, or the mutability of the requested component is mismatched with the original query. + /// + /// # Example + /// + /// ``` + /// # use bevy_ecs::{prelude::*, query::QueryComponentError}; + /// # + /// # #[derive(Component, PartialEq, Debug)] + /// # struct RequestedComponent; + /// # + /// # #[derive(Resource)] + /// # struct SpecificEntity { + /// # entity: Entity, + /// # } + /// # + /// fn get_missing_write_access_error(mut query: Query<&RequestedComponent>, res: Res) { + /// assert_eq!( + /// query.get_component::(res.entity), + /// Err(QueryComponentError::MissingWriteAccess), + /// ); + /// println!("query doesn't have write access to RequestedComponent because it doesn't have &mut in Query<&RequestedComponent>"); + /// } + /// # bevy_ecs::system::assert_is_system(get_missing_write_access_error); + /// ``` + MissingWriteAccess, + /// The given [`Entity`] does not have the requested component. + MissingComponent, + /// The requested [`Entity`] does not exist. + NoSuchEntity, +} + +impl std::error::Error for QueryComponentError {} + +impl std::fmt::Display for QueryComponentError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + QueryComponentError::MissingReadAccess => { + write!( + f, + "This query does not have read access to the requested component." + ) + } + QueryComponentError::MissingWriteAccess => { + write!( + f, + "This query does not have write access to the requested component." + ) + } + QueryComponentError::MissingComponent => { + write!(f, "The given entity does not have the requested component.") + } + QueryComponentError::NoSuchEntity => { + write!(f, "The requested entity does not exist.") + } + } + } +} diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 863122d90a8b6..c30292cb76a2d 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -1,6 +1,7 @@ //! Contains APIs for retrieving component data from the world. mod access; +mod error; mod fetch; mod filter; mod iter; @@ -8,6 +9,7 @@ mod par_iter; mod state; pub use access::*; +pub use error::*; pub use fetch::*; pub use filter::*; pub use iter::*; diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 73c1956b705ea..9a240f284008b 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -16,7 +16,10 @@ use bevy_utils::tracing::Instrument; use fixedbitset::FixedBitSet; use std::{any::TypeId, borrow::Borrow, fmt, mem::MaybeUninit}; -use super::{NopWorldQuery, QueryManyIter, ROQueryItem, ReadOnlyWorldQuery}; +use super::{ + NopWorldQuery, QueryComponentError, QueryEntityError, QueryManyIter, ROQueryItem, + ReadOnlyWorldQuery, +}; /// Provides scoped access to a [`World`] state according to a given [`WorldQuery`] and query filter. #[repr(C)] @@ -1495,131 +1498,6 @@ impl QueryState { } } -/// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`]. -// TODO: return the type_name as part of this error -#[derive(Debug, PartialEq, Eq, Clone, Copy)] -pub enum QueryEntityError { - /// The given [`Entity`]'s components do not match the query. - /// - /// Either it does not have a requested component, or it has a component which the query filters out. - QueryDoesNotMatch(Entity), - /// The given [`Entity`] does not exist. - NoSuchEntity(Entity), - /// The [`Entity`] was requested mutably more than once. - /// - /// See [`QueryState::get_many_mut`] for an example. - AliasedMutability(Entity), -} - -impl std::error::Error for QueryEntityError {} - -impl fmt::Display for QueryEntityError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - QueryEntityError::QueryDoesNotMatch(_) => { - write!(f, "The given entity's components do not match the query.") - } - QueryEntityError::NoSuchEntity(_) => write!(f, "The requested entity does not exist."), - QueryEntityError::AliasedMutability(_) => { - write!(f, "The entity was requested mutably more than once.") - } - } - } -} - -/// An error that occurs when retrieving a specific [`Entity`]'s component from a [`Query`](crate::system::Query). -#[derive(Debug, PartialEq, Eq)] -pub enum QueryComponentError { - /// The [`Query`](crate::system::Query) does not have read access to the requested component. - /// - /// This error occurs when the requested component is not included in the original query. - /// - /// # Example - /// - /// ``` - /// # use bevy_ecs::{prelude::*, query::QueryComponentError}; - /// # - /// # #[derive(Component)] - /// # struct OtherComponent; - /// # - /// # #[derive(Component, PartialEq, Debug)] - /// # struct RequestedComponent; - /// # - /// # #[derive(Resource)] - /// # struct SpecificEntity { - /// # entity: Entity, - /// # } - /// # - /// fn get_missing_read_access_error(query: Query<&OtherComponent>, res: Res) { - /// assert_eq!( - /// query.get_component::(res.entity), - /// Err(QueryComponentError::MissingReadAccess), - /// ); - /// println!("query doesn't have read access to RequestedComponent because it does not appear in Query<&OtherComponent>"); - /// } - /// # bevy_ecs::system::assert_is_system(get_missing_read_access_error); - /// ``` - MissingReadAccess, - /// The [`Query`](crate::system::Query) does not have write access to the requested component. - /// - /// This error occurs when the requested component is not included in the original query, or the mutability of the requested component is mismatched with the original query. - /// - /// # Example - /// - /// ``` - /// # use bevy_ecs::{prelude::*, query::QueryComponentError}; - /// # - /// # #[derive(Component, PartialEq, Debug)] - /// # struct RequestedComponent; - /// # - /// # #[derive(Resource)] - /// # struct SpecificEntity { - /// # entity: Entity, - /// # } - /// # - /// fn get_missing_write_access_error(mut query: Query<&RequestedComponent>, res: Res) { - /// assert_eq!( - /// query.get_component::(res.entity), - /// Err(QueryComponentError::MissingWriteAccess), - /// ); - /// println!("query doesn't have write access to RequestedComponent because it doesn't have &mut in Query<&RequestedComponent>"); - /// } - /// # bevy_ecs::system::assert_is_system(get_missing_write_access_error); - /// ``` - MissingWriteAccess, - /// The given [`Entity`] does not have the requested component. - MissingComponent, - /// The requested [`Entity`] does not exist. - NoSuchEntity, -} - -impl std::error::Error for QueryComponentError {} - -impl std::fmt::Display for QueryComponentError { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - match self { - QueryComponentError::MissingReadAccess => { - write!( - f, - "This query does not have read access to the requested component." - ) - } - QueryComponentError::MissingWriteAccess => { - write!( - f, - "This query does not have write access to the requested component." - ) - } - QueryComponentError::MissingComponent => { - write!(f, "The given entity does not have the requested component.") - } - QueryComponentError::NoSuchEntity => { - write!(f, "The requested entity does not exist.") - } - } - } -} - #[cfg(test)] mod tests { use crate::{prelude::*, query::QueryEntityError}; From 62eb5d92793fda8d76e7bdd9f84e055a1f3684bf Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Wed, 6 Sep 2023 13:21:46 +1000 Subject: [PATCH 09/15] Fixed Documentation Links --- crates/bevy_ecs/src/query/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/error.rs b/crates/bevy_ecs/src/query/error.rs index 9788f4c3d91c2..d7a19a0853355 100644 --- a/crates/bevy_ecs/src/query/error.rs +++ b/crates/bevy_ecs/src/query/error.rs @@ -2,7 +2,7 @@ use std::fmt; use crate::entity::Entity; -/// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`]. +/// An error that occurs when retrieving a specific [`Entity`]'s query result from [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState). // TODO: return the type_name as part of this error #[derive(Debug, PartialEq, Eq, Clone, Copy)] pub enum QueryEntityError { @@ -14,7 +14,7 @@ pub enum QueryEntityError { NoSuchEntity(Entity), /// The [`Entity`] was requested mutably more than once. /// - /// See [`QueryState::get_many_mut`] for an example. + /// See [`QueryState::get_many_mut`](crate::query::QueryState::get_many_mut) for an example. AliasedMutability(Entity), } From fca01f6cf9e6461c89869181e39850b2f6dae9fc Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Thu, 7 Sep 2023 08:43:49 +1000 Subject: [PATCH 10/15] Moved `QuerySingleError` into `error.rs`; Added Richer `unwrap` handling in `state.rs` --- crates/bevy_ecs/src/query/error.rs | 23 ++++++++++++++ crates/bevy_ecs/src/query/state.rs | 49 +++++++++++------------------- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/crates/bevy_ecs/src/query/error.rs b/crates/bevy_ecs/src/query/error.rs index d7a19a0853355..eb2769aecb5c2 100644 --- a/crates/bevy_ecs/src/query/error.rs +++ b/crates/bevy_ecs/src/query/error.rs @@ -126,3 +126,26 @@ impl std::fmt::Display for QueryComponentError { } } } + +/// An error that occurs when evaluating a [`Query`](crate::system::Query) or [`QueryState`](crate::query::QueryState) as a single expected result via +/// [`get_single`](crate::system::Query::get_single) or [`get_single_mut`](crate::system::Query::get_single_mut). +#[derive(Debug)] +pub enum QuerySingleError { + /// No entity fits the query. + NoEntities(&'static str), + /// Multiple entities fit the query. + MultipleEntities(&'static str), +} + +impl std::error::Error for QuerySingleError {} + +impl std::fmt::Display for QuerySingleError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + QuerySingleError::NoEntities(query) => write!(f, "No entities fit the query {query}"), + QuerySingleError::MultipleEntities(query) => { + write!(f, "Multiple entities fit the query {query}!") + } + } + } +} diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 9a240f284008b..013484cabc7e7 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -17,8 +17,8 @@ use fixedbitset::FixedBitSet; use std::{any::TypeId, borrow::Borrow, fmt, mem::MaybeUninit}; use super::{ - NopWorldQuery, QueryComponentError, QueryEntityError, QueryManyIter, ROQueryItem, - ReadOnlyWorldQuery, + NopWorldQuery, QueryComponentError, QueryEntityError, QueryManyIter, QuerySingleError, + ROQueryItem, ReadOnlyWorldQuery, }; /// Provides scoped access to a [`World`] state according to a given [`WorldQuery`] and query filter. @@ -696,8 +696,10 @@ impl QueryState { last_run: Tick, this_run: Tick, ) -> [ROQueryItem<'w, Q>; N] { - self.get_many_read_only_manual(world, entities, last_run, this_run) - .unwrap() + match self.get_many_read_only_manual(world, entities, last_run, this_run) { + Ok(items) => items, + Err(error) => panic!("Cannot get query results: {error}"), + } } /// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and @@ -758,8 +760,10 @@ impl QueryState { last_run: Tick, this_run: Tick, ) -> [Q::Item<'w>; N] { - self.get_many_unchecked_manual(world, entities, last_run, this_run) - .unwrap() + match self.get_many_unchecked_manual(world, entities, last_run, this_run) { + Ok(items) => items, + Err(error) => panic!("Cannot get query result: {error}"), + } } /// Returns an [`Iterator`] over the query results for the given [`World`]. @@ -1383,7 +1387,10 @@ impl QueryState { #[track_caller] #[inline] pub fn single<'w>(&mut self, world: &'w World) -> ROQueryItem<'w, Q> { - self.get_single(world).unwrap() + match self.get_single(world) { + Ok(items) => items, + Err(error) => panic!("Cannot get single mutable query result: {error}"), + } } /// Returns a single immutable query result when there is exactly one entity matching @@ -1422,7 +1429,10 @@ impl QueryState { #[inline] pub fn single_mut<'w>(&mut self, world: &'w mut World) -> Q::Item<'w> { // SAFETY: query has unique world access - self.get_single_mut(world).unwrap() + match self.get_single_mut(world) { + Ok(items) => items, + Err(error) => panic!("Cannot get single query result: {error}"), + } } /// Returns a single mutable query result when there is exactly one entity matching @@ -1606,26 +1616,3 @@ mod tests { let _panics = query_state.get_many_mut(&mut world_2, []); } } - -/// An error that occurs when evaluating a [`Query`](crate::system::Query) or [`QueryState`] as a single expected result via -/// [`get_single`](crate::system::Query::get_single) or [`get_single_mut`](crate::system::Query::get_single_mut). -#[derive(Debug)] -pub enum QuerySingleError { - /// No entity fits the query. - NoEntities(&'static str), - /// Multiple entities fit the query. - MultipleEntities(&'static str), -} - -impl std::error::Error for QuerySingleError {} - -impl std::fmt::Display for QuerySingleError { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - match self { - QuerySingleError::NoEntities(query) => write!(f, "No entities fit the query {query}"), - QuerySingleError::MultipleEntities(query) => { - write!(f, "Multiple entities fit the query {query}!") - } - } - } -} From 3a6c689259149bb5ef52bdd6121bc48d42273a83 Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Fri, 8 Sep 2023 08:12:15 +1000 Subject: [PATCH 11/15] Update crates/bevy_ecs/src/query/state.rs Co-authored-by: Tristan Guichaoua <33934311+tguichaoua@users.noreply.github.com> --- crates/bevy_ecs/src/query/state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 013484cabc7e7..8440c0d44a652 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -604,7 +604,7 @@ impl QueryState { .archetype_component_access .has_write(archetype_component) { - // SEMI-SAFETY: It is the responsibility of the caller to ensure it is sound to get a + // SAFETY: It is the responsibility of the caller to ensure it is sound to get a // mutable reference to this entity's component `T`. let result = unsafe { entity_ref.get_mut_using_ticks::(last_run, this_run) }; From 95ea8c380971dc3a41527cecb8aec0731199d6d8 Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Fri, 8 Sep 2023 08:15:48 +1000 Subject: [PATCH 12/15] Removed `component_unchecked_mut` --- crates/bevy_ecs/src/system/query.rs | 30 ----------------------------- 1 file changed, 30 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 58ee03a156f05..379ed51faf21c 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1212,36 +1212,6 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { } } - /// Returns a mutable reference to the component `T` of the given entity. - /// - /// # Panics - /// - /// Panics in case of a nonexisting entity or mismatched component. - /// - /// # Safety - /// - /// 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. - /// - /// # See also - /// - /// - [`get_component_mut`](Self::get_component_mut) for the safe version. - #[inline] - pub unsafe fn component_unchecked_mut(&self, entity: Entity) -> Mut<'_, T> { - // This check is required to ensure soundness in the case of `to_readonly().get_component_mut()` - // See the comments on the `force_read_only_component_access` field for more info. - if self.force_read_only_component_access { - panic!("Missing write access"); - } - - // SAFETY: The above check ensures we are not a readonly query. - // It is the callers responsibility to ensure multiple mutable access is not provided. - unsafe { - self.state - .component_unchecked_mut(self.world, entity, self.last_run, self.this_run) - } - } - /// Returns a single read-only query item when there is exactly one entity matching the query. /// /// # Panics From b2f27f4b54c12a71d0a2abd68731c916ada4647c Mon Sep 17 00:00:00 2001 From: Zac Harrold Date: Fri, 8 Sep 2023 08:31:07 +1000 Subject: [PATCH 13/15] Follow Up on Consequences --- crates/bevy_ecs/src/system/query.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 379ed51faf21c..f2120f4b3ccc7 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1178,7 +1178,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { #[track_caller] pub fn component_mut(&mut self, entity: Entity) -> Mut<'_, T> { // SAFETY: unique access to query (preventing aliased access) - unsafe { self.component_unchecked_mut(entity) } + unsafe { + self.state + .component_unchecked_mut(self.world, entity, self.last_run, self.this_run) + } } /// Returns a mutable reference to the component `T` of the given entity. From f3dadd9e057656afe01c37ff51da33fd3b0cf2da Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Fri, 8 Sep 2023 20:42:47 +1000 Subject: [PATCH 14/15] Removed Potentially Extraneous `unsafe` `QueryState` Methods --- crates/bevy_ecs/src/query/state.rs | 83 ----------------------------- crates/bevy_ecs/src/system/query.rs | 33 ++++++------ 2 files changed, 17 insertions(+), 99 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 8440c0d44a652..45115c2d540f5 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -614,38 +614,6 @@ impl QueryState { } } - /// Returns a mutable reference to the component `T` of the given entity. - /// - /// # Panics - /// - /// Panics in case of a nonexisting entity or mismatched component. - /// - /// # Safety - /// - /// 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(crate) unsafe fn component_unchecked_mut<'w, 's, 'r, T: Component>( - &'s self, - world: UnsafeWorldCell<'w>, - entity: Entity, - last_run: Tick, - this_run: Tick, - ) -> Mut<'r, T> - where - 'w: 'r, - { - match self.get_component_unchecked_mut(world, entity, last_run, this_run) { - Ok(component) => component, - Err(error) => { - panic!( - "Cannot get component `{:?}` from {entity:?}: {error}", - TypeId::of::() - ) - } - } - } - /// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and /// the current change tick are given. /// @@ -678,30 +646,6 @@ impl QueryState { Ok(values.map(|x| x.assume_init())) } - /// Gets the read-only query results for the given [`World`] and array of [`Entity`], where the last change and - /// the current change tick are given. - /// - /// # Panics - /// - /// Panics if any item cannot be retrieved. - /// - /// # Safety - /// - /// This must be called on the same `World` that the `Query` was generated from: - /// use `QueryState::validate_world` to verify this. - pub(crate) unsafe fn many_read_only_manual<'w, const N: usize>( - &self, - world: &'w World, - entities: [Entity; N], - last_run: Tick, - this_run: Tick, - ) -> [ROQueryItem<'w, Q>; N] { - match self.get_many_read_only_manual(world, entities, last_run, this_run) { - Ok(items) => items, - Err(error) => panic!("Cannot get query results: {error}"), - } - } - /// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and /// the current change tick are given. /// @@ -739,33 +683,6 @@ impl QueryState { Ok(values.map(|x| x.assume_init())) } - /// Gets the query results for the given [`World`] and array of [`Entity`], where the last change and - /// the current change tick are given. - /// - /// # Panics - /// - /// Panics if any item cannot be retrieved. - /// - /// # Safety - /// - /// This does not check for unique access to subsets of the entity-component data. - /// To be safe, make sure mutable queries have unique access to the components they query. - /// - /// This must be called on the same `World` that the `Query` was generated from: - /// use `QueryState::validate_world` to verify this. - pub(crate) unsafe fn many_unchecked_manual<'w, const N: usize>( - &self, - world: UnsafeWorldCell<'w>, - entities: [Entity; N], - last_run: Tick, - this_run: Tick, - ) -> [Q::Item<'w>; N] { - match self.get_many_unchecked_manual(world, entities, last_run, this_run) { - Ok(items) => items, - Err(error) => panic!("Cannot get query result: {error}"), - } - } - /// Returns an [`Iterator`] over the query results for the given [`World`]. /// /// This can only be called for read-only queries, see [`Self::iter_mut`] for write-queries. diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index f2120f4b3ccc7..437ee085c4454 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -8,7 +8,7 @@ use crate::{ }, world::{unsafe_world_cell::UnsafeWorldCell, Mut}, }; -use std::borrow::Borrow; +use std::{any::TypeId, borrow::Borrow}; /// [System parameter] that provides selective access to the [`Component`] data stored in a [`World`]. /// @@ -928,14 +928,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`get_many`](Self::get_many) for the non-panicking version. #[inline] pub fn many(&self, entities: [Entity; N]) -> [ROQueryItem<'_, Q>; N] { - // SAFETY: it is the scheduler's responsibility to ensure that `Query` is never handed out on the wrong `World`. - unsafe { - self.state.many_read_only_manual( - self.world.unsafe_world(), - entities, - self.last_run, - self.this_run, - ) + match self.get_many(entities) { + Ok(items) => items, + Err(error) => panic!("Cannot get query results: {error}"), } } @@ -1047,10 +1042,9 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { /// - [`many`](Self::many) to get read-only query items. #[inline] pub fn many_mut(&mut self, entities: [Entity; N]) -> [Q::Item<'_>; N] { - // SAFETY: scheduler ensures safe Query world access - unsafe { - self.state - .many_unchecked_manual(self.world, entities, self.last_run, self.this_run) + match self.get_many_mut(entities) { + Ok(items) => items, + Err(error) => panic!("Cannot get query result: {error}"), } } @@ -1178,9 +1172,16 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { #[track_caller] pub fn component_mut(&mut self, entity: Entity) -> Mut<'_, T> { // SAFETY: unique access to query (preventing aliased access) - unsafe { - self.state - .component_unchecked_mut(self.world, entity, self.last_run, self.this_run) + let result = unsafe { self.get_component_unchecked_mut(entity) }; + + match result { + Ok(component) => component, + Err(error) => { + panic!( + "Cannot get component `{:?}` from {entity:?}: {error}", + TypeId::of::() + ) + } } } From c9b28cf6c9b1482ed1b3db41d967f76eeb1c1653 Mon Sep 17 00:00:00 2001 From: Zachary Harrold Date: Sun, 10 Sep 2023 12:53:48 +1000 Subject: [PATCH 15/15] Slightly Less Naive Version of This Function --- crates/bevy_ecs/src/system/query.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 437ee085c4454..e3b1f16a2b314 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -1171,10 +1171,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> { #[inline] #[track_caller] pub fn component_mut(&mut self, entity: Entity) -> Mut<'_, T> { - // SAFETY: unique access to query (preventing aliased access) - let result = unsafe { self.get_component_unchecked_mut(entity) }; - - match result { + match self.get_component_mut(entity) { Ok(component) => component, Err(error) => { panic!(