From: Daniel Wagner-Hall Date: Thu, 31 Jan 2019 22:01:23 +0000 (+0000) Subject: wildcard_enum_match_arm gives suggestions X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=422c9a0fa24bfbc9615d6cfe0bf314de91abb6e3;p=rust.git wildcard_enum_match_arm gives suggestions And is also more robust --- 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,19 +467,85 @@ 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() { + 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 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", - ); + 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, + ) } } 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)] - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum Color { - Red, - Green, - Blue, - Rgb(u8, u8, u8), - Cyan, -} - -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, - } +#![warn(clippy::wildcard_enum_match_arm)] + +#[derive(Debug)] +enum Maybe { + Some(T), + Probably(T), + None, +} + +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 + = note: `-D clippy::wildcard-enum-match-arm` implied by `-D warnings` + +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 | #![deny(clippy::wildcard_enum_match_arm)] - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: to resolve, match each variant explicitly +LL | _ => "Who knows", + | ^ help: try this: `Maybe::Some(..) | Maybe::Probably(..) | Maybe::None` -error: aborting due to previous error +error: aborting due to 4 previous errors