Account for late-bound lifetimes in generics #103448
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
T-compiler
|
Some changes occurred in src/librustdoc/clean/types.rs cc @camelid Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
rust-highfive
added
the
S-waiting-on-review
oli-obk
added
I-types-nominated
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
|
☔ The latest upstream changes (presumably #103337) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #103623) made this pull request unmergeable. Please resolve the merge conflicts. |
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Nov 8, 2022
…d, r=nikomatsakis avoid making substs of type aliases late bound when used as fn args fixes rust-lang#47511 fixes rust-lang#85533 (although I did not know theses issues existed when i was working on this 🙃) currently `Alias<...>` is treated the same as `Struct<...>` when deciding if generics should be late bound or early bound but this is not correct as `Alias` might normalize to a projection which does not constrain the generics. I think this needs more tests before merging more explanation of PR [here](https://hackmd.io/v44a-QVjTIqqhK9uretyQg?view) Hackmd inline for future readers: --- This assumes reader is familiar with the concept of early/late bound lifetimes. There's a section on rustc-dev-guide if not (although i think some details are a bit out of date) ## problem & background Not all lifetimes on a fn can be late bound: ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef { type Output = &'a (); // uh oh unconstrained lifetime } ``` so we make make them early bound ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef<'a> {// wow look at all that lifetimey type Output = &'a (); } ``` (Closures have the same constraint however it is not enforced leading to soundness bugs, [rust-lang#84385](rust-lang#84385) implements this "downgrading late bound to early bound" for closures) lifetimes on fn items are only late bound when they are "constrained" by the fn args: ```rust fn foo<'a>(_: &'a ()) -> &'a (); // late bound, not present on `FooFnItem` // vv impl<'a> Trait<(&'a (),)> for FooFnItem { type Output = &'a (); } // projections do not constrain inputs fn bar<'a, T: Trait>(_: <T as Trait<'a>>::Assoc) -> &'a (); // early bound // vv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for BarFnItem<'a, T> { type Output = &'a (); } ``` current logic for determining if inputs "constrain" a lifetime works off of HIR so does not normalize aliases. It also assumes that any path with no self type constrains all its substs (i.e. `Foo<'a, u32>` has no self type but `T::Assoc` does). This falls apart for top level type aliases (see linked issues): ```rust type Alias<'a, T> = <T as Trait<'a>>::Assoc; // wow look its a path with no self type uwu // i bet that constrains `'a` so it should be latebound // vvvvvvvvvvv fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a (); // `Alias` normalized to make things clearer // vvvvvvvvvvvvvvvvvvvvvvv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for FooFnDef<T> { type Output = &'a (); // oh no `'a` isnt constrained wah wah waaaah *trumbone noises* // i think, idk what musical instrument that is } ``` ## solution The PR solves this by having the hir visitor that checks for lifetimes in constraining uses check if the path is a `DefKind::Alias`. If it is we ""normalize"" it by calling `type_of` and walking the returned type. This is a bit hacky as it requires a mapping between the substs on the path in hir, and the generics of the `type Alias<...>` which is on the ty layer. Alternative solutions may involve calculating the "late boundness" of lifetimes after/during astconv rather than relying on hir at all. We already have code to determine whether a lifetime SHOULD be late bound or not as this is currently how the error for `fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();` gets emitted. It is probably not possible to do this right now, late boundness is used by `generics_of` and `gather_explicit_predicates_of` as we currently do not put late bound lifetimes in `Generics`. Although this seems sus to me as the long term goal is to make all generics late bound which would result in `generics_of(function)` being empty? [rust-lang#103448](rust-lang#103448) places all lifetimes in `Generics` regardless of late boundness so that may be a good step towards making this possible.
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Nov 8, 2022
…d, r=nikomatsakis avoid making substs of type aliases late bound when used as fn args fixes rust-lang#47511 fixes rust-lang#85533 (although I did not know theses issues existed when i was working on this 🙃) currently `Alias<...>` is treated the same as `Struct<...>` when deciding if generics should be late bound or early bound but this is not correct as `Alias` might normalize to a projection which does not constrain the generics. I think this needs more tests before merging more explanation of PR [here](https://hackmd.io/v44a-QVjTIqqhK9uretyQg?view) Hackmd inline for future readers: --- This assumes reader is familiar with the concept of early/late bound lifetimes. There's a section on rustc-dev-guide if not (although i think some details are a bit out of date) ## problem & background Not all lifetimes on a fn can be late bound: ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef { type Output = &'a (); // uh oh unconstrained lifetime } ``` so we make make them early bound ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef<'a> {// wow look at all that lifetimey type Output = &'a (); } ``` (Closures have the same constraint however it is not enforced leading to soundness bugs, [rust-lang#84385](rust-lang#84385) implements this "downgrading late bound to early bound" for closures) lifetimes on fn items are only late bound when they are "constrained" by the fn args: ```rust fn foo<'a>(_: &'a ()) -> &'a (); // late bound, not present on `FooFnItem` // vv impl<'a> Trait<(&'a (),)> for FooFnItem { type Output = &'a (); } // projections do not constrain inputs fn bar<'a, T: Trait>(_: <T as Trait<'a>>::Assoc) -> &'a (); // early bound // vv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for BarFnItem<'a, T> { type Output = &'a (); } ``` current logic for determining if inputs "constrain" a lifetime works off of HIR so does not normalize aliases. It also assumes that any path with no self type constrains all its substs (i.e. `Foo<'a, u32>` has no self type but `T::Assoc` does). This falls apart for top level type aliases (see linked issues): ```rust type Alias<'a, T> = <T as Trait<'a>>::Assoc; // wow look its a path with no self type uwu // i bet that constrains `'a` so it should be latebound // vvvvvvvvvvv fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a (); // `Alias` normalized to make things clearer // vvvvvvvvvvvvvvvvvvvvvvv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for FooFnDef<T> { type Output = &'a (); // oh no `'a` isnt constrained wah wah waaaah *trumbone noises* // i think, idk what musical instrument that is } ``` ## solution The PR solves this by having the hir visitor that checks for lifetimes in constraining uses check if the path is a `DefKind::Alias`. If it is we ""normalize"" it by calling `type_of` and walking the returned type. This is a bit hacky as it requires a mapping between the substs on the path in hir, and the generics of the `type Alias<...>` which is on the ty layer. Alternative solutions may involve calculating the "late boundness" of lifetimes after/during astconv rather than relying on hir at all. We already have code to determine whether a lifetime SHOULD be late bound or not as this is currently how the error for `fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();` gets emitted. It is probably not possible to do this right now, late boundness is used by `generics_of` and `gather_explicit_predicates_of` as we currently do not put late bound lifetimes in `Generics`. Although this seems sus to me as the long term goal is to make all generics late bound which would result in `generics_of(function)` being empty? [rust-lang#103448](rust-lang#103448) places all lifetimes in `Generics` regardless of late boundness so that may be a good step towards making this possible.
Manishearth
added a commit
to Manishearth/rust
that referenced
this pull request
Nov 9, 2022
…d, r=nikomatsakis avoid making substs of type aliases late bound when used as fn args fixes rust-lang#47511 fixes rust-lang#85533 (although I did not know theses issues existed when i was working on this 🙃) currently `Alias<...>` is treated the same as `Struct<...>` when deciding if generics should be late bound or early bound but this is not correct as `Alias` might normalize to a projection which does not constrain the generics. I think this needs more tests before merging more explanation of PR [here](https://hackmd.io/v44a-QVjTIqqhK9uretyQg?view) Hackmd inline for future readers: --- This assumes reader is familiar with the concept of early/late bound lifetimes. There's a section on rustc-dev-guide if not (although i think some details are a bit out of date) ## problem & background Not all lifetimes on a fn can be late bound: ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef { type Output = &'a (); // uh oh unconstrained lifetime } ``` so we make make them early bound ```rust fn foo<'a>() -> &'a (); impl<'a> Fn<()> for FooFnDef<'a> {// wow look at all that lifetimey type Output = &'a (); } ``` (Closures have the same constraint however it is not enforced leading to soundness bugs, [rust-lang#84385](rust-lang#84385) implements this "downgrading late bound to early bound" for closures) lifetimes on fn items are only late bound when they are "constrained" by the fn args: ```rust fn foo<'a>(_: &'a ()) -> &'a (); // late bound, not present on `FooFnItem` // vv impl<'a> Trait<(&'a (),)> for FooFnItem { type Output = &'a (); } // projections do not constrain inputs fn bar<'a, T: Trait>(_: <T as Trait<'a>>::Assoc) -> &'a (); // early bound // vv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for BarFnItem<'a, T> { type Output = &'a (); } ``` current logic for determining if inputs "constrain" a lifetime works off of HIR so does not normalize aliases. It also assumes that any path with no self type constrains all its substs (i.e. `Foo<'a, u32>` has no self type but `T::Assoc` does). This falls apart for top level type aliases (see linked issues): ```rust type Alias<'a, T> = <T as Trait<'a>>::Assoc; // wow look its a path with no self type uwu // i bet that constrains `'a` so it should be latebound // vvvvvvvvvvv fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a (); // `Alias` normalized to make things clearer // vvvvvvvvvvvvvvvvvvvvvvv impl<'a, T: Trait> Fn<(<T as Trait<'a>>::Assoc,)> for FooFnDef<T> { type Output = &'a (); // oh no `'a` isnt constrained wah wah waaaah *trumbone noises* // i think, idk what musical instrument that is } ``` ## solution The PR solves this by having the hir visitor that checks for lifetimes in constraining uses check if the path is a `DefKind::Alias`. If it is we ""normalize"" it by calling `type_of` and walking the returned type. This is a bit hacky as it requires a mapping between the substs on the path in hir, and the generics of the `type Alias<...>` which is on the ty layer. Alternative solutions may involve calculating the "late boundness" of lifetimes after/during astconv rather than relying on hir at all. We already have code to determine whether a lifetime SHOULD be late bound or not as this is currently how the error for `fn foo<'a, T: Trait>(_: Alias<'a, T>) -> &'a ();` gets emitted. It is probably not possible to do this right now, late boundness is used by `generics_of` and `gather_explicit_predicates_of` as we currently do not put late bound lifetimes in `Generics`. Although this seems sus to me as the long term goal is to make all generics late bound which would result in `generics_of(function)` being empty? [rust-lang#103448](rust-lang#103448) places all lifetimes in `Generics` regardless of late boundness so that may be a good step towards making this possible.
|
r? types |
|
we should probably land
in a separate PR so we can sign off on it there in parallel to reviewing this PR. |
lcnr
added
S-waiting-on-team
Marking it as blocked on this, also note that that pr linked above has been closed |
Dylan-DPC
added
S-blocked
|
We discussed this in the types team planning meeting today (https://rust-lang.zulipchat.com/#narrow/stream/326132-t-types.2Fmeetings/topic/2023-07-03.20planning.20meeting/near/372029871). We decided we'd like to have two deep dives related to this. First, a background on early/late lifetime distinctions, and second, a proposal on this change (or similar). Going to unnominate for now, but we should definitely come back to this during the second deep dive. |
|
blocked on rust-lang/types-team#62, unnominating |
BoxyUwU
removed
the
I-types-nominated
lcnr
removed
the
I-types-nominated
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, functions have two kinds of lifetime parameters: early-bound (which appear in where clauses or do not appear in inputs), and late-bound. Late-bound lifetimes do not appear in generics, they only exist inside the function signature. For instance, take this code:
Then we have
generics_of(foo) = ['b, T]andfn_sig(foo::<'b, T>) = for<'a> fn().This PR proposes to make all lifetimes on functions early-bound.
The main motivation is to simplify RPIT and async desugaring, to be submitted as a separate PR (#103449).
To achieve that, all lifetimes are included when constructing
generics_of. The early vs late distinction is still saved inGenericParamDefKind::Lifetime. Thefn_sigquery no longer returns an higher-rankedfntype, but afntype with all lifetimes early-bound. The higher-ranked version is computed byTy::fn_sigfunction by rebinding lifetimes based onGenericParamDefKind::Lifetime. The implicit decay fromFnDefto higher-rankedfnis handled in type coercion.This PR has to work around some the former difference in treatment between early- and late-bound lifetimes. Notably, changing the amount of late-bound lifetimes between trait and impl is allowed.
This makes the current API around rebasing substs an impl to a trait a bit of a footgun. I shifted the late-bound lifetimes to the end of the generics to mitigate the impact. This may cause a issues with RPITIT, except if we disallow such liberties with RPITIT.
To simplify the implementation, I also make
late_bound_lifetime_argumentslint (#42868) a hard error. That lint has been there for 5 years.There are still a few edges to sand off here. I'll file an MCP depending on reception by the types team.
Still TODO:
rebase_ontoAPI.r? types