single_match should not lint exhaustive match

[camsteffen]

Summary

single_match should not lint an exhaustive match

Lint Name

single_match

Reproducer

I tried this code:

enum Foo { Bar }

match Some(Foo::Bar) {
    Some(Foo::Bar) => {dbg!()}
    None => {}
}

I saw this happen:

warning: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
 --> src/main.rs:5:5
  |
5 | /     match Some(Foo::Bar) {
6 | |         Some(Foo::Bar) => {dbg!()}
7 | |         None => {}
8 | |     }
  | |_____^ help: try this: `if let Some(Foo::Bar) = Some(Foo::Bar) {dbg!()}`

I expected to see this happen: nothing

Version

No response

Additional Labels

No response

[camelid]

Context: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Lint.20group.20for.20.60return_self_not_must_use.60/near/268089511

[jubnzv]

The problem is in this line, for some reasons implements_trait(cx, ty, pe_trait_id, &[ty.into()]) doesn't work as expected for Option<Foo> type. I'm looking at this.

[camsteffen]

I think the important thing here is that there is no wild pattern (_) anywhere in an exhaustive match. You can use Pat::walk_short to search the pattern of each match arm for one with PatKind::Wild.

[camelid]

There's also match arms like (_, _) -- basically, when matching on a single-constructor (excluding constructors that are uninhabited) type, applying that constructor to wildcards behaves like a plain wildcard. Not sure how you want to handle those.

[camsteffen]

The presence of _ in (_, _) means it's a non-exhaustive match, right?

Also, a wild in a branch with an if guard doesn't count.

[camelid]

The presence of _ in (_, _) means it's a non-exhaustive match, right?

I'm not sure what you mean. "non-exhaustive" means the match doesn't cover all cases. _ in fact makes the match certainly exhaustive (because it catches all cases).

Also, a wild in a branch with an if guard doesn't count.

Right.

[camsteffen]

I'm not sure what you mean. "non-exhaustive" means the match doesn't cover all cases. _ in fact makes the match certainly exhaustive (because it catches all cases).

Oh you're right, there's a crack in my logic somewhere... I guess the criteria should be "the 'else' branch contains a wild".

[camelid]

I think the lint should only trigger if one of the match arms is a "catch-all", where "catch-all" is defined as:

• _ (pure wildcard)
• (_, _), Struct(_, _), SingleVariantEnum::Variant(_, _) (wildcards wrapped by a single-contructor type)

[camsteffen]

I see. The definition needs to be recursive though so that StructA(StructB(_), _) is "catch-all". Would it be correct to say "the else branch must be exhaustive on its own"?

else_pat.walk_short(|p| {
    if matches!(p.kind, Path | Lit | Range | Slice) {
        // not exhaustive
    }
})

[camelid]

What do you mean by "else branch"? E.g., there can be multiple wildcard branches, possibly overlapping.

[camsteffen]

The lint detects a match with two branches. One of them is transformed to an if let and the other is dropped. I'm referring to the latter as the "else branch".

[camelid]

Ah, then I think your definition from above is correct:

Would it be correct to say "the else branch must be exhaustive on its own"?

[jubnzv]

Well, as far as I understood from the original zulip message, the problem is that the user wants to lint only exhaustive matches. They want to match explicitly defined cases by them own, to avoid possible regressions, when changing the data structure to be matched.

So, what we actually want here is to avoid running this check if there is no wildcard matches.

Consider the following examples (they may be included as tests):

    // We can't actually write non-exhaustive matching for ranges, because compiler will force us
    // to write exhaustive match anyway. The two examples below will be linted.
    match (1u8, 1u8) {
        (1, 2) => (),
        (_, _) => (),
    }
    match (1u8, 1u8) {
        (1, 2) => (),
        _ => (),
    }

    enum E {
        V1(u32),
    }
    // Don't lint. The point is that `E` enum could be extended later. So, if the user will apply
    // the suggested lint, they may lose information that the added field must be hanlded here too.
    match Some(E::V1(42)) {
        Some(E::V1(_)) => (),
        None => (),
    }

    // Do lint, because the user doesn't care about the type inside `Some`.
    match Some(E::V1(42)) {
        Some(_) => (),
        None => (),
    }

    struct S {
        f1: u32,
        f2: i32,
    }
    // I think, we should lint in all these cases, because applying the refactoring suggested by
    // the linter could not cause errors.
    match Some(S { f1: 1, f2: 2 }) {
        Some(S { f1, f2 }) => (),
        None => (),
    }
    match Some(S { f1: 1, f2: 2 }) {
        Some(S { .. }) => (),
        None => (),
    }
    match Some(S { f1: 1, f2: 2 }) {
        Some(S { f1, .. }) => (),
        None => (),
    }
    match Some(S { f1: 1, f2: 2 }) {
        Some(_) => (),
        None => (),
    }

What do you think?

[jubnzv]

@rustbot claim

[camsteffen]

This keeps getting more complicated... Consider these examples:

// don't lint 
match x {
    (Some(E::V), _) => todo!(),
    (None, _) => {}
}

// lint
match x {
    (Some(_), _) => todo!(),
    (None, _) => {}
}

// lint
match x {
    (Some(E::V), _) => todo!(),
    (_, _) => {}
}

I think we have to walk the two patterns at the same time in order to detect a pairing like Some(E::V) and None.