diff --git a/_release-content/migration-guides/querydata_safety.md b/_release-content/migration-guides/querydata_safety.md new file mode 100644 index 0000000000000..a5adec6a77686 --- /dev/null +++ b/_release-content/migration-guides/querydata_safety.md @@ -0,0 +1,21 @@ +--- +title: Safety requirements for `QueryData::ReadOnly` have been clarified +pull_requests: [25652] +--- + +The safety requirements for `QueryData::ReadOnly` have been reworded, +Any existing sound implementation should still be sound, +but manual implementations of `QueryData` will want to update their `SAFETY` comments to match the new wording. + +The type system constraint that `ReadOnly::State == Self::State` has been relaxed +in favor of a safety constraint that `Self::State` can be *transmuted* to `ReadOnly::State`. + +In addition, the safety requirements are now explicit that the state must be a *valid* value of `ReadOnly::State`. +For example, `&T` has `type State = ComponentId;`, +and has always required that the value be the `ComponentId` of `T`, +but this was not noted explicitly in the safety requirements. + +Read-only `QueryData` that has `type ReadOnly = Self;` can use +```rust +// SAFETY: `Self` is the same as `Self::ReadOnly` +``` diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 13cd5501e5a81..55e91868d7f71 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -311,8 +311,10 @@ 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` +/// - It must be valid to transmute `Self::State` to `Self::ReadOnly::State`, +/// and the resulting `Self::ReadOnly` must have a non-strict subset of the access of `Self` +/// and must match exactly the same archetypes/tables as `Self`. +/// Note that this is trivially true if `Self::ReadOnly == Self`. /// - `IS_READ_ONLY` must be `true` if and only if `Self: ReadOnlyQueryData` /// /// [`ReadOnly`]: Self::ReadOnly @@ -335,7 +337,7 @@ pub unsafe trait QueryData: WorldQuery { const IS_ARCHETYPAL: bool; /// The read-only variant of this [`QueryData`], which satisfies the [`ReadOnlyQueryData`] trait. - type ReadOnly: ReadOnlyQueryData::State>; + type ReadOnly: ReadOnlyQueryData; /// The item returned by this [`WorldQuery`] /// This will be the data retrieved by the query, @@ -879,9 +881,7 @@ unsafe impl WorldQuery for SpawnDetails { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } -// SAFETY: -// No components are accessed. -// Is its own ReadOnlyQueryData. +// SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for SpawnDetails { const IS_READ_ONLY: bool = true; const IS_ARCHETYPAL: bool = true; @@ -1149,7 +1149,12 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } -// SAFETY: access of `EntityRef` is a subset of `EntityMut` +// SAFETY: +// - `State` is `()` for both `EntityMut` and `EntityRef`, +// so transmuting always results in a valid state. +// - `EntityMut` has full read and write access and +// `EntityRef` has full read access, so the access is a subset +// - Both `EntityMut` and `EntityRef` match all entities. unsafe impl<'a> QueryData for EntityMut<'a> { const IS_READ_ONLY: bool = false; const IS_ARCHETYPAL: bool = true; @@ -1415,7 +1420,12 @@ unsafe impl WorldQuery for FilteredEntityMut<'_, '_> { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } -// SAFETY: access of `FilteredEntityRef` is a subset of `FilteredEntityMut` +// SAFETY: +// - `State` is `Access` for both `FilteredEntityMut` and `FilteredEntityRef`. +// `FilteredEntityRef` accepts any `Access`, so the transmute is always valid. +// - `FilteredEntityMut` has all read and write access from the `Access` and +// `FilteredEntityRef` has all read access from the same `Access`, so the access is a subset +// - Both `FilteredEntityMut` and `FilteredEntityRef` match all entities. unsafe impl<'a, 'b> QueryData for FilteredEntityMut<'a, 'b> { const IS_READ_ONLY: bool = false; const IS_ARCHETYPAL: bool = true; @@ -1697,8 +1707,13 @@ where fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } -// SAFETY: All accesses that `EntityRefExcept` provides are also accesses that -// `EntityMutExcept` provides. +// SAFETY: +// - `State` for both `EntityRefExcept` and `EntityMutExcept` is an `Access` +// with access to all components except those in `B` in the current world. +// `EntityRefExcept` will ignore the extra write access in the transmuted `Access`. +// - `EntityMutExcept` has all read and write access except to components in `B` and +// `EntityRefExcept` has all read access access except to components in `B`, so the access is a subset +// - Both `EntityMutExcept` and `EntityRefExcept` match all entities. unsafe impl<'a, 'b, B> QueryData for EntityMutExcept<'a, 'b, B> where B: Bundle, @@ -2514,7 +2529,10 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } -// SAFETY: access of `&T` is a subset of `&mut T` +// SAFETY: `State` for both `&T` and `&mut T` is +// the `ComponentId` of `T` in the current world, +// and `&T` matches the same archetypes as `&mut T` +// with a subset of the access. unsafe impl<'__w, T: Component> QueryData for &'__w mut T { const IS_READ_ONLY: bool = false; const IS_ARCHETYPAL: bool = true; @@ -2746,7 +2764,10 @@ unsafe impl<'__w, T: Component> WorldQuery for Mut<'__w, T> { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } -// SAFETY: access of `Ref` is a subset of `Mut` +// SAFETY: `State` for both `Ref` and `Mut` is +// the `ComponentId` of `T` in the current world, +// and `Ref` matches the same archetypes as `Mut` +// with a subset of the access. unsafe impl<'__w, T: Component> QueryData for Mut<'__w, T> { const IS_READ_ONLY: bool = false; const IS_ARCHETYPAL: bool = true; @@ -2812,7 +2833,7 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// It is not normally useful to query directly, /// since it's equivalent to adding another [`Query`] parameter to a system. /// -/// Note that this requires the inner query to be a [`ReadOnlyQueryData`] +/// Note that this is only an [`IterQueryData`] if the underlying query data is [`ReadOnlyQueryData`], /// to prevent mutable aliasing. /// /// ``` @@ -2821,11 +2842,13 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// # /// # #[derive(Component)] /// # struct A; -/// fn system(mut query: Query>) { +/// fn system(mut query: Query>, entity: Entity) { /// // This works, because it performs read-only iteration /// for a in &query { /// let a: Query<&A> = a; /// } +/// // And this works, because it can only be called for one entity at a time +/// let a: Query<&mut A> = query.get_mut(entity).unwrap(); /// } /// ``` /// @@ -2838,6 +2861,7 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// fn system(mut query: Query>) { /// // This fails, because it would allow mutable aliasing of `&mut A` /// for a in &mut query { +/// // ^^^^^^^^^^ `&mut bevy_ecs::system::Query<'_, '_, NestedQuery<&mut A>>` is not an iterator /// let a: Query<&mut A> = a; /// } /// } @@ -2924,7 +2948,7 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// // This is the relational query data. /// // This will never actually be constructed, /// // and is only used as a `QueryData` type. -/// pub struct Parent(D, F); +/// pub struct Parent(D, F); /// /// // A type alias to delegate the `QueryData` impls to. /// // We need to refer to this type a lot, so the alias will help. @@ -2938,7 +2962,7 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// NestedQuery, /// ); /// -/// unsafe impl QueryData for Parent { +/// unsafe impl QueryData for Parent { /// // Set `Item` to what we need for this relational query. /// // Here we use the output of `D`. /// type Item<'w, 's> = D::Item<'w, 's>; @@ -2958,7 +2982,7 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// /// // Set `ReadOnly` to `Self`, /// // as `NestedQuery` does not yet support mutable queries. -/// type ReadOnly = Self; +/// type ReadOnly = Parent; /// /// // Delegate everything else on `QueryData` and `WorldQuery` to the type alias. /// // This is sound for `unsafe` items because they delegate to the @@ -2971,7 +2995,7 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// } /// } /// -/// unsafe impl WorldQuery for Parent { +/// unsafe impl WorldQuery for Parent { /// type Fetch<'w> = as WorldQuery>::Fetch<'w>; /// type State = as WorldQuery>::State; /// @@ -3018,15 +3042,17 @@ impl<'__w, T: Component> ContiguousQueryData for Mut<'__w, /// } /// } /// -/// // Also impl `ReadOnlyQueryData`, `IterQueryData`, and `ReleaseStateQueryData` -/// // These are safe because they delegate to the type alias, which is also read-only. +/// // Also impl `ReadOnlyQueryData`, `IterQueryData`, and `ReleaseStateQueryData`. +/// // `ReadOnlyQueryData` and `IterQueryData` must only be implemented for read-only queries. +/// // Multiple entities may have the same parent, and iteration allows values for multiple entities to be alive, +/// // so iterating `Query>` could cause mutable aliasing on `T`. /// // Do *not* impl `ArchetypeQueryData`, because `fetch` sometimes returns `None`, /// // and do *not* impl `SingleEntityQueryData`, because `NestedQuery` accesses other entities. /// unsafe impl ReadOnlyQueryData for Parent {} /// /// unsafe impl IterQueryData for Parent {} /// -/// impl +/// impl /// ReleaseStateQueryData for Parent /// { /// fn release_state<'w>(item: Self::Item<'w, '_>) -> Self::Item<'w, 'static> { @@ -3049,9 +3075,7 @@ pub struct NestedQueryFetch<'w> { // SAFETY: // Does not access any components on the current entity // Accesses through the nested query are registered in `init_nested_access` -unsafe impl WorldQuery - for NestedQuery -{ +unsafe impl WorldQuery for NestedQuery { type Fetch<'w> = NestedQueryFetch<'w>; type State = QueryState; @@ -3131,18 +3155,18 @@ unsafe impl WorldQuery } // SAFETY: -// `Self::ReadOnly` accesses `D::ReadOnly`, which is a subset of the data accessed by `D` -// `IS_READ_ONLY` iff `D::IS_READ_ONLY` iff `D: ReadOnlyQueryData` iff `Self: ReadOnlyQueryData` -unsafe impl QueryData - for NestedQuery -{ +// - `Self::State` is a `QueryState`, which can always be transmuted to a valid `QueryState`. +// - `Self::ReadOnly` accesses `D::ReadOnly`, which is a non-strict subset of the data accessed by `D`. +// - `NestedQuery` always matches all entities. +// - `IS_READ_ONLY` iff `D::IS_READ_ONLY` iff `D: ReadOnlyQueryData` iff `Self: ReadOnlyQueryData` +unsafe impl QueryData for NestedQuery { const IS_READ_ONLY: bool = D::IS_READ_ONLY; // Nested queries are always archetypal because `fetch` always returns `Some`. // If `D::IS_ARCHETYPAL == false` or `F::IS_ARCHETYPAL == false`, // then the nested query may filter out some entities that *it* matches, // but it will not filter the outer query. const IS_ARCHETYPAL: bool = true; - type ReadOnly = NestedQuery; + type ReadOnly = NestedQuery; type Item<'w, 's> = Query<'w, 's, D, F>; fn shrink<'wlong: 'wshort, 'wshort, 's>( @@ -3191,7 +3215,7 @@ unsafe impl IterQueryData for NestedQuery< // If `D::IS_ARCHETYPAL == false` or `F::IS_ARCHETYPAL == false`, // then the nested query may filter out some entities that *it* matches, // but it will never filter the outer query. -impl ArchetypeQueryData for NestedQuery {} +impl ArchetypeQueryData for NestedQuery {} #[doc(hidden)] pub struct OptionFetch<'w, T: WorldQuery> { diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index 083d47ca0f1ce..fa0326cbe48a8 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -122,9 +122,10 @@ impl FromWorld for QueryState { impl QueryState { /// Converts this `QueryState` reference to a `QueryState` that does not access anything mutably. pub fn as_readonly(&self) -> &QueryState { - // SAFETY: invariant on `WorldQuery` trait upholds that `D::ReadOnly` and `F::ReadOnly` - // have a subset of the access, and match the exact same archetypes/tables as `D`/`F` respectively. - unsafe { self.as_transmuted_state::() } + // SAFETY: `WorldQuery` trait ensures `D::State` may be transmuted to + // `D::ReadOnly::State` and that the resulting `QueryData` + // has a non-strict subset of the access and matches the exact same archetypes/tables as `D`. + unsafe { self.as_transmuted_state::() } } /// Converts this `QueryState` reference to a `QueryState` that does not return any data @@ -133,9 +134,10 @@ impl QueryState { /// This doesn't use `NopWorldQuery` as it loses filter functionality, for example /// `NopWorldQuery>` is functionally equivalent to `With`. pub(crate) fn as_nop(&self) -> &QueryState, F> { - // SAFETY: `NopWorldQuery` doesn't have any accesses and defers to + // SAFETY: `NopWorldQuery::State == D::State`, + // `NopWorldQuery` doesn't have any accesses and defers to // `D` for table/archetype matching - unsafe { self.as_transmuted_state::, F>() } + unsafe { self.as_transmuted_state::>() } } /// Converts this `QueryState` reference to any other `QueryState` with @@ -145,15 +147,13 @@ impl QueryState { /// /// # Safety /// - /// `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: ReadOnlyQueryData, - NewF: QueryFilter, - >( + /// It must be valid to transmute `D::State` to `NewD::State`, + /// and the resulting state must have a non-strict subset of the access that + /// `D` does and match the exact same archetypes/tables + pub(crate) unsafe fn as_transmuted_state( &self, - ) -> &QueryState { - &*ptr::from_ref(self).cast::>() + ) -> &QueryState { + &*ptr::from_ref(self).cast::>() } /// Returns the components accessed by this query. diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 11328d2223c69..5d05bd66be91f 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -918,6 +918,33 @@ mod tests { run_system(&mut world, sys); } + #[test] + #[should_panic = "error[B0001]"] + fn mut_nested_query_conflicts_with_main_query() { + fn sys(_: Query<(&A, NestedQuery<&mut A>)>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn mut_nested_query_conflicts_with_earlier_query() { + fn sys(_: Query<&A>, _: Query>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + + #[test] + #[should_panic = "error[B0001]"] + fn mut_nested_query_conflicts_with_later_query() { + fn sys(_: Query>, _: Query<&A>) {} + + let mut world = World::default(); + run_system(&mut world, sys); + } + #[test] fn query_set_system() { fn sys(mut _set: ParamSet<(Query<&mut A>, Query<&A>)>) {} diff --git a/crates/bevy_extract/src/sync_world.rs b/crates/bevy_extract/src/sync_world.rs index 9ed9bf0e15297..0ec2b9c70532c 100644 --- a/crates/bevy_extract/src/sync_world.rs +++ b/crates/bevy_extract/src/sync_world.rs @@ -375,8 +375,7 @@ mod sub_entities_world_query_impls { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } - // SAFETY: Component access of Self::ReadOnly is a subset of Self. - // Self::ReadOnly matches exactly the same archetypes/tables as Self. + // SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for SubEntity { const IS_READ_ONLY: bool = true; const IS_ARCHETYPAL: bool = <&MainEntity as QueryData>::IS_ARCHETYPAL; @@ -506,8 +505,7 @@ mod sub_entities_world_query_impls { fn update_archetypes(_state: &mut Self::State, _world: UnsafeWorldCell) {} } - // SAFETY: Component access of Self::ReadOnly is a subset of Self. - // Self::ReadOnly matches exactly the same archetypes/tables as Self. + // SAFETY: `Self` is the same as `Self::ReadOnly` unsafe impl QueryData for MainEntity { const IS_READ_ONLY: bool = true; const IS_ARCHETYPAL: bool = <&MainEntity as QueryData>::IS_ARCHETYPAL;