`manual_let_else` should reuse the identifier from the original let statement

[Serial-ATA]

Summary

The lint will emit an invalid suggestion when the binding name in the pattern differs from the name used in the let statement.

Reproducer

I tried this code:

#![warn(clippy::manual_let_else)]

fn main() {
    let foo = Some(1);

    let value = match foo {
        Some(v) => v,
        _ => return,
    };
    
    println!("We have a value of: {value}");
}

I expected to see this happen:

#![warn(clippy::manual_let_else)]

fn main() {
    let foo = Some(1);

    let Some(value) = foo else { return };
    
    println!("We have a value of: {value}");
}

This is the suggestion it should emit, reusing the name value.

Instead, this happened:

#![warn(clippy::manual_let_else)]

fn main() {
    let foo = Some(1);

	// It reuses the name `v`, which is not expected later on
    let Some(v) = foo else { return };
    
    println!("We have a value of: {value}");
}

This suggestion causes an error:

error[[E0425]](https://doc.rust-lang.org/nightly/error-index.html#E0425): cannot find value `value` in this scope
 --> src/main.rs:8:36
  |
8 |     println!("We have a value of: {value}");
  |                                    ^^^^^ not found in this scope

Version

rustc 1.67.0-nightly (70f8737b2 2022-11-23)
binary: rustc
commit-hash: 70f8737b2f5d3bf7d6b784fad00b663b7ff9feda
commit-date: 2022-11-23
host: x86_64-unknown-linux-gnu
release: 1.67.0-nightly
LLVM version: 15.0.4

Additional Labels

@rustbot label +I-suggestion-causes-error

[andersk]

Just to note a corner case where both identifiers will need to be present:

enum Foo {
    Yes { v: i32 },
    No,
}
let foo = Foo::Yes { v: 1 };
let value = match foo {
    Foo::Yes { v } => v,
    _ => return,
};
println!("We have a value of: {value}");

The current incorrect suggestion

let Foo::Yes { v } = foo else { return };

will need to become

let Foo::Yes { v: value } = foo else { return };

[est31]

There is also the shadowing corner case, e.g.:

enum Foo {
    Yes { foo: u32, bar: u32 },
    No,
}
let _foo = 42;
let bar = if let Foo::Yes { foo: _foo, bar } = make_foo() {
    bar
} else {
    return
};
println!("{_foo}");

You can't change that to:

let Foo::Yes { foo: _foo, bar } = make_foo() else { return };

Because that might print a different _foo value :).

The issue exists however only in code that is either receiving one of the unused rustc lints, or code where you use a variable with a leading _, which is a bit weird on its own. It's thus a corner case, but there is the possibility to change behaviour!

[smoelius]

A related example, if it helps:

#![warn(clippy::pedantic)]
#![allow(clippy::unnecessary_wraps)]

use std::io::Result;

fn foo() -> Result<Option<(u32, u32)>> {
    Ok(Some((1, 2)))
}

fn main() -> Result<()> {
    let (x, y) = if let Some(pair) = foo()? {
        pair
    } else {
        return Ok(());
    };
    println!("{x}, {y}");
    Ok(())
}

The suggestion removes the bindings for x and y:

     let Some(pair) = foo()? else {
         return Ok(());
     };
     println!("{x}, {y}"); // `x`, `y` not found in this scope

(Aside: I genuinely love this lint! ❤️)

[est31]

Oh yeah there are two more potential footguns to be aware of. First, as you point out correctly @smoelius , the let expression can also have an irrefutable pattern instead of bare identifiers. The code has to combine the two patterns. This is not trivial however, as the outer pattern can create unused bindings that overlap in name with the inner binding. Think e.g.:

let (_x, y) = if let _x @ Some(pair) = foo()? {
    pair
} else {
    return Ok(());
};

A let _x @ Some((_x, y)) = ... else { ... }; will not compile, it will complain about re-use of the binding. Admittedly this case might be rare but it's still a potential problem.

And there is the further issue of the lint's tuple support. Often you can find people write code like:

let (a, b) = if let Foo { a_res: Ok(a), b_opt: Some(b) } = foo() {
    (a, b)
} else {
    panic!()
};

People would employ this pattern in cases where they needed to build two bindings a and b. Personally I think these cases are those which benefit the most from being changed to a let...else, as "new data" is being created to emulate the feature.

The lint recognizes the returned tuple as a "simple identity" and thus still fires (if there'd be a function call it wouldn't fire as it doesn't know if the function call has a side effect or not).

The issue is however that the order can be flipped, so instead (b, a) can be returned:

let (a, b) = if let Foo { a_res: Ok(a), b_opt: Some(b) } = foo() {
    (b, a)
} else {
    panic!()
};

Here a and b are flipped by the tuple returning. One can write code that respects this order change, it just needs to be kept in mind.

(Aside: I genuinely love this lint! ❤️)

❤️ ❤️ 😄