let_else returns in surprising borrowck errors with impl trait

[lcnr]

#![feature(let_else)]
use std::fmt::Debug;

fn foo<'a>(x: &'a str) -> Result<impl Debug + 'a, ()> {
    Ok(x)
}

fn fail() {
    let x = String::from("Hey");

    let Ok(s) = foo(&x) else { return };
}

fn ok() {
    let x = String::from("Hey");

    let s = if let Ok(s) = foo(&x) { s } else { return };
}

results in

error[E0597]: `x` does not live long enough
  --> src/main.rs:11:21
   |
11 |     let Ok(s) = foo(&x) else { return };
   |                 ----^^-
   |                 |   |
   |                 |   borrowed value does not live long enough
   |                 a temporary with access to the borrow is created here ...
12 | }
   | -
   | |
   | `x` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `Result<impl Debug, ()>`
   |
help: consider adding semicolon after the expression so its temporaries are dropped sooner, before the local variables declared by the block are dropped
   |
11 |     let Ok(s) = foo(&x) else { return };;
   |                                        +

I am not sure if this issue exists only in combination with impl Trait of if it can be triggered in some other ways as well, haven't managed to get this bug some other way.

cc #93628 @est31 do you know whether this behavior is known already?

[est31]

The bug is not known to me. cc @cormacrelf who has improved the borrow checking behavior. The suggestion is also weird.

[cormacrelf]

The let-else actually desugars to this if-let:

fn main() {
    if let Ok(s) = foo(&x) {} else { return }
}

And you get the same error. However the suggestion actually makes sense for if-let, because putting a semicolon afterwards does indeed fix it. The suggestion is wrong for the let-else because let-else's desugaring is always in tail position in its enclosing block, any statements thereafter get moved into the if-branch and so no amount of semicolons can take the desugaring out of tail position.

[cormacrelf]

Also note that the old desugaring, which was let temp = foo(&x); if let Ok(s) = temp { ... } else { return }, does not suffer this particular problem because the temporary has a name and lives outside the tail position expression. That way there is no ordering problem with the ends of x's and the unnamed temporary's lifetimes.

So a few things:

1. Using a named temporary like that is a good suggestion to make instead for let-else, and potentially for if-let as well.
2. The presence or absence of a semicolon on a tail position expression of type () having that much significance is pretty bad. Arguably it should not be significant whatsoever.
3. borrowck is being a bit obtuse. It isn't explained very well even for the if-let version why the unnamed temporary is still alive when x is being dropped. Conceptually I think borrowck is trying to say that the real implicit return expression is actually that empty space inside the braces of type (). I don't know.
4. Even if it did explain better, is there a technical reason why borrowck can't figure out that it could simply drop the unnamed temporary first? Because that would be ideal.

[camsteffen]

Could we just wrap the if in the let-else desugaring with a DropTemps?

[cormacrelf]

Well isn't that convenient! Yeah, I suppose we could.

[cormacrelf]

I've just tried this, and it also suffers the same thing #94012 did, with the unreachable code warnings when the desugaring ends up with a return in both branches. It is probably complaining about the DropTemps expression itself being unreachable. We can't slap an #[allow(unreachable_code)] on it lest it ignore all unreachable code inside, but I think we can disable the emission of such warnings on any DropTemps in typeck's FnCtxt::warn_if_unreachable. Right? (Edit, maybe instead in rustc_passes::Liveness::warn_about_unreachable, because this DropTemps will rarely if ever be ! such that typeck could warn about it.)

Example warning:

warning: unreachable expression
  --> /Users/cormac/src/rust/src/test/ui/let-else/let-else-run-pass.rs:33:5
   |
LL |     let Some(1) = Some(2) else { return };
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^------^^^
   |     |                            |
   |     |                            any code following this expression is unreachable
   |     unreachable expression

[cormacrelf]

Ah cool, turned out to be pretty easy.

[est31]

This compiles successfully as of rustc 1.64.0-nightly (87588a2af 2022-07-13), which includes #98574. So I'd say it's fixed, but it needs a test before the issue can be closed. #94012 includes a test for this, but it's not merged and probably not going to be.

The other test in that PR also looks interesting.