diff --git a/src/matches.rs b/src/matches.rs index 5e207093c..4cb4df19b 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -8,7 +8,8 @@ use std::cmp::Ordering; use syntax::ast::Lit_::LitBool; use syntax::codemap::Span; -use utils::{snippet, span_lint, span_note_and_lint, span_help_and_lint, in_external_macro, expr_block}; +use utils::{COW_PATH, OPTION_PATH, RESULT_PATH}; +use utils::{match_type, snippet, span_lint, span_note_and_lint, span_lint_and_then, in_external_macro, expr_block}; /// **What it does:** This lint checks for matches with a single arm where an `if let` will usually suffice. It is `Warn` by default. /// @@ -109,25 +110,73 @@ impl LateLintPass for MatchPass { } fn check_single_match(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { - if arms.len() == 2 && arms[0].pats.len() == 1 && arms[0].guard.is_none() && arms[1].pats.len() == 1 && - arms[1].guard.is_none() && arms[1].pats[0].node == PatWild && is_unit_expr(&arms[1].body) && - (cx.tcx.expr_ty(ex).sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow) { - span_help_and_lint(cx, + if arms.len() == 2 && + arms[0].pats.len() == 1 && arms[0].guard.is_none() && + arms[1].pats.len() == 1 && arms[1].guard.is_none() && + is_unit_expr(&arms[1].body) { + let ty = cx.tcx.expr_ty(ex); + if ty.sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow { + check_single_match_single_pattern(cx, ex, arms, expr); + check_single_match_opt_like(cx, ex, arms, expr, ty); + } + } +} + +fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { + if arms[1].pats[0].node == PatWild { + span_lint_and_then(cx, SINGLE_MATCH, expr.span, - "you seem to be trying to use match for destructuring a single pattern. Consider using \ - `if let`", - &format!("try\nif let {} = {} {}", - snippet(cx, arms[0].pats[0].span, ".."), - snippet(cx, ex.span, ".."), - expr_block(cx, &arms[0].body, None, ".."))); + "you seem to be trying to use match for destructuring a single pattern. \ + Consider using `if let`", |db| { + db.span_suggestion(expr.span, "try this", + format!("if let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + expr_block(cx, &arms[0].body, None, ".."))); + }); + } +} + +fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty) { + // list of candidate Enums we know will never get any more membre + let candidates = &[ + (&COW_PATH, "Borrowed"), + (&COW_PATH, "Cow::Borrowed"), + (&COW_PATH, "Cow::Owned"), + (&COW_PATH, "Owned"), + (&OPTION_PATH, "None"), + (&RESULT_PATH, "Err"), + (&RESULT_PATH, "Ok"), + ]; + + let path = match arms[1].pats[0].node { + PatEnum(ref path, _) => path.to_string(), + PatIdent(BindByValue(MutImmutable), ident, None) => ident.node.to_string(), + _ => return + }; + + for &(ty_path, pat_path) in candidates { + if &path == pat_path && match_type(cx, ty, ty_path) { + span_lint_and_then(cx, + SINGLE_MATCH, + expr.span, + "you seem to be trying to use match for destructuring a single pattern. \ + Consider using `if let`", |db| { + db.span_suggestion(expr.span, "try this", + format!("if let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + expr_block(cx, &arms[0].body, None, ".."))); + }); + } } } fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { // type of expression == bool if cx.tcx.expr_ty(ex).sty == ty::TyBool { - if arms.len() == 2 && arms[0].pats.len() == 1 { + let sugg = if arms.len() == 2 && arms[0].pats.len() == 1 { // no guards let exprs = if let PatLit(ref arm_bool) = arms[0].pats[0].node { if let ExprLit(ref lit) = arm_bool.node { @@ -142,56 +191,42 @@ fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { } else { None }; + if let Some((ref true_expr, ref false_expr)) = exprs { if !is_unit_expr(true_expr) { if !is_unit_expr(false_expr) { - span_help_and_lint(cx, - MATCH_BOOL, - expr.span, - "you seem to be trying to match on a boolean expression. Consider using \ - an if..else block:", - &format!("try\nif {} {} else {}", - snippet(cx, ex.span, "b"), - expr_block(cx, true_expr, None, ".."), - expr_block(cx, false_expr, None, ".."))); + Some(format!("if {} {} else {}", + snippet(cx, ex.span, "b"), + expr_block(cx, true_expr, None, ".."), + expr_block(cx, false_expr, None, ".."))) } else { - span_help_and_lint(cx, - MATCH_BOOL, - expr.span, - "you seem to be trying to match on a boolean expression. Consider using \ - an if..else block:", - &format!("try\nif {} {}", - snippet(cx, ex.span, "b"), - expr_block(cx, true_expr, None, ".."))); + Some(format!("if {} {}", + snippet(cx, ex.span, "b"), + expr_block(cx, true_expr, None, ".."))) } } else if !is_unit_expr(false_expr) { - span_help_and_lint(cx, - MATCH_BOOL, - expr.span, - "you seem to be trying to match on a boolean expression. Consider using an \ - if..else block:", - &format!("try\nif !{} {}", - snippet(cx, ex.span, "b"), - expr_block(cx, false_expr, None, ".."))); + Some(format!("try\nif !{} {}", + snippet(cx, ex.span, "b"), + expr_block(cx, false_expr, None, ".."))) } else { - span_lint(cx, - MATCH_BOOL, - expr.span, - "you seem to be trying to match on a boolean expression. Consider using an if..else \ - block"); + None } } else { - span_lint(cx, - MATCH_BOOL, - expr.span, - "you seem to be trying to match on a boolean expression. Consider using an if..else block"); + None } } else { - span_lint(cx, - MATCH_BOOL, - expr.span, - "you seem to be trying to match on a boolean expression. Consider using an if..else block"); - } + None + }; + + span_lint_and_then(cx, + MATCH_BOOL, + expr.span, + "you seem to be trying to match on a boolean expression. Consider using \ + an if..else block:", move |db| { + if let Some(ref sugg) = sugg { + db.span_suggestion(expr.span, "try this", sugg.clone()); + } + }); } } diff --git a/src/utils.rs b/src/utils.rs index 74a15927a..77e63fe44 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -29,6 +29,7 @@ pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"]; pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"]; pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"]; pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"]; +pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"]; /// Produce a nested chain of if-lets and ifs from the patterns: /// diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index b569f9566..c58e62419 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -4,8 +4,14 @@ #![deny(clippy)] #![allow(unused)] +use std::borrow::Cow; + +enum Foo { Bar, Baz(u8) } +use Foo::*; + fn single_match(){ let x = Some(1u8); + match x { //~ ERROR you seem to be trying to use match //~^ HELP try Some(y) => { @@ -13,11 +19,7 @@ fn single_match(){ } _ => () } - // Not linted - match x { - Some(y) => println!("{:?}", y), - None => () - } + let z = (1u8,1u8); match z { //~ ERROR you seem to be trying to use match //~^ HELP try @@ -38,6 +40,43 @@ fn single_match(){ } } +fn single_match_know_enum() { + let x = Some(1u8); + let y : Result<_, i8> = Ok(1i8); + + match x { //~ ERROR you seem to be trying to use match + //~^ HELP try + Some(y) => println!("{:?}", y), + None => () + } + + match y { //~ ERROR you seem to be trying to use match + //~^ HELP try + Ok(y) => println!("{:?}", y), + Err(..) => () + } + + let c = Cow::Borrowed(""); + + match c { //~ ERROR you seem to be trying to use match + //~^ HELP try + Cow::Borrowed(..) => println!("42"), + Cow::Owned(..) => (), + } + + let z = Foo::Bar; + // no warning + match z { + Bar => println!("42"), + Baz(_) => (), + } + + match z { + Baz(_) => println!("42"), + Bar => (), + } +} + fn match_bool() { let test: bool = true;