Not linting irrefutable_let_patterns on let chains

[Natural-selection1]

Description

this PR makes the lint irrefutable_let_patterns not check for let chains,
only check for single if let, while let, and if let guard.

Motivation

Since let chains were stabilized, the following code has become common:

fn max() -> usize { 42 }

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

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.

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

Another common case is progressively destructuring a struct with enum fields, or an enum with struct variants:

struct NameOfOuterStruct {
    middle: NameOfMiddleEnum,
    other: (),
}
enum NameOfMiddleEnum {
    Inner(NameOfInnerStruct),
    Other(()),
}
struct NameOfInnerStruct {
    id: u32,
}

fn test(outer: NameOfOuterStruct) {
    if let NameOfOuterStruct { middle, .. } = outer
        && let NameOfMiddleEnum::Inner(inner) = middle
        && let NameOfInnerStruct { id } = inner
    {
        /* */
    }
}

Current Output:

warning: leading irrefutable pattern in let chain
  --> src\main.rs:17:8
   |
17 |     if let NameOfOuterStruct { middle, .. } = outer
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this pattern will always match
   = help: consider moving it outside of the construct
   = note: `#[warn(irrefutable_let_patterns)]` on by default


warning: trailing irrefutable pattern in let chain
  --> src\main.rs:19:12
   |
19 |         && let NameOfInnerStruct { id } = inner
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this pattern will always match
   = help: consider moving it into the body

To avoid the warning, the readability would be much worse:

fn test(outer: NameOfOuterStruct) {
    if let NameOfOuterStruct {
        middle: NameOfMiddleEnum::Inner(NameOfInnerStruct { id }),
        ..
    } = outer
    {
        /* */
    }
}

related issue

• Spurious irrefutable_let_patterns warning with let-chain #139369

possible questions

1. Moving the irrefutable pattern at the head of the chain out of it would cause a variable that was intended to be temporary to remain in scope, so we remove it.
   However, should we keep the check for moving the irrefutable pattern at the tail into the body?

2. Should we still lint entire chain is made up of irrefutable let?

---

This is my first time contributing non-documentation code to Rust. If there are any irregularities, please feel free to point them out.
: )

[rustbot]

Some changes occurred in match checking

cc @Nadrieril

The Miri subtree was changed

cc @rust-lang/miri

[Natural-selection1]

It seems that only modifying tests/ui/rfcs/rfc-2497-if-let-chains/irrefutable-lets.rs is needed to cover this change. Do I still need to add an additional new test?

[traviscross]

It seems that only modifying tests/ui/rfcs/rfc-2497-if-let-chains/irrefutable-lets.rs is needed to cover this change. Do I still need to add an additional new test?

All the lints seem to be removed in that test, but you'll want to ensure there are tests that cover the edges of what is still linted here.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Natural-selection1]

but you'll want to ensure there are tests that cover the edges of what is still linted here.

match chain_refutabilities.len() {
    0   => "`assert!(self.let_source != LetSource::None)` ensure this case doesn't exist"
    2.. => "`if let [Some((_, refutability))] = chain_refutabilities[..]` ensure not checked"  // The purpose of this PR: don't check let chains
    1 if chain_refutabilities[0].is_none() 
        => "A boolean expression at the start of `while let` | `if let` | `else if let`, this case cannot pass syntax analysis"
    1 if chain_refutabilities[0].is_some() 
        => "There is exactly one let binding in `while let` | `if let` | `else if let` statement, proceed to check"
}

[Natural-selection1]

Hi, I really hope this PR can be merged before the 1.91 release. Is there any progress now
: )

[fmease]

I really hope this PR can be merged before the 1.91 release.

That's not possible, master is 1.92 already and 1.91 is beta meaning the latter only accepts patches for stable-to-beta and stable-to-stable regressions (also, 1.91 will be promoted to stable next week).

[joshtriplett]

@rfcbot merge

[rust-rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Suggested change

	if let [Some((_, refutability))] = chain_refutabilities[..] {
	if let [Some((_, Irrefutable))] = chain_refutabilities[..] {

Could go either way, I suppose, but it almost seems worth just doing the rest of the check inline here. It's already checking the "only one" part; maybe it should just check the "irrefutable" part also. Then either check_single_let could be renamed to lint_single_let (since the check has been moved out of it) or perhaps be fully inlined.

[Natural-selection1]

I've pushed a new commit addressing this suggestion. Not sure if it meets your expectations

[traviscross]

Yes, looks right. With it factored this way, might as well use lint_single_let in check_let as well.

[traviscross]

@rfcbot reviewed

[cjgillot]

What if there are multiple bindings, all irrefutable?

[Natural-selection1]

Not linting as long as it is chains. In #139369 , people have nearly reached a consensus on this point.

[scottmcm]

Note that the listed motivation

if let mx = max() && mx < usize::MAX { /* */ }

could of course be written as

if let mx @ ..usize::MAX = max() { /* */ }

which would make it refutable again.

---

To avoid the warning, the readability would be much worse:

Well, it could also be

fn test(outer: NameOfOuterStruct) {
    let NameOfOuterStruct { middle, .. } = outer
    if let NameOfMiddleEnum::Inner(inner) = middle
        && let NameOfInnerStruct { id } = inner
    {
        /* */
    }
}

where the readability is fine?

[scottmcm]

We chatted in the meeting; while that rewrite is arguably better for references (since NLL will do the right thing) the case that for Drop things you don't want to move a leading irrefutable pattern outside the if is convincing to me.

That said, for trailing irrefutable patterns, why would you ever not want to move those bindings inside the block instead of having them in the condition?

[traviscross]

That said, for trailing irrefutable patterns, why would you ever not want to move those bindings inside the block instead of having them in the condition?

Agreed they could always go in the consequent block. However, I can foresee reasons related to code regularity where one may prefer not doing so. E.g., if we're setting an x, y, and z,

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

it may be just prettier and more regular to keep the binding of z within the chain. This may be especially true if this chain is one instance of a pattern in our code that sometimes instead does a check on z also:

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

Whether or not we buy, stylistically, that this is the best way to do it rather than moving it into the block, the plausibility of the reasons to keep it in the chain is enough for this to fall below the threshold of where I'd want to push people away from it with a warn-by-default rustc lint.