Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve type inference in DynSystemParam::downcast() by making the type parameter match the return value. #15103

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 54 additions & 17 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1894,7 +1894,9 @@ unsafe impl<T: ?Sized> ReadOnlySystemParam for PhantomData<T> {}
/// assert!(param.is::<Res<A>>());
/// assert!(!param.is::<Res<B>>());
/// assert!(param.downcast_mut::<Res<B>>().is_none());
/// let foo: Res<A> = param.downcast::<Res<A>>().unwrap();
/// let res = param.downcast_mut::<Res<A>>().unwrap();
/// // The type parameter can be left out if it can be determined from use.
/// let res: Res<A> = param.downcast().unwrap();
Comment on lines -1897 to +1899
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice documentation, clearly shows the benefit this PR adds.

/// }
///
/// let system = (
Expand Down Expand Up @@ -1942,13 +1944,21 @@ impl<'w, 's> DynSystemParam<'w, 's> {
}

/// Returns `true` if the inner system param is the same as `T`.
pub fn is<T: SystemParam + 'static>(&self) -> bool {
self.state.is::<ParamState<T>>()
pub fn is<T: SystemParam>(&self) -> bool
// See downcast() function for an explanation of the where clause
where
T::Item<'static, 'static>: SystemParam<Item<'w, 's> = T> + 'static,
{
self.state.is::<ParamState<T::Item<'static, 'static>>>()
}

/// 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<T: SystemParam + 'static>(self) -> Option<T::Item<'w, 's>> {
pub fn downcast<T: SystemParam>(self) -> Option<T>
// See downcast() function for an explanation of the where clause
where
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This where clause could probably use a doc comment: it's very complex.

T::Item<'static, 'static>: SystemParam<Item<'w, 's> = T> + 'static,
{
// SAFETY:
// - `DynSystemParam::new()` ensures `state` is a `ParamState<T>`, that the world matches,
// and that it has access required by the inner system param.
Expand All @@ -1958,7 +1968,11 @@ 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<T: SystemParam + 'static>(&mut self) -> Option<T::Item<'_, '_>> {
pub fn downcast_mut<'a, T: SystemParam>(&'a mut self) -> Option<T>
// See downcast() function for an explanation of the where clause
where
T::Item<'static, 'static>: SystemParam<Item<'a, 'a> = T> + 'static,
{
// SAFETY:
// - `DynSystemParam::new()` ensures `state` is a `ParamState<T>`, that the world matches,
// and that it has access required by the inner system param.
Expand All @@ -1971,9 +1985,11 @@ 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<T: ReadOnlySystemParam + 'static>(
&mut self,
) -> Option<T::Item<'w, '_>> {
pub fn downcast_mut_inner<'a, T: ReadOnlySystemParam>(&'a mut self) -> Option<T>
// See downcast() function for an explanation of the where clause
where
T::Item<'static, 'static>: SystemParam<Item<'w, 'a> = T> + 'static,
{
// SAFETY:
// - `DynSystemParam::new()` ensures `state` is a `ParamState<T>`, that the world matches,
// and that it has access required by the inner system param.
Expand All @@ -1988,19 +2004,32 @@ 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<T::Item<'w, 's>> {
state.downcast_mut::<ParamState<T>>().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<T>
// 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<Item<'w, 's> = T> + 'static,
{
state
.downcast_mut::<ParamState<T::Item<'static, 'static>>>()
.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`].
Expand Down Expand Up @@ -2323,4 +2352,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();
}
}