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
Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/system/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Local<usize>>().unwrap();
let query_count = p1.downcast_mut::<Query<()>>().unwrap().iter().count();
let _entities = p2.downcast_mut::<&Entities>().unwrap();
assert!(p0.downcast_mut::<Query<()>>().is_none());
local + query_count
let local: Local<usize> = p0.downcast_mut().unwrap();
let query: Query<()> = p1.downcast_mut().unwrap();
let _entities: &Entities = p2.downcast_mut().unwrap();
*local + query.iter().count()
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that this change you're proposing allows for the variable type to infer the type parameter on downcast/_mut, but this change feels rather pointless. In the case of query_count, I'd even say it's a regression to move away from turbofish syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I had this here to check that inference actually worked, but @alice-i-cecile's suggestion to make a dedicated test for that means I can put this back.

},
);

Expand Down
44 changes: 29 additions & 15 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 @@ -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<T: SystemParam + 'static>(self) -> Option<T::Item<'w, 's>> {
pub fn downcast<T: SystemParam>(self) -> Option<T>
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 +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<T: SystemParam + 'static>(&mut self) -> Option<T::Item<'_, '_>> {
pub fn downcast_mut<'a, T: SystemParam>(&'a mut self) -> Option<T>
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 +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<T: ReadOnlySystemParam + 'static>(
&mut self,
) -> Option<T::Item<'w, '_>> {
pub fn downcast_mut_inner<'a, T: ReadOnlySystemParam>(&'a mut self) -> Option<T>
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 +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<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>
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