Let-else source expression move behavior

[phaylon]

The move behavior on non-matching of expressions by feature(let_else) seems currently inconsistent with behavior of, for example, if let.

An if-let construct does not move from the matched expression if the pattern doesn't match:

fn example_if_let(value: Option<String>) {
    if let Some(inner) = value {
        println!("inner: {}", inner);
    } else {
        println!("other: {:?}", value);
    }
}

fn main() {
    example_if_let(Some("foo".into()));
    example_if_let(None);
}

compiles and prints

inner: foo
other: None

However, let-else seems to unconditionally move from the matched expression:

#![feature(let_else)]

fn example_let_else(value: Option<String>) {
    let Some(inner) = value else {
        println!("other: {:?}", value);
        return;
    };
    println!("inner: {}", inner);
}

fn main() {
    example_let_else(Some("foo".into()));
    example_let_else(None);
}

fails to compile:

error[E0382]: borrow of moved value: `value`
 --> src/main.rs:5:33
  |
3 | fn example_let_else(value: Option<String>) {
  |                     ----- move occurs because `value` has type `Option<String>`, which does not implement the `Copy` trait
4 |     let Some(inner) = value else {
  |                       ----- value moved here
5 |         println!("other: {:?}", value);
  |                                 ^^^^^ value borrowed here after move

I'm wondering if this is intentional, as it would seem to make it impossible to use the original unmatched value in an panic message (or diverge otherwise with the value or any value based on it).

rustc 1.57.0-nightly (485ced56b 2021-10-07)
binary: rustc
commit-hash: 485ced56b8753ec86936903f2a8c95e9be8996a1
commit-date: 2021-10-07
host: x86_64-unknown-linux-gnu
release: 1.57.0-nightly
LLVM version: 13.0.0

Proposed Label: F-let-else

[nagisa]

This definitely seems wrong. The value should be available for use within the else block.

As per reference level explanation in the RFC the construct should desugar into a match, pretty much. And the desugared version does allow for using the matchee in the "otherwise" branch.

[joshtriplett]

That's my understanding as well: the RHS of the let should be available in the else expression.

[phaylon]

Yes, that's what I'm thinking as well. Though the RFC doesn't seem to have examples showing it. My reasoning was that since let P = E else { X... }; R... should be transformable into if let P = E { R... } else { X... } there should be no issue with leaving the E intact in the else branch.

I took a look at the code, but I'm not familiar enough with the internals to see where the "place identity" of the expression gets lost.

[Fishrock123]

I thought I covered this in the RFC, but this is definitely a bug in my (the RFC author’s) view.

[Fishrock123]

A practical use case of this is logging from the the matchee in an error case.

[cormacrelf]

The desugaring is not made to a plain if-let-then-else, it is into let dummy: $ty = val; if let $pat = dummy .... The move occurs in the first statement. The reason for this is to give effect to the optional type annotation (let $pat: $ty = ... else { ... }).

rust/compiler/rustc_ast_lowering/src/block.rs

Line 141 in 1067e2c

// first statement which basically exists for the type annotation

Using the dummy causes a move regardless of whether you add the annotation or not. That is the key difference between the let-else and the manual if-let desugarings posted in this thread. If you desugar this according to the current implementation:

let Some(inner) = value else {
    println!("other: {:?}", value);
    return
};
println!("inner: {:?}", inner);

You get, in fact:

let dummy = value;
if let Some(inner) = dummy {
    println!("inner: {:?}", inner);
} else {
    // value has moved into _dummy, above
    println!("other: {:?}", value);
    return
}

... which does not compile. Same goes for when you write let Some(inner): Option<String> = value else {...}.

I don't really see how you can desugar the type annotation in such a way that the original scrutinee does not get moved. But also:

• if let does not support type annotations
• match does not support type annotations
• the RFC for let-else does not motivate the : TYPE syntax and why let-else should have it but not the others. In fact the type annotation is not mentioned except in the summary, and even then, only as the single word : TYPE in the syntax bit.
• (Edit: best I can do as a justification would be that regular let bindings support it, but that isn't super convincing if if let does not.)
• You can add a dummy yourself if you really need it, just like you can (and must) for if let and match.

So I think you could just remove that syntax, desugar without the dummy binding, and it would all work as expected.

[joshtriplett]

let-else is supposed to be a straightforward extension to let; it should support type annotation just as let does.

If we can't support it with a trivial desugaring, then we should support it "natively" as a construct.

[nagisa]

So I kinda though that using ExprKind::Type (i.e. type ascription) and a match would help here, but the implementation PR does note that type ascription disables coercion (needs test, this isn't tested currently!), which makes it an invalid approach. I can see ways to adjust the current lowering slightly by adjusting HirIds to make it work, but I also don't think the hacks are worth it here and that adding a built-in HIR type makes more sense.

Unassigning self for now since I don't see myself working on this for a while, but I may come back to it.

[cormacrelf]

I have been searching in vain for a use case for type annotations on a refutable let binding, and I found another bug that whatever replacement implementation will have to address too. This will never make much sense as the Vec<_> would have to describe a sub-expression somehow, but it gives you a dodgy error message, asking you to put a [..] after the final };:

let [single]: Vec<_> = iter.collect() else {
    return;
};

The general case is this, with the same error

let [single] = Vec::new() else {
    return;
};

error[E0529]: expected an array or slice, found `Vec<_>`
 --> src/main.rs:5:9
  |
5 |     let [single] = Vec::new() else {
  |         ^^^^^^^^ pattern cannot match with input type `Vec<_>`
  |
help: consider slicing here
  |
5 ~     let [single] = Vec::new() else {
6 +         return;
7 +     };[..]
  |

(Incidentally there should be a FromIterator for this that doesn't allocate! Like so.)

[joshtriplett]

The most obvious use case I can think of is try_into.

[cormacrelf]

Yeah, but I think it might be the only one, and it is not great. The let-else was meant to help you get the type you're decomposing out of the way, but you have to specify that it's fallible operation no less than three times (Ok, Result, try_into) if you want to write the target type explicitly.

// explicit handling
let Ok(x): Result<u32, _> = y.try_into() else {
    return u32::MAX;
};
// remember we want people to write all that instead of this:
let x = y as u32;
// and the alternative when you have Result return type
let x: u32 = y.try_into()?;

I'm with you, but it brings me no joy.

[cormacrelf]

I'm having a crack at it by adding an Option<&'hir Ty<'hir>>to hir::ExprKind::Let, which is the HIR's boolean expression in an if let construct. This would, if the parser allowed it, permit if let to have type annotations too.

I'm new at this, is it weird that typeck ExprUseVisitor walks hir::ExprKind::Let via self.walk_local which treats the pattern as irrefutable?

rust/compiler/rustc_typeck/src/expr_use_visitor.rs

Lines 461 to 470 in 9475e60

	fn walk_local<F>(&mut self, expr: &hir::Expr<'_>, pat: &hir::Pat<'_>, mut f: F)
	where
	F: FnMut(&mut Self),
	{
	self.walk_expr(expr);
	let expr_place = return_if_err!(self.mc.cat_expr(expr));
	f(self);
	self.walk_irrefutable_pat(&expr_place, &pat);
	}

rust/compiler/rustc_typeck/src/expr_use_visitor.rs

Lines 629 to 631 in 9475e60

	/// Walks a pat that occurs in isolation (i.e., top-level of fn argument or
	/// let binding, and *not* a match arm or nested pat.)
	fn walk_irrefutable_pat(&mut self, discr_place: &PlaceWithHirId<'tcx>, pat: &hir::Pat<'_>) {

This just seems suspicious, I am not certain it would fix anything, but it feels wrong.

(Edit: I think it's just the name walk_irrefutable_pat that's incorrect. It doesn't do anything differently from walk_arm.)

[cormacrelf]

Yeah I've fixed this, will PR when I finish the knock-on hir changes in clippy etc