-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Properly deduce object lifetime defaults in projections & trait refs #129543
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
base: main
Are you sure you want to change the base?
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
|
|
🚧 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
|
c72baac to
4753491
Compare
|
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. |
4753491 to
27e7703
Compare
Notably, this excludes the self ty. This automatically fixes object lifetime defaulting for trait 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.
* 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)
…ndings if the assoc is lifetime-generic
27e7703 to
7c17fcd
Compare
Object Lifetime Defaults (Primer, Refresher & Definitions)
You can read this section in The Reference but has several issues1. 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 object lifetime bound from context (without relying onrustc_infer's region inference as we might not be in a body2). The "context" means the closest — what I call — eligible generic containerC<X0, …, Xn>that wraps this trait object type. (Eligible generic) container is almost synonymous with type constructor but it also includes type aliases, traits & enum variants.So if we have
C<…, dyn Trait, …>(e.g.,&'r dyn TraitorStruct<'r, dyn Trait>) orC<…, N<…, dyn Trait, …>, …>(e.g.,&'r (dyn Trait,)orStruct<'r, (dyn Trait,)>) whereNdenotes a generic type that is not an eligible generic container, we use the explicit3 outlives-bounds on the corresp. type param ofCto determine the object lifetime bound (the details4 aren't relevant here) (e.g., givenstruct Struct<'a, T: 'a + ?Sized>(…);, we elaborateStruct<'r, dyn Trait>toStruct<'r, dyn Trait + 'r>).Lastly, I call object lifetime bounds used as the default for constituent trait object types of an eligible generic container
Cthe ambient object lifetime defaults for / induced byC(these ambient defaults may be shadowed by inner containers).Changes Made by This PR
<Y0 as TraitRef<X0, …, Xn>>::AssocTy<Y1, …, Ym>now induces ambient object lifetime defaults for constituents Y0 to Ym (TraitRefis considered a separate container, see also list item (2)).Selftype 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::AssocTy<Y1, …, Ym>consider the ambient object lifetime default indeterminateTraitRef<X0, …, Xn>(this fell out from the previous changes). They used to be completely broken due to a gnarly 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)These changes are theoretically breaking because in certain cases they lead to different object lifetime bounds getting deduced compared to master which is obviously user observable. However, the latest crater run found 0 non-spurious regressions.
Motivation: Both object lifetime default RFCs never explicitly specify what constitutes an — what I call — eligible generic container but it only makes sense to include any type constructor or (generic) type alias that can bear outlives-bounds … like associated types. So it's only consistent to make this change.
Fixes #115379.
Fixes #141997.
TODO(#129543 (comment)): Add a clear example regression to the PR description.
TODO: Add lcnr's tests.
TODO: #129543 (comment) needs to be decided.
Footnotes
https://github.com/rust-lang/reference/issues/1407 ↩
If we are in a body, we do however use to normal region inference as a fallback. ↩
Indeed, we don't consider implied bounds (inferred outlives-bounds). ↩
Like how we deal with 'ambiguities' or how bounds 'on' inner TOTs take precedence. ↩