Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

Aaron1011
Copy link
Member

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 type T, we also ensure that
any 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 that closure_ret_ty: 'a also holds. Since closure_ret_ty appears in the output of the
FnOnce impl for closures, it was possible to extend a lifetime by
causing 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 all
cases, we needed to add extra lifetime bounds to generic parameters that
ended up in the return type R of impl Fn() -> R.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2021
@@ -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>(
Copy link
Member Author

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.

Copy link
Member

@steffahn steffahn Apr 21, 2021

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

@BoxyUwU BoxyUwU Nov 8, 2022

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 the Self type of the return closure because it would have unconstrained lifetimes therefore impl FnMut((), T) -> ControlFlow<B> + 'a requires B: 'a
    • the argument f has the same inputs so if our returned closure had to have B in the self type, so would f
      • f outlives 'b and B is inside f and 'b outlives 'a therefore B: 'a
  • B does not have to be part of the Self 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

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.

@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

It seems like we want a crater run.

@nikomatsakis nikomatsakis added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 21, 2021
@nikomatsakis
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@nikomatsakis nikomatsakis left a 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!

compiler/rustc_middle/src/ty/outlives.rs Outdated Show resolved Hide resolved
@Aaron1011 Aaron1011 force-pushed the closure-outlives-ret branch 2 times, most recently from 96a18d5 to 6aba64c Compare May 4, 2021 23:42
@Aaron1011
Copy link
Member Author

@nikomatsakis: I've accounted for the generator yield type, and added tests involving generator return and yield types.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a 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.
Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

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.

@rust-log-analyzer

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

@Aaron1011 I think this does not fully close the original issue because it does not address FnDef. We should talk that over. It might be worth approaching that case in a second PR. I also have to take another look at the fallout here.

@bors
Copy link
Contributor

bors commented May 18, 2021

☔ The latest upstream changes (presumably #84767) made this pull request unmergeable. Please resolve the merge conflicts.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021
@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 26, 2021
@crlf0710
Copy link
Member

Triage: There's merge conflict now.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@craterbot
Copy link
Collaborator

🎉 Experiment pr-84385 is completed!
📊 13128 regressed and 8 fixed (245056 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 10, 2022
@Aaron1011
Copy link
Member Author

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 Generators couldn't have the normal impl<G: Generator> Future for G impl apply, since it lets you do the same bad projection.

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.

@Aaron1011 Aaron1011 added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Oct 10, 2022
@nikomatsakis
Copy link
Contributor

@rustbot label +I-types-nominated

This is totally out of cache for me, but we should discuss the problem and/or possible solutions.

@rustbot rustbot added the I-types-nominated Nominated for discussion during a types team meeting. label Oct 11, 2022
@apiraino
Copy link
Contributor

apiraino commented Oct 13, 2022

visited in T-compiler meeting (notes): I'll untag t-compiler and let t-types have a say (which looks more appropriate here)

@rustbot label -I-compiler-nominated t-types

@rustbot rustbot added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Oct 13, 2022
@SoniEx2
Copy link
Contributor

SoniEx2 commented Oct 13, 2022

we think soundness fixes that cause huge breakage like this tend to become forbid-by-default lints, rather than leaving older editions unsound; no?

@jackh726
Copy link
Member

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.

@jackh726 jackh726 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. I-types-nominated Nominated for discussion during a types team meeting. labels Oct 21, 2022
@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 8, 2022

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 'a is latebound and compute_components_recursive doesnt add late bound lifetimes to out. (The fact that this example uses feature(closure_lifetime_binder) shouldnt change the fact that this is fundamentally still a problem with closures, although I don't know if its actually possible to get a closure to infer that signature because closure inference is not super smart rn lol)

I suspect that 'a should not actually be late bound here as the equivalent fn makes it early bound (fn foo<'a>() -> &'a () { &() })

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.
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// - 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// closuers, and is not required for soundness.
// closures, and is not required for soundness.

@wesleywiser
Copy link
Member

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?

@BoxyUwU
Copy link
Member

BoxyUwU commented Mar 16, 2023

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

@pnkfelix
Copy link
Member

pnkfelix commented Apr 13, 2023

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...

@apiraino
Copy link
Contributor

hello checking for progress. Any objection to close this PR and move on?

@JohnCSimon
Copy link
Member

JohnCSimon commented May 28, 2023

Ping from triage:
Last commit + comment was October.
I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this May 28, 2023
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) relnotes Marks issues that should be documented in the release notes of the next release. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'static closures/FnDefs/futures with non-'static return type are unsound