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

Closures which return a reference that depends on an argument unconditionally fail borrow checking #111662

Closed
saethlin opened this issue May 16, 2023 · 6 comments
Labels
A-closures Area: closures (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related A-NLL Area: Non Lexical Lifetimes (NLL) C-bug Category: This is a bug. NLL-diagnostics Working torwads the "diagnostic parity" goal

Comments

@saethlin
Copy link
Member

saethlin commented May 16, 2023

This code does not compile:

fn main() {
    let _c = |b: &u8| b;
}

It did in the past though. Starting in 1.36, it comes with this warning (https://godbolt.org/z/W7KGM3369):

warning: lifetime may not live long enough
 --> <source>:2:23
  |
2 |     let _c = |b: &u8| b;
  |                  -  - ^ returning this value requires that `'1` must outlive `'2`
  |                  |  |
  |                  |  return type of closure is &'2 u8
  |                  let's call the lifetime of this reference `'1`
  |
  = warning: this error has been downgraded to a warning for backwards compatibility with previous releases
  = warning: this represents potential undefined behavior in your code and this warning will become a hard error in the future

(I do not think there is any chance that this code has UB)

But this code does compile:

fn oof<'a>() {
    let _c = |b: &'a u8| b;
}

At the very least, I think that the diagnostic here is bad. I can't figure out what it is trying to tell me.

This was all re-minimized from an already-minimzed example from @jyn514 :

fn sort(blobs: &mut Vec<(String, Vec<u8>)>) {
    blobs.sort_unstable_by_key(|blob| &blob.0);
}

This is the code we'd really like to see compile.


I'm going to just uh apply all the labels I think are applicable please fix them if they're wrong.

@saethlin saethlin added A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related A-closures Area: closures (`|args| { .. }`) A-NLL Area: Non Lexical Lifetimes (NLL) C-bug Category: This is a bug. NLL-diagnostics Working torwads the "diagnostic parity" goal labels May 16, 2023
@Jules-Bertholet
Copy link
Contributor

This code does not compile:

fn main() {
    let _c = |b: &u8| b;
}

This is an issue with the algorithm for inferring whether closure lifetimes are higher-ranked, I think. The below code compiles fine:

#![feature(closure_lifetime_binder)]
fn main() {
    let _c = for<'a> |b: &'a u8| -> &'a u8 { b };
}

This was all re-minimized from an already-minimzed example from @jyn514 :

fn sort(blobs: &mut Vec<(String, Vec<u8>)>) {
    blobs.sort_unstable_by_key(|blob| &blob.0);
}

This is the code we'd really like to see compile.

This is just #34162.

@GregoryConrad
Copy link

GregoryConrad commented Oct 18, 2023

This issue also appears when using a struct with a lifetime:

#![feature(closure_lifetime_binder)]
fn main() {
    let _c = |b: S<'_>| -> & () { b.thing() };
}

struct S<'a>(&'a ());
impl<'a> S<'a> {
    fn thing(self) -> &'a () {
        self.0
    }
}

And the proposed workaround with closure lifetimes also works.
Unfortunately, in my case, the closure also has to return an impl Fn which is disallowed so the workaround doesn't apply.

The following workaround (normally) works, even in stable Rust:

#![feature(closure_lifetime_binder)]
fn main() {
    // ERROR: 
    // let _c = |b: S<'_>| -> & () { b.thing() };
    // WORKAROUND:
    let _c = for<'a> |b: S<'a>| -> &'a () { b.thing() };
    
    // However, workaround above isn't always possible
    // (1. Requires nightly, 2. closures can't return impl Fn/Traits)
    // So instead let's try another workaround by helping out the compiler:
    let _c = fix_lifetime(|b: S| b.thing());
    let _c = fix_lifetime(for<'a> |b: S<'a>| -> &'a () { b.thing() }); // for demonstration, but this is overkill
}

// Helps the compiler out a little
fn fix_lifetime<F, T>(f: F) -> F
where
    F: for<'a> FnOnce(S<'a>) -> &'a T,
{
    f
}

struct S<'a>(&'a ());
impl<'a> S<'a> {
    fn thing(self) -> &'a () {
        self.0
    }
}

Unfortunately, in my situation, I must be doing something a little too advanced for the compiler to be able to handle and I'm getting a "error: higher-ranked lifetime error" with no further information. Filing a separate issue for that, because it's not as easy to reproduce.

@fmease
Copy link
Member

fmease commented Mar 1, 2024

Closing as duplicate of #58052 which will get fixed once closure_lifetime_binder gets stabilized.
If you disagree with my assessment I can of course reopen this issue.

@fmease fmease closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2024
@GregoryConrad
Copy link

GregoryConrad commented Mar 1, 2024

@fmease I wouldn’t mind the closure_lifetime_binder approach (even though it really smells like a workaround rather than a true fix), other than the fact that it can’t work for all closures. If a closure returns an impl Trait, you can’t explicitly declare the return type, and thus you can’t use the workaround.

@fmease
Copy link
Member

fmease commented Mar 1, 2024

even though it really smells like a workaround rather than a true fix

Yeah, it sure feels like that, I agree. Note that as far as I'm aware, type inference algorithms generally work pretty poorly when higher-ranked type-level entities (types, lifetimes, …) are involved because it's hard to design a good algorithm, there are always trade-offs to be made (let me just link to a random paper why not).

Of course it would be wonderful if we were to default to inferring higher-ranked lifetimes in the types of closure arguments but that might be backward incompatible or have other unforeseen consequences (maybe the closure_lifetime_binder RFC talks about this a bit, I haven't read it in ages).

Not being able to write closures with RPIT is also a bummer but I'm pretty sure that this is being discussed as part of the ITE initiative (“impl-Trait everywhere”) and that this restriction might get lifted in the future. Note that you can already use the feature type_alias_impl_trait to work around that (yes, another workaround :P): E.g,

#![feature(type_alias_impl_trait, closure_lifetime_binder)]

fn main() {
    type F<'a> = impl FnOnce(&i32) -> (&'a i32, &i32);
    let f = for<'a> |x: &'a i32| -> F<'a> { move |y: &i32| (x, y) };
}

And if you're already using nightly / CLBs then you could very well also use TAITs (/half serious).

Finally, I'd like to clarify that I'm extremely grateful for the work the types team & the lang team is doing and while progress might appear to be slow, there's a lot of development and many discussions happening almost every single day to drive all those features forward.

@GregoryConrad
Copy link

@fmease thank you for the thoughtful response! Forgot about TAIT for a minute there. That workaround seems fine to me for now, once it stabilizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related A-NLL Area: Non Lexical Lifetimes (NLL) C-bug Category: This is a bug. NLL-diagnostics Working torwads the "diagnostic parity" goal
Projects
None yet
Development

No branches or pull requests

4 participants