From 832e9e01cbe09a755ea491356f07da1c416c0a66 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Fri, 23 Aug 2024 22:59:22 -0400 Subject: [PATCH 1/5] Improve type inference in DynSystemParam::downcast() by making the type parameter match the return value. --- crates/bevy_ecs/src/system/builder.rs | 8 ++-- crates/bevy_ecs/src/system/system_param.rs | 44 ++++++++++++++-------- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index ea5b070010b2d..e1404d4420ef3 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -545,11 +545,11 @@ mod tests { .build_state(&mut world) .build_system( |mut p0: DynSystemParam, mut p1: DynSystemParam, mut p2: DynSystemParam| { - let local = *p0.downcast_mut::>().unwrap(); - let query_count = p1.downcast_mut::>().unwrap().iter().count(); - let _entities = p2.downcast_mut::<&Entities>().unwrap(); assert!(p0.downcast_mut::>().is_none()); - local + query_count + let local: Local = p0.downcast_mut().unwrap(); + let query: Query<()> = p1.downcast_mut().unwrap(); + let _entities: &Entities = p2.downcast_mut().unwrap(); + *local + query.iter().count() }, ); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 6b604c54c2a9a..f3c0a13b9cfd1 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1894,7 +1894,9 @@ unsafe impl ReadOnlySystemParam for PhantomData {} /// assert!(param.is::>()); /// assert!(!param.is::>()); /// assert!(param.downcast_mut::>().is_none()); -/// let foo: Res = param.downcast::>().unwrap(); +/// let res = param.downcast_mut::>().unwrap(); +/// // The type parameter can be left out if it can be determined from use. +/// let res: Res = param.downcast().unwrap(); /// } /// /// let system = ( @@ -1948,7 +1950,10 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// Returns the inner system param if it is the correct type. /// This consumes the dyn param, so the returned param can have its original world and state lifetimes. - pub fn downcast(self) -> Option> { + pub fn downcast(self) -> Option + where + T::Item<'static, 'static>: SystemParam = T> + 'static, + { // SAFETY: // - `DynSystemParam::new()` ensures `state` is a `ParamState`, that the world matches, // and that it has access required by the inner system param. @@ -1958,7 +1963,10 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// Returns the inner system parameter if it is the correct type. /// This borrows the dyn param, so the returned param is only valid for the duration of that borrow. - pub fn downcast_mut(&mut self) -> Option> { + pub fn downcast_mut<'a, T: SystemParam>(&'a mut self) -> Option + where + T::Item<'static, 'static>: SystemParam = T> + 'static, + { // SAFETY: // - `DynSystemParam::new()` ensures `state` is a `ParamState`, that the world matches, // and that it has access required by the inner system param. @@ -1971,9 +1979,10 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// but since it only performs read access it can keep the original world lifetime. /// This can be useful with methods like [`Query::iter_inner()`] or [`Res::into_inner()`] /// to obtain references with the original world lifetime. - pub fn downcast_mut_inner( - &mut self, - ) -> Option> { + pub fn downcast_mut_inner<'a, T: ReadOnlySystemParam>(&'a mut self) -> Option + where + T::Item<'static, 'static>: SystemParam = T> + 'static, + { // SAFETY: // - `DynSystemParam::new()` ensures `state` is a `ParamState`, that the world matches, // and that it has access required by the inner system param. @@ -1988,19 +1997,24 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// in [`init_state`](SystemParam::init_state) for the inner system param. /// - `world` must be the same `World` that was used to initialize /// [`state`](SystemParam::init_state) for the inner system param. -unsafe fn downcast<'w, 's, T: SystemParam + 'static>( +unsafe fn downcast<'w, 's, T: SystemParam>( state: &'s mut dyn Any, system_meta: &SystemMeta, world: UnsafeWorldCell<'w>, change_tick: Tick, -) -> Option> { - state.downcast_mut::>().map(|state| { - // SAFETY: - // - The caller ensures the world has access for the underlying system param, - // and since the downcast succeeded, the underlying system param is T. - // - The caller ensures the `world` matches. - unsafe { T::get_param(&mut state.0, system_meta, world, change_tick) } - }) +) -> Option +where + T::Item<'static, 'static>: SystemParam = T> + 'static, +{ + state + .downcast_mut::>>() + .map(|state| { + // SAFETY: + // - The caller ensures the world has access for the underlying system param, + // and since the downcast succeeded, the underlying system param is T. + // - The caller ensures the `world` matches. + unsafe { T::Item::get_param(&mut state.0, system_meta, world, change_tick) } + }) } /// The [`SystemParam::State`] for a [`DynSystemParam`]. From 7393c4a36e2f303daa6d8c355cf1db11a7d2c933 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sun, 8 Sep 2024 20:12:08 -0400 Subject: [PATCH 2/5] Add dedicated test for inference. --- crates/bevy_ecs/src/system/system_param.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index f3c0a13b9cfd1..6739a4fec2b05 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -2337,4 +2337,12 @@ mod tests { schedule.add_systems((non_send_param_set, non_send_param_set, non_send_param_set)); schedule.run(&mut world); } + + fn _dyn_system_param_type_inference(mut p: DynSystemParam) { + // Make sure the downcast() methods are able to infer their type parameters from the use of the return type. + // This is just a compilation test, so there is nothing to run. + let _query: Query<()> = p.downcast_mut().unwrap(); + let _query: Query<()> = p.downcast_mut_inner().unwrap(); + let _query: Query<()> = p.downcast().unwrap(); + } } From b436a6259959160929fff46879ffaefba627c32e Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Sun, 8 Sep 2024 20:14:42 -0400 Subject: [PATCH 3/5] Document the complex where clause. --- crates/bevy_ecs/src/system/system_param.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 6739a4fec2b05..5c15f209795e5 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1951,6 +1951,7 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// Returns the inner system param if it is the correct type. /// This consumes the dyn param, so the returned param can have its original world and state lifetimes. pub fn downcast(self) -> Option + // See downcast() function for an explanation of the where clause where T::Item<'static, 'static>: SystemParam = T> + 'static, { @@ -1964,6 +1965,7 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// Returns the inner system parameter if it is the correct type. /// This borrows the dyn param, so the returned param is only valid for the duration of that borrow. pub fn downcast_mut<'a, T: SystemParam>(&'a mut self) -> Option + // See downcast() function for an explanation of the where clause where T::Item<'static, 'static>: SystemParam = T> + 'static, { @@ -1980,6 +1982,7 @@ impl<'w, 's> DynSystemParam<'w, 's> { /// This can be useful with methods like [`Query::iter_inner()`] or [`Res::into_inner()`] /// to obtain references with the original world lifetime. pub fn downcast_mut_inner<'a, T: ReadOnlySystemParam>(&'a mut self) -> Option + // See downcast() function for an explanation of the where clause where T::Item<'static, 'static>: SystemParam = T> + 'static, { @@ -2003,6 +2006,14 @@ unsafe fn downcast<'w, 's, T: SystemParam>( world: UnsafeWorldCell<'w>, change_tick: Tick, ) -> Option +// We need a 'static version of the SystemParam to use with `Any::downcast_mut()`, +// and we need a <'w, 's> version to actually return. +// The type parameter T must be the one we return in order to get type inference from the return value. +// So we use `T::Item<'static, 'static>` as the 'static version, and require that it be 'static. +// That means the return value will be T::Item<'static, 'static>::Item<'w, 's>, +// so we constrain that to be equal to T. +// Every actual `SystemParam` implementation has `T::Item == T` up to lifetimes, +// so they should all work with this constraint. where T::Item<'static, 'static>: SystemParam = T> + 'static, { From 84375dcdcbec9d7ef83e541db610a72a4985b224 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Mon, 9 Sep 2024 13:23:13 -0400 Subject: [PATCH 4/5] Revert changes to dyn_builder test now that there is a dedicated test for type inference. --- crates/bevy_ecs/src/system/builder.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/builder.rs b/crates/bevy_ecs/src/system/builder.rs index e1404d4420ef3..ea5b070010b2d 100644 --- a/crates/bevy_ecs/src/system/builder.rs +++ b/crates/bevy_ecs/src/system/builder.rs @@ -545,11 +545,11 @@ mod tests { .build_state(&mut world) .build_system( |mut p0: DynSystemParam, mut p1: DynSystemParam, mut p2: DynSystemParam| { + let local = *p0.downcast_mut::>().unwrap(); + let query_count = p1.downcast_mut::>().unwrap().iter().count(); + let _entities = p2.downcast_mut::<&Entities>().unwrap(); assert!(p0.downcast_mut::>().is_none()); - let local: Local = p0.downcast_mut().unwrap(); - let query: Query<()> = p1.downcast_mut().unwrap(); - let _entities: &Entities = p2.downcast_mut().unwrap(); - *local + query.iter().count() + local + query_count }, ); From 263fe68e3787fba46f37207a1417b206d5b1775d Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Mon, 9 Sep 2024 13:41:14 -0400 Subject: [PATCH 5/5] Make the where clause on the `is` method match the ones on the `downcast` methods. --- crates/bevy_ecs/src/system/system_param.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 5c15f209795e5..ac038dff1a792 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1944,8 +1944,12 @@ impl<'w, 's> DynSystemParam<'w, 's> { } /// Returns `true` if the inner system param is the same as `T`. - pub fn is(&self) -> bool { - self.state.is::>() + pub fn is(&self) -> bool + // See downcast() function for an explanation of the where clause + where + T::Item<'static, 'static>: SystemParam = T> + 'static, + { + self.state.is::>>() } /// Returns the inner system param if it is the correct type.