Improve type inference in DynSystemParam::downcast() by making the type parameter match the return value.
#15103
Conversation
…pe parameter match the return value.
alice-i-cecile
added
A-ECS
|
@re0312 are you able to review this? |
| @@ -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 | |||
There was a problem hiding this comment.
This where clause could probably use a doc comment: it's very complex.
alice-i-cecile
added
S-Waiting-on-Author
There was a problem hiding this comment.
This is a nice ergonomics change for the API, but I agree it needs a comment explaining the where clause since it's quite challenging to parse on first glance.
For consistency, the type signature and function body of is should also be updated.
// Before
pub fn is<T: SystemParam + 'static>(&self) -> bool {
self.state.is::<ParamState<T>>()
}
// After
pub fn is<T: SystemParam>(&self) -> bool
where
T::Item<'static, 'static>: SystemParam<Item<'w, 's> = T> + 'static,
{
self.state.is::<ParamState<T::Item<'static, 'static>>>()
}| 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| /// 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(); |
There was a problem hiding this comment.
Nice documentation, clearly shows the benefit this PR adds.
Hmm, I had been thinking that it would be simpler to leave it with the simpler type signature, but I think you're right that being consistent is more clear. I'll change it to match. |
chescock
added
S-Needs-Review
alice-i-cecile
added
S-Ready-For-Final-Review
Objective
Right now,
DynSystemParam::downcast()always requires the type parameter to be specified with a turbofish. Make it so that it can be inferred from the use of the return value, like:Solution
The reason this doesn't currently work is that the type parameter is a
'staticversion of theSystemParamso that it can be used withAny::downcast_mut(). Change the method signature so that the type parameter matches the return type, and useT::Item<'static, 'static>to get the'staticversion. That means we wind up returning aT::Item<'static, 'static>::Item<'w, 's>, so constrain that to be equal toT. That works with everySystemParamimplementation, since they haveT::Item == Tup to lifetimes.