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: match_wildcard_for_single_variants #5582

Merged
merged 8 commits into from
May 20, 2020
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,7 @@ Released 2018-09-13
[`match_same_arms`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_same_arms
[`match_single_binding`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
[`match_wild_err_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_wild_err_arm
[`match_wildcard_for_single_variants`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_wildcard_for_single_variants
[`maybe_infinite_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#maybe_infinite_iter
[`mem_discriminant_non_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum
[`mem_forget`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_forget
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/float_literal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for FloatLiteral {
let type_suffix = match lit_float_ty {
LitFloatType::Suffixed(FloatTy::F32) => Some("f32"),
LitFloatType::Suffixed(FloatTy::F64) => Some("f64"),
_ => None
LitFloatType::Unsuffixed => None
};
let (is_whole, mut float_str) = match fty {
FloatTy::F32 => {
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&matches::MATCH_OVERLAPPING_ARM,
&matches::MATCH_REF_PATS,
&matches::MATCH_SINGLE_BINDING,
&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
&matches::MATCH_WILD_ERR_ARM,
&matches::REST_PAT_IN_FULLY_BOUND_STRUCTS,
&matches::SINGLE_MATCH,
Expand Down Expand Up @@ -1147,6 +1148,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&macro_use::MACRO_USE_IMPORTS),
LintId::of(&match_on_vec_items::MATCH_ON_VEC_ITEMS),
LintId::of(&matches::MATCH_BOOL),
LintId::of(&matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS),
LintId::of(&matches::SINGLE_MATCH_ELSE),
LintId::of(&methods::FILTER_MAP),
LintId::of(&methods::FILTER_MAP_NEXT),
Expand Down
68 changes: 64 additions & 4 deletions clippy_lints/src/matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ declare_clippy_lint! {
/// # enum Foo { A(usize), B(usize) }
/// # let x = Foo::B(1);
/// match x {
/// A => {},
/// Foo::A(_) => {},
/// _ => {},
/// }
/// ```
Expand All @@ -229,6 +229,40 @@ declare_clippy_lint! {
"a wildcard enum match arm using `_`"
}

declare_clippy_lint! {
/// **What it does:** Checks for wildcard enum matches for a single variant.
///
/// **Why is this bad?** New enum variants added by library updates can be missed.
///
/// **Known problems:** Suggested replacements may not use correct path to enum
/// if it's not present in the current scope.
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
///
/// **Example:**
///
/// ```rust
/// # enum Foo { A, B, C }
/// # let x = Foo::B;
/// match x {
/// Foo::A => {},
/// Foo::B => {},
/// _ => {},
/// }
/// ```
/// Use instead:
/// ```rust
/// # enum Foo { A, B, C }
/// # let x = Foo::B;
/// match x {
/// Foo::A => {},
/// Foo::B => {},
/// Foo::C => {},
/// }
/// ```
pub MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
pedantic,
"a wildcard enum match for a single variant"
}

declare_clippy_lint! {
/// **What it does:** Checks for wildcard pattern used with others patterns in same match arm.
///
Expand Down Expand Up @@ -356,6 +390,7 @@ impl_lint_pass!(Matches => [
MATCH_WILD_ERR_ARM,
MATCH_AS_REF,
WILDCARD_ENUM_MATCH_ARM,
MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
WILDCARD_IN_OR_PATTERNS,
MATCH_SINGLE_BINDING,
INFALLIBLE_DESTRUCTURING_MATCH,
Expand Down Expand Up @@ -729,9 +764,21 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_
if let QPath::Resolved(_, p) = path {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
}
} else if let PatKind::TupleStruct(ref path, ..) = arm.pat.kind {
} else if let PatKind::TupleStruct(ref path, ref patterns, ..) = arm.pat.kind {
if let QPath::Resolved(_, p) = path {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
// Some simple checks for exhaustive patterns.
// There is a room for improvements to detect more cases,
// but it can be more expensive to do so.
let is_pattern_exhaustive = |pat: &&Pat<'_>| {
if let PatKind::Wild | PatKind::Binding(.., None) = pat.kind {
true
} else {
false
}
};
if patterns.iter().all(is_pattern_exhaustive) {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
}
}
}
}
Expand Down Expand Up @@ -766,14 +813,27 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr<'_>, arms: &[Arm<'_
}
}

if suggestion.len() == 1 {
// No need to check for non-exhaustive enum as in that case len would be greater than 1
span_lint_and_sugg(
cx,
MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
wildcard_span,
message,
"try this",
suggestion[0].clone(),
Applicability::MaybeIncorrect,
)
};

span_lint_and_sugg(
cx,
WILDCARD_ENUM_MATCH_ARM,
wildcard_span,
message,
"try this",
suggestion.join(" | "),
Applicability::MachineApplicable,
Applicability::MaybeIncorrect,
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/misc_early.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ impl EarlyLintPass for MiscEarlyLints {
let left_binding = match left {
BindingMode::ByRef(Mutability::Mut) => "ref mut ",
BindingMode::ByRef(Mutability::Not) => "ref ",
_ => "",
BindingMode::ByValue(..) => "",
};

if let PatKind::Wild = right.kind {
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/missing_const_for_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingConstForFn {
return;
}
},
_ => return,
FnKind::Closure(..) => return,
}

let mir = cx.tcx.optimized_mir(def_id);
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/needless_pass_by_value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue {
}
},
FnKind::Method(..) => (),
_ => return,
FnKind::Closure(..) => return,
}

// Exclude non-inherent impls
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/trivially_copy_pass_by_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TriviallyCopyPassByRef {
}
},
FnKind::Method(..) => (),
_ => return,
FnKind::Closure(..) => return,
}

// Exclude non-inherent impls
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'_, 'tcx>, hir_id: HirId) -> O
pub fn has_drop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
match ty.ty_adt_def() {
Some(def) => def.has_dtor(cx.tcx),
_ => false,
None => false,
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "matches",
},
Lint {
name: "match_wildcard_for_single_variants",
group: "pedantic",
desc: "a wildcard enum match for a single variant",
deprecation: None,
module: "matches",
},
Lint {
name: "maybe_infinite_iter",
group: "pedantic",
Expand Down
2 changes: 1 addition & 1 deletion tests/compile-test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ fn third_party_crates() -> String {
for entry in fs::read_dir(dep_dir).unwrap() {
let path = match entry {
Ok(entry) => entry.path(),
_ => continue,
Err(_) => continue,
};
if let Some(name) = path.file_name().and_then(OsStr::to_str) {
for dep in CRATES {
Expand Down
59 changes: 59 additions & 0 deletions tests/ui/match_wildcard_for_single_variants.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// run-rustfix

#![warn(clippy::match_wildcard_for_single_variants)]
#![allow(dead_code)]

enum Foo {
A,
B,
C,
}

enum Color {
Red,
Green,
Blue,
Rgb(u8, u8, u8),
}

fn main() {
let f = Foo::A;
match f {
Foo::A => {},
Foo::B => {},
Foo::C => {},
}

let color = Color::Red;

// check exhaustive bindings
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(_r, _g, _b) => {},
Color::Blue => {},
}

// check exhaustive wild
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(..) => {},
Color::Blue => {},
}
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(_, _, _) => {},
Color::Blue => {},
}

// shouldn't lint as there is one missing variant
// and one that isn't exhaustively covered
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(255, _, _) => {},
_ => {},
}
}
59 changes: 59 additions & 0 deletions tests/ui/match_wildcard_for_single_variants.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// run-rustfix

#![warn(clippy::match_wildcard_for_single_variants)]
#![allow(dead_code)]

enum Foo {
A,
B,
C,
}

enum Color {
Red,
Green,
Blue,
Rgb(u8, u8, u8),
}

fn main() {
let f = Foo::A;
match f {
Foo::A => {},
Foo::B => {},
_ => {},
}

let color = Color::Red;

// check exhaustive bindings
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(_r, _g, _b) => {},
_ => {},
}

// check exhaustive wild
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(..) => {},
_ => {},
}
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(_, _, _) => {},
_ => {},
}

// shouldn't lint as there is one missing variant
// and one that isn't exhaustively covered
match color {
Color::Red => {},
Color::Green => {},
Color::Rgb(255, _, _) => {},
_ => {},
}
}
28 changes: 28 additions & 0 deletions tests/ui/match_wildcard_for_single_variants.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
error: wildcard match will miss any future added variants
--> $DIR/match_wildcard_for_single_variants.rs:24:9
|
LL | _ => {},
| ^ help: try this: `Foo::C`
|
= note: `-D clippy::match-wildcard-for-single-variants` implied by `-D warnings`

error: wildcard match will miss any future added variants
--> $DIR/match_wildcard_for_single_variants.rs:34:9
|
LL | _ => {},
| ^ help: try this: `Color::Blue`

error: wildcard match will miss any future added variants
--> $DIR/match_wildcard_for_single_variants.rs:42:9
|
LL | _ => {},
| ^ help: try this: `Color::Blue`

error: wildcard match will miss any future added variants
--> $DIR/match_wildcard_for_single_variants.rs:48:9
|
LL | _ => {},
| ^ help: try this: `Color::Blue`

error: aborting due to 4 previous errors