From 422c9a0fa24bfbc9615d6cfe0bf314de91abb6e3 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 31 Jan 2019 22:01:23 +0000 Subject: [PATCH 1/3] wildcard_enum_match_arm gives suggestions And is also more robust --- clippy_lints/src/matches.rs | 95 ++++++++++++++++++++---- tests/ui/wildcard_enum_match_arm.rs | 99 ++++++++++++++++--------- tests/ui/wildcard_enum_match_arm.stderr | 33 ++++++--- 3 files changed, 170 insertions(+), 57 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 9cb160685ca..a7c66c22968 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -6,13 +6,15 @@ snippet_with_applicability, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, }; use if_chain::if_chain; +use rustc::hir::def::CtorKind; use rustc::hir::*; use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintContext, LintPass}; -use rustc::ty::{self, Ty}; +use rustc::ty::{self, Ty, TyKind}; use rustc::{declare_tool_lint, lint_array}; use rustc_errors::Applicability; use std::cmp::Ordering; use std::collections::Bound; +use std::ops::Deref; use syntax::ast::LitKind; use syntax::source_map::Span; @@ -191,7 +193,8 @@ /// /// **Why is this bad?** New enum variants added by library updates can be missed. /// -/// **Known problems:** Nested wildcards a la `Foo(_)` are currently not detected. +/// **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 @@ -464,20 +467,86 @@ fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) { } fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) { - if cx.tables.expr_ty(ex).is_enum() { - for arm in arms { - if is_wild(&arm.pats[0]) { - span_note_and_lint( - cx, - WILDCARD_ENUM_MATCH_ARM, - arm.pats[0].span, - "wildcard match will miss any future added variants.", - arm.pats[0].span, - "to resolve, match each variant explicitly", - ); + let ty = cx.tables.expr_ty(ex); + if !ty.is_enum() { + // If there isn't a nice closed set of possible values that can be conveniently enumerated, + // don't complain about not enumerating the mall. + return; + } + + // First pass - check for violation, but don't do much book-keeping because this is hopefully + // the uncommon case, and the book-keeping is slightly expensive. + let mut wildcard_span = None; + let mut wildcard_ident = None; + for arm in arms { + for pat in &arm.pats { + if let PatKind::Wild = pat.node { + wildcard_span = Some(pat.span); + } else if let PatKind::Binding(_, _, ident, None) = pat.node { + wildcard_span = Some(pat.span); + wildcard_ident = Some(ident); } } } + + if let Some(wildcard_span) = wildcard_span { + // Accumulate the variants which should be put in place of the wildcard because they're not + // already covered. + + let mut missing_variants = vec![]; + if let TyKind::Adt(def, _) = ty.sty { + for variant in &def.variants { + missing_variants.push(variant); + } + } + + for arm in arms { + if arm.guard.is_some() { + // Guards mean that this case probably isn't exhaustively covered. Technically + // this is incorrect, as we should really check whether each variant is exhaustively + // covered by the set of guards that cover it, but that's really hard to do. + continue; + } + for pat in &arm.pats { + if let PatKind::Path(ref path) = pat.deref().node { + if let QPath::Resolved(_, p) = path { + missing_variants.retain(|e| e.did != p.def.def_id()); + } + } else if let PatKind::TupleStruct(ref path, ..) = pat.deref().node { + if let QPath::Resolved(_, p) = path { + missing_variants.retain(|e| e.did != p.def.def_id()); + } + } + } + } + + let suggestion: Vec = missing_variants + .iter() + .map(|v| { + let suffix = match v.ctor_kind { + CtorKind::Fn => "(..)", + CtorKind::Const | CtorKind::Fictive => "", + }; + let ident_str = if let Some(ident) = wildcard_ident { + format!("{} @ ", ident.name) + } else { + String::new() + }; + // This path assumes that the enum type is imported into scope. + format!("{}{}{}", ident_str, cx.tcx.item_path_str(v.did), suffix) + }) + .collect(); + + span_lint_and_sugg( + cx, + WILDCARD_ENUM_MATCH_ARM, + wildcard_span, + "wildcard match will miss any future added variants.", + "try this", + suggestion.join(" | "), + Applicability::MachineApplicable, + ) + } } // If the block contains only a `panic!` macro (as expression or statement) diff --git a/tests/ui/wildcard_enum_match_arm.rs b/tests/ui/wildcard_enum_match_arm.rs index 58daabf4268..86d4c7f28c4 100644 --- a/tests/ui/wildcard_enum_match_arm.rs +++ b/tests/ui/wildcard_enum_match_arm.rs @@ -1,42 +1,73 @@ -#![deny(clippy::wildcard_enum_match_arm)] +#![warn(clippy::wildcard_enum_match_arm)] -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum Color { - Red, - Green, - Blue, - Rgb(u8, u8, u8), - Cyan, +#[derive(Debug)] +enum Maybe { + Some(T), + Probably(T), + None, } -impl Color { - fn is_monochrome(self) -> bool { - match self { - Color::Red | Color::Green | Color::Blue => true, - Color::Rgb(r, g, b) => r | g == 0 || r | b == 0 || g | b == 0, - Color::Cyan => false, - } +fn is_it_wildcard(m: Maybe) -> &'static str { + match m { + Maybe::Some(_) => "Some", + _ => "Could be", + } +} + +fn is_it_bound(m: Maybe) -> &'static str { + match m { + Maybe::None => "None", + _other => "Could be", + } +} + +fn is_it_binding(m: Maybe) -> String { + match m { + Maybe::Some(v) => "Large".to_string(), + n => format!("{:?}", n), + } +} + +fn is_it_binding_exhaustive(m: Maybe) -> String { + match m { + Maybe::Some(v) => "Large".to_string(), + n @ Maybe::Probably(_) | n @ Maybe::None => format!("{:?}", n), + } +} + +fn is_it_with_guard(m: Maybe) -> &'static str { + match m { + Maybe::Some(v) if v > 100 => "Large", + _ => "Who knows", + } +} + +fn is_it_exhaustive(m: Maybe) -> &'static str { + match m { + Maybe::None => "None", + Maybe::Some(_) | Maybe::Probably(..) => "Could be", + } +} + +fn is_one_or_three(i: i32) -> bool { + match i { + 1 | 3 => true, + _ => false, } } fn main() { - let color = Color::Rgb(0, 0, 127); - match color { - Color::Red => println!("Red"), - _ => eprintln!("Not red"), - }; - match color { - Color::Red => {}, - Color::Green => {}, - Color::Blue => {}, - Color::Cyan => {}, - c if c.is_monochrome() => {}, - Color::Rgb(_, _, _) => {}, - }; - let x: u8 = unimplemented!(); - match x { - 0 => {}, - 140 => {}, - _ => {}, - }; + println!("{}", is_it_wildcard(Maybe::Some("foo"))); + + println!("{}", is_it_bound(Maybe::Some("foo"))); + + println!("{}", is_it_binding(Maybe::Some(1))); + + println!("{}", is_it_binding_exhaustive(Maybe::Some(1))); + + println!("{}", is_it_with_guard(Maybe::Some(1))); + + println!("{}", is_it_exhaustive(Maybe::Some("foo"))); + + println!("{}", is_one_or_three(2)); } diff --git a/tests/ui/wildcard_enum_match_arm.stderr b/tests/ui/wildcard_enum_match_arm.stderr index 6319a3f3d46..1d6f3f662a3 100644 --- a/tests/ui/wildcard_enum_match_arm.stderr +++ b/tests/ui/wildcard_enum_match_arm.stderr @@ -1,15 +1,28 @@ error: wildcard match will miss any future added variants. - --> $DIR/wildcard_enum_match_arm.rs:26:9 + --> $DIR/wildcard_enum_match_arm.rs:13:9 | -LL | _ => eprintln!("Not red"), - | ^ +LL | _ => "Could be", + | ^ help: try this: `Maybe::Probably(..) | Maybe::None` | -note: lint level defined here - --> $DIR/wildcard_enum_match_arm.rs:1:9 - | -LL | #![deny(clippy::wildcard_enum_match_arm)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: to resolve, match each variant explicitly + = note: `-D clippy::wildcard-enum-match-arm` implied by `-D warnings` -error: aborting due to previous error +error: wildcard match will miss any future added variants. + --> $DIR/wildcard_enum_match_arm.rs:20:9 + | +LL | _other => "Could be", + | ^^^^^^ help: try this: `_other @ Maybe::Some(..) | _other @ Maybe::Probably(..)` + +error: wildcard match will miss any future added variants. + --> $DIR/wildcard_enum_match_arm.rs:27:9 + | +LL | n => format!("{:?}", n), + | ^ help: try this: `n @ Maybe::Probably(..) | n @ Maybe::None` + +error: wildcard match will miss any future added variants. + --> $DIR/wildcard_enum_match_arm.rs:41:9 + | +LL | _ => "Who knows", + | ^ help: try this: `Maybe::Some(..) | Maybe::Probably(..) | Maybe::None` + +error: aborting due to 4 previous errors From bcefd688c99895275317eb95a1ec62743526208a Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 18 Feb 2019 22:55:16 +0000 Subject: [PATCH 2/3] Restore tests Also, fix existing test --- clippy_lints/src/matches.rs | 4 + tests/ui/wildcard_enum_match_arm.rs | 119 +++++++++++------------- tests/ui/wildcard_enum_match_arm.stderr | 30 +++--- 3 files changed, 75 insertions(+), 78 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index a7c66c22968..3de1d01285c 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -537,6 +537,10 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) { }) .collect(); + if suggestion.is_empty() { + return; + } + span_lint_and_sugg( cx, WILDCARD_ENUM_MATCH_ARM, diff --git a/tests/ui/wildcard_enum_match_arm.rs b/tests/ui/wildcard_enum_match_arm.rs index 86d4c7f28c4..94d69d3c8a4 100644 --- a/tests/ui/wildcard_enum_match_arm.rs +++ b/tests/ui/wildcard_enum_match_arm.rs @@ -1,73 +1,62 @@ -#![warn(clippy::wildcard_enum_match_arm)] +#![deny(clippy::wildcard_enum_match_arm)] -#[derive(Debug)] -enum Maybe { - Some(T), - Probably(T), - None, +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum Color { + Red, + Green, + Blue, + Rgb(u8, u8, u8), + Cyan, } -fn is_it_wildcard(m: Maybe) -> &'static str { - match m { - Maybe::Some(_) => "Some", - _ => "Could be", - } -} - -fn is_it_bound(m: Maybe) -> &'static str { - match m { - Maybe::None => "None", - _other => "Could be", - } -} - -fn is_it_binding(m: Maybe) -> String { - match m { - Maybe::Some(v) => "Large".to_string(), - n => format!("{:?}", n), - } -} - -fn is_it_binding_exhaustive(m: Maybe) -> String { - match m { - Maybe::Some(v) => "Large".to_string(), - n @ Maybe::Probably(_) | n @ Maybe::None => format!("{:?}", n), - } -} - -fn is_it_with_guard(m: Maybe) -> &'static str { - match m { - Maybe::Some(v) if v > 100 => "Large", - _ => "Who knows", - } -} - -fn is_it_exhaustive(m: Maybe) -> &'static str { - match m { - Maybe::None => "None", - Maybe::Some(_) | Maybe::Probably(..) => "Could be", - } -} - -fn is_one_or_three(i: i32) -> bool { - match i { - 1 | 3 => true, - _ => false, +impl Color { + fn is_monochrome(self) -> bool { + match self { + Color::Red | Color::Green | Color::Blue => true, + Color::Rgb(r, g, b) => r | g == 0 || r | b == 0 || g | b == 0, + Color::Cyan => false, + } } } fn main() { - println!("{}", is_it_wildcard(Maybe::Some("foo"))); - - println!("{}", is_it_bound(Maybe::Some("foo"))); - - println!("{}", is_it_binding(Maybe::Some(1))); - - println!("{}", is_it_binding_exhaustive(Maybe::Some(1))); - - println!("{}", is_it_with_guard(Maybe::Some(1))); - - println!("{}", is_it_exhaustive(Maybe::Some("foo"))); - - println!("{}", is_one_or_three(2)); + let color = Color::Rgb(0, 0, 127); + match color { + Color::Red => println!("Red"), + _ => eprintln!("Not red"), + }; + match color { + Color::Red => println!("Red"), + _not_red => eprintln!("Not red"), + }; + let _str = match color { + Color::Red => "Red".to_owned(), + not_red => format!("{:?}", not_red), + }; + match color { + Color::Red => {}, + Color::Green => {}, + Color::Blue => {}, + Color::Cyan => {}, + c if c.is_monochrome() => {}, + Color::Rgb(_, _, _) => {}, + }; + let _str = match color { + Color::Red => "Red", + c @ Color::Green | c @ Color::Blue | c @ Color::Rgb(_, _, _) | c @ Color::Cyan => "Not red", + }; + match color { + Color::Rgb(r, _, _) if r > 0 => "Some red", + _ => "No red", + }; + match color { + Color::Red | Color::Green | Color::Blue | Color::Cyan => {}, + Color::Rgb(..) => {}, + }; + let x: u8 = unimplemented!(); + match x { + 0 => {}, + 140 => {}, + _ => {}, + }; } diff --git a/tests/ui/wildcard_enum_match_arm.stderr b/tests/ui/wildcard_enum_match_arm.stderr index 1d6f3f662a3..999c1693301 100644 --- a/tests/ui/wildcard_enum_match_arm.stderr +++ b/tests/ui/wildcard_enum_match_arm.stderr @@ -1,28 +1,32 @@ error: wildcard match will miss any future added variants. - --> $DIR/wildcard_enum_match_arm.rs:13:9 + --> $DIR/wildcard_enum_match_arm.rs:26:9 | -LL | _ => "Could be", - | ^ help: try this: `Maybe::Probably(..) | Maybe::None` +LL | _ => eprintln!("Not red"), + | ^ help: try this: `Color::Green | Color::Blue | Color::Rgb(..) | Color::Cyan` | - = note: `-D clippy::wildcard-enum-match-arm` implied by `-D warnings` +note: lint level defined here + --> $DIR/wildcard_enum_match_arm.rs:1:9 + | +LL | #![deny(clippy::wildcard_enum_match_arm)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: wildcard match will miss any future added variants. - --> $DIR/wildcard_enum_match_arm.rs:20:9 + --> $DIR/wildcard_enum_match_arm.rs:30:9 | -LL | _other => "Could be", - | ^^^^^^ help: try this: `_other @ Maybe::Some(..) | _other @ Maybe::Probably(..)` +LL | _not_red => eprintln!("Not red"), + | ^^^^^^^^ help: try this: `_not_red @ Color::Green | _not_red @ Color::Blue | _not_red @ Color::Rgb(..) | _not_red @ Color::Cyan` error: wildcard match will miss any future added variants. - --> $DIR/wildcard_enum_match_arm.rs:27:9 + --> $DIR/wildcard_enum_match_arm.rs:34:9 | -LL | n => format!("{:?}", n), - | ^ help: try this: `n @ Maybe::Probably(..) | n @ Maybe::None` +LL | not_red => format!("{:?}", not_red), + | ^^^^^^^ help: try this: `not_red @ Color::Green | not_red @ Color::Blue | not_red @ Color::Rgb(..) | not_red @ Color::Cyan` error: wildcard match will miss any future added variants. - --> $DIR/wildcard_enum_match_arm.rs:41:9 + --> $DIR/wildcard_enum_match_arm.rs:50:9 | -LL | _ => "Who knows", - | ^ help: try this: `Maybe::Some(..) | Maybe::Probably(..) | Maybe::None` +LL | _ => "No red", + | ^ help: try this: `Color::Red | Color::Green | Color::Blue | Color::Rgb(..) | Color::Cyan` error: aborting due to 4 previous errors From 4009a441189ff8fa9a46acd52a9f7d2f629e47df Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Mon, 18 Feb 2019 23:16:53 +0000 Subject: [PATCH 3/3] Fix Binding for rustc update --- clippy_lints/src/matches.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 3de1d01285c..fcff1e16f38 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -482,7 +482,7 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) { for pat in &arm.pats { if let PatKind::Wild = pat.node { wildcard_span = Some(pat.span); - } else if let PatKind::Binding(_, _, ident, None) = pat.node { + } else if let PatKind::Binding(_, _, _, ident, None) = pat.node { wildcard_span = Some(pat.span); wildcard_ident = Some(ident); }