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.

[jnkr-ifx]

I find the irrefutable_let_patterns lint beneficial occasionally because I have a bad habit of writing if let x = ... when I'm trying to unwrap an Option. (I picked up the habit from Swift, where if let x = ... is syntactic sugar for if let Some(x) = ...).

#![allow(irrefutable_let_patterns)]

fn foo() -> Option<u32> {
    None
}

fn main() {
    if let x = foo() {
        println!("{:?}", x);
    }
}

This prints None, with no warning.

In most cases this mistake will lead to a type error really quickly because the Option isn't unwrapped in the body of the if statement -- I had to use {:?} instead of {} to write that example -- but I can definitely remember seeing this lint fire and remind me "hey, you're not writing Swift" before, especially when I was newer to Rust.

[traviscross]

We did talk about how some aspects of the lint might be most beneficial to new users, and that's why we pinged the clippy team here, as adding a lint to help such users might better fit in their domain.

[tmandry]

The comparison to Swift is interesting; I'd forgotten about that syntax.

The specific case of if let x = ... being confused with if x == ... was mentioned, and it was brought up that this would usually result in an unused variable warning. Of course the snippet you posted doesn't hit this since x is used, and since debug-printing is agnostic to whether the type of the binding x is an option. One question is whether we think cases like this that aren't caught by unused variables or the type checker are ever "important".

That said, with example from @jnkr-ifx I've switched back to thinking most non-chain uses of if let qualify as either likely bugs or needless divergence.

What pushed me toward removing the lint is that I don't want overly paternalistic lints that limit expressiveness needlessly. It's true that we've already experienced that with some of the "irrefutable" lints (particularly in the where clause department where we've already relaxed it some). In fact I felt some annoyance about this arising in the discussion today.

But I also don't buy the case I've heard so far for how it's nice to support if let x = foo() { ... } outside of a let chain. The potential for confusion with Swift feels much more concrete to me. The place where I sort of buy wanting it is something like this:

if let Some(x) = foo() {
  ...
} else if let Ok(y) = bar() {
  ...
} else if let z = baz() {
  ...
}

And I don't know, maybe I would want to write that for aesthetic reasons, but it isn't so bad to write else { let z = baz(); ... }. Crucially it's also clearer to the reader, who naturally would expect any usage of if to be followed by some condition.

In contrast, I find the example with a let chain brought up by @kurikomoe extremely motivating. Assigning to a variable and checking a condition on its value immediately is a very common pattern, and it's also common to only need that variable to exist inside the if block. Other languages have forms like this for the same reason.

Outside of let chains, my thinking now is this is one of the places where Rust should err more in the direction of Go (simple and uniform) than Haskell (elegant and inscrutable). In terms of design goals, I would argue that "clarity of purpose" and "correctness by construction" are both on the side of keeping the lint but removing it for let chains.

[traviscross]

In that case, I feel like the lint you might want is about a trivially true condition rather than about an irrefutable pattern. These all seem about the same to me:

if let () = () { .. }
if let () = () && true { .. }
if let () = () && 0 == 0 { .. }
if true { .. }
if 0 == 0 { .. }

Similarly, a trailing else if let .. { .. } strikes me about the same as a trailing else if .. { .. } with any other trivially true conditional expression.

[traviscross]

We're already convinced on let chains, but for posterity, here's what the most motivating case for that looks like to me:

if let x = op_one()
&& check_one(x)
&& let y = op_two(x)
&& check_two(y)
&& let z = op_three(y)
&& check_three(z)
{
    ..
}

That is, we don't want to get y at all if the test on x fails, etc. This factoring is saving us from having to nest many conditionals, which is really the whole point of let chains.

[RalfJung]

These all seem about the same to me:

I don't agree for if true. That can be quite useful to write during debugging etc and it's annoying to have the compiler complain about it. Java does that and people resort to if 0 == 0 etc to work around it...

[traviscross]

Yes, that's actually why I feel the same about if let x = foo() { .. }. Because you start with if let x = foo() && x > 8 && x < 16 { .. }, and then for debugging (etc.) you want to truncate off parts of that conditional, so you'll delete characters until you maybe have if let x = foo() { .. }, and then the compiler will complain and want you to unwrap that body. You can't just say if true { .. } here without moving around that binding. And at that point, you could just truncate it all the way to { let x = foo(); .. } if you still want to keep the indent and the block semantics.

So I don't really see a principled or practical case for distinguishing an irrefutable let expression from any other trivially true expression. If we want to nag people to unwrap these blocks (or at least remove the if), it seems to apply to both, and if we don't, that seems to apply to both as well.

[joshtriplett]

Generally speaking, if I'm in the middle of debugging, and have deleted some code or added true && to a conditional or similar, I don't mind getting compiler warnings, because I'm going to ignore the ones I caused while I'm debugging. If anything, they're a useful reminder that debugging is in progress.