-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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: master
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
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Properly deduce object lifetime defaults in projections & trait refs #### Object Lifetime Defaults (Primer, Refresher & Definitions) You can read [this section](https://doc.rust-lang.org/reference/lifetime-elision.html#default-trait-object-lifetimes) in The Reference but it's not perfect IMO. 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 on `rustc_infer`'s region inference as we might not be in a body[^1]). The "context" means the closest — what I call — *eligible generic container* `C<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 Trait` or `Struct<'r, dyn Trait>`) or `C<…, N<…, dyn Trait, …>, …>` (e.g., `&'r (dyn Trait,)` or `Struct<'r, (dyn Trait,)>`) where `N` denotes a generic type that is **not** an eligible generic container, we use the explicit[^2] outlives-bounds on the corresp. type param of `C` to determine the object lifetime bound (the details[^3] aren't relevant here) (e.g., given `struct Struct<'a, T: 'a + ?Sized>(…);`, we elaborate `Struct<'r, dyn Trait>` to `Struct<'r, dyn Trait + 'r>`). Lastly, I call object lifetime bounds used as the default for *constituent* trait object types of an eligible generic container `C` the *ambient object lifetime defaults* for / induced by `C` (these ambient defaults may be shadowed by inner containers). --- #### Changes Made by This PR 1. Make associated type paths / projections *eligible generic containers*. * `<Y0 as TraitRef<X0, …, Xn>>::AssocTy<Y1, …, Ym>` now induces *ambient object lifetime defaults* for constituents Y0 to Ym (`TraitRef` is considered a separate container, see also list item **(2)**). * Similar case with type-relative ("shorthand") paths `Y0::AssocTy<Y1, …, Ym>` * Notably, for the self type Y0 of resolved projections we now look at the bounds on the `Self` type param of the relevant trait (e.g., given `trait Outer<'a>: 'a { type Proj; }` or `trait Outer<'a> where Self: 'a { type Proj; }` we elaborate `<dyn Inner as Outer<'r>>::Proj` to `<dyn Inner + 'r as Outer<'r>>::Proj`). 2. Fixes object lifetime defaults inside trait refs `TraitRef<X0, …, X1>` (this fell out from the previous changes). There used be completely broken due to a gnarly off-by-one error for not accounting for the implicit `Self` type param of traits which leads to cases like * `Outer<'r, dyn Inner>` (with `trait Outer<'a, T: 'a + ?Sized> {}`) getting rejected as "inderminate" (it tries to access a *lifetime* at index 1 instead 0) ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=0069c89b2313f0f447ff8b6f7de9adfa)) * `Outer<'r, 's, dyn Inner>` (with `trait Outer<'a, 'b: T: 'a + ?Sized> {}`) elaborating `dyn Inner` to `dyn Inner + 's` instead of `dyn Inner + 'r`(!) ([playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=9c521165e0ac0d868a8087cd7ca861fe)) **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](rust-lang#129543 (comment)) 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 rust-lang#115379. r? ghost [^1]: If we *are* in a body, we do however use to normal region inference as a fallback. [^2]: Indeed, we don't consider implied bounds (inferred outlives-bounds). [^3]: Like how we deal with 'ambiguities' or how we look at the bounds of inner TOT as a fallback.
⌛ Trying commit ec131fe with merge 99962d8169c938d94d1a9642217089b43363e569... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (99962d8): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.7%, secondary -0.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 770.356s -> 769.507s (-0.11%) |
fn g<'r>(_: <() as Outer>::Ty<'r, dyn Inner>) {} | ||
} | ||
|
||
mod parent { // the object lifetime default comes from the parent generics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add a test with both self and parent lifetimes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean one where the assoc ty is type AssocTy<'own, T: ?Sized + 'own + 'parent>;
or type AssocTy<'own, T: ?Sized + 'own, U: ?Sized + 'parent>;
or something else entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had in mind just type AssocTy<'own, T: ?Sized + 'own>
(basically identical to the first example, but showing that the extra lifetime doesn't throw it off), and ideally some sort of failure tests of cases that are using the wrong lifetime.
@rfcbot merge See summary the main PR comment. For an explanation and rationale of what is going on here. |
Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
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 of 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.
Could you add an example to the "theoretically breaking" section with a little bit of prose explaining why it breaks? My understanding is that something like this would break: trait Identity<'a>: 'a {
type Assoc: ?Sized;
}
impl<'a, T: ?Sized + 'a> Identity<'a> for T {
type Assoc = T;
}
trait Trait {}
fn foo<'b>(a: Box<<dyn Trait as Identity<'b>>::Assoc>) {
let a: Box<dyn Trait + 'static> = a;
} Right now it compiles because we don't use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel generally positive towards the conceptual change to the type system here. It feels like a very straightforward "bug fix" change.
} | ||
|
||
let (depth, index) = param_to_depth_and_index(generics, self.tcx, param_def_id); | ||
segments[segments.len() - depth - 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems to ICE with on this example:
trait Identity<'a> {
type Assoc<U: ?Sized + 'a>: ?Sized;
}
impl<'a, T> Identity<'a> for T {
type Assoc<U: ?Sized + 'a> = U;
}
trait Trait {}
fn bar<'b, T: for<'c> Identity<'b, Assoc<dyn Trait + 'c> = dyn Trait + 'c>>(
a: Box<<T>::Assoc<dyn Trait>>,
) {
}
fn main() {
bar::<()>;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I didn't think about parent-supplied lifetimes in type-relative paths. Thanks for catching this! Reduced:
trait Outer<'a> { type Ty<T: ?Sized + 'a>; }
impl<'a, X> Outer<'a> for X { type Ty<T: ?Sized + 'a> = (); }
trait Inner {}
fn f<'r, T: Outer<'r>>(a: T::Ty<dyn Inner>) {}
fn main() {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much less messy than my example 😆
// FIXME: Duplicating efforts is not robust or sustainable/maintainable. | ||
// Ideally, we'd simply obtain the resulting type-dependent defs from | ||
// HIR ty lowering (not only in FnCtxts but also in ItemCtxts!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you said it yourself here :)
Handling TypeRelative
paths by duplicating hir ty lowering logic here seems unfortunate. Do you think it'd be reasonable to just continue with the stable behaviour for TypeRelative
paths, this PR would still be a positive change for fully qualified aliases and trait refs.
Though, admittedly, I'm not sure how we'd ever make this work "properly". Propagating type dependent def resolutions from inside ty lowering into the internals here seems unlikely to happen.
Maybe we could have a special resolve_dyn_type_lifetime_bound
query that returns whatever we resolved and how many type dependent defs we stepped through (i.e. some kind of [unknown, unknown, 'a, unknown]
). Hir ty lowering could then track its own set of lifetime defaults coming from type dependent paths and pair that together to figure out what it should actually use.... Seems somewhat complicated 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh, I'd find it really unfortunate if there were such an obscure behavior difference between resolved & type-relative paths! Esp. since my personal motivation for this PR is to 'streamline' the rules of object lifetime defaults by making them apply everywhere where one would expect them to.
For the longest time this branch simply rejected implicit object lifetime bounds in type-relative associated types as "indeterminate" (in item ctxts) — that I'd be more than fine with and crater also didn't complain about that. However, that'd regress tests/ui/deriving/issue-89188-gat-hrtb.rs
/ issue #89188.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that limited_resolve_type_relative_path
is already used by RTN, I only extracted the logic into a separate function, so I can use it, too. So even without this PR, in PR #126651 (supporting T::AssocTy0::AssocTy1
I (still) need to update it (demonstrating how 'unsustainable' this approach already is)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay 🤔 If it's pre-existing then I think it's fine 👍 even if long term would be nice to not have to do. Ah I guess RTN is unstable though so it would be somewhat reasonable to hold stable codepaths to a higher standard 🤔
Object Lifetime Defaults (Primer, Refresher & Definitions)
You can read this section in The Reference but it's not perfect IMO. 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 body1). 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 Trait
orStruct<'r, dyn Trait>
) orC<…, N<…, dyn Trait, …>, …>
(e.g.,&'r (dyn Trait,)
orStruct<'r, (dyn Trait,)>
) whereN
denotes a generic type that is not an eligible generic container, we use the explicit2 outlives-bounds on the corresp. type param ofC
to determine the object lifetime bound (the details3 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
C
the 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 (TraitRef
is considered a separate container, see also list item (2)).Y0::AssocTy<Y1, …, Ym>
Self
type 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>>::Proj
to<dyn Inner + 'r as Outer<'r>>::Proj
).TraitRef<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 implicitSelf
type 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 Inner
todyn Inner + 's
instead 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.
r? ghost
Footnotes
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 we look at the bounds of inner TOTs as a fallback. ↩