constrain WorldQuery::get_state to only use &Components (bevyengine#1…

…3343) # 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](https://github.com/JoJoJet/bevy-trait-query/blob/0c0e7dd646b4a77fa3496ef5e9686107d17fdd1e/src/lib.rs#L445) inside of [`get_state`](https://github.com/JoJoJet/bevy-trait-query/blob/0c0e7dd646b4a77fa3496ef5e9686107d17fdd1e/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.
robtfm · May 13, 2024 · 0eb4bb6 · 0eb4bb6
1 parent 2037b88
commit 0eb4bb6
Show file tree

Hide file tree

Showing 6 changed files with 89 additions and 95 deletions.
diff --git a/crates/bevy_ecs/macros/src/world_query.rs b/crates/bevy_ecs/macros/src/world_query.rs
@@ -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)?,)*
                 })
             }
 

diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs
@@ -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(())
     }
 

diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs
@@ -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(