diff --git a/crates/bevy_ecs/macros/src/query_data.rs b/crates/bevy_ecs/macros/src/query_data.rs index ffac58ef1d..d919d0b05e 100644 --- a/crates/bevy_ecs/macros/src/query_data.rs +++ b/crates/bevy_ecs/macros/src/query_data.rs @@ -254,6 +254,7 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream { /// SAFETY: we assert fields are readonly below unsafe impl #user_impl_generics #path::query::QueryData for #read_only_struct_name #user_ty_generics #user_where_clauses { + const IS_READ_ONLY: bool = true; type ReadOnly = #read_only_struct_name #user_ty_generics; type Item<'__w> = #read_only_item_struct_name #user_ty_generics_with_world; @@ -284,10 +285,13 @@ pub fn derive_query_data_impl(input: TokenStream) -> TokenStream { quote! {} }; + let is_read_only = !attributes.is_mutable; + quote! { /// SAFETY: we assert fields are readonly below unsafe impl #user_impl_generics #path::query::QueryData for #struct_name #user_ty_generics #user_where_clauses { + const IS_READ_ONLY: bool = #is_read_only; type ReadOnly = #read_only_struct_name #user_ty_generics; type Item<'__w> = #item_struct_name #user_ty_generics_with_world; diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 08e06d03f4..cd632f7b14 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -265,8 +265,9 @@ use variadics_please::all_tuples; /// /// # Safety /// -/// Component access of `Self::ReadOnly` must be a subset of `Self` -/// and `Self::ReadOnly` must match exactly the same archetypes/tables as `Self` +/// - Component access of `Self::ReadOnly` must be a subset of `Self` +/// and `Self::ReadOnly` must match exactly the same archetypes/tables as `Self` +/// - `IS_READ_ONLY` must be `true` if and only if `Self: ReadOnlyQueryData` /// /// [`Query`]: crate::system::Query /// [`ReadOnly`]: Self::ReadOnly @@ -276,6 +277,9 @@ use variadics_please::all_tuples; note = "if `{Self}` is a component type, try using `&{Self}` or `&mut {Self}`" )] pub unsafe trait QueryData: WorldQuery { + /// True if this query is read-only and may not perform mutable access. + const IS_READ_ONLY: bool; + /// The read-only variant of this [`QueryData`], which satisfies the [`ReadOnlyQueryData`] trait. type ReadOnly: ReadOnlyQueryData::State>; @@ -367,6 +371,7 @@ unsafe impl WorldQuery for Entity { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for Entity { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = Entity; @@ -443,6 +448,7 @@ unsafe impl WorldQuery for EntityLocation { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for EntityLocation { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = EntityLocation; @@ -524,6 +530,7 @@ unsafe impl<'a> WorldQuery for EntityRef<'a> { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl<'a> QueryData for EntityRef<'a> { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = EntityRef<'w>; @@ -604,6 +611,7 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { /// SAFETY: access of `EntityRef` is a subset of `EntityMut` unsafe impl<'a> QueryData for EntityMut<'a> { + const IS_READ_ONLY: bool = false; type ReadOnly = EntityRef<'a>; type Item<'w> = EntityMut<'w>; @@ -696,6 +704,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityRef<'a> { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl<'a> QueryData for FilteredEntityRef<'a> { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = FilteredEntityRef<'w>; @@ -790,6 +799,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityMut<'a> { /// SAFETY: access of `FilteredEntityRef` is a subset of `FilteredEntityMut` unsafe impl<'a> QueryData for FilteredEntityMut<'a> { + const IS_READ_ONLY: bool = false; type ReadOnly = FilteredEntityRef<'a>; type Item<'w> = FilteredEntityMut<'w>; @@ -888,6 +898,7 @@ unsafe impl<'a, B> QueryData for EntityRefExcept<'a, B> where B: Bundle, { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = EntityRefExcept<'w, B>; @@ -988,6 +999,7 @@ unsafe impl<'a, B> QueryData for EntityMutExcept<'a, B> where B: Bundle, { + const IS_READ_ONLY: bool = false; type ReadOnly = EntityRefExcept<'a, B>; type Item<'w> = EntityMutExcept<'w, B>; @@ -1060,6 +1072,7 @@ unsafe impl WorldQuery for &Archetype { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for &Archetype { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = &'w Archetype; @@ -1204,6 +1217,7 @@ unsafe impl WorldQuery for &T { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for &T { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = &'w T; @@ -1375,6 +1389,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl<'__w, T: Component> QueryData for Ref<'__w, T> { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = Ref<'w, T>; @@ -1569,6 +1584,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { /// SAFETY: access of `&T` is a subset of `&mut T` unsafe impl<'__w, T: Component> QueryData for &'__w mut T { + const IS_READ_ONLY: bool = false; type ReadOnly = &'__w T; type Item<'w> = Mut<'w, T>; @@ -1711,6 +1727,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Mut<'__w, T> { // SAFETY: access of `Ref` is a subset of `Mut` unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { + const IS_READ_ONLY: bool = false; type ReadOnly = Ref<'__w, T>; type Item<'w> = Mut<'w, T>; @@ -1838,6 +1855,7 @@ unsafe impl WorldQuery for Option { // SAFETY: defers to soundness of `T: WorldQuery` impl unsafe impl QueryData for Option { + const IS_READ_ONLY: bool = T::IS_READ_ONLY; type ReadOnly = Option; type Item<'w> = Option>; @@ -2001,6 +2019,7 @@ unsafe impl WorldQuery for Has { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for Has { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = bool; @@ -2049,6 +2068,7 @@ macro_rules! impl_tuple_query_data { $(#[$meta])* // SAFETY: defers to soundness `$name: WorldQuery` impl unsafe impl<$($name: QueryData),*> QueryData for ($($name,)*) { + const IS_READ_ONLY: bool = true $(&& $name::IS_READ_ONLY)*; type ReadOnly = ($($name::ReadOnly,)*); type Item<'w> = ($($name::Item<'w>,)*); @@ -2211,6 +2231,7 @@ macro_rules! impl_anytuple_fetch { $(#[$meta])* // SAFETY: defers to soundness of `$name: WorldQuery` impl unsafe impl<$($name: QueryData),*> QueryData for AnyOf<($($name,)*)> { + const IS_READ_ONLY: bool = true $(&& $name::IS_READ_ONLY)*; type ReadOnly = AnyOf<($($name::ReadOnly,)*)>; type Item<'w> = ($(Option<$name::Item<'w>>,)*); @@ -2315,6 +2336,7 @@ unsafe impl WorldQuery for NopWorldQuery { /// SAFETY: `Self::ReadOnly` is `Self` unsafe impl QueryData for NopWorldQuery { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = (); @@ -2384,6 +2406,7 @@ unsafe impl WorldQuery for PhantomData { /// SAFETY: `Self::ReadOnly` is `Self` unsafe impl QueryData for PhantomData { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'a> = (); diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 9c8e2323c2..9ed6995876 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -881,6 +881,7 @@ mod tests { /// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for ReadsRData { + const IS_READ_ONLY: bool = true; type ReadOnly = Self; type Item<'w> = (); diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index f6a40610b8..7013520f94 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -72,6 +72,9 @@ pub struct QueryState { pub(crate) matched_archetypes: FixedBitSet, /// [`FilteredAccess`] computed by combining the `D` and `F` access. Used to check which other queries /// this query can run in parallel with. + /// Note that because we do a zero-cost reference conversion in `Query::as_readonly`, + /// the access for a read-only query may include accesses for the original mutable version, + /// but the `Query` does not have exclusive access to those components. pub(crate) component_access: FilteredAccess, // NOTE: we maintain both a bitset and a vec because iterating the vec is faster pub(super) matched_storage_ids: Vec, @@ -132,7 +135,7 @@ impl QueryState { /// `NewD` must have a subset of the access that `D` does and match the exact same archetypes/tables /// `NewF` must have a subset of the access that `F` does and match the exact same archetypes/tables pub(crate) unsafe fn as_transmuted_state< - NewD: QueryData, + NewD: ReadOnlyQueryData, NewF: QueryFilter, >( &self, @@ -750,7 +753,21 @@ impl QueryState { let mut fetch_state = NewD::get_state(world.components()).expect("Could not create fetch_state, Please initialize all referenced components before transmuting."); let filter_state = NewF::get_state(world.components()).expect("Could not create filter_state, Please initialize all referenced components before transmuting."); - NewD::set_access(&mut fetch_state, &self.component_access); + fn to_readonly(mut access: FilteredAccess) -> FilteredAccess { + access.access_mut().clear_writes(); + access + } + + let self_access = if D::IS_READ_ONLY && self.component_access.access().has_any_write() { + // The current state was transmuted from a mutable + // `QueryData` to a read-only one. + // Ignore any write access in the current state. + &to_readonly(self.component_access.clone()) + } else { + &self.component_access + }; + + NewD::set_access(&mut fetch_state, self_access); NewD::update_component_access(&fetch_state, &mut component_access); let mut filter_component_access = FilteredAccess::default(); @@ -758,7 +775,7 @@ impl QueryState { component_access.extend(&filter_component_access); assert!( - component_access.is_subset(&self.component_access), + component_access.is_subset(self_access), "Transmuted state for {} attempts to access terms that are not allowed by original state {}.", core::any::type_name::<(NewD, NewF)>(), core::any::type_name::<(D, F)>() ); @@ -840,7 +857,31 @@ impl QueryState { let new_filter_state = NewF::get_state(world.components()) .expect("Could not create filter_state, Please initialize all referenced components before transmuting."); - NewD::set_access(&mut new_fetch_state, &self.component_access); + let mut joined_component_access = self.component_access.clone(); + joined_component_access.extend(&other.component_access); + + if D::IS_READ_ONLY && self.component_access.access().has_any_write() + || OtherD::IS_READ_ONLY && other.component_access.access().has_any_write() + { + // One of the input states was transmuted from a mutable + // `QueryData` to a read-only one. + // Ignore any write access in that current state. + // The simplest way to do this is to clear *all* writes + // and then add back in any writes that are valid + joined_component_access.access_mut().clear_writes(); + if !D::IS_READ_ONLY { + joined_component_access + .access_mut() + .extend(self.component_access.access()); + } + if !OtherD::IS_READ_ONLY { + joined_component_access + .access_mut() + .extend(other.component_access.access()); + } + } + + NewD::set_access(&mut new_fetch_state, &joined_component_access); NewD::update_component_access(&new_fetch_state, &mut component_access); let mut new_filter_component_access = FilteredAccess::default(); @@ -848,9 +889,6 @@ impl QueryState { component_access.extend(&new_filter_component_access); - let mut joined_component_access = self.component_access.clone(); - joined_component_access.extend(&other.component_access); - assert!( component_access.is_subset(&joined_component_access), "Joined state for {} attempts to access terms that are not allowed by state {} joined with {}.", @@ -1759,8 +1797,11 @@ impl From> for QueryState>(&world); } + #[test] + #[should_panic( + expected = "Transmuted state for (&mut bevy_ecs::query::state::tests::A, ()) attempts to access terms that are not allowed by original state (&bevy_ecs::query::state::tests::A, ())." + )] + fn cannot_transmute_mutable_after_readonly() { + let mut world = World::new(); + // Calling this method would mean we had aliasing queries. + fn bad(_: Query<&mut A>, _: Query<&A>) {} + world + .run_system_once(|query: Query<&mut A>| { + let mut readonly = query.as_readonly(); + let mut lens: QueryLens<&mut A> = readonly.transmute_lens(); + bad(lens.query(), query.as_readonly()); + }) + .unwrap(); + } + // Regression test for #14629 #[test] #[should_panic] @@ -2120,6 +2178,37 @@ mod tests { let _: QueryState> = query_1.join_filtered(&world, &query_2); } + #[test] + #[should_panic( + expected = "Joined state for ((&mut bevy_ecs::query::state::tests::A, &mut bevy_ecs::query::state::tests::B), ()) attempts to access terms that are not allowed by state (&bevy_ecs::query::state::tests::A, ()) joined with (&mut bevy_ecs::query::state::tests::B, ())." + )] + fn cannot_join_mutable_after_readonly() { + let mut world = World::new(); + // Calling this method would mean we had aliasing queries. + fn bad(_: Query<(&mut A, &mut B)>, _: Query<&A>) {} + world + .run_system_once(|query_a: Query<&mut A>, mut query_b: Query<&mut B>| { + let mut readonly = query_a.as_readonly(); + let mut lens: QueryLens<(&mut A, &mut B)> = readonly.join(&mut query_b); + bad(lens.query(), query_a.as_readonly()); + }) + .unwrap(); + } + + #[test] + fn join_to_filtered_entity_mut() { + let mut world = World::new(); + world.spawn((A(2), B(3))); + + let query_1 = QueryState::<&mut A>::new(&mut world); + let query_2 = QueryState::<&mut B>::new(&mut world); + let mut new_query: QueryState = query_1.join(&world, &query_2); + + let mut entity = new_query.single_mut(&mut world).unwrap(); + assert!(entity.get_mut::().is_some()); + assert!(entity.get_mut::().is_some()); + } + #[test] fn query_respects_default_filters() { let mut world = World::new(); diff --git a/crates/bevy_ecs/src/system/query.rs b/crates/bevy_ecs/src/system/query.rs index fff2a91b2f..a9662f28c9 100644 --- a/crates/bevy_ecs/src/system/query.rs +++ b/crates/bevy_ecs/src/system/query.rs @@ -441,10 +441,13 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> { } /// Returns another `Query` from this does not return any data, which can be faster. - fn as_nop(&self) -> Query<'w, 's, NopWorldQuery, F> { + fn as_nop(&self) -> Query<'_, 's, NopWorldQuery, F> { let new_state = self.state.as_nop(); // SAFETY: - // - This is memory safe because it performs no access. + // - The reborrowed query is converted to read-only, so it cannot perform mutable access, + // and the original query is held with a shared borrow, so it cannot perform mutable access either. + // Note that although `NopWorldQuery` itself performs *no* access and could soundly alias a mutable query, + // it has the original `QueryState::component_access` and could be `transmute`d to a read-only query. // - The world matches because it was the same one used to construct self. unsafe { Query::new(self.world, new_state, self.last_run, self.this_run) } } diff --git a/crates/bevy_render/src/sync_world.rs b/crates/bevy_render/src/sync_world.rs index 8d4bb89f5a..83fabd40fd 100644 --- a/crates/bevy_render/src/sync_world.rs +++ b/crates/bevy_render/src/sync_world.rs @@ -358,6 +358,7 @@ mod render_entities_world_query_impls { // SAFETY: Component access of Self::ReadOnly is a subset of Self. // Self::ReadOnly matches exactly the same archetypes/tables as Self. unsafe impl QueryData for RenderEntity { + const IS_READ_ONLY: bool = true; type ReadOnly = RenderEntity; type Item<'w> = Entity; @@ -457,6 +458,7 @@ mod render_entities_world_query_impls { // SAFETY: Component access of Self::ReadOnly is a subset of Self. // Self::ReadOnly matches exactly the same archetypes/tables as Self. unsafe impl QueryData for MainEntity { + const IS_READ_ONLY: bool = true; type ReadOnly = MainEntity; type Item<'w> = Entity;