feat: 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 modulo 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.
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.
There was a problem hiding this comment.
I found a way to work around it, without callbacks, took a lot of trial and error to get the borrow checker right 😅
Now to implement it and see if it doesn't panic anywhere.
| { | ||
| 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.
|
Two tests will be failing, one of which I think value in expect is correct but after infer it gets turned into a |
|
Const eval doesn't care about lifetimes, as long as you only have lifetimes there you can pass erased or identity. |
|
I'm not sure what you mean? |
|
If all args are lifetime, you can pass a dummy |
|
Passing the |
|
Found out why we get error region on other test, when we resolve the types, we fold the |
This comment has been minimized.
This comment has been minimized.
e4832ca to
6526ecd
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| fn elide_return_lifetime(&mut self) { | ||
| let old_elision_kind = | ||
| mem::replace(&mut self.lifetime_elision_kind, LifetimeElisionKind::Error); | ||
|
|
||
| let new_elision_kind = | ||
| if let LifetimeElisionKind::NewLifetimeParam { return_lt, total_created, .. } = | ||
| old_elision_kind | ||
| { | ||
| let lifetime_param_id = | ||
| return_lt.and_then(|(lifetime_param_id, elided_source)| match elided_source { | ||
| ArgumentElisionContext::Self_ => Some(lifetime_param_id), | ||
| ArgumentElisionContext::Param if total_created == 1 => { | ||
| Some(lifetime_param_id) | ||
| } | ||
| ArgumentElisionContext::Param => None, | ||
| }); | ||
| LifetimeElisionKind::Lifetime(lifetime_param_id) | ||
| } else { | ||
| unreachable!("Method should not be called with other lifetime_elision_kind") | ||
| }; | ||
| self.lifetime_elision_kind = new_elision_kind; | ||
| } |
There was a problem hiding this comment.
This setup doesn't quite handle these correctly i think
fn get<'a>(&'a self) -> &str
fn identity<'a>(value: &'a str) -> &str
type Callback = for<'a> fn(&'a str) -> &str;These are valid signatures that participate in elision. The return types here all have 'a as their elided lifetimes.
And for this it incorrectly elides to y's lifetime.
fn bad<'a>(x: &'a str, y: &str) -> &str
That signature is an error, return type elision only happens if the args have exactly one lifetime (or if there is a self param).
Reason being that lower_lifetime_ref doesn't participate (from what I can tell in lifetime_elision_kind).
There was a problem hiding this comment.
I have done the return type elision very haphazardly, I should take a look at it and fix it.
There was a problem hiding this comment.
I think I should introduce a new LifetimeRef for elided lifetimes or designate LifetimeRef:::Param and LifetimeRef::HrtbParam only to be used for elided (by renaming or something). Because with the fix I'm doing for this, a function like this:
fn identity<'a>(value: &'a str) -> &str
// would be lowered and printed as
fn identity<'a>(value: &'a str) -> &'a strBecause I use the LifetimeRef itself, since this is also a valid elision:
fn identity(value: &'static str) -> &str
// it would lower and print
fn identity(value: &'static str) -> &'static strNote that the LifetimeRefId are different in argument and return but the LifetimeRef itself will be same.
There was a problem hiding this comment.
The way elision works, is that first you lowered the args. Then you take all lifetimes defined from the args and for<...> in the fn, in other words all HRTB lifetimes in the last HRTB stack entry for fn ptrs or all lifetime params in a fn definition. If there is exactly one, all elided lifetimes in the return type get it. Otherwise, eliding lifetimes in the return type is an error.
There was a problem hiding this comment.
Yeah I understand that now and is somewhat similar to what I have done, but as you can see from question above since I re-use the LifetimeRef of the lifetime (be it elided or named or static) that can be used for return type, it would result in lowering and analysis that comes after HIR lowering to think, the lifetime in return type was actually provided.
There was a problem hiding this comment.
What is the problem that the code will think that the lifetimes were provided? What does it matter for?
Like for the two test we ignored in ide-diagnostic.
Not sure what the actual behavior is, need to check rustc.
Well I didn't check the rustc code itself, but I printed out the HIR tree for some of the functions discussed here and it seems it's all input lifetimes.
There was a problem hiding this comment.
Like for the two test we ignored in ide-diagnostic.
The end-goal is to remove the elision in hir-ty.
Well I didn't check the rustc code itself, but I printed out the HIR tree for some of the functions discussed here and it seems it's all input lifetimes.
But what is an input lifetime? Any lifetime mentioned in the input?
There was a problem hiding this comment.
Input lifetimes are lifetimes used in the parameters, output are the ones in the return type
- Each elided lifetime in the parameters becomes a distinct lifetime parameter.
- If there is exactly one lifetime used in the parameters (elided or not), that lifetime is assigned to all elided output lifetimes.
In method signatures there is another rule
- If the receiver has type &Self or &mut Self, then the lifetime of that reference to Self is assigned to all elided output lifetime parameters.
There was a problem hiding this comment.
So this is the function in rustc that retrieves the candidate for return elision: resolve_fn_params
It records distinct lifetime usages separately for each input type. I think I make some refactors based on this. But I think my current implementation (unpushed) is very similar.
There was a problem hiding this comment.
Also based on this: https://github.com/rust-lang/rust/blob/main/compiler/rustc_hir/src/def.rs#L932-L964
We can consider LifetimeRef::Param to be elided always?
| (def, is_trait_assoc_item) | ||
| }; | ||
|
|
||
| if collector.argument_elision_context.is_some() && !is_trait_assoc_item { |
There was a problem hiding this comment.
I think this branch means means omitted lifetime arguments are materialized only while lowering parameters. with_param_lt_elision restores argument_elision_context before the return type is lowered, so a signature like:
struct Wrapper<'a>(&'a str);
fn wrap(value: &str) -> Wrapper;creates a lifetime for value, but does not add the selected lifetime to the return Wrapper.
There was a problem hiding this comment.
I think we can change that branch to match the new lifetime_elision_kind instead, i.e if it's Static or Error don't call that function?
There was a problem hiding this comment.
Oh yeah, absolutely. Only resolve paths when it's required.
There was a problem hiding this comment.
But the collector.argument_elision_context.is_some() check is not a good idea. If we don't want to need to duplicate handling of lifetime elision within hir lowering and hir-ty lowering, we need to elide lifetimes even when they appear in the body (but only in non-inferred positions).
| pub(in crate::expr_store) fn lower_generic_args_from_fn_path( | ||
| &mut self, | ||
| args: Option<ast::ParenthesizedArgList>, | ||
| ret_type: Option<ast::RetType>, | ||
| impl_trait_lower_fn: ImplTraitLowerFn<'_>, | ||
| ) -> Option<GenericArgs> { | ||
| let params = args?; | ||
| let mut param_types = Vec::new(); | ||
| for param in params.type_args() { | ||
| let type_ref = self.lower_type_ref_opt(param.ty(), impl_trait_lower_fn); | ||
| param_types.push(type_ref); | ||
| } | ||
| let args = Box::new([GenericArg::Type( | ||
| self.alloc_type_ref_desugared(TypeRef::Tuple(ThinVec::from_iter(param_types))), | ||
| )]); | ||
| let bindings = if let Some(ret_type) = ret_type { | ||
| let type_ref = self.lower_type_ref_opt(ret_type.ty(), impl_trait_lower_fn); | ||
| Box::new([AssociatedTypeBinding { | ||
| name: Name::new_symbol_root(sym::Output), | ||
| args: None, | ||
| type_ref: Some(type_ref), | ||
| bounds: Box::default(), | ||
| }]) | ||
| } else { | ||
| // -> () | ||
| let type_ref = self.alloc_type_ref_desugared(TypeRef::unit()); | ||
| Box::new([AssociatedTypeBinding { | ||
| name: Name::new_symbol_root(sym::Output), | ||
| args: None, | ||
| type_ref: Some(type_ref), | ||
| bounds: Box::default(), | ||
| }]) | ||
| }; | ||
| Some(GenericArgs { | ||
| args, | ||
| has_self_type: false, | ||
| bindings, | ||
| parenthesized: GenericArgsParentheses::ParenSugar, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Parenthesized Fn syntax needs its own lifetime-elision state, like the FnPtrType branch.
type Callback = dyn Fn(&str) -> &str;This should lower as dyn for<'a> Fn(&'a str) -> &'a str.
In a function parameter its a bit different as well:
fn run(callback: &dyn Fn(&str) -> &str) -> &dyn Fn(&str) -> &str;should lower to
fn run<'p>(
callback: &'p dyn for<'a> Fn(&'a str) -> &'a str,
) -> &'p dyn for<'b> Fn(&'b str) -> &'b str;There was a problem hiding this comment.
Oof, I was reminding myself to do it, only to forget it :D
There was a problem hiding this comment.
we can skip this in this PR fwiw and do it as a follow up as long as we track this somewhere
| is_lowering_coroutine: bool, | ||
|
|
||
| for_type_binder: Option<ThinVec<Name>>, | ||
| lifetime_arena: Option<&'a mut Arena<LifetimeParamData>>, |
There was a problem hiding this comment.
This should be in LifetimeElisionKind::NewLifetime. This way we avoid the Option and unwrap().
| return_lt: Option<(Either<HrtbLifetimeParamId, LifetimeParamId>, ArgumentElisionContext)>, | ||
| total_created: u32, | ||
| }, | ||
| Lifetime(Option<Either<HrtbLifetimeParamId, LifetimeParamId>>), |
There was a problem hiding this comment.
This should not be Option. If there is no lifetime to elide, we should set LifetimeElisionKind::Error.
| @@ -157,6 +158,7 @@ pub enum LifetimeRef { | |||
| Static, | |||
| Placeholder, | |||
There was a problem hiding this comment.
We should remove this. It's only used in one place that should be replaced with Error.
| None | Some((_, ForBinderSource::FnPtrType | ForBinderSource::ForBound)) => { | ||
| self.for_type_binder.replace((ThinVec::new(), ForBinderSource::FnPtrType)) | ||
| } | ||
| Some((_, ForBinderSource::ForType)) => None, |
There was a problem hiding this comment.
We should replace this with ForBinderSource::FnPtrType, otherwise nested for<...> fn(for<...> fn()) will be handled incorrectly.
|
|
||
| #[derive(Debug)] | ||
| pub enum LifetimeElisionKind { | ||
| NewLifetimeParam { |
There was a problem hiding this comment.
We should split this and remove ElisionBinderSource. One variant will be NewLifetimeParam { bound_type: LifetimeBoundType }, the other NewHrtbLifetime { for_type_binder: ThinVec<Name> }. parent, return_lt and total_created are not needed, because they're used to determine the elided lifetime for a return type, but it should instead be based on all lifetime generic parameters/hrtb lifetimes, because of the what @Veykril said (non-elided lifetimes also participate). ArgumentElisionContext is not needed either: when determining the elided lifetime for the return type, we can see if we have a &[mut ]self type and if yes its lifetime should always be used. We should also remove ExprCollector::for_type_binder, having it here will avoid unwraps.
| (def, is_trait_assoc_item) | ||
| }; | ||
|
|
||
| if collector.argument_elision_context.is_some() && !is_trait_assoc_item { |
There was a problem hiding this comment.
Oh yeah, absolutely. Only resolve paths when it's required.
| (def, is_trait_assoc_item) | ||
| }; | ||
|
|
||
| if collector.argument_elision_context.is_some() && !is_trait_assoc_item { |
There was a problem hiding this comment.
But the collector.argument_elision_context.is_some() check is not a good idea. If we don't want to need to duplicate handling of lifetime elision within hir lowering and hir-ty lowering, we need to elide lifetimes even when they appear in the body (but only in non-inferred positions).
| }; | ||
|
|
||
| if collector.argument_elision_context.is_some() && !is_trait_assoc_item { | ||
| let args_in_source = generic_args.last().and_then(|g| g.as_ref()); |
There was a problem hiding this comment.
| let args_in_source = generic_args.last().and_then(|g| g.as_ref()); | |
| let args_in_source = generic_args.pop(); |
Then push back. So collect_path_elided_liftetimes() can have an owned value to make use of.
| } | ||
| } | ||
|
|
||
| pub(crate) fn collect_path_elided_liftetimes( |
There was a problem hiding this comment.
| pub(crate) fn collect_path_elided_liftetimes( | |
| pub(crate) fn collect_path_elided_lifetimes( |
Typo.
| let is_trait_assoc_item = matches!(def, Some(ModuleDefId::TraitId(..))) | ||
| && remaining_idx.is_some_and(|idx| idx > 0); |
There was a problem hiding this comment.
Unfortunately this is not correct; lifetimes in trait's assoc types can still be elided, but only for the trait, not for the assoc type.
Yes I think we should |
|
Also I really appreciate you tackling this complex task up @dfireBird! (whether you knew how much work this was gonna end up being or not 😄) |
Veykril
changed the title
I did not know it would be this much but this is fun compared to my day-to-day work :) Also, sorry I've been ignoring @ChayimFriedman2 round 2 Review, will get to it, once I finish the return elision correctly. |
5 participants
No description provided.