Spurious irrefutable_let_patterns warning with let-chain

[RalfJung]

Code

#![feature(let_chains)]

fn max() -> usize {
    42
}

fn main() {
    if let mx = max() && mx < usize::MAX {
        // ...
    }
}

Current output

warning: leading irrefutable pattern in let chain
 --> src/main.rs:7:8
  |
7 |     if let mx = max() && mx < usize::MAX {
  |        ^^^^^^^^^^^^^^
  |
  = note: this pattern will always match
  = help: consider moving it outside of the construct
  = note: `#[warn(irrefutable_let_patterns)]` on by default

Desired output

(no warning)

Rationale and extra context

This code naturally expresses "please call that function and then do something if the return value satisfies a condition". Putting the let binding outside the if would be bad as then it remains in scope after the if, which is not the intent. I could wrap it in an extra layer of curly braces but that increases the overall indentation level.

I think this warning should never trigger for let chains.

Other cases

Rust Version

current nightly

Anything else?

No response

[kurikomoe]

another example:

#[allow(irrefutable_let_patterns)]
if let errCode = GetLastError() && errCode != ERROR_NO_MORE_ITEMS {
    let _ = dbg!(errCode);
    // logging the errCode to db, etc
}

Honestly, I don't want to introduce the errCode into the outer scope; I just want to keep it inside this if-else branch to avoid the possibility of using the variable elsewhere.

[RalfJung]

Seems like the lint was introduced in #49469 as part of #44495 -- previously, this was a hard error. With #57535, the lint was made warn-by-default.

@rust-lang/lang I'd like to propose to further weaken this lint from warn-by-default to allow-by-default or even entirely remove it.

[traviscross]

Do you want to weaken or remove this lint in general, or do you just want it to not fire on let chains?

[joshtriplett]

I think there's a reasonable case for not applying it to let chains, specifically. In the absence of let chains, if let name = expr() { ... } is unnecessary and always true, and it seems reasonable to lint on that. But with let chains, it makes sense to bind something for use later in the chain.

I seem to recall at some point that we talked about making it not fire for any subsequent let in the chain, but keeping it for the first one since that one can be moved out. (But, of course, that's not true for while let.)

[RalfJung]

I think there's a reasonable case for not applying it to let chains, specifically.

That works for me.

[RalfJung]

I seem to recall at some point that we talked about making it not fire for any subsequent let in the chain, but keeping it for the first one since that one can be moved out.

Both of the examples posted above argue against that: moving it out extends the scope of the binding, which might not be desired.

[traviscross]

If someone wants to put up a PR to make it not fire on let chains and nominate that, I'll propose FCP merge on it. I'd estimate that being likely to save us a step here.

[joshtriplett]

@RalfJung I'm aware, but that isn't necessarily a reason to allow it, just an argument that someone might want to write it that way.

The downside of allowing it for let chains in general is that it may not be intentional, and sometimes this catches errors where you meant to write a refutable pattern.

But nonetheless, I'd be 👍 for allowing it for let chains, for now, with the possibility of coming up with something more specific in the future to catch potential mistakes.

[RalfJung]

I'm aware, but that isn't necessarily a reason to allow it, just an argument that someone might want to write it that way.

Fair. It just seemed odd to make a suggestion that would change nothing for 100% of the examples considered in this issue.

[traviscross]

@rustbot labels -I-lang-nominated +I-lang-radar

We talked about this in the @rust-lang/lang call today. We decided we just wanted to rip out the irrefutable_let_patterns lint entirely. Given the existing dead code and unused variable lints, we just weren't seeing a lot of incremental value in this one, and we could think of many reasons for people to write code that would hit this. Whoever does this should ensure that dead_code catches e.g. let () = () else { /* dead code */ }; and if let () = () { .. } else { /* dead code */ } and whatnot, and then nominate the PR for us.

And CC @rust-lang/clippy in the event that clippy might want to add something here.