System Param Lifetime Split (#2605)

# Objective

Enable using exact World lifetimes during read-only access . This is motivated by the new renderer's need to allow read-only world-only queries to outlive the query itself (but still be constrained by the world lifetime).

For example:
https://github.com/bevyengine/bevy/blob/115b170d1f11a91146bb6d6e9684dceb8b21f786/pipelined/bevy_pbr2/src/render/mod.rs#L774

## Solution

Split out SystemParam state and world lifetimes and pipe those lifetimes up to read-only Query ops (and add into_inner for Res). According to every safety test I've run so far (except one), this is safe (see the temporary safety test commit). Note that changing the mutable variants to the new lifetimes would allow aliased mutable pointers (try doing that to see how it affects the temporary safety tests).

The new state lifetime on SystemParam does make `#[derive(SystemParam)]` more cumbersome (the current impl requires PhantomData if you don't use both lifetimes). We can make this better by detecting whether or not a lifetime is used in the derive and adjusting accordingly, but that should probably be done in its own pr.  

## Why is this a draft?

The new lifetimes break QuerySet safety in one very specific case (see the query_set system in system_safety_test). We need to solve this before we can use the lifetimes given.

This is due to the fact that QuerySet is just a wrapper over Query, which now relies on world lifetimes instead of `&self` lifetimes to prevent aliasing (but in systems, each Query has its own implied lifetime, not a centralized world lifetime).  I believe the fix is to rewrite QuerySet to have its own World lifetime (and own the internal reference). This will complicate the impl a bit, but I think it is doable. I'm curious if anyone else has better ideas.

Personally, I think these new lifetimes need to happen. We've gotta have a way to directly tie read-only World queries to the World lifetime. The new renderer is the first place this has come up, but I doubt it will be the last. Worst case scenario we can come up with a second `WorldLifetimeQuery<Q, F = ()>` parameter to enable these read-only scenarios, but I'd rather not add another type to the type zoo.

Author: cart

crates/bevy_ecs/macros/src/lib.rs

@@ -9,8 +9,8 @@ use syn::{
     parse_macro_input,
     punctuated::Punctuated,
     token::Comma,
-    Data, DataStruct, DeriveInput, Field, Fields, GenericParam, Ident, Index, Lifetime, LitInt,
-    Path, Result, Token,
+    Data, DataStruct, DeriveInput, Field, Fields, GenericParam, Ident, Index, LitInt, Path, Result,
+    Token,
 };
 
 struct AllTuples {
@@ -176,48 +176,36 @@ fn get_idents(fmt_string: fn(usize) -> String, count: usize) -> Vec<Ident> {
         .collect::<Vec<Ident>>()
 }
 
-fn get_lifetimes(fmt_string: fn(usize) -> String, count: usize) -> Vec<Lifetime> {
-    (0..count)
-        .map(|i| Lifetime::new(&fmt_string(i), Span::call_site()))
-        .collect::<Vec<Lifetime>>()
-}
-
 #[proc_macro]
 pub fn impl_query_set(_input: TokenStream) -> TokenStream {
     let mut tokens = TokenStream::new();
     let max_queries = 4;
     let queries = get_idents(|i| format!("Q{}", i), max_queries);
     let filters = get_idents(|i| format!("F{}", i), max_queries);
-    let lifetimes = get_lifetimes(|i| format!("'q{}", i), max_queries);
-    let mut query_fns = Vec::new();
     let mut query_fn_muts = Vec::new();
     for i in 0..max_queries {
         let query = &queries[i];
         let filter = &filters[i];
-        let lifetime = &lifetimes[i];
         let fn_name = Ident::new(&format!("q{}", i), Span::call_site());
-        let fn_name_mut = Ident::new(&format!("q{}_mut", i), Span::call_site());
         let index = Index::from(i);
-        query_fns.push(quote! {
-            pub fn #fn_name(&self) -> &Query<#lifetime, #query, #filter> {
-                &self.0.#index
-            }
-        });
         query_fn_muts.push(quote! {
-            pub fn #fn_name_mut(&mut self) -> &mut Query<#lifetime, #query, #filter> {
-                &mut self.0.#index
+            pub fn #fn_name(&mut self) -> Query<'_, '_, #query, #filter> {
+                // SAFE: systems run without conflicts with other systems.
+                // Conflicting queries in QuerySet are not accessible at the same time
+                // QuerySets are guaranteed to not conflict with other SystemParams
+                unsafe {
+                    Query::new(self.world, &self.query_states.#index, self.last_change_tick, self.change_tick)
+                }
             }
         });
     }
 
     for query_count in 1..=max_queries {
         let query = &queries[0..query_count];
         let filter = &filters[0..query_count];
-        let lifetime = &lifetimes[0..query_count];
-        let query_fn = &query_fns[0..query_count];
         let query_fn_mut = &query_fn_muts[0..query_count];
         tokens.extend(TokenStream::from(quote! {
-            impl<#(#lifetime,)*  #(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParam for QuerySet<(#(Query<#lifetime, #query, #filter>,)*)>
+            impl<'w, 's, #(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParam for QuerySet<'w, 's, (#(QueryState<#query, #filter>,)*)>
                 where #(#filter::Fetch: FilterFetch,)*
             {
                 type Fetch = QuerySetState<(#(QueryState<#query, #filter>,)*)>;
@@ -270,27 +258,30 @@ pub fn impl_query_set(_input: TokenStream) -> TokenStream {
                 fn default_config() {}
             }
 
-            impl<'a, #(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParamFetch<'a> for QuerySetState<(#(QueryState<#query, #filter>,)*)>
+            impl<'w, 's, #(#query: WorldQuery + 'static,)* #(#filter: WorldQuery + 'static,)*> SystemParamFetch<'w, 's> for QuerySetState<(#(QueryState<#query, #filter>,)*)>
                 where #(#filter::Fetch: FilterFetch,)*
             {
-                type Item = QuerySet<(#(Query<'a, #query, #filter>,)*)>;
+                type Item = QuerySet<'w, 's, (#(QueryState<#query, #filter>,)*)>;
 
                 #[inline]
                 unsafe fn get_param(
-                    state: &'a mut Self,
+                    state: &'s mut Self,
                     system_meta: &SystemMeta,
-                    world: &'a World,
+                    world: &'w World,
                     change_tick: u32,
                 ) -> Self::Item {
-                    let (#(#query,)*) = &state.0;
-                    QuerySet((#(Query::new(world, #query, system_meta.last_change_tick, change_tick),)*))
+                    QuerySet {
+                        query_states: &state.0,
+                        world,
+                        last_change_tick: system_meta.last_change_tick,
+                        change_tick,
+                    }
                 }
             }
 
-            impl<#(#lifetime,)* #(#query: WorldQuery,)* #(#filter: WorldQuery,)*> QuerySet<(#(Query<#lifetime, #query, #filter>,)*)>
+            impl<'w, 's, #(#query: WorldQuery,)* #(#filter: WorldQuery,)*> QuerySet<'w, 's, (#(QueryState<#query, #filter>,)*)>
                 where #(#filter::Fetch: FilterFetch,)*
             {
-                #(#query_fn)*
                 #(#query_fn_mut)*
             }
         }));
@@ -415,12 +406,12 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
             }
         }
 
-        impl #impl_generics #path::system::SystemParamFetch<'a> for #fetch_struct_name <(#(<#field_types as #path::system::SystemParam>::Fetch,)*), #punctuated_generic_idents> {
+        impl #impl_generics #path::system::SystemParamFetch<'w, 's> for #fetch_struct_name <(#(<#field_types as #path::system::SystemParam>::Fetch,)*), #punctuated_generic_idents> {
             type Item = #struct_name#ty_generics;
             unsafe fn get_param(
-                state: &'a mut Self,
+                state: &'s mut Self,
                 system_meta: &#path::system::SystemMeta,
-                world: &'a #path::world::World,
+                world: &'w #path::world::World,
                 change_tick: u32,
             ) -> Self::Item {
                 #struct_name {

crates/bevy_ecs/src/event.rs

@@ -151,18 +151,20 @@ fn map_instance_event<T>(event_instance: &EventInstance<T>) -> &T {
 
 /// Reads events of type `T` in order and tracks which events have already been read.
 #[derive(SystemParam)]
-pub struct EventReader<'a, T: Component> {
-    last_event_count: Local<'a, (usize, PhantomData<T>)>,
-    events: Res<'a, Events<T>>,
+pub struct EventReader<'w, 's, T: Component> {
+    last_event_count: Local<'s, (usize, PhantomData<T>)>,
+    events: Res<'w, Events<T>>,
 }
 
 /// Sends events of type `T`.
 #[derive(SystemParam)]
-pub struct EventWriter<'a, T: Component> {
-    events: ResMut<'a, Events<T>>,
+pub struct EventWriter<'w, 's, T: Component> {
+    events: ResMut<'w, Events<T>>,
+    #[system_param(ignore)]
+    marker: PhantomData<&'s usize>,
 }
 
-impl<'a, T: Component> EventWriter<'a, T> {
+impl<'w, 's, T: Component> EventWriter<'w, 's, T> {
     pub fn send(&mut self, event: T) {
         self.events.send(event);
     }
@@ -252,7 +254,7 @@ fn internal_event_reader<'a, T>(
     }
 }
 
-impl<'a, T: Component> EventReader<'a, T> {
+impl<'w, 's, T: Component> EventReader<'w, 's, T> {
     /// Iterates over the events this EventReader has not seen yet. This updates the EventReader's
     /// event counter, which means subsequent event reads will not include events that happened
     /// before now.

crates/bevy_ecs/src/query/state.rs

@@ -117,11 +117,11 @@ where
     }
 
     #[inline]
-    pub fn get<'w>(
-        &mut self,
+    pub fn get<'w, 's>(
+        &'s mut self,
         world: &'w World,
         entity: Entity,
-    ) -> Result<<Q::Fetch as Fetch<'w, '_>>::Item, QueryEntityError>
+    ) -> Result<<Q::Fetch as Fetch<'w, 's>>::Item, QueryEntityError>
     where
         Q::Fetch: ReadOnlyFetch,
     {
@@ -130,11 +130,11 @@ where
     }
 
     #[inline]
-    pub fn get_mut<'w>(
-        &mut self,
+    pub fn get_mut<'w, 's>(
+        &'s mut self,
         world: &'w mut World,
         entity: Entity,
-    ) -> Result<<Q::Fetch as Fetch<'w, '_>>::Item, QueryEntityError> {
+    ) -> Result<<Q::Fetch as Fetch<'w, 's>>::Item, QueryEntityError> {
         // SAFETY: query has unique world access
         unsafe { self.get_unchecked(world, entity) }
     }
@@ -144,11 +144,11 @@ where
     /// This does not check for mutable query correctness. To be safe, make sure mutable queries
     /// have unique access to the components they query.
     #[inline]
-    pub unsafe fn get_unchecked<'w>(
-        &mut self,
+    pub unsafe fn get_unchecked<'w, 's>(
+        &'s mut self,
         world: &'w World,
         entity: Entity,
-    ) -> Result<<Q::Fetch as Fetch<'w, '_>>::Item, QueryEntityError> {
+    ) -> Result<<Q::Fetch as Fetch<'w, 's>>::Item, QueryEntityError> {
         self.validate_world_and_update_archetypes(world);
         self.get_unchecked_manual(
             world,
@@ -161,13 +161,13 @@ where
     /// # Safety
     /// This does not check for mutable query correctness. To be safe, make sure mutable queries
     /// have unique access to the components they query.
-    pub unsafe fn get_unchecked_manual<'w>(
-        &self,
+    pub unsafe fn get_unchecked_manual<'w, 's>(
+        &'s self,
         world: &'w World,
         entity: Entity,
         last_change_tick: u32,
         change_tick: u32,
-    ) -> Result<<Q::Fetch as Fetch<'w, '_>>::Item, QueryEntityError> {
+    ) -> Result<<Q::Fetch as Fetch<'w, 's>>::Item, QueryEntityError> {
         let location = world
             .entities
             .get(entity)

crates/bevy_ecs/src/system/commands/mod.rs

@@ -16,14 +16,14 @@ pub trait Command: Send + Sync + 'static {
 }
 
 /// A list of commands that will be run to modify a [`World`].
-pub struct Commands<'a> {
-    queue: &'a mut CommandQueue,
-    entities: &'a Entities,
+pub struct Commands<'w, 's> {
+    queue: &'s mut CommandQueue,
+    entities: &'w Entities,
 }
 
-impl<'a> Commands<'a> {
+impl<'w, 's> Commands<'w, 's> {
     /// Create a new `Commands` from a queue and a world.
-    pub fn new(queue: &'a mut CommandQueue, world: &'a World) -> Self {
+    pub fn new(queue: &'s mut CommandQueue, world: &'w World) -> Self {
         Self {
             queue,
             entities: world.entities(),
@@ -50,7 +50,7 @@ impl<'a> Commands<'a> {
     /// }
     /// # example_system.system();
     /// ```
-    pub fn spawn(&mut self) -> EntityCommands<'a, '_> {
+    pub fn spawn<'a>(&'a mut self) -> EntityCommands<'w, 's, 'a> {
         let entity = self.entities.reserve_entity();
         EntityCommands {
             entity,
@@ -98,7 +98,7 @@ impl<'a> Commands<'a> {
     /// }
     /// # example_system.system();
     /// ```
-    pub fn spawn_bundle<'b, T: Bundle>(&'b mut self, bundle: T) -> EntityCommands<'a, 'b> {
+    pub fn spawn_bundle<'a, T: Bundle>(&'a mut self, bundle: T) -> EntityCommands<'w, 's, 'a> {
         let mut e = self.spawn();
         e.insert_bundle(bundle);
         e
@@ -123,7 +123,7 @@ impl<'a> Commands<'a> {
     /// }
     /// # example_system.system();
     /// ```
-    pub fn entity(&mut self, entity: Entity) -> EntityCommands<'a, '_> {
+    pub fn entity<'a>(&'a mut self, entity: Entity) -> EntityCommands<'w, 's, 'a> {
         EntityCommands {
             entity,
             commands: self,
@@ -159,12 +159,12 @@ impl<'a> Commands<'a> {
 }
 
 /// A list of commands that will be run to modify an [`Entity`].
-pub struct EntityCommands<'a, 'b> {
+pub struct EntityCommands<'w, 's, 'a> {
     entity: Entity,
-    commands: &'b mut Commands<'a>,
+    commands: &'a mut Commands<'w, 's>,
 }
 
-impl<'a, 'b> EntityCommands<'a, 'b> {
+impl<'w, 's, 'a> EntityCommands<'w, 's, 'a> {
     /// Retrieves the current entity's unique [`Entity`] id.
     #[inline]
     pub fn id(&self) -> Entity {
@@ -252,7 +252,7 @@ impl<'a, 'b> EntityCommands<'a, 'b> {
     }
 
     /// Returns the underlying `[Commands]`.
-    pub fn commands(&mut self) -> &mut Commands<'a> {
+    pub fn commands(&mut self) -> &mut Commands<'w, 's> {
         self.commands
     }
 }

crates/bevy_ecs/src/system/function_system.rs

@@ -87,7 +87,10 @@ impl<Param: SystemParam> SystemState<Param> {
 
     /// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only.
     #[inline]
-    pub fn get<'a>(&'a mut self, world: &'a World) -> <Param::Fetch as SystemParamFetch<'a>>::Item
+    pub fn get<'w, 's>(
+        &'s mut self,
+        world: &'w World,
+    ) -> <Param::Fetch as SystemParamFetch<'w, 's>>::Item
     where
         Param::Fetch: ReadOnlySystemParamFetch,
     {
@@ -98,10 +101,10 @@ impl<Param: SystemParam> SystemState<Param> {
 
     /// Retrieve the mutable [`SystemParam`] values.
     #[inline]
-    pub fn get_mut<'a>(
-        &'a mut self,
-        world: &'a mut World,
-    ) -> <Param::Fetch as SystemParamFetch<'a>>::Item {
+    pub fn get_mut<'w, 's>(
+        &'s mut self,
+        world: &'w mut World,
+    ) -> <Param::Fetch as SystemParamFetch<'w, 's>>::Item {
         self.validate_world_and_update_archetypes(world);
         // SAFE: World is uniquely borrowed and matches the World this SystemState was created with.
         unsafe { self.get_unchecked_manual(world) }
@@ -142,10 +145,10 @@ impl<Param: SystemParam> SystemState<Param> {
     /// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was
     /// created with.   
     #[inline]
-    pub unsafe fn get_unchecked_manual<'a>(
-        &'a mut self,
-        world: &'a World,
-    ) -> <Param::Fetch as SystemParamFetch<'a>>::Item {
+    pub unsafe fn get_unchecked_manual<'w, 's>(
+        &'s mut self,
+        world: &'w World,
+    ) -> <Param::Fetch as SystemParamFetch<'w, 's>>::Item {
         let change_tick = world.increment_change_tick();
         let param = <Param::Fetch as SystemParamFetch>::get_param(
             &mut self.param_state,