lint matchable if let for pattern

[dswij]

What it does

Checks for if let for patterns that can be re-written with matches!

This can be an improvement to equatable_if_let lint.

For background see #9102 (comment)

Lint Name

No response

Category

No response

Advantage

Similar to equatable_if_let:

1. It reads better and has less cognitive load because equality won’t cause binding.
2. It avoids a Yoda condition.
3. Equality is a simple bool expression and can be merged with && and || and reuse if blocks

Drawbacks

No response

Example

#![allow(dead_code)]
enum Foo {
    Bar,
    Baz,
}

fn test(foo: Foo) {
    if let Foo::Baz = foo {
        println!("Foo::Baz");
    }
}

can be written as

#![allow(dead_code)]
enum Foo {
    Bar,
    Baz,
}

fn test(foo: Foo) {
    if matches!(foo, Foo::Baz) {
        println!("Foo::Baz");
    }
}

[dswij]

We can use equatable_if_let as a starting point, this should be a good first issue

[dswij]

Also, the logic would also be similar to match_like_matches_macro.

[nahuakang]

@rustbot claim

[nahuakang]

@dswij 👋 May I ask if you meant that we should add the new linting within equatable_if_let, i.e. expanding the logic of the existing equatable_if_let lint to include this new pattern? Thanks :)

[dswij]

I haven't taken a deep look into it, but if we plan to add this to a new lint, we can factor out reusable logic from equatable_if_let. Alternatively, we can just add this case to be linted by equatable_if_let and update the lint's docs.

[nahuakang]

Thanks. I'll take a deeper look at the 2 lints that you've written down and circle back when I have a clearer picture :)

[nahuakang]

@dswij It seems that equatable_if_let already takes care of the case you mentioned (I wonder if dogfood would have caught it). In this case, do we prefer: if foo == Foo::Baz or if matches!(foo, Foo::Baz)?

[dswij]

@nahuakang

it seems like it won't fire if PartialEq is not implemented. See (playground). The test cases for this lint seems to agree.

Makes sense since if PartialEq is not implemented, then you can't suggest if foo == Foo::Baz

So I think this is more of an improvement - if it doesn't implement PartialEq, then we can suggest using matches instead.

[nahuakang]

@dswij Thanks for the tip. I was trying to understand this lint but had some trouble yesterday. Does your suggestion mean that if is_structural_partial_eq returns false, we could consider suggesting matches?

[dswij]

Yes, exactly. Suggest matches if it doesn't implement PartialEq.

I'm trying to think of scenarios where suggesting matches would be wrong, but I'm having trouble coming up with one. Just something to keep in mind.

[nahuakang]

Will do! I'll start adding some new test cases and do the improvement. Maybe by the time of review we'd think of more edge cases.

[nahuakang]

@dswij Feel free to take a look at the PR :) I'm still not familiar with things on this level so might miss some edge cases.

[ivan-aksamentov]

@nahuakang Excuse me if this is a wrong place to ask, but after this change to the equatable_if_let, it suggests turning conditions like

if let (None, None) = (&self.x, &self.y) {

into

if matches!((&self.x, &self.y), (None, None)) {

which I would consider worse.

Otherwise equatable_if_let is a fantastic, helpful lint, but I think this matches!() addition is bolted into a wrong place. Is it not too late to extract the matches!() part of it into a separate lint (which I would disable) and to roll back the previous version of the equatable_if_let, which only suggests things like ==?

How do I properly request a feature or open a discussion for this?