internal: implement lifetime elision - #22927
Conversation
rustbot
added
the
S-waiting-on-review
|
Not sure why this fails in CI and passes in my system? |
You need to set an extra env var to run slow tests |
|
Oh I didn't know, we ran slow tests. My bad 😅 |
06a014c to
88e02b3
Compare
| fn bar(x: &u32) {} | ||
| "#, | ||
| expect!["ty: &'_ &'_ u32, name: x"], | ||
| expect!["ty: &'_ &'<erased> u32, name: x"], |
There was a problem hiding this comment.
we probably need to check our rendering now, we shouldn't show erased lifetimes to the user
There was a problem hiding this comment.
I could make the RegionKind::ReErased to display '_ for now, would that be fine?
| lc world &WorldSnapshot [type_could_unify+name+local] | ||
| ex world [type_could_unify] |
There was a problem hiding this comment.
Interesting, so we lose a lot of type equality now which I guess makes sense. Worth to keep in mind if we want t o have something like type-equal module lifetimes
|
Very excited for this, I've been wanting this for so long! |
| self.lower_type_ref( | ||
| it, | ||
| impl_trait_lower_fn, | ||
| &mut Self::elided_lifetime_placeholder_allocator, |
There was a problem hiding this comment.
This does not look correct. We should first lower the arguments. If there is exactly one HRTB lifetime in them, any elided lifetime in the return type should refer to it. Otherwise, it's an error lifetime (plus a diagnostic). In fact, I'm pretty sure we should remove LifetimeRef::Placeholder.
There was a problem hiding this comment.
I use LifetimeRef::Placeholder for return types, so that I can resolve it correctly during hir-ty lowering. But I missed to copy from my old WIP of the lifetime elision.
There was a problem hiding this comment.
I see no reason to resolve it during hir-ty lowering and complicate the code with additional variant when we can resolve it during hir lowering instead.
| &mut self, | ||
| node: ast::Type, | ||
| impl_trait_lower_fn: ImplTraitLowerFn<'_>, | ||
| lifetime_elision_fn: LifetimeElisionFn<'_>, |
There was a problem hiding this comment.
I'm not pleased with passing another callback everywhere. I would prefer an enum field saying what to do: an error, a 'static lifetime (for const and static types), a specific lifetime, or a new anonymous lifetime. In fact you can look at the old LifetimeElisionKind, which implements this (but only for diagnostics) and was copied from rustc.
There was a problem hiding this comment.
I would anyways need access to lifetime Arena in GenericParamCollector.
There was a problem hiding this comment.
The callback works by pushing a '_ and using that Idx to create the LifetimeParamId.
There was a problem hiding this comment.
So? Why can't this be done via an enum field?
There was a problem hiding this comment.
yea because of that i went with the callback setup for collect_impl_trait
There was a problem hiding this comment.
Maybe instead of having multiple callback factories, we can have that a state in ExprCollector and based on which a the callback (so a single callback factory) returns appropriate LifetimeRef?
There was a problem hiding this comment.
You can borrow just the parts you need, like you do for a callback.
There was a problem hiding this comment.
I don't think it would work as well, only when lowering functions, because it would be borrowed mutably twice, once when I borrow it for the Arena and the other when it borrows for collect_impl_trait.
It might work in other places tho.
There was a problem hiding this comment.
Closures borrow only the fields they use since edition 2024. If they wouldn't the code using callbacks wouldn't compile either.
| { | ||
| TypeBound::ForLifetime(binder, path) | ||
| } else { | ||
| TypeBound::Path(path, m) |
There was a problem hiding this comment.
Do we even need TypeBound::Path now? After all in rustc_type_ir every predicate has a binder. We could just always produce ForLifetime.
minor fixes related to review comments
5 participants
No description provided.