diff --git a/crates/bevy_ecs/macros/src/fetch.rs b/crates/bevy_ecs/macros/src/fetch.rs index 50adea5691..5d21a93f64 100644 --- a/crates/bevy_ecs/macros/src/fetch.rs +++ b/crates/bevy_ecs/macros/src/fetch.rs @@ -90,6 +90,11 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { user_generics_with_world.split_for_impl(); let struct_name = ast.ident.clone(); + let read_only_struct_name = if fetch_struct_attributes.is_mutable { + Ident::new(&format!("{}ReadOnly", struct_name), Span::call_site()) + } else { + struct_name.clone() + }; let item_struct_name = Ident::new(&format!("{}Item", struct_name), Span::call_site()); let read_only_item_struct_name = if fetch_struct_attributes.is_mutable { @@ -178,7 +183,8 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { #(#ignored_field_idents: #ignored_field_types,)* } - impl #user_impl_generics_with_world #path::query::Fetch<'__w> + // SAFETY: `update_component_access` and `update_archetype_component_access` are called on every field + unsafe impl #user_impl_generics_with_world #path::query::Fetch<'__w> for #fetch_struct_name #user_ty_generics_with_world #user_where_clauses_with_world { type Item = #item_struct_name #user_ty_generics_with_world; @@ -248,6 +254,17 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { unsafe fn archetype_filter_fetch(&mut self, _archetype_index: usize) -> bool { true #(&& self.#field_idents.archetype_filter_fetch(_archetype_index))* } + + fn update_component_access(state: &Self::State, _access: &mut #path::query::FilteredAccess<#path::component::ComponentId>) { + #( #path::query::#fetch_type_alias::<'static, #field_types> :: update_component_access(&state.#field_idents, _access); )* + } + + fn update_archetype_component_access(state: &Self::State, _archetype: &#path::archetype::Archetype, _access: &mut #path::query::Access<#path::archetype::ArchetypeComponentId>) { + #( + #path::query::#fetch_type_alias::<'static, #field_types> + :: update_archetype_component_access(&state.#field_idents, _archetype, _access); + )* + } } } }; @@ -262,8 +279,7 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { #(#ignored_field_idents: #ignored_field_types,)* } - // SAFETY: `update_component_access` and `update_archetype_component_access` are called for each item in the struct - unsafe impl #user_impl_generics #path::query::FetchState for #state_struct_name #user_ty_generics #user_where_clauses { + impl #user_impl_generics #path::query::FetchState for #state_struct_name #user_ty_generics #user_where_clauses { fn init(world: &mut #path::world::World) -> Self { #state_struct_name { #(#field_idents: <<#field_types as #path::query::WorldQuery>::State as #path::query::FetchState>::init(world),)* @@ -271,14 +287,6 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { } } - fn update_component_access(&self, _access: &mut #path::query::FilteredAccess<#path::component::ComponentId>) { - #(self.#field_idents.update_component_access(_access);)* - } - - fn update_archetype_component_access(&self, _archetype: &#path::archetype::Archetype, _access: &mut #path::query::Access<#path::archetype::ArchetypeComponentId>) { - #(self.#field_idents.update_archetype_component_access(_archetype, _access);)* - } - fn matches_component_set(&self, _set_contains_id: &impl Fn(#path::component::ComponentId) -> bool) -> bool { true #(&& self.#field_idents.matches_component_set(_set_contains_id))* @@ -290,29 +298,66 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { impl_fetch( true, read_only_fetch_struct_name.clone(), - read_only_item_struct_name, + read_only_item_struct_name.clone(), ) } else { quote! {} }; + let read_only_world_query_impl = if fetch_struct_attributes.is_mutable { + quote! { + #[automatically_derived] + #visibility struct #read_only_struct_name #user_impl_generics #user_where_clauses { + #( #field_idents: < #field_types as #path::query::WorldQuery >::ReadOnly, )* + #(#(#ignored_field_attrs)* #ignored_field_visibilities #ignored_field_idents: #ignored_field_types,)* + } + + // SAFETY: `ROQueryFetch` is the same as `QueryFetch` + unsafe impl #user_impl_generics #path::query::WorldQuery for #read_only_struct_name #user_ty_generics #user_where_clauses { + type ReadOnly = Self; + type State = #state_struct_name #user_ty_generics; + + fn shrink<'__wlong: '__wshort, '__wshort>(item: #path::query::#item_type_alias<'__wlong, Self>) + -> #path::query::#item_type_alias<'__wshort, Self> { + #read_only_item_struct_name { + #( + #field_idents : < + < #field_types as #path::query::WorldQuery >::ReadOnly as #path::query::WorldQuery + > :: shrink( item.#field_idents ), + )* + #( + #ignored_field_idents: item.#ignored_field_idents, + )* + } + } + } + + impl #user_impl_generics_with_world #path::query::WorldQueryGats<'__w> for #read_only_struct_name #user_ty_generics #user_where_clauses { + type Fetch = #read_only_fetch_struct_name #user_ty_generics_with_world; + type _State = #state_struct_name #user_ty_generics; + } + } + } else { + quote! {} + }; + let read_only_asserts = if fetch_struct_attributes.is_mutable { quote! { - // Double-check that the data fetched by `ROQueryFetch` is read-only. - // This is technically unnecessary as `<_ as WorldQueryGats<'world>>::ReadOnlyFetch: ReadOnlyFetch` - // but to protect against future mistakes we assert the assoc type implements `ReadOnlyFetch` anyway - #( assert_readonly::<#path::query::ROQueryFetch<'__w, #field_types>>(); )* + // Double-check that the data fetched by `<_ as WorldQuery>::ReadOnly` is read-only. + // This is technically unnecessary as `<_ as WorldQuery>::ReadOnly: ReadOnlyWorldQuery` + // but to protect against future mistakes we assert the assoc type implements `ReadOnlyWorldQuery` anyway + #( assert_readonly::< < #field_types as #path::query::WorldQuery > :: ReadOnly >(); )* } } else { quote! { - // Statically checks that the safety guarantee of `ReadOnlyFetch` for `$fetch_struct_name` actually holds true. - // We need this to make sure that we don't compile `ReadOnlyFetch` if our struct contains nested `WorldQuery` + // Statically checks that the safety guarantee of `ReadOnlyWorldQuery` for `$fetch_struct_name` actually holds true. + // We need this to make sure that we don't compile `ReadOnlyWorldQuery` if our struct contains nested `WorldQuery` // members that don't implement it. I.e.: // ``` // #[derive(WorldQuery)] // pub struct Foo { a: &'static mut MyComponent } // ``` - #( assert_readonly::<#path::query::QueryFetch<'__w, #field_types>>(); )* + #( assert_readonly::<#field_types>(); )* } }; @@ -323,7 +368,12 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { #read_only_fetch_impl - impl #user_impl_generics #path::query::WorldQuery for #struct_name #user_ty_generics #user_where_clauses { + #read_only_world_query_impl + + // SAFETY: if the worldquery is mutable this defers to soundness of the `#field_types: WorldQuery` impl, otherwise + // if the world query is immutable then `#read_only_struct_name #user_ty_generics` is the same type as `#struct_name #user_ty_generics` + unsafe impl #user_impl_generics #path::query::WorldQuery for #struct_name #user_ty_generics #user_where_clauses { + type ReadOnly = #read_only_struct_name #user_ty_generics; type State = #state_struct_name #user_ty_generics; fn shrink<'__wlong: '__wshort, '__wshort>(item: #path::query::#item_type_alias<'__wlong, Self>) -> #path::query::#item_type_alias<'__wshort, Self> { @@ -340,19 +390,18 @@ pub fn derive_world_query_impl(ast: DeriveInput) -> TokenStream { impl #user_impl_generics_with_world #path::query::WorldQueryGats<'__w> for #struct_name #user_ty_generics #user_where_clauses { type Fetch = #fetch_struct_name #user_ty_generics_with_world; - type ReadOnlyFetch = #read_only_fetch_struct_name #user_ty_generics_with_world; type _State = #state_struct_name #user_ty_generics; } /// SAFETY: each item in the struct is read only - unsafe impl #user_impl_generics_with_world #path::query::ReadOnlyFetch - for #read_only_fetch_struct_name #user_ty_generics_with_world #user_where_clauses_with_world {} + unsafe impl #user_impl_generics #path::query::ReadOnlyWorldQuery + for #read_only_struct_name #user_ty_generics #user_where_clauses {} #[allow(dead_code)] const _: () = { fn assert_readonly() where - T: #path::query::ReadOnlyFetch, + T: #path::query::ReadOnlyWorldQuery, { } diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index fafd634f8a..33b3f47529 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -143,6 +143,30 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// # bevy_ecs::system::assert_is_system(my_system); /// ``` /// +/// Mutable queries will also have a read only version derived: +/// ```rust +/// # use bevy_ecs::prelude::*; +/// use bevy_ecs::query::WorldQuery; +/// +/// #[derive(Component)] +/// pub struct MyComponent; +/// +/// #[derive(WorldQuery)] +/// #[world_query(mutable)] +/// pub struct Foo { +/// my_component_yay: &'static mut MyComponent, +/// } +/// +/// fn my_system(mut my_query: Query<(FooReadOnly, FooReadOnly)>) { +/// for (i1, i2) in my_query.iter_mut() { +/// let _: FooReadOnlyItem<'_> = i1; +/// let _: FooReadOnlyItem<'_> = i2; +/// } +/// } +/// +/// # bevy_ecs::system::assert_is_system(my_system); +/// ``` +/// /// **Note:** if you omit the `mutable` attribute for a query that doesn't implement /// `ReadOnlyFetch`, compilation will fail. We insert static checks as in the example above for /// every query component and a nested query. @@ -292,26 +316,36 @@ use std::{cell::UnsafeCell, marker::PhantomData}; /// /// # bevy_ecs::system::assert_is_system(my_system); /// ``` -pub trait WorldQuery: for<'w> WorldQueryGats<'w, _State = Self::State> { +/// # Safety +/// +/// component access of `ROQueryFetch` should be a subset of `QueryFetch` +pub unsafe trait WorldQuery: for<'w> WorldQueryGats<'w, _State = Self::State> { + type ReadOnly: ReadOnlyWorldQuery; type State: FetchState; /// This function manually implements variance for the query items. fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self>; } +/// A world query that is read only. +/// +/// # Safety +/// +/// This must only be implemented for read-only [`WorldQuery`]'s. +pub unsafe trait ReadOnlyWorldQuery: WorldQuery {} + /// The [`Fetch`] of a [`WorldQuery`], which declares which data it needs access to pub type QueryFetch<'w, Q> = >::Fetch; /// The item type returned when a [`WorldQuery`] is iterated over pub type QueryItem<'w, Q> = <>::Fetch as Fetch<'w>>::Item; /// The read-only [`Fetch`] of a [`WorldQuery`], which declares which data it needs access to when accessed immutably -pub type ROQueryFetch<'w, Q> = >::ReadOnlyFetch; +pub type ROQueryFetch<'w, Q> = QueryFetch<'w, ::ReadOnly>; /// The read-only variant of the item type returned when a [`WorldQuery`] is iterated over immutably -pub type ROQueryItem<'w, Q> = <>::ReadOnlyFetch as Fetch<'w>>::Item; +pub type ROQueryItem<'w, Q> = QueryItem<'w, ::ReadOnly>; /// A helper trait for [`WorldQuery`] that works around Rust's lack of Generic Associated Types pub trait WorldQueryGats<'world> { type Fetch: Fetch<'world, State = Self::_State>; - type ReadOnlyFetch: Fetch<'world, State = Self::_State> + ReadOnlyFetch; type _State: FetchState; } @@ -320,7 +354,14 @@ pub trait WorldQueryGats<'world> { /// /// Every type that implements [`WorldQuery`] have their associated [`WorldQuery::Fetch`](WorldQueryGats::Fetch) and /// [`WorldQuery::State`] types that are essential for fetching component data. -pub trait Fetch<'world>: Sized { +/// +/// # Safety +/// +/// Implementor must ensure that [`Fetch::update_component_access`] and +/// [`Fetch::update_archetype_component_access`] exactly reflects the results of +/// [`FetchState::matches_component_set`], [`Fetch::archetype_fetch`], and +/// [`Fetch::table_fetch`]. +pub unsafe trait Fetch<'world>: Sized { type Item; type State: FetchState; @@ -411,37 +452,30 @@ pub trait Fetch<'world>: Sized { unsafe fn table_filter_fetch(&mut self, table_row: usize) -> bool { true } + + // This does not have a default body of `{}` because 99% of cases need to add accesses + // and forgetting to do so would be unsound. + fn update_component_access(state: &Self::State, access: &mut FilteredAccess); + // This does not have a default body of `{}` becaues 99% of cases need to add accesses + // and forgetting to do so would be unsound. + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ); } /// State used to construct a Fetch. This will be cached inside [`QueryState`](crate::query::QueryState), /// so it is best to move as much data / computation here as possible to reduce the cost of /// constructing Fetch. -/// -/// # Safety -/// -/// Implementor must ensure that [`FetchState::update_component_access`] and -/// [`FetchState::update_archetype_component_access`] exactly reflects the results of -/// [`FetchState::matches_component_set`], [`Fetch::archetype_fetch`], and -/// [`Fetch::table_fetch`]. -pub unsafe trait FetchState: Send + Sync + Sized { +pub trait FetchState: Send + Sync + Sized { fn init(world: &mut World) -> Self; - fn update_component_access(&self, access: &mut FilteredAccess); - fn update_archetype_component_access( - &self, - archetype: &Archetype, - access: &mut Access, - ); fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool; } -/// A fetch that is read only. -/// -/// # Safety -/// -/// This must only be implemented for read-only fetches. -pub unsafe trait ReadOnlyFetch {} - -impl WorldQuery for Entity { +/// SAFETY: no component or archetype access +unsafe impl WorldQuery for Entity { + type ReadOnly = Self; type State = EntityState; fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { @@ -457,27 +491,17 @@ pub struct EntityFetch<'w> { } /// SAFETY: access is read only -unsafe impl<'w> ReadOnlyFetch for EntityFetch<'w> {} +unsafe impl ReadOnlyWorldQuery for Entity {} /// The [`FetchState`] of [`Entity`]. #[doc(hidden)] pub struct EntityState; -// SAFETY: no component or archetype access -unsafe impl FetchState for EntityState { +impl FetchState for EntityState { fn init(_world: &mut World) -> Self { Self } - fn update_component_access(&self, _access: &mut FilteredAccess) {} - - fn update_archetype_component_access( - &self, - _archetype: &Archetype, - _access: &mut Access, - ) { - } - #[inline] fn matches_component_set(&self, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { true @@ -486,11 +510,11 @@ unsafe impl FetchState for EntityState { impl<'w> WorldQueryGats<'w> for Entity { type Fetch = EntityFetch<'w>; - type ReadOnlyFetch = EntityFetch<'w>; type _State = EntityState; } -impl<'w> Fetch<'w> for EntityFetch<'w> { +/// SAFETY: no component or archetype access +unsafe impl<'w> Fetch<'w> for EntityFetch<'w> { type Item = Entity; type State = EntityState; @@ -533,10 +557,21 @@ impl<'w> Fetch<'w> for EntityFetch<'w> { let entities = self.entities.unwrap_or_else(|| debug_checked_unreachable()); *entities.get(archetype_index) } + + fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess) {} + + fn update_archetype_component_access( + _state: &Self::State, + _archetype: &Archetype, + _access: &mut Access, + ) { + } } -impl WorldQuery for &T { - type State = ReadState; +/// SAFETY: `ROQueryFetch` is the same as `QueryFetch` +unsafe impl WorldQuery for &T { + type ReadOnly = Self; + type State = ComponentIdState; fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { item @@ -545,43 +580,20 @@ impl WorldQuery for &T { /// The [`FetchState`] of `&T`. #[doc(hidden)] -pub struct ReadState { +pub struct ComponentIdState { component_id: ComponentId, marker: PhantomData, } -// SAFETY: component access and archetype component access are properly updated to reflect that T is -// read -unsafe impl FetchState for ReadState { +impl FetchState for ComponentIdState { fn init(world: &mut World) -> Self { let component_id = world.init_component::(); - ReadState { + ComponentIdState { component_id, marker: PhantomData, } } - fn update_component_access(&self, access: &mut FilteredAccess) { - assert!( - !access.access().has_write(self.component_id), - "&{} conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.", - std::any::type_name::(), - ); - access.add_read(self.component_id); - } - - fn update_archetype_component_access( - &self, - archetype: &Archetype, - access: &mut Access, - ) { - if let Some(archetype_component_id) = - archetype.get_archetype_component_id(self.component_id) - { - access.add_read(archetype_component_id); - } - } - fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { set_contains_id(self.component_id) } @@ -610,17 +622,18 @@ impl Clone for ReadFetch<'_, T> { } /// SAFETY: access is read only -unsafe impl<'w, T: Component> ReadOnlyFetch for ReadFetch<'w, T> {} +unsafe impl ReadOnlyWorldQuery for &T {} impl<'w, T: Component> WorldQueryGats<'w> for &T { type Fetch = ReadFetch<'w, T>; - type ReadOnlyFetch = ReadFetch<'w, T>; - type _State = ReadState; + type _State = ComponentIdState; } -impl<'w, T: Component> Fetch<'w> for ReadFetch<'w, T> { +// SAFETY: component access and archetype component access are properly updated to reflect that T is +// read +unsafe impl<'w, T: Component> Fetch<'w> for ReadFetch<'w, T> { type Item = &'w T; - type State = ReadState; + type State = ComponentIdState; const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { @@ -633,7 +646,7 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch<'w, T> { unsafe fn init( world: &'w World, - state: &ReadState, + state: &ComponentIdState, _last_change_tick: u32, _change_tick: u32, ) -> ReadFetch<'w, T> { @@ -713,10 +726,33 @@ impl<'w, T: Component> Fetch<'w> for ReadFetch<'w, T> { .unwrap_or_else(|| debug_checked_unreachable()); components.get(table_row).deref() } + + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + assert!( + !access.access().has_write(state.component_id), + "&{} conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.", + std::any::type_name::(), + ); + access.add_read(state.component_id); + } + + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ) { + if let Some(archetype_component_id) = + archetype.get_archetype_component_id(state.component_id) + { + access.add_read(archetype_component_id); + } + } } -impl WorldQuery for &mut T { - type State = WriteState; +/// SAFETY: access of `&T` is a subset of `&mut T` +unsafe impl<'w, T: Component> WorldQuery for &'w mut T { + type ReadOnly = &'w T; + type State = ComponentIdState; fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { item @@ -752,83 +788,16 @@ impl Clone for WriteFetch<'_, T> { } } -/// The [`ReadOnlyFetch`] of `&mut T`. -pub struct ReadOnlyWriteFetch<'w, T> { - // T::Storage = TableStorage - table_components: Option>>, - entity_table_rows: Option>, - // T::Storage = SparseStorage - entities: Option>, - sparse_set: Option<&'w ComponentSparseSet>, -} - -/// SAFETY: access is read only -unsafe impl<'w, T: Component> ReadOnlyFetch for ReadOnlyWriteFetch<'w, T> {} - -impl Clone for ReadOnlyWriteFetch<'_, T> { - fn clone(&self) -> Self { - Self { - table_components: self.table_components, - entities: self.entities, - entity_table_rows: self.entity_table_rows, - sparse_set: self.sparse_set, - } - } -} - -/// The [`FetchState`] of `&mut T`. -#[doc(hidden)] -pub struct WriteState { - component_id: ComponentId, - marker: PhantomData, -} - -// SAFETY: component access and archetype component access are properly updated to reflect that T is -// written -unsafe impl FetchState for WriteState { - fn init(world: &mut World) -> Self { - let component_id = world.init_component::(); - WriteState { - component_id, - marker: PhantomData, - } - } - - fn update_component_access(&self, access: &mut FilteredAccess) { - assert!( - !access.access().has_read(self.component_id), - "&mut {} conflicts with a previous access in this query. Mutable component access must be unique.", - std::any::type_name::(), - ); - access.add_write(self.component_id); - } - - fn update_archetype_component_access( - &self, - archetype: &Archetype, - access: &mut Access, - ) { - if let Some(archetype_component_id) = - archetype.get_archetype_component_id(self.component_id) - { - access.add_write(archetype_component_id); - } - } - - fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { - set_contains_id(self.component_id) - } -} - impl<'w, T: Component> WorldQueryGats<'w> for &mut T { type Fetch = WriteFetch<'w, T>; - type ReadOnlyFetch = ReadOnlyWriteFetch<'w, T>; - type _State = WriteState; + type _State = ComponentIdState; } -impl<'w, T: Component> Fetch<'w> for WriteFetch<'w, T> { +/// SAFETY: component access and archetype component access are properly updated to reflect that T is +/// read and write +unsafe impl<'w, T: Component> Fetch<'w> for WriteFetch<'w, T> { type Item = Mut<'w, T>; - type State = WriteState; + type State = ComponentIdState; const IS_DENSE: bool = { match T::Storage::STORAGE_TYPE { @@ -841,7 +810,7 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<'w, T> { unsafe fn init( world: &'w World, - state: &WriteState, + state: &ComponentIdState, last_change_tick: u32, change_tick: u32, ) -> Self { @@ -943,106 +912,32 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<'w, T> { }, } } -} -impl<'w, T: Component> Fetch<'w> for ReadOnlyWriteFetch<'w, T> { - type Item = &'w T; - type State = WriteState; - - const IS_DENSE: bool = { - match T::Storage::STORAGE_TYPE { - StorageType::Table => true, - StorageType::SparseSet => false, - } - }; - - const IS_ARCHETYPAL: bool = true; - - unsafe fn init( - world: &'w World, - state: &WriteState, - _last_change_tick: u32, - _change_tick: u32, - ) -> Self { - Self { - table_components: None, - entities: None, - entity_table_rows: None, - sparse_set: (T::Storage::STORAGE_TYPE == StorageType::SparseSet).then(|| { - world - .storages() - .sparse_sets - .get(state.component_id) - .unwrap() - }), - } - } - - #[inline] - unsafe fn set_archetype( - &mut self, - state: &Self::State, - archetype: &'w Archetype, - tables: &'w Tables, - ) { - match T::Storage::STORAGE_TYPE { - StorageType::Table => { - self.entity_table_rows = Some(archetype.entity_table_rows().into()); - let column = tables[archetype.table_id()] - .get_column(state.component_id) - .unwrap(); - self.table_components = Some(column.get_data_slice().into()); - } - StorageType::SparseSet => self.entities = Some(archetype.entities().into()), - } - } - - #[inline] - unsafe fn set_table(&mut self, state: &Self::State, table: &'w Table) { - self.table_components = Some( - table - .get_column(state.component_id) - .unwrap() - .get_data_slice() - .into(), + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + assert!( + !access.access().has_read(state.component_id), + "&mut {} conflicts with a previous access in this query. Mutable component access must be unique.", + std::any::type_name::(), ); + access.add_write(state.component_id); } - #[inline] - unsafe fn archetype_fetch(&mut self, archetype_index: usize) -> Self::Item { - match T::Storage::STORAGE_TYPE { - StorageType::Table => { - let (entity_table_rows, table_components) = self - .entity_table_rows - .zip(self.table_components) - .unwrap_or_else(|| debug_checked_unreachable()); - let table_row = *entity_table_rows.get(archetype_index); - table_components.get(table_row).deref() - } - StorageType::SparseSet => { - let (entities, sparse_set) = self - .entities - .zip(self.sparse_set) - .unwrap_or_else(|| debug_checked_unreachable()); - let entity = *entities.get(archetype_index); - sparse_set - .get(entity) - .unwrap_or_else(|| debug_checked_unreachable()) - .deref::() - } + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ) { + if let Some(archetype_component_id) = + archetype.get_archetype_component_id(state.component_id) + { + access.add_write(archetype_component_id); } } - - #[inline] - unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item { - let components = self - .table_components - .unwrap_or_else(|| debug_checked_unreachable()); - components.get(table_row).deref() - } } -impl WorldQuery for Option { +// SAFETY: defers to soundness of `T: WorldQuery` impl +unsafe impl WorldQuery for Option { + type ReadOnly = Option; type State = OptionState; fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { @@ -1059,7 +954,7 @@ pub struct OptionFetch { } /// SAFETY: [`OptionFetch`] is read only because `T` is read only -unsafe impl ReadOnlyFetch for OptionFetch {} +unsafe impl ReadOnlyWorldQuery for Option {} /// The [`FetchState`] of `Option`. #[doc(hidden)] @@ -1067,39 +962,13 @@ pub struct OptionState { state: T, } -// SAFETY: component access and archetype component access are properly updated according to the -// internal Fetch -unsafe impl FetchState for OptionState { +impl FetchState for OptionState { fn init(world: &mut World) -> Self { Self { state: T::init(world), } } - fn update_component_access(&self, access: &mut FilteredAccess) { - // We don't want to add the `with`/`without` of `T` as `Option` will match things regardless of - // `T`'s filters. for example `Query<(Option<&U>, &mut V)>` will match every entity with a `V` component - // regardless of whether it has a `U` component. If we dont do this the query will not conflict with - // `Query<&mut V, Without>` which would be unsound. - let mut intermediate = access.clone(); - self.state.update_component_access(&mut intermediate); - access.extend_access(&intermediate); - } - - fn update_archetype_component_access( - &self, - archetype: &Archetype, - access: &mut Access, - ) { - if self - .state - .matches_component_set(&|id| archetype.contains(id)) - { - self.state - .update_archetype_component_access(archetype, access); - } - } - fn matches_component_set(&self, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { true } @@ -1107,11 +976,12 @@ unsafe impl FetchState for OptionState { impl<'w, T: WorldQueryGats<'w>> WorldQueryGats<'w> for Option { type Fetch = OptionFetch; - type ReadOnlyFetch = OptionFetch; type _State = OptionState; } -impl<'w, T: Fetch<'w>> Fetch<'w> for OptionFetch { +// SAFETY: component access and archetype component access are properly updated according to the +// internal Fetch +unsafe impl<'w, T: Fetch<'w>> Fetch<'w> for OptionFetch { type Item = Option; type State = OptionState; @@ -1173,6 +1043,26 @@ impl<'w, T: Fetch<'w>> Fetch<'w> for OptionFetch { None } } + + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + // We don't want to add the `with`/`without` of `T` as `Option` will match things regardless of + // `T`'s filters. for example `Query<(Option<&U>, &mut V)>` will match every entity with a `V` component + // regardless of whether it has a `U` component. If we dont do this the query will not conflict with + // `Query<&mut V, Without>` which would be unsound. + let mut intermediate = access.clone(); + T::update_component_access(&state.state, &mut intermediate); + access.extend_access(&intermediate); + } + + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ) { + if state.matches_component_set(&|id| archetype.contains(id)) { + T::update_archetype_component_access(&state.state, archetype, access); + } + } } /// [`WorldQuery`] that tracks changes and additions for component `T`. @@ -1239,7 +1129,9 @@ impl ChangeTrackers { } } -impl WorldQuery for ChangeTrackers { +// SAFETY: `ROQueryFetch` is the same as `QueryFetch` +unsafe impl WorldQuery for ChangeTrackers { + type ReadOnly = Self; type State = ChangeTrackersState; fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { @@ -1254,9 +1146,7 @@ pub struct ChangeTrackersState { marker: PhantomData, } -// SAFETY: component access and archetype component access are properly updated to reflect that T is -// read -unsafe impl FetchState for ChangeTrackersState { +impl FetchState for ChangeTrackersState { fn init(world: &mut World) -> Self { let component_id = world.init_component::(); Self { @@ -1265,27 +1155,6 @@ unsafe impl FetchState for ChangeTrackersState { } } - fn update_component_access(&self, access: &mut FilteredAccess) { - assert!( - !access.access().has_write(self.component_id), - "ChangeTrackers<{}> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.", - std::any::type_name::() - ); - access.add_read(self.component_id); - } - - fn update_archetype_component_access( - &self, - archetype: &Archetype, - access: &mut Access, - ) { - if let Some(archetype_component_id) = - archetype.get_archetype_component_id(self.component_id) - { - access.add_read(archetype_component_id); - } - } - fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { set_contains_id(self.component_id) } @@ -1321,15 +1190,16 @@ impl Clone for ChangeTrackersFetch<'_, T> { } /// SAFETY: access is read only -unsafe impl<'w, T: Component> ReadOnlyFetch for ChangeTrackersFetch<'w, T> {} +unsafe impl ReadOnlyWorldQuery for ChangeTrackers {} impl<'w, T: Component> WorldQueryGats<'w> for ChangeTrackers { type Fetch = ChangeTrackersFetch<'w, T>; - type ReadOnlyFetch = ChangeTrackersFetch<'w, T>; type _State = ChangeTrackersState; } -impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch<'w, T> { +// SAFETY: component access and archetype component access are properly updated to reflect that T is +// read +unsafe impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch<'w, T> { type Item = ChangeTrackers; type State = ChangeTrackersState; @@ -1448,6 +1318,27 @@ impl<'w, T: Component> Fetch<'w> for ChangeTrackersFetch<'w, T> { change_tick: self.change_tick, } } + + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + assert!( + !access.access().has_write(state.component_id), + "ChangeTrackers<{}> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.", + std::any::type_name::() + ); + access.add_read(state.component_id); + } + + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ) { + if let Some(archetype_component_id) = + archetype.get_archetype_component_id(state.component_id) + { + access.add_read(archetype_component_id); + } + } } macro_rules! impl_tuple_fetch { @@ -1456,12 +1347,12 @@ macro_rules! impl_tuple_fetch { #[allow(non_snake_case)] impl<'w, $($name: WorldQueryGats<'w>),*> WorldQueryGats<'w> for ($($name,)*) { type Fetch = ($($name::Fetch,)*); - type ReadOnlyFetch = ($($name::ReadOnlyFetch,)*); type _State = ($($name::_State,)*); } #[allow(non_snake_case)] - impl<'w, $($name: Fetch<'w>),*> Fetch<'w> for ($($name,)*) { + // SAFETY: update_component_access and update_archetype_component_access are called for each item in the tuple + unsafe impl<'w, $($name: Fetch<'w>),*> Fetch<'w> for ($($name,)*) { type Item = ($($name::Item,)*); type State = ($($name::State,)*); @@ -1516,26 +1407,25 @@ macro_rules! impl_tuple_fetch { let ($($name,)*) = self; true $(&& $name.archetype_filter_fetch(archetype_index))* } + + fn update_component_access(state: &Self::State, _access: &mut FilteredAccess) { + let ($($name,)*) = state; + $($name::update_component_access($name, _access);)* + } + + fn update_archetype_component_access(state: &Self::State, _archetype: &Archetype, _access: &mut Access) { + let ($($name,)*) = state; + $($name::update_archetype_component_access($name, _archetype, _access);)* + } } - // SAFETY: update_component_access and update_archetype_component_access are called for each item in the tuple #[allow(non_snake_case)] #[allow(clippy::unused_unit)] - unsafe impl<$($name: FetchState),*> FetchState for ($($name,)*) { + impl<$($name: FetchState),*> FetchState for ($($name,)*) { fn init(_world: &mut World) -> Self { ($($name::init(_world),)*) } - fn update_component_access(&self, _access: &mut FilteredAccess) { - let ($($name,)*) = self; - $($name.update_component_access(_access);)* - } - - fn update_archetype_component_access(&self, _archetype: &Archetype, _access: &mut Access) { - let ($($name,)*) = self; - $($name.update_archetype_component_access(_archetype, _access);)* - } - fn matches_component_set(&self, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { let ($($name,)*) = self; true $(&& $name.matches_component_set(_set_contains_id))* @@ -1544,7 +1434,9 @@ macro_rules! impl_tuple_fetch { #[allow(non_snake_case)] #[allow(clippy::unused_unit)] - impl<$($name: WorldQuery),*> WorldQuery for ($($name,)*) { + // SAFETY: defers to soundness `$name: WorldQuery` impl + unsafe impl<$($name: WorldQuery),*> WorldQuery for ($($name,)*) { + type ReadOnly = ($($name::ReadOnly,)*); type State = ($($name::State,)*); fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { @@ -1556,7 +1448,7 @@ macro_rules! impl_tuple_fetch { } /// SAFETY: each item in the tuple is read only - unsafe impl<'w, $($name: ReadOnlyFetch),*> ReadOnlyFetch for ($($name,)*) {} + unsafe impl<$($name: ReadOnlyWorldQuery),*> ReadOnlyWorldQuery for ($($name,)*) {} }; } @@ -1574,12 +1466,12 @@ macro_rules! impl_anytuple_fetch { #[allow(non_snake_case)] impl<'w, $($name: WorldQueryGats<'w>),*> WorldQueryGats<'w> for AnyOf<($($name,)*)> { type Fetch = AnyOf<($(($name::Fetch, bool),)*)>; - type ReadOnlyFetch = AnyOf<($(($name::ReadOnlyFetch, bool),)*)>; type _State = AnyOf<($($name::_State,)*)>; } #[allow(non_snake_case)] - impl<'w, $($name: Fetch<'w>),*> Fetch<'w> for AnyOf<($(($name, bool),)*)> { + // SAFETY: update_component_access and update_archetype_component_access are called for each item in the tuple + unsafe impl<'w, $($name: Fetch<'w>),*> Fetch<'w> for AnyOf<($(($name, bool),)*)> { type Item = ($(Option<$name::Item>,)*); type State = AnyOf<($($name::State,)*)>; @@ -1634,18 +1526,9 @@ macro_rules! impl_anytuple_fetch { $name.1.then(|| $name.0.archetype_fetch(_archetype_index)), )*) } - } - // SAFETY: update_component_access and update_archetype_component_access are called for each item in the tuple - #[allow(non_snake_case)] - #[allow(clippy::unused_unit)] - unsafe impl<$($name: FetchState),*> FetchState for AnyOf<($($name,)*)> { - fn init(_world: &mut World) -> Self { - AnyOf(($($name::init(_world),)*)) - } - - fn update_component_access(&self, _access: &mut FilteredAccess) { - let ($($name,)*) = &self.0; + fn update_component_access(state: &Self::State, _access: &mut FilteredAccess) { + let ($($name,)*) = &state.0; // We do not unconditionally add `$name`'s `with`/`without` accesses to `_access` // as this would be unsound. For example the following two queries should conflict: @@ -1664,11 +1547,12 @@ macro_rules! impl_anytuple_fetch { $( if _not_first { let mut intermediate = _access.clone(); - $name.update_component_access(&mut intermediate); + $name::update_component_access($name, &mut intermediate); _intersected_access.extend_intersect_filter(&intermediate); _intersected_access.extend_access(&intermediate); } else { - $name.update_component_access(&mut _intersected_access); + + $name::update_component_access($name, &mut _intersected_access); _not_first = true; } )* @@ -1676,14 +1560,23 @@ macro_rules! impl_anytuple_fetch { *_access = _intersected_access; } - fn update_archetype_component_access(&self, _archetype: &Archetype, _access: &mut Access) { - let ($($name,)*) = &self.0; + fn update_archetype_component_access(state: &Self::State, _archetype: &Archetype, _access: &mut Access) { + let ($($name,)*) = &state.0; $( if $name.matches_component_set(&|id| _archetype.contains(id)) { - $name.update_archetype_component_access(_archetype, _access); + $name::update_archetype_component_access($name, _archetype, _access); } )* } + } + + #[allow(non_snake_case)] + #[allow(clippy::unused_unit)] + impl<$($name: FetchState),*> FetchState for AnyOf<($($name,)*)> { + fn init(_world: &mut World) -> Self { + AnyOf(($($name::init(_world),)*)) + } + fn matches_component_set(&self, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { let ($($name,)*) = &self.0; false $(|| $name.matches_component_set(_set_contains_id))* @@ -1692,7 +1585,9 @@ macro_rules! impl_anytuple_fetch { #[allow(non_snake_case)] #[allow(clippy::unused_unit)] - impl<$($name: WorldQuery),*> WorldQuery for AnyOf<($($name,)*)> { + // SAFETY: defers to soundness of `$name: WorldQuery` impl + unsafe impl<$($name: WorldQuery),*> WorldQuery for AnyOf<($($name,)*)> { + type ReadOnly = AnyOf<($($name::ReadOnly,)*)>; type State = AnyOf<($($name::State,)*)>; fn shrink<'wlong: 'wshort, 'wshort>(item: QueryItem<'wlong, Self>) -> QueryItem<'wshort, Self> { @@ -1704,7 +1599,7 @@ macro_rules! impl_anytuple_fetch { } /// SAFETY: each item in the tuple is read only - unsafe impl<'w, $($name: ReadOnlyFetch),*> ReadOnlyFetch for AnyOf<($(($name, bool),)*)> {} + unsafe impl<$($name: ReadOnlyWorldQuery),*> ReadOnlyWorldQuery for AnyOf<($($name,)*)> {} }; } @@ -1719,7 +1614,8 @@ pub struct NopFetch { state: PhantomData, } -impl<'w, State: FetchState> Fetch<'w> for NopFetch { +// SAFETY: NopFetch doesnt access anything +unsafe impl<'w, State: FetchState> Fetch<'w> for NopFetch { type Item = (); type State = State; @@ -1754,4 +1650,13 @@ impl<'w, State: FetchState> Fetch<'w> for NopFetch { #[inline(always)] unsafe fn table_fetch(&mut self, _table_row: usize) -> Self::Item {} + + fn update_component_access(_state: &Self::State, _access: &mut FilteredAccess) {} + + fn update_archetype_component_access( + _state: &Self::State, + _archetype: &Archetype, + _access: &mut Access, + ) { + } } diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index f7ef292d6b..1626dd43fc 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -4,7 +4,7 @@ use crate::{ entity::Entity, query::{ debug_checked_unreachable, Access, Fetch, FetchState, FilteredAccess, QueryFetch, - ROQueryFetch, WorldQuery, WorldQueryGats, + WorldQuery, WorldQueryGats, }, storage::{ComponentSparseSet, Table, Tables}, world::World, @@ -13,7 +13,7 @@ use bevy_ecs_macros::all_tuples; use bevy_ptr::{ThinSlicePtr, UnsafeCellDeref}; use std::{cell::UnsafeCell, marker::PhantomData}; -use super::ReadOnlyFetch; +use super::ReadOnlyWorldQuery; /// Filter that selects entities with a component `T`. /// @@ -44,7 +44,9 @@ use super::ReadOnlyFetch; /// ``` pub struct With(PhantomData); -impl WorldQuery for With { +// SAFETY: `ROQueryFetch` is the same as `QueryFetch` +unsafe impl WorldQuery for With { + type ReadOnly = Self; type State = WithState; #[allow(clippy::semicolon_if_nothing_returned)] @@ -68,8 +70,7 @@ pub struct WithState { marker: PhantomData, } -// SAFETY: no component access or archetype component access -unsafe impl FetchState for WithState { +impl FetchState for WithState { fn init(world: &mut World) -> Self { let component_id = world.init_component::(); Self { @@ -78,19 +79,6 @@ unsafe impl FetchState for WithState { } } - #[inline] - fn update_component_access(&self, access: &mut FilteredAccess) { - access.add_with(self.component_id); - } - - #[inline] - fn update_archetype_component_access( - &self, - _archetype: &Archetype, - _access: &mut Access, - ) { - } - fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { set_contains_id(self.component_id) } @@ -98,11 +86,11 @@ unsafe impl FetchState for WithState { impl WorldQueryGats<'_> for With { type Fetch = WithFetch; - type ReadOnlyFetch = WithFetch; type _State = WithState; } -impl<'w, T: Component> Fetch<'w> for WithFetch { +// SAFETY: no component access or archetype component access +unsafe impl<'w, T: Component> Fetch<'w> for WithFetch { type Item = (); type State = WithState; @@ -143,10 +131,23 @@ impl<'w, T: Component> Fetch<'w> for WithFetch { #[inline] unsafe fn table_fetch(&mut self, _table_row: usize) {} + + #[inline] + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + access.add_with(state.component_id); + } + + #[inline] + fn update_archetype_component_access( + _state: &Self::State, + _archetype: &Archetype, + _access: &mut Access, + ) { + } } // SAFETY: no component access or archetype component access -unsafe impl ReadOnlyFetch for WithFetch {} +unsafe impl ReadOnlyWorldQuery for With {} impl Clone for WithFetch { fn clone(&self) -> Self { @@ -184,7 +185,9 @@ impl Copy for WithFetch {} /// ``` pub struct Without(PhantomData); -impl WorldQuery for Without { +// SAFETY: `ROQueryFetch` is the same as `QueryFetch` +unsafe impl WorldQuery for Without { + type ReadOnly = Self; type State = WithoutState; #[allow(clippy::semicolon_if_nothing_returned)] @@ -208,8 +211,7 @@ pub struct WithoutState { marker: PhantomData, } -// SAFETY: no component access or archetype component access -unsafe impl FetchState for WithoutState { +impl FetchState for WithoutState { fn init(world: &mut World) -> Self { let component_id = world.init_component::(); Self { @@ -218,18 +220,6 @@ unsafe impl FetchState for WithoutState { } } - #[inline] - fn update_component_access(&self, access: &mut FilteredAccess) { - access.add_without(self.component_id); - } - - #[inline] - fn update_archetype_component_access( - &self, - _archetype: &Archetype, - _access: &mut Access, - ) { - } fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { !set_contains_id(self.component_id) } @@ -237,11 +227,11 @@ unsafe impl FetchState for WithoutState { impl WorldQueryGats<'_> for Without { type Fetch = WithoutFetch; - type ReadOnlyFetch = WithoutFetch; type _State = WithoutState; } -impl<'w, T: Component> Fetch<'w> for WithoutFetch { +// SAFETY: no component access or archetype component access +unsafe impl<'w, T: Component> Fetch<'w> for WithoutFetch { type Item = (); type State = WithoutState; @@ -282,10 +272,23 @@ impl<'w, T: Component> Fetch<'w> for WithoutFetch { #[inline] unsafe fn table_fetch(&mut self, _table_row: usize) {} + + #[inline] + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + access.add_without(state.component_id); + } + + #[inline] + fn update_archetype_component_access( + _state: &Self::State, + _archetype: &Archetype, + _access: &mut Access, + ) { + } } // SAFETY: no component access or archetype component access -unsafe impl ReadOnlyFetch for WithoutFetch {} +unsafe impl ReadOnlyWorldQuery for Without {} impl Clone for WithoutFetch { fn clone(&self) -> Self { @@ -343,7 +346,9 @@ macro_rules! impl_query_filter_tuple { ($(($filter: ident, $state: ident)),*) => { #[allow(unused_variables)] #[allow(non_snake_case)] - impl<$($filter: WorldQuery),*> WorldQuery for Or<($($filter,)*)> { + // SAFETY: defers to soundness of `$filter: WorldQuery` impl + unsafe impl<$($filter: WorldQuery),*> WorldQuery for Or<($($filter,)*)> { + type ReadOnly = Or<($($filter::ReadOnly,)*)>; type State = Or<($($filter::State,)*)>; fn shrink<'wlong: 'wshort, 'wshort>(item: super::QueryItem<'wlong, Self>) -> super::QueryItem<'wshort, Self> { @@ -355,13 +360,13 @@ macro_rules! impl_query_filter_tuple { #[allow(non_snake_case)] impl<'w, $($filter: WorldQueryGats<'w>),*> WorldQueryGats<'w> for Or<($($filter,)*)> { type Fetch = Or<($(OrFetch<'w, QueryFetch<'w, $filter>>,)*)>; - type ReadOnlyFetch = Or<($(OrFetch<'w, ROQueryFetch<'w, $filter>>,)*)>; type _State = Or<($($filter::_State,)*)>; } #[allow(unused_variables)] #[allow(non_snake_case)] - impl<'w, $($filter: Fetch<'w>),*> Fetch<'w> for Or<($(OrFetch<'w, $filter>,)*)> { + // SAFETY: update_component_access and update_archetype_component_access are called for each item in the tuple + unsafe impl<'w, $($filter: Fetch<'w>),*> Fetch<'w> for Or<($(OrFetch<'w, $filter>,)*)> { type State = Or<($(<$filter as Fetch<'w>>::State,)*)>; type Item = bool; @@ -423,18 +428,9 @@ macro_rules! impl_query_filter_tuple { unsafe fn archetype_filter_fetch(&mut self, archetype_index: usize) -> bool { self.archetype_fetch(archetype_index) } - } - // SAFETY: update_component_access and update_archetype_component_access are called for each item in the tuple - #[allow(unused_variables)] - #[allow(non_snake_case)] - unsafe impl<$($filter: FetchState),*> FetchState for Or<($($filter,)*)> { - fn init(world: &mut World) -> Self { - Or(($($filter::init(world),)*)) - } - - fn update_component_access(&self, access: &mut FilteredAccess) { - let ($($filter,)*) = &self.0; + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + let ($($filter,)*) = &state.0; // We do not unconditionally add `$filter`'s `with`/`without` accesses to `access` // as this would be unsound. For example the following two queries should conflict: @@ -453,11 +449,11 @@ macro_rules! impl_query_filter_tuple { $( if _not_first { let mut intermediate = access.clone(); - $filter.update_component_access(&mut intermediate); + $filter::update_component_access($filter, &mut intermediate); _intersected_access.extend_intersect_filter(&intermediate); _intersected_access.extend_access(&intermediate); } else { - $filter.update_component_access(&mut _intersected_access); + $filter::update_component_access($filter, &mut _intersected_access); _not_first = true; } )* @@ -465,9 +461,17 @@ macro_rules! impl_query_filter_tuple { *access = _intersected_access; } - fn update_archetype_component_access(&self, archetype: &Archetype, access: &mut Access) { - let ($($filter,)*) = &self.0; - $($filter.update_archetype_component_access(archetype, access);)* + fn update_archetype_component_access(state: &Self::State, archetype: &Archetype, access: &mut Access) { + let ($($filter,)*) = &state.0; + $($filter::update_archetype_component_access($filter, archetype, access);)* + } + } + + #[allow(unused_variables)] + #[allow(non_snake_case)] + impl<$($filter: FetchState),*> FetchState for Or<($($filter,)*)> { + fn init(world: &mut World) -> Self { + Or(($($filter::init(world),)*)) } fn matches_component_set(&self, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { @@ -477,7 +481,7 @@ macro_rules! impl_query_filter_tuple { } // SAFE: filters are read only - unsafe impl<'w, $($filter: Fetch<'w> + ReadOnlyFetch),*> ReadOnlyFetch for Or<($(OrFetch<'w, $filter>,)*)> {} + unsafe impl<$($filter: ReadOnlyWorldQuery),*> ReadOnlyWorldQuery for Or<($($filter,)*)> {} }; } @@ -515,7 +519,9 @@ macro_rules! impl_tick_filter { marker: PhantomData, } - impl WorldQuery for $name { + // SAFETY: `ROQueryFetch` is the same as `QueryFetch` + unsafe impl WorldQuery for $name { + type ReadOnly = Self; type State = $state_name; fn shrink<'wlong: 'wshort, 'wshort>(item: super::QueryItem<'wlong, Self>) -> super::QueryItem<'wshort, Self> { @@ -523,8 +529,7 @@ macro_rules! impl_tick_filter { } } - // SAFETY: this reads the T component. archetype component access and component access are updated to reflect that - unsafe impl FetchState for $state_name { + impl FetchState for $state_name { fn init(world: &mut World) -> Self { Self { component_id: world.init_component::(), @@ -532,26 +537,6 @@ macro_rules! impl_tick_filter { } } - #[inline] - fn update_component_access(&self, access: &mut FilteredAccess) { - if access.access().has_write(self.component_id) { - panic!("$state_name<{}> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.", - std::any::type_name::()); - } - access.add_read(self.component_id); - } - - #[inline] - fn update_archetype_component_access( - &self, - archetype: &Archetype, - access: &mut Access, - ) { - if let Some(archetype_component_id) = archetype.get_archetype_component_id(self.component_id) { - access.add_read(archetype_component_id); - } - } - fn matches_component_set(&self, set_contains_id: &impl Fn(ComponentId) -> bool) -> bool { set_contains_id(self.component_id) } @@ -559,11 +544,11 @@ macro_rules! impl_tick_filter { impl<'w, T: Component> WorldQueryGats<'w> for $name { type Fetch = $fetch_name<'w, T>; - type ReadOnlyFetch = $fetch_name<'w, T>; type _State = $state_name; } - impl<'w, T: Component> Fetch<'w> for $fetch_name<'w, T> { + // SAFETY: this reads the T component. archetype component access and component access are updated to reflect that + unsafe impl<'w, T: Component> Fetch<'w> for $fetch_name<'w, T> { type State = $state_name; type Item = bool; @@ -637,10 +622,30 @@ macro_rules! impl_tick_filter { unsafe fn archetype_filter_fetch(&mut self, archetype_index: usize) -> bool { self.archetype_fetch(archetype_index) } + + #[inline] + fn update_component_access(state: &Self::State, access: &mut FilteredAccess) { + if access.access().has_write(state.component_id) { + panic!("$state_name<{}> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.", + std::any::type_name::()); + } + access.add_read(state.component_id); + } + + #[inline] + fn update_archetype_component_access( + state: &Self::State, + archetype: &Archetype, + access: &mut Access, + ) { + if let Some(archetype_component_id) = archetype.get_archetype_component_id(state.component_id) { + access.add_read(archetype_component_id); + } + } } /// SAFETY: read-only access - unsafe impl<'w, T: Component> ReadOnlyFetch for $fetch_name<'w, T> {} + unsafe impl ReadOnlyWorldQuery for $name {} impl Clone for $fetch_name<'_, T> { fn clone(&self) -> Self { diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index ebc0e32792..b3c304f231 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -7,7 +7,7 @@ use crate::{ }; use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit}; -use super::{QueryFetch, QueryItem, ReadOnlyFetch}; +use super::{QueryFetch, QueryItem, ReadOnlyWorldQuery}; /// An [`Iterator`] over query results of a [`Query`](crate::system::Query). /// @@ -302,11 +302,11 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter< // Iterator type is intentionally implemented only for read-only access. // Doing so for mutable references would be unsound, because calling `next` // multiple times would allow multiple owned references to the same data to exist. -impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> Iterator +impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, const K: usize> Iterator for QueryCombinationIter<'w, 's, Q, F, K> where - QueryFetch<'w, Q>: Clone + ReadOnlyFetch, - QueryFetch<'w, F>: Clone + ReadOnlyFetch, + QueryFetch<'w, Q>: Clone, + QueryFetch<'w, F>: Clone, { type Item = [QueryItem<'w, Q>; K]; @@ -366,11 +366,11 @@ where } // This is correct as [`QueryCombinationIter`] always returns `None` once exhausted. -impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> FusedIterator +impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery, const K: usize> FusedIterator for QueryCombinationIter<'w, 's, Q, F, K> where - QueryFetch<'w, Q>: Clone + ReadOnlyFetch, - QueryFetch<'w, F>: Clone + ReadOnlyFetch, + QueryFetch<'w, Q>: Clone, + QueryFetch<'w, F>: Clone, { } diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index a01fc4019d..cc769886b3 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -47,13 +47,16 @@ impl QueryState { let filter_state = ::init(world); let mut component_access = FilteredAccess::default(); - fetch_state.update_component_access(&mut component_access); + QueryFetch::<'static, Q>::update_component_access(&fetch_state, &mut component_access); // Use a temporary empty FilteredAccess for filters. This prevents them from conflicting with the // main Query's `fetch_state` access. Filters are allowed to conflict with the main query fetch // because they are evaluated *before* a specific reference is constructed. let mut filter_component_access = FilteredAccess::default(); - filter_state.update_component_access(&mut filter_component_access); + QueryFetch::<'static, F>::update_component_access( + &filter_state, + &mut filter_component_access, + ); // Merge the temporary filter access with the main access. This ensures that filter access is // properly considered in a global "cross-query" context (both within systems and across systems). @@ -122,10 +125,16 @@ impl QueryState { .filter_state .matches_component_set(&|id| archetype.contains(id)) { - self.fetch_state - .update_archetype_component_access(archetype, &mut self.archetype_component_access); - self.filter_state - .update_archetype_component_access(archetype, &mut self.archetype_component_access); + QueryFetch::<'static, Q>::update_archetype_component_access( + &self.fetch_state, + archetype, + &mut self.archetype_component_access, + ); + QueryFetch::<'static, F>::update_archetype_component_access( + &self.filter_state, + archetype, + &mut self.archetype_component_access, + ); let archetype_index = archetype.id().index(); if !self.matched_archetypes.contains(archetype_index) { self.matched_archetypes.grow(archetype_index + 1); diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index 0bfe2711dc..fd4ecb408e 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -3,7 +3,7 @@ use crate::{ entity::Entity, query::{ NopFetch, QueryCombinationIter, QueryEntityError, QueryFetch, QueryItem, QueryIter, - QueryManyIter, QuerySingleError, QueryState, ROQueryFetch, ROQueryItem, ReadOnlyFetch, + QueryManyIter, QuerySingleError, QueryState, ROQueryFetch, ROQueryItem, ReadOnlyWorldQuery, WorldQuery, }, world::{Mut, World}, @@ -1306,10 +1306,7 @@ impl std::fmt::Display for QueryComponentError { } } -impl<'w, 's, Q: WorldQuery, F: WorldQuery> Query<'w, 's, Q, F> -where - QueryFetch<'w, Q>: ReadOnlyFetch, -{ +impl<'w, 's, Q: ReadOnlyWorldQuery, F: WorldQuery> Query<'w, 's, Q, F> { /// Returns the query result for the given [`Entity`], with the actual "inner" world lifetime. /// /// In case of a nonexisting entity or mismatched component, a [`QueryEntityError`] is diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 60c9c56ac7..96a574dd56 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -6,8 +6,7 @@ use crate::{ component::{Component, ComponentId, ComponentTicks, Components}, entity::{Entities, Entity}, query::{ - Access, FilteredAccess, FilteredAccessSet, QueryFetch, QueryState, ReadOnlyFetch, - WorldQuery, + Access, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery, }, system::{CommandQueue, Commands, Query, SystemMeta}, world::{FromWorld, World}, @@ -136,10 +135,7 @@ impl<'w, 's, Q: WorldQuery + 'static, F: WorldQuery + 'static> SystemParam for Q } // SAFE: QueryState is constrained to read-only fetches, so it only reads World. -unsafe impl ReadOnlySystemParamFetch for QueryState where - for<'x> QueryFetch<'x, Q>: ReadOnlyFetch -{ -} +unsafe impl ReadOnlySystemParamFetch for QueryState {} // SAFE: Relevant query ComponentId and ArchetypeComponentId access is applied to SystemMeta. If // this QueryState conflicts with any prior access, a panic will occur. diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr index 870e2fb313..a6da80b259 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/system_param_derive_readonly.stderr @@ -6,12 +6,13 @@ warning: unused import: `SystemState` | = note: `#[warn(unused_imports)]` on by default -error[E0277]: the trait bound `for<'x> WriteFetch<'x, Foo>: ReadOnlyFetch` is not satisfied +error[E0277]: the trait bound `&'static mut Foo: ReadOnlyWorldQuery` is not satisfied --> tests/ui/system_param_derive_readonly.rs:18:5 | 18 | assert_readonly::(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `for<'x> ReadOnlyFetch` is not implemented for `WriteFetch<'x, Foo>` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `ReadOnlyWorldQuery` is not implemented for `&'static mut Foo` | + = note: `ReadOnlyWorldQuery` is implemented for `&'static Foo`, but not for `&'static mut Foo` = note: required because of the requirements on the impl of `ReadOnlySystemParamFetch` for `QueryState<&'static mut Foo>` = note: 2 redundant requirements hidden = note: required because of the requirements on the impl of `ReadOnlySystemParamFetch` for `_::FetchState<(QueryState<&'static mut Foo>,)>` diff --git a/crates/bevy_ecs_compile_fail_tests/tests/ui/world_query_derive.stderr b/crates/bevy_ecs_compile_fail_tests/tests/ui/world_query_derive.stderr index 6ced5ae9c7..397c5685b6 100644 --- a/crates/bevy_ecs_compile_fail_tests/tests/ui/world_query_derive.stderr +++ b/crates/bevy_ecs_compile_fail_tests/tests/ui/world_query_derive.stderr @@ -1,8 +1,8 @@ -error[E0277]: the trait bound `WriteFetch<'_, Foo>: ReadOnlyFetch` is not satisfied - --> tests/ui/world_query_derive.rs:7:10 +error[E0277]: the trait bound `&'static mut Foo: ReadOnlyWorldQuery` is not satisfied + --> tests/ui/world_query_derive.rs:9:8 | -7 | #[derive(WorldQuery)] - | ^^^^^^^^^^ the trait `ReadOnlyFetch` is not implemented for `WriteFetch<'_, Foo>` +9 | a: &'static mut Foo, + | ^^^^^^^^^^^^^^^^ the trait `ReadOnlyWorldQuery` is not implemented for `&'static mut Foo` | note: required by a bound in `_::assert_readonly` --> tests/ui/world_query_derive.rs:7:10 @@ -11,11 +11,11 @@ note: required by a bound in `_::assert_readonly` | ^^^^^^^^^^ required by this bound in `_::assert_readonly` = note: this error originates in the derive macro `WorldQuery` (in Nightly builds, run with -Z macro-backtrace for more info) -error[E0277]: the trait bound `MutableMarkedFetch<'_>: ReadOnlyFetch` is not satisfied - --> tests/ui/world_query_derive.rs:18:10 +error[E0277]: the trait bound `MutableMarked: ReadOnlyWorldQuery` is not satisfied + --> tests/ui/world_query_derive.rs:20:8 | -18 | #[derive(WorldQuery)] - | ^^^^^^^^^^ the trait `ReadOnlyFetch` is not implemented for `MutableMarkedFetch<'_>` +20 | a: MutableMarked, + | ^^^^^^^^^^^^^ the trait `ReadOnlyWorldQuery` is not implemented for `MutableMarked` | note: required by a bound in `_::assert_readonly` --> tests/ui/world_query_derive.rs:18:10