-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Enforce that closure: 'a
requires that closure_ret_ty: 'a
holds
#84385
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
@@ -39,7 +39,7 @@ fn filter_map_fold<T, B, Acc>( | |||
} | |||
} | |||
|
|||
fn filter_map_try_fold<'a, T, B, Acc, R: Try<Ok = Acc>>( | |||
fn filter_map_try_fold<'a, T, B, Acc, R: Try<Ok = Acc> + 'a>( |
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 think that we could make this change unnecessary with smarter inference. The requirement comes about because we return a closure which captures the lifetime 'a
(via the upvar f
), so the return type impl FnMut(Acc, T) -> R + 'a
needs to outlive 'a
rather than 'static
. Therefore, when we return the closure, we end up needing to prove closure: 'a
, which leads to the requirement that the return type R
also outlives 'a
(via the new rule introduced by this PR).
Technically, I think we could get away with requiring something weaker than closure_ty: 'a
. Since the opaque type itself is only known to outlive 'a
, then the 'inputs' to any projection we make using it can only be known to outlive 'a
. Therefore, we can only conclude that the return type R
outlives 'a
, and not any other region.
However, I don't know how we'd go about expressing that requirement without making some subtle and complicated code even more subtle and 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.
Another thing that might help is that the type of the
mut fold: impl FnMut(Acc, B) -> R + 'a
argument already effectively requires/proves R: 'a
.
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.
Indeed, I am wondering if we can mitigate the impact by leveraging implied bounds.
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.
Revisiting this: I don't think that the argument requires R: 'a
. We could pass in a function pointer for fold
that looks like:
fn my_fold<'b>(acc: &'b u8, other: ()) -> &'b u8 { acc }
If the 'a
from the original function is 'static
, then we can not conclude that R: 'a
holds.
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.
That function should outlive 'static
purely because the 'b
lifetime is constrained by the inputs so it doesn't need to live in the self type and play a role in outlives: playground You're right afaict though that impl FnMut(T) -> R + 'a
does not prove R: 'a
since that only requires /* CLOSURE */: 'a
to hold, you'd also need T: 'a
to prove that R: 'a
holds. (maybe its clearer if we write it as where ANON_T: Fn<(Acc, B), Output = R> + 'a
?)
I think the kind of implied bounds we would want to allow this to compile "as is" is different than the rules that tell us <T as Trait<U>>::Assoc: 'a if T: 'a, U: 'a
. The reason for proving the return type outlives 'a
when proving Closure: 'a
is because conceptually we're wanting to avoid the builtin closure impls having unconstrained lifetimes for example:
impl<'a> Fn<()> for Closure { type Output = &'a () }
This PR solves that by conceptually having the Output
type in the Self
type:
impl<'a> Fn<()> for Closure<&'a ()> { type Output = &'a () }
Which then means that proving Closure: 'lifetime
also has to prove &'a (): 'lifetime
.
(fn
items currently have a similar workaround where fn foo<'a>() -> &'a ()
makes 'a
early bound causing the foo
item to not outlive 'static
.)
If we have a closure with a return type containing a generic param we have to conservatively assume there might be lifetimes in it that are unconstrained, i.e. || { T::default() }
called with T=&'a ()
shouldn't lead to a Self: 'static
closure. (From what I can tell this seems to be a lot of cases of back incompat changes in this PR?)
So I think the reason we can "prove" that the following is OK:
fn find<'a, 'b, T, B>(
f: &'a mut impl FnMut(T) -> Option<B> + 'b,
) -> impl FnMut((), T) -> ControlFlow<B> + 'a {
move |(), x| match f(x) {
Some(x) => ControlFlow::Break(x),
None => ControlFlow::CONTINUE,
}
}
is either:
B
has to be part of theSelf
type of the return closure because it would have unconstrained lifetimes thereforeimpl FnMut((), T) -> ControlFlow<B> + 'a
requiresB: 'a
- the argument
f
has the same inputs so if our returned closure had to haveB
in the self type, so wouldf
f
outlives'b
andB
is insidef
and'b
outlives'a
thereforeB: 'a
- the argument
B
does not have to be part of theSelf
type because all of its lifetimes are constrained by input types- pre-this-PR outlives rules only taking into account the
f
upvar which we know outlives'b
which outlives'a
- pre-this-PR outlives rules only taking into account the
Annoyingly you cant actually know which one of those is true. The important thing is that one of the two must be true, and either case allows you to prove the closure outlives 'a
.
This comment has been minimized.
This comment has been minimized.
It seems like we want a crater run. |
Thanks @Aaron1011; I've been wanting to revisit those outlives rules (for both fn types and closures) for some time. I agree with the general direction of this PR, the main question I have is what we can do to help reduce its impact. |
// This is inconsistent with function pointers, which require that all of their | ||
// argument types (as well as the return type) outlive `'a` in order for | ||
// `fn(A, B) -> R : ' a` to hold. It would be a breaking change to enforce this for | ||
// closuers, and is not required for soundness. |
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 agree that this is sound, but there is something slightly missing from the description. What RFC 1214 actually states is that
<C as FnOnce<A>>::Output: 'x
can be concluded from the fact that C: 'x
and A: 'x
. The A
here are precisely the argument types. So in some sense the argument types of the closure should not be seen as part of the closure type: they are stored here for "convenience" but really they are a function of the closure type's impl.
Actually, we should probably move them out from the closure substs and into some side-table.
Hmm, the same is roughly true of the return type. Perhaps the real problem begins earlier, at some earlier stage of the compiler.
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'm going to leave a comment on the main issue while I think about this.
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.
Ok, I'm happy with the approach, but I think it's not quite right for generators, and it needs two new tests. Explanation here. Nice work @Aaron1011!
96a18d5
to
6aba64c
Compare
@nikomatsakis: I've accounted for the generator yield type, and added tests involving generator return and yield types. |
This comment has been minimized.
This comment has been minimized.
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.
Thanks @Aaron1011. I gave it another skim and noticed again the fallout, which is mildly worrisome. I am having some slight second thoughts. I'm going to sleep on it and see if I can't propose any alternative approches.
// closuers, and is not required for soundness. | ||
// | ||
// Note: The 'skip_binder()' call here matches the way we handle fn substs | ||
// via `walk_shallow`, which also skips binders. |
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.
Hmm, I don't love the skip_binder
call. But I'm not sure how better to fix just yet.
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.
If you think about it in terms of the impl one would write, however, the binder would not really be skipped...
// `fn(A, B) -> R : ' a` to hold. It would be a breaking change to enforce this for | ||
// closuers, and is not required for soundness. | ||
// | ||
// Note: The 'skip_binder()' call here matches the way we handle fn substs |
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 might like to rewrite this more in terms of the comment I left-- but now I'm wanting to think just a bit more about this fix, so I wouldn't try to do that just yet.
This comment has been minimized.
This comment has been minimized.
@Aaron1011 I think this does not fully close the original issue because it does not address |
☔ The latest upstream changes (presumably #84767) made this pull request unmergeable. Please resolve the merge conflicts. |
Triage: There's merge conflict now. |
🎉 Experiment
|
Unfortunately, I think the only option here is to make this a warning (and a hard error in the next edition). Modifying the projection lifetime inference rules wouldn't really help - it would mean that all of the bad I hope I'm overlooking a much simpler fix, but I think the combination of "crate A can do projection lifetime inference on their own trait CrateATrait" and "crate B can implement CrateATrait for their own future-wrapper struct, and pass it to crate A" really paints us into a corner. |
@rustbot label +I-types-nominated This is totally out of cache for me, but we should discuss the problem and/or possible solutions. |
we think soundness fixes that cause huge breakage like this tend to become forbid-by-default lints, rather than leaving older editions unsound; no? |
Going to un-nominate this, but mark this as waiting-on-team. The types team is going to schedule a deep dive to dig into this. |
This PR doesn't fix: #![feature(closure_lifetime_binder)]
fn main() {
takes_blah(for<'a> || -> &'a () { &() });
}
fn takes_blah<T: 'static>(_: T) {} // no error :( as the lifetime I suspect that |
…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.
…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.
…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.
// we must enforce that requiring `closure: 'a` also requires that. | ||
// the return type of the closure outlive `a`. | ||
// We do not require that any of the closure's *argument* types outlive 'a | ||
// - this is sound, because there is no rule that would allow us to conclide |
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 is sound, because there is no rule that would allow us to conclide | |
// - this is sound, because there is no rule that would allow us to conclude |
// This is inconsistent with function pointers, which require that all of their | ||
// argument types (as well as the return type) outlive `'a` in order for | ||
// `fn(A, B) -> R : ' a` to hold. It would be a breaking change to enforce this for | ||
// closuers, and is not required for soundness. |
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.
// closuers, and is not required for soundness. | |
// closures, and is not required for soundness. |
What's the current status of this PR? It sounds like there is general consensus we should try to turn this into a lint. Is that correct? |
There is not a general census for that afaik, t-types has had a few meetings on this but nothing concrete for what exact steps to take. People do seem to generally agree though that as written this PR is overly conservative and errors on code that should be allowed but it's somewhat unclear what changes to actually make in order to allow all the code it should be allowing |
Discussed in T-compiler triage meeting. We are not sure at this point whether it makes sense for this PR to remain open and bitrotting, given that T-types is still wrestling with the details and some think that a re-implementation will be the best path rather than trying to rebase and then fix this... |
hello checking for progress. Any objection to close this PR and move on? |
Ping from triage: @rustbot label: +S-inactive |
Fixes #84366
RFC 1214 allows us to infer that
<T as Trait<P0, ... PN>>::Out: 'a
holds, so long as
T: 'a, P0: 'a, ..., PN: 'a
all hold.In order for this to be sound, we must ensure that whenever we need to
prove
T: 'a
for some concrete typeT
, we also ensure thatany type which could appear in the output type for an impl also outlives
'a
However, we failed to enforce this for closures. Specifically, we
allowed
closure: 'a
to hold without requiring thatclosure_ret_ty: 'a
also holds. Sinceclosure_ret_ty
appears in the output of theFnOnce
impl for closures, it was possible to extend a lifetime bycausing rustc to assume that the closure return type outlived a lifetime
based on the fact that the closure itself outlived a lifetime.
This commit includes the closure return type as one of the 'components'
used when computing outlives requirements. To minimize the extent of
this breaking change, we do not include the arguments of the closure
as 'components'. Doing so is still sound, but introduces an
inconsistency with function pointers (which do treat their arguments as
'components')
Several uses of closures in libcore needed to be adjusted, all of them
involving returning a closure via an
impl Fn
opaque type. In allcases, we needed to add extra lifetime bounds to generic parameters that
ended up in the return type
R
ofimpl Fn() -> R
.