Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: suggest using the only remaining variant instead of a wildcard #5556

Closed
ebroto opened this issue May 1, 2020 · 6 comments · Fixed by #5582
Closed

New lint: suggest using the only remaining variant instead of a wildcard #5556

ebroto opened this issue May 1, 2020 · 6 comments · Fixed by #5582
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST

Comments

@ebroto
Copy link
Member

ebroto commented May 1, 2020

When matching an enum, if there is only one variant left, suggest using that instead of the wildcard pattern.

For example, for this enum:

enum E {
    First,
    Second,
    Third,
}

When matching it like this:

match e {
    E::First => {}
    E::Second => {}
    _ => {}
}

Suggest the following replacement:

match e {
    E::First => {}
    E::Second => {}
    E::Third => {}
}

The lint should be under the pedantic category.

Note that for #[non_exhaustive] enums the wildcard arm should be kept.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints T-AST Type: Requires working with the AST labels May 1, 2020
@vtmargaryan
Copy link
Contributor

Hi! I think it is covered by the lint "wildcard enum match arm" implemented in matches.rs
It also suggests a fix with an arm consisting of the union of the remaining variants.

declare_clippy_lint! {
    /// **What it does:** Checks for wildcard enum matches using `_`.
    ///
    /// **Why is this bad?** New enum variants added by library updates can be missed.
    ///
    /// **Known problems:** Suggested replacements may be incorrect if guards exhaustively cover some
    /// variants, and also may not use correct path to enum if it's not present in the current scope.
    ///
    /// **Example:**
    /// ```rust
    /// # enum Foo { A(usize), B(usize) }
    /// # let x = Foo::B(1);
    /// match x {
    ///     A => {},
    ///     _ => {},
    /// }
    /// ```
    pub WILDCARD_ENUM_MATCH_ARM,
    restriction,
    "a wildcard enum match arm using `_`"
}

@flip1995
Copy link
Member

flip1995 commented May 7, 2020

Not quite. The wildcard_enum_match_arm will trigger for every use of _ => ..., while this lint should only trigger on the once, where only one variant is left to cover.

But thanks for pointing out, that a lint like this is already implemented. This means, that the only thing that has to be implemented here is if variants_left() == 1 { lint_new_lint() }.

@flip1995 flip1995 added the C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages label May 7, 2020
@vtmargaryan
Copy link
Contributor

I want to try to do that, if you don't mind

@flip1995
Copy link
Member

flip1995 commented May 8, 2020

That would be great, if you need help with anything don't hesitate to ask here or open a WIP PR.

@vtmargaryan
Copy link
Contributor

I've implemented at least a starting version, but I have some questions.

  1. The dogfood_clippy test doesn't pass on 18 matches. Should I fix them or should I turn off the lint? If I should turn it off, then what is the best way to do so? The cargo test output to see the places where the lints are spanned dogfood_clippy.txt
  2. I've found a small typo in one of the lint examples. Is it ok to include the fix in the same PR where the lint is implemented?
  3. I've named the lint as "redundant_wildcard_enum_match". Is it good enough or is it better to rename it to something else?

@flip1995
Copy link
Member

  1. Please fix all occurrences. If some cannot be fixed for some reason or another leave an allow there and let the reviewer take a look.
  2. Yes, but put it in its own commit please.
  3. Maybe match_wildcard_for_single_variants to point out the single variant part of the lint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants