Make trait refs & assoc ty paths properly induce trait object lifetime defaults#129543
Make trait refs & assoc ty paths properly induce trait object lifetime defaults#129543fmease wants to merge 7 commits intorust-lang:mainfrom
Conversation
|
@bors try |
[crater] Properly deduce the object lifetime default in GAT paths Fixes rust-lang#115379. r? ghost
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
[CRATER] Crater Rollup This is a " crater rollup" of: * rust-lang#126452 * rust-lang#128784 * rust-lang#129392 * rust-lang#129422 * rust-lang#129543 **What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build. r? `@ghost`
[CRATER] Crater Rollup This is a " crater rollup" of: * rust-lang#126452 * rust-lang#128784 * rust-lang#129392 * rust-lang#129422 * rust-lang#129543 * rust-lang#129604 **What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build. r? `@ghost`
[CRATER] Crater Rollup This is a " crater rollup" of: * rust-lang#126452 * rust-lang#128784 * rust-lang#129392 * rust-lang#129422 * rust-lang#129543 * rust-lang#129604 **What is a crater rollup?** It's simply a crater job that is run on all of the containing PRs *together*, and then we can set the crates list for each of these jobs to just the failures after it's done. It should cut out on the bulk of "normal" crates that do nothing and simply just take time to build. r? `@ghost`
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
@craterbot crates=https://gist.githubusercontent.com/compiler-errors/4a09d64cd15dc3dca50edeea26cc9938/raw/b4181c225709e120a11d91cce69d0d4da3e652d0/regressed.txt p=1 (Bump it up the queue as this will go quickly.) |
|
📝 Configuration of the ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
This comment has been minimized.
This comment has been minimized.
|
Alright, this is now finally ready for a rereview wrt. both implementation and pFCP'ed semantic changes! Changes since last comment of mine:
@lcnr, @BoxyUwU, I'd be super happy if you both could take a look esp. in regards to the (latest version of the) pFCP'ed semantic changes since I'd love it if this can finally go into FCP :D Thanks a lot in advance :) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
This test didn't actually test what it meant to test. I've therefore replaced it with a new one (tests/ui/object-lifetime/object-lifetime-default-inferred-args.rs). It was added in the initial generic_arg_infer PR.
It's meant to test that the presence of _ in a generic arg list doesn't throw off the trait object lifetime default calculations which is a very valuable thing to test (hence me adding a replacement). Sadly, it doesn't test that.
First of all, the only places in this file that contain implicit trait object lifetime bounds that are in the mere vicinity of an inferred arg _ are the two &dyn Tests at the bottom. However, these just get elab'ed to &'?0 (dyn Test + '?1) and &'2 (dyn Test + '?3) since the implicit inferred lifetimes passed to & aren't named, so the default is indeterminate meaning fresh region vars for both since we're in a body.
The two _ passed as part of the fn calls have no influence whatsoever on the defaults chosen for the dyn Test "in random as-casts"; that's not how trait object lifetime defaulting works.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ir own args (excl. self ty) This automatically fixes trait object lifetime defaulting for trait refs and trait alias refs, too. These used to be broken because the index calculation for going from middle generic args back to HIR ones didn't take into account the implicit self ty param present on traits. Moreover, in item signatures reject hidden trait object lifetime bounds inside type-relative paths (excl. the self ty) on grounds of the default being indeterminate.
* print `Empty` as it's called in code, otherwise it's unnecessarily confusing * go through the middle::ty Generics instead of the HIR ones, so we can dump the default for the implicit `Self` type parameter of traits, too * allow the attribute on more targets (it used to be allowed anywhere for the longest time but someone must've incorrectly restricted it during the migration to the new attribute parsing API)
…ntiated assoc ty bindings Namely, on the RHS and in the args of the bindings.
|
This PR was rebased onto a different main 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. |
| // | ||
| // FIXME(mgca, #151649): Assoc (and free?) consts should also qualify. | ||
| // FIXME(return_type_notation, #151662): Assoc fns should also qualify. | ||
| let depth = path.segments.len() - index - 1; |
There was a problem hiding this comment.
can you add a comment explaining what "depth" is, afaict what we care about for AssocTy is if we have T::Assoc, are we in T and for variant we have T::Variant we check whether we're in Variant? That feels odd to me.
I guess assoc types want the info from their trait and everything else wants it from itself, but that doesn't explain variant, with enums you can have generic args both on the enum and on the variant, i.e. both Enum::<args>::Variant and Enum::Variant::<args> are valid
| ) if depth == 0 => Some((def_id, path.segments)), | ||
| // Note: We don't need to care about definition kinds that may have generics if they | ||
| // can only ever appear in positions where we can perform type inference (i.e., bodies). | ||
| _ => None, |
There was a problem hiding this comment.
how annoying is it to make this exhaustive?
| generics: &ty::Generics, | ||
| tcx: TyCtxt<'_>, | ||
| def_id: DefId, | ||
| ) -> (usize, usize) { |
There was a problem hiding this comment.
comment please 🤔 depth is "index of segment in reverse order" and "index" is "index of args of segment"?
| //@ check-pass | ||
|
|
||
| trait Trait<T: ?Sized> { type Assoc<'a> where T: 'a; } | ||
| impl<T: ?Sized> Trait<T> for () { type Assoc<'a> = &'a T where T: 'a; } |
| // We deduce `dyn Inner + 'r` from bound `'a` on ty param `T` of trait `Outer`. | ||
| fn assoc_ty_proj<'r>(_: <() as Outer<'r, dyn Inner>>::Ty) {} | ||
|
|
||
| impl<'a, T: 'a + AbideBy<'a> + ?Sized> Outer<'a, T> for () { type Ty = &'a T; } |
There was a problem hiding this comment.
move this to trait definition?
|
|
||
| impl<'a, T: 'a + AbideBy<'a> + ?Sized> Outer<'a, T> for () { type Ty = &'a T; } | ||
|
|
||
| trait Inner {} |
| self.tcx.parent(def_id), | ||
| &path.segments[..path.segments.len() - 1], | ||
| )), | ||
| _ => None, |
There was a problem hiding this comment.
what are the cases where this change goes from indeterminate to pass-through due to this change?
something something exhaustive match possible?
|
|
||
| fn is_static<T>(_: T) where T: 'static {} | ||
|
|
||
| // FIXME: Ideally, we'd elaborate `dyn Bar` to `dyn Bar + 'static` instead of rejecting it. |
There was a problem hiding this comment.
do we? 😅 I am not even sure whether this is desirable or not.
|
In the list of examples, can you match them to each of the changes from this PR. i.e have
// this now chooses `'a` and compiles
// this previously chose `'b` and now chooses `'c`, causing an error... after this I am gonna start the FCP this is a really impressive PR :> sorry for taking so long to get to it. |
View all comments
Trait Object Lifetime Defaults
Primer & Definitions
You could read this section in the Reference but it has several issues (see rust-lang/reference#1407). Here's a small explainer by me that only mentions the parts relevant to this PR:
Basically, given
dyn Trait(≠dyn Trait + '_) we want to deduce its trait object lifetime bound from context without relying on normal region inference as we might not be in a body1. The "context" means the closest – what I call — (eligible) containerC<X0, …, Xn>that wraps this trait object type. A container is to be understood as a use site of a "parametrized definition" (more general than type constructors). Currently eligible are ADTs, type aliases, traits and enum variants.So if we have
C<dyn Trait>(e.g.,&'r dyn TraitorStruct<'r, dyn Trait>),D<C<dyn Trait>>orC<N<dyn Trait>>(e.g.,Struct<'r, (dyn Trait,)>), we use the explicit2 outlives-bounds on the corresponding type parameter ofCto determine the trait object lifetime bound. Here,C&Ddenote (eligible) containers andNdenotes a generic type that is not an eligible container. E.g., givenstruct Struct<'a, T: 'a + ?Sized>(…);, we elaborateStruct<'r, dyn Trait>toStruct<'r, dyn Trait + 'r>.Finally, we call lifetime bounds used as the default for constituent trait object types of an eligible container
Cthe trait object lifetime defaults (induced byC) which may be shadowed by inner containers.Changes Made
<Y0 as TraitRef<X0, …, Xn>>::AssocTy<Y1, …, Ym>now induces trait object lifetime defaults for constituentsY0toYm(TraitRefis considered a separate container, see also list item (2)).Y0of (resolved) projections we now look at the bounds on theSelftype param of the relevant trait (e.g., giventrait Outer<'a>: 'a { type Proj; }ortrait Outer<'a> where Self: 'a { type Proj; }we elaborate<dyn Inner as Outer<'r>>::Projto<dyn Inner + 'r as Outer<'r>>::Proj).Y0::Name<Y1, …, Ym>consider the trait object lifetime default indeterminateTraitRef<X0, …, Xn>(this fell out from the previous changes). They used to be completely broken due to a nasty off-by-one error for not accounting for the implicitSelftype param of traits which lead to cases likeOuter<'r, dyn Inner>(withtrait Outer<'a, T: 'a + ?Sized> {}) getting rejected as indeterminate (it tries to access a lifetime at index 1 instead 0) (playground)Outer<'r, 's, dyn Inner>(withtrait Outer<'a, 'b, T: 'a + ?Sized> {}) elaboratingdyn Innertodyn Inner + 'sinstead ofdyn Inner + 'r(!) (playground)trait_alias)TraitRef<AssocTy<X0, …, Xn> = Y>consider the trait object lifetime default indeterminate (inX0, …,XnandY) ifX0, …,Xncontains any lifetime arguments.AssocTyin the future when computing the default forY(2) take into account the parameter bounds ofAssocTyin the future when computing the defaults forX0, …,Xn.TraitRef<X0, …, Xn, AssocTy<Y0, …, Ym> = Z>– treats the default indeterminate inY0, …,YmandZifX0, …,Xncontains any lifetime arguments.Motivation
Both trait object lifetime default RFCs (599 and 1156) never explicitly specify what constitutes a — what I call — (eligible) container but it only makes sense to include anything that can be parametrized by generics and can be mentioned in places where we don't perform full region inference … like associated types. So it's only consistent to make this change.
Breakages
These changes are theoretically breaking because they can lead to different trait object lifetime bounds getting deduced compared to main which is obviously user observable. Moreover, we're now explicitly rejecting implicit trait object lifetime bounds inside type-relative paths (excl. the self type) and on the RHS of assoc type bindings if the assoc type has lifetime params.
However, the latest crater run found 0 non-spurious regressions (see here and here).
Examples (5)
Fixes #115379.
Fixes #140710.
Fixes #141997.
Footnotes
If we are in a body we do use to normal region inference as a fallback. ↩
Indeed, we don't consider implied bounds (inferred outlives-bounds). ↩