forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

constrain WorldQuery::get_state to only use &Components (#13343)

Browse Source 
# Objective

Passing `&World` in the `WorldQuery::get_state` method is unnecessary,
as all implementations of this method in the engine either only access
`Components` in `&World`, or do nothing with it.
It can introduce UB by necessitating the creation of a `&World` from a
`UnsafeWorldCell`.
This currently happens in `Query::transmute_lens`, which obtains a
`&World` from the internal `UnsafeWorldCell` solely to pass to
`get_state`. `Query::join` suffers from the same issue.
Other cases of UB come from allowing implementors of `WorldQuery` to
freely access `&World`, like in the `bevy-trait-query` crate, where a
[reference to a resource is
obtained](0c0e7dd646/src/lib.rs (L445))
inside of
[`get_state`](0c0e7dd646/src/one.rs (L245)),
potentially aliasing with a `ResMut` parameter in the same system.

`WorldQuery::init_state` currently requires `&mut World`, which doesn't
suffer from these issues.
But that too can be changed to receive a wrapper around `&mut
Components` and `&mut Storages` for consistency in a follow-up PR.

## Solution

Replace the `&World` parameter in `get_state` with `&Components`.

## Changelog

 `WorldQuery::get_state` now takes `&Components` instead of `&World`.
The `transmute`, `transmute_filtered`, `join` and `join_filtered`
methods on `QueryState` now similarly take `&Components` instead of
`&World`.

## Migration Guide

Users of `WorldQuery::get_state` or `transmute`, `transmute_filtered`,
`join` and `join_filtered` methods on `QueryState` now need to pass
`&Components` instead of `&World`.
`&Components` can be trivially obtained from either `components` method
on `&World` or `UnsafeWorldCell`.
For implementors of `WorldQuery::get_state` that were accessing more
than the `Components` inside `&World` and its methods, this is no longer
allowed.
This commit is contained in:
Vic 2024-05-13 23:00:01 +02:00 committed by GitHub
parent 2037b880ac
commit 0eb4bb6bab
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 89 additions and 95 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
crates/bevy_ecs/macros/src/world_query.rs
View File

@ -170,9 +170,9 @@ pub(crate) fn world_query_impl(
}
}
fn get_state(world: &#path::world::World) -> Option<#state_struct_name #user_ty_generics> {
fn get_state(components: &#path::component::Components) -> Option<#state_struct_name #user_ty_generics> {
Some(#state_struct_name {
#(#named_field_idents: <#field_types>::get_state(world)?,)*
#(#named_field_idents: <#field_types>::get_state(components)?,)*
})
}

54
crates/bevy_ecs/src/query/fetch.rs
View File

@ -1,7 +1,7 @@
use crate::{
archetype::{Archetype, Archetypes},
change_detection::{Ticks, TicksMut},
component::{Component, ComponentId, StorageType, Tick},
component::{Component, ComponentId, Components, StorageType, Tick},
entity::{Entities, Entity, EntityLocation},
query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery},
storage::{ComponentSparseSet, Table, TableRow},
@ -333,7 +333,7 @@ unsafe impl WorldQuery for Entity {
fn init_state(_world: &mut World) {}
fn get_state(_world: &World) -> Option<()> {
fn get_state(_components: &Components) -> Option<()> {
Some(())
}
@ -405,7 +405,7 @@ unsafe impl WorldQuery for EntityLocation {
fn init_state(_world: &mut World) {}
fn get_state(_world: &World) -> Option<()> {
fn get_state(_components: &Components) -> Option<()> {
Some(())
}
@ -484,7 +484,7 @@ unsafe impl<'a> WorldQuery for EntityRef<'a> {
fn init_state(_world: &mut World) {}
fn get_state(_world: &World) -> Option<()> {
fn get_state(_components: &Components) -> Option<()> {
Some(())
}
@ -560,7 +560,7 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> {
fn init_state(_world: &mut World) {}
fn get_state(_world: &World) -> Option<()> {
fn get_state(_components: &Components) -> Option<()> {
Some(())
}
@ -660,7 +660,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityRef<'a> {
FilteredAccess::default()
}
fn get_state(_world: &World) -> Option<Self::State> {
fn get_state(_components: &Components) -> Option<Self::State> {
Some(FilteredAccess::default())
}
@ -772,7 +772,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityMut<'a> {
FilteredAccess::default()
}
fn get_state(_world: &World) -> Option<Self::State> {
fn get_state(_components: &Components) -> Option<Self::State> {
Some(FilteredAccess::default())
}
@ -844,7 +844,7 @@ unsafe impl WorldQuery for &Archetype {
fn init_state(_world: &mut World) {}
fn get_state(_world: &World) -> Option<()> {
fn get_state(_components: &Components) -> Option<()> {
Some(())
}
@ -995,8 +995,8 @@ unsafe impl<T: Component> WorldQuery for &T {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<Self::State> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<Self::State> {
components.component_id::<T>()
}
fn matches_component_set(
@ -1178,8 +1178,8 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<Self::State> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<Self::State> {
components.component_id::<T>()
}
fn matches_component_set(
@ -1361,8 +1361,8 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<Self::State> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<Self::State> {
components.component_id::<T>()
}
fn matches_component_set(
@ -1480,8 +1480,8 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {
T::init_state(world)
}
fn get_state(world: &World) -> Option<Self::State> {
T::get_state(world)
fn get_state(components: &Components) -> Option<Self::State> {
T::get_state(components)
}
fn matches_component_set(
@ -1635,8 +1635,8 @@ unsafe impl<T: Component> WorldQuery for Has<T> {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<Self::State> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<Self::State> {
components.component_id::<T>()
}
fn matches_component_set(
@ -1776,13 +1776,13 @@ macro_rules! impl_anytuple_fetch {
*_access = _new_access;
}
fn init_state(_world: &mut World) -> Self::State {
($($name::init_state(_world),)*)
#[allow(unused_variables)]
fn init_state(world: &mut World) -> Self::State {
($($name::init_state(world),)*)
}
fn get_state(_world: &World) -> Option<Self::State> {
Some(($($name::get_state(_world)?,)*))
#[allow(unused_variables)]
fn get_state(components: &Components) -> Option<Self::State> {
Some(($($name::get_state(components)?,)*))
}
fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool {
@ -1858,8 +1858,8 @@ unsafe impl<D: QueryData> WorldQuery for NopWorldQuery<D> {
D::init_state(world)
}
fn get_state(world: &World) -> Option<Self::State> {
D::get_state(world)
fn get_state(components: &Components) -> Option<Self::State> {
D::get_state(components)
}
fn matches_component_set(
@ -1923,7 +1923,7 @@ unsafe impl<T: ?Sized> WorldQuery for PhantomData<T> {
fn init_state(_world: &mut World) -> Self::State {}
fn get_state(_world: &World) -> Option<Self::State> {
fn get_state(_components: &Components) -> Option<Self::State> {
Some(())
}

22
crates/bevy_ecs/src/query/filter.rs
View File

@ -1,6 +1,6 @@
use crate::{
archetype::Archetype,
component::{Component, ComponentId, StorageType, Tick},
component::{Component, ComponentId, Components, StorageType, Tick},
entity::Entity,
query::{DebugCheckedUnwrap, FilteredAccess, WorldQuery},
storage::{Column, ComponentSparseSet, Table, TableRow},
@ -183,8 +183,8 @@ unsafe impl<T: Component> WorldQuery for With<T> {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<Self::State> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<Self::State> {
components.component_id::<T>()
}
fn matches_component_set(
@ -291,8 +291,8 @@ unsafe impl<T: Component> WorldQuery for Without<T> {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<Self::State> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<Self::State> {
components.component_id::<T>()
}
fn matches_component_set(
@ -461,8 +461,8 @@ macro_rules! impl_or_query_filter {
($($filter::init_state(world),)*)
}
fn get_state(world: &World) -> Option<Self::State> {
Some(($($filter::get_state(world)?,)*))
fn get_state(components: &Components) -> Option<Self::State> {
Some(($($filter::get_state(components)?,)*))
}
fn matches_component_set(_state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool {
@ -691,8 +691,8 @@ unsafe impl<T: Component> WorldQuery for Added<T> {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<ComponentId> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<ComponentId> {
components.component_id::<T>()
}
fn matches_component_set(
@ -900,8 +900,8 @@ unsafe impl<T: Component> WorldQuery for Changed<T> {
world.init_component::<T>()
}
fn get_state(world: &World) -> Option<ComponentId> {
world.component_id::<T>()
fn get_state(components: &Components) -> Option<ComponentId> {
components.component_id::<T>()
}
fn matches_component_set(

70
crates/bevy_ecs/src/query/state.rs
View File

@ -1,7 +1,7 @@
use crate::{
archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId},
batching::BatchingStrategy,
component::{ComponentId, Tick},
component::{ComponentId, Components, Tick},
entity::Entity,
prelude::FromWorld,
query::{
@ -476,8 +476,8 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
/// You might end up with a mix of archetypes that only matched the original query + archetypes that only match
/// the new [`QueryState`]. Most of the safe methods on [`QueryState`] call [`QueryState::update_archetypes`] internally, so this
/// best used through a [`Query`](crate::system::Query).
pub fn transmute<NewD: QueryData>(&self, world: &World) -> QueryState<NewD> {
self.transmute_filtered::<NewD, ()>(world)
pub fn transmute<NewD: QueryData>(&self, components: &Components) -> QueryState<NewD> {
self.transmute_filtered::<NewD, ()>(components)
}
/// Creates a new [`QueryState`] with the same underlying [`FilteredAccess`], matched tables and archetypes
@ -486,11 +486,11 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
/// Panics if `NewD` or `NewF` require accesses that this query does not have.
pub fn transmute_filtered<NewD: QueryData, NewF: QueryFilter>(
&self,
world: &World,
components: &Components,
) -> QueryState<NewD, NewF> {
let mut component_access = FilteredAccess::default();
let mut fetch_state = NewD::get_state(world).expect("Could not create fetch_state, Please initialize all referenced components before transmuting.");
let filter_state = NewF::get_state(world).expect("Could not create filter_state, Please initialize all referenced components before transmuting.");
let mut fetch_state = NewD::get_state(components).expect("Could not create fetch_state, Please initialize all referenced components before transmuting.");
let filter_state = NewF::get_state(components).expect("Could not create filter_state, Please initialize all referenced components before transmuting.");
NewD::set_access(&mut fetch_state, &self.component_access);
NewD::update_component_access(&fetch_state, &mut component_access);
@ -544,10 +544,10 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
/// Will panic if `NewD` contains accesses not in `Q` or `OtherQ`.
pub fn join<OtherD: QueryData, NewD: QueryData>(
&self,
world: &World,
components: &Components,
other: &QueryState<OtherD>,
) -> QueryState<NewD, ()> {
self.join_filtered::<_, (), NewD, ()>(world, other)
self.join_filtered::<_, (), NewD, ()>(components, other)
}
/// Use this to combine two queries. The data accessed will be the intersection
@ -563,7 +563,7 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
NewF: QueryFilter,
>(
&self,
world: &World,
components: &Components,
other: &QueryState<OtherD, OtherF>,
) -> QueryState<NewD, NewF> {
if self.world_id != other.world_id {
@ -571,9 +571,9 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
}
let mut component_access = FilteredAccess::default();
let mut new_fetch_state = NewD::get_state(world)
let mut new_fetch_state = NewD::get_state(components)
.expect("Could not create fetch_state, Please initialize all referenced components before transmuting.");
let new_filter_state = NewF::get_state(world)
let new_filter_state = NewF::get_state(components)
.expect("Could not create filter_state, Please initialize all referenced components before transmuting.");
NewD::set_access(&mut new_fetch_state, &self.component_access);
@ -1779,7 +1779,7 @@ mod tests {
world.spawn((A(1), B(0)));
let query_state = world.query::<(&A, &B)>();
let mut new_query_state = query_state.transmute::<&A>(&world);
let mut new_query_state = query_state.transmute::<&A>(world.components());
assert_eq!(new_query_state.iter(&world).len(), 1);
let a = new_query_state.single(&world);
@ -1793,7 +1793,7 @@ mod tests {
world.spawn((A(1), B(0), C(0)));
let query_state = world.query_filtered::<(&A, &B), Without<C>>();
let mut new_query_state = query_state.transmute::<&A>(&world);
let mut new_query_state = query_state.transmute::<&A>(world.components());
// even though we change the query to not have Without<C>, we do not get the component with C.
let a = new_query_state.single(&world);
@ -1807,7 +1807,7 @@ mod tests {
let entity = world.spawn(A(10)).id();
let q = world.query::<()>();
let mut q = q.transmute::<Entity>(&world);
let mut q = q.transmute::<Entity>(world.components());
assert_eq!(q.single(&world), entity);
}
@ -1817,11 +1817,11 @@ mod tests {
world.spawn(A(10));
let q = world.query::<&A>();
let mut new_q = q.transmute::<Ref<A>>(&world);
let mut new_q = q.transmute::<Ref<A>>(world.components());
assert!(new_q.single(&world).is_added());
let q = world.query::<Ref<A>>();
let _ = q.transmute::<&A>(&world);
let _ = q.transmute::<&A>(world.components());
}
#[test]
@ -1830,8 +1830,8 @@ mod tests {
world.spawn(A(0));
let q = world.query::<&mut A>();
let _ = q.transmute::<Ref<A>>(&world);
let _ = q.transmute::<&A>(&world);
let _ = q.transmute::<Ref<A>>(world.components());
let _ = q.transmute::<&A>(world.components());
}
#[test]
@ -1840,7 +1840,7 @@ mod tests {
world.spawn(A(0));
let q: QueryState<EntityMut<'_>> = world.query::<EntityMut>();
let _ = q.transmute::<EntityRef>(&world);
let _ = q.transmute::<EntityRef>(world.components());
}
#[test]
@ -1849,8 +1849,8 @@ mod tests {
world.spawn((A(0), B(0)));
let query_state = world.query::<(Option<&A>, &B)>();
let _ = query_state.transmute::<Option<&A>>(&world);
let _ = query_state.transmute::<&B>(&world);
let _ = query_state.transmute::<Option<&A>>(world.components());
let _ = query_state.transmute::<&B>(world.components());
}
#[test]
@ -1864,7 +1864,7 @@ mod tests {
world.spawn(A(0));
let query_state = world.query::<&A>();
let mut _new_query_state = query_state.transmute::<(&A, &B)>(&world);
let mut _new_query_state = query_state.transmute::<(&A, &B)>(world.components());
}
#[test]
@ -1876,7 +1876,7 @@ mod tests {
world.spawn(A(0));
let query_state = world.query::<&A>();
let mut _new_query_state = query_state.transmute::<&mut A>(&world);
let mut _new_query_state = query_state.transmute::<&mut A>(world.components());
}
#[test]
@ -1888,7 +1888,7 @@ mod tests {
world.spawn(C(0));
let query_state = world.query::<Option<&A>>();
let mut new_query_state = query_state.transmute::<&A>(&world);
let mut new_query_state = query_state.transmute::<&A>(world.components());
let x = new_query_state.single(&world);
assert_eq!(x.0, 1234);
}
@ -1902,15 +1902,15 @@ mod tests {
world.init_component::<A>();
let q = world.query::<EntityRef>();
let _ = q.transmute::<&A>(&world);
let _ = q.transmute::<&A>(world.components());
}
#[test]
fn can_transmute_filtered_entity() {
let mut world = World::new();
let entity = world.spawn((A(0), B(1))).id();
let query =
QueryState::<(Entity, &A, &B)>::new(&mut world).transmute::<FilteredEntityRef>(&world);
let query = QueryState::<(Entity, &A, &B)>::new(&mut world)
.transmute::<FilteredEntityRef>(world.components());
let mut query = query;
// Our result is completely untyped
@ -1927,7 +1927,7 @@ mod tests {
let entity_a = world.spawn(A(0)).id();
let mut query = QueryState::<(Entity, &A, Has<B>)>::new(&mut world)
.transmute_filtered::<(Entity, Has<B>), Added<A>>(&world);
.transmute_filtered::<(Entity, Has<B>), Added<A>>(world.components());
assert_eq!((entity_a, false), query.single(&world));
@ -1947,7 +1947,7 @@ mod tests {
let entity_a = world.spawn(A(0)).id();
let mut detection_query = QueryState::<(Entity, &A)>::new(&mut world)
.transmute_filtered::<Entity, Changed<A>>(&world);
.transmute_filtered::<Entity, Changed<A>>(world.components());
let mut change_query = QueryState::<&mut A>::new(&mut world);
assert_eq!(entity_a, detection_query.single(&world));
@ -1970,7 +1970,7 @@ mod tests {
world.init_component::<A>();
world.init_component::<B>();
let query = QueryState::<&A>::new(&mut world);
let _new_query = query.transmute_filtered::<Entity, Changed<B>>(&world);
let _new_query = query.transmute_filtered::<Entity, Changed<B>>(world.components());
}
#[test]
@ -1983,7 +1983,8 @@ mod tests {
let query_1 = QueryState::<&A, Without<C>>::new(&mut world);
let query_2 = QueryState::<&B, Without<C>>::new(&mut world);
let mut new_query: QueryState<Entity, ()> = query_1.join_filtered(&world, &query_2);
let mut new_query: QueryState<Entity, ()> =
query_1.join_filtered(world.components(), &query_2);
assert_eq!(new_query.single(&world), entity_ab);
}
@ -1998,7 +1999,8 @@ mod tests {
let query_1 = QueryState::<&A>::new(&mut world);
let query_2 = QueryState::<&B, Without<C>>::new(&mut world);
let mut new_query: QueryState<Entity, ()> = query_1.join_filtered(&world, &query_2);
let mut new_query: QueryState<Entity, ()> =
query_1.join_filtered(world.components(), &query_2);
assert!(new_query.get(&world, entity_ab).is_ok());
// should not be able to get entity with c.
@ -2014,7 +2016,7 @@ mod tests {
world.init_component::<C>();
let query_1 = QueryState::<&A>::new(&mut world);
let query_2 = QueryState::<&B>::new(&mut world);
let _query: QueryState<&C> = query_1.join(&world, &query_2);
let _query: QueryState<&C> = query_1.join(world.components(), &query_2);
}
#[test]
@ -2028,6 +2030,6 @@ mod tests {
let mut world = World::new();
let query_1 = QueryState::<&A, Without<C>>::new(&mut world);
let query_2 = QueryState::<&B, Without<C>>::new(&mut world);
let _: QueryState<Entity, Changed<C>> = query_1.join_filtered(&world, &query_2);
let _: QueryState<Entity, Changed<C>> = query_1.join_filtered(world.components(), &query_2);
}
}

18
crates/bevy_ecs/src/query/world_query.rs
View File

@ -1,6 +1,6 @@
use crate::{
archetype::Archetype,
component::{ComponentId, Tick},
component::{ComponentId, Components, Tick},
entity::Entity,
query::FilteredAccess,
storage::{Table, TableRow},
@ -130,8 +130,8 @@ pub unsafe trait WorldQuery {
fn init_state(world: &mut World) -> Self::State;
/// Attempts to initialize a [`State`](WorldQuery::State) for this [`WorldQuery`] type using read-only
/// access to the [`World`].
fn get_state(world: &World) -> Option<Self::State>;
/// access to [`Components`].
fn get_state(components: &Components) -> Option<Self::State>;
/// Returns `true` if this query matches a set of components. Otherwise, returns `false`.
///
@ -212,13 +212,13 @@ macro_rules! impl_tuple_world_query {
let ($($name,)*) = state;
$($name::update_component_access($name, _access);)*
}
fn init_state(_world: &mut World) -> Self::State {
($($name::init_state(_world),)*)
#[allow(unused_variables)]
fn init_state(world: &mut World) -> Self::State {
($($name::init_state(world),)*)
}
fn get_state(_world: &World) -> Option<Self::State> {
Some(($($name::get_state(_world)?,)*))
#[allow(unused_variables)]
fn get_state(components: &Components) -> Option<Self::State> {
Some(($($name::get_state(components)?,)*))
}
fn matches_component_set(state: &Self::State, _set_contains_id: &impl Fn(ComponentId) -> bool) -> bool {

16
crates/bevy_ecs/src/system/query.rs
View File

@ -1368,12 +1368,8 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
pub fn transmute_lens_filtered<NewD: QueryData, NewF: QueryFilter>(
&mut self,
) -> QueryLens<'_, NewD, NewF> {
// SAFETY:
// - We have exclusive access to the query
// - `self` has correctly captured its access
// - Access is checked to be a subset of the query's access when the state is created.
let world = unsafe { self.world.world() };
let state = self.state.transmute_filtered::<NewD, NewF>(world);
let components = self.world.components();
let state = self.state.transmute_filtered::<NewD, NewF>(components);
QueryLens {
world: self.world,
state,
@ -1464,14 +1460,10 @@ impl<'w, 's, D: QueryData, F: QueryFilter> Query<'w, 's, D, F> {
&mut self,
other: &mut Query<OtherD, OtherF>,
) -> QueryLens<'_, NewD, NewF> {
// SAFETY:
// - The queries have correctly captured their access.
// - We have exclusive access to both queries.
// - Access for QueryLens is checked when state is created.
let world = unsafe { self.world.world() };
let components = self.world.components();
let state = self
.state
.join_filtered::<OtherD, OtherF, NewD, NewF>(world, other.state);
.join_filtered::<OtherD, OtherF, NewD, NewF>(components, other.state);
QueryLens {
world: self.world,
state,