diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 94b36d28e..de21cabc6 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -235,57 +235,54 @@ fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: 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 { - let sugg = if arms.len() == 2 && arms[0].pats.len() == 1 { - // no guards - let exprs = if let PatKind::Lit(ref arm_bool) = arms[0].pats[0].node { - if let ExprLit(ref lit) = arm_bool.node { - match lit.node { - LitKind::Bool(true) => Some((&*arms[0].body, &*arms[1].body)), - LitKind::Bool(false) => Some((&*arms[1].body, &*arms[0].body)), - _ => None, - } - } else { - None - } - } else { - None - }; - - if let Some((ref true_expr, ref false_expr)) = exprs { - match (is_unit_expr(true_expr), is_unit_expr(false_expr)) { - (false, false) => { - Some(format!("if {} {} else {}", - snippet(cx, ex.span, "b"), - expr_block(cx, true_expr, None, ".."), - expr_block(cx, false_expr, None, ".."))) - } - (false, true) => { - Some(format!("if {} {}", snippet(cx, ex.span, "b"), expr_block(cx, true_expr, None, ".."))) - } - (true, false) => { - let test = Sugg::hir(cx, ex, ".."); - Some(format!("if {} {}", - !test, - expr_block(cx, false_expr, None, ".."))) - } - (true, true) => None, - } - } else { - None - } - } else { - 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:", + "you seem to be trying to match on a boolean expression", move |db| { - if let Some(sugg) = sugg { - db.span_suggestion(expr.span, "try this", sugg); - } - }); + if arms.len() == 2 && arms[0].pats.len() == 1 { + // no guards + let exprs = if let PatKind::Lit(ref arm_bool) = arms[0].pats[0].node { + if let ExprLit(ref lit) = arm_bool.node { + match lit.node { + LitKind::Bool(true) => Some((&*arms[0].body, &*arms[1].body)), + LitKind::Bool(false) => Some((&*arms[1].body, &*arms[0].body)), + _ => None, + } + } else { + None + } + } else { + None + }; + + if let Some((ref true_expr, ref false_expr)) = exprs { + let sugg = match (is_unit_expr(true_expr), is_unit_expr(false_expr)) { + (false, false) => { + Some(format!("if {} {} else {}", + snippet(cx, ex.span, "b"), + expr_block(cx, true_expr, None, ".."), + expr_block(cx, false_expr, None, ".."))) + } + (false, true) => { + Some(format!("if {} {}", snippet(cx, ex.span, "b"), expr_block(cx, true_expr, None, ".."))) + } + (true, false) => { + let test = Sugg::hir(cx, ex, ".."); + Some(format!("if {} {}", + !test, + expr_block(cx, false_expr, None, ".."))) + } + (true, true) => None, + }; + + if let Some(sugg) = sugg { + db.span_suggestion(expr.span, "consider using an if/else expression", sugg); + } + } + } + + }); } } @@ -309,26 +306,28 @@ fn check_overlapping_arms(cx: &LateContext, ex: &Expr, arms: &[Arm]) { fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: MatchSource, expr: &Expr) { if has_only_ref_pats(arms) { if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { - let template = match_template(cx, expr.span, source, "", inner); span_lint_and_then(cx, MATCH_REF_PATS, expr.span, "you don't need to add `&` to both the expression and the patterns", |db| { - db.span_suggestion(expr.span, "try", template); - }); + let inner = Sugg::hir(cx, inner, ".."); + let template = match_template(expr.span, source, inner); + db.span_suggestion(expr.span, "try", template); + }); } else { - let template = match_template(cx, expr.span, source, "*", ex); span_lint_and_then(cx, MATCH_REF_PATS, expr.span, "you don't need to add `&` to all patterns", |db| { - db.span_suggestion(expr.span, - "instead of prefixing all patterns with `&`, you can \ - dereference the expression", - template); - }); + let ex = Sugg::hir(cx, ex, ".."); + let template = match_template(expr.span, source, ex.deref()); + db.span_suggestion(expr.span, + "instead of prefixing all patterns with `&`, you can \ + dereference the expression", + template); + }); } } } @@ -411,12 +410,11 @@ fn has_only_ref_pats(arms: &[Arm]) -> bool { mapped.map_or(false, |v| v.iter().any(|el| *el)) } -fn match_template(cx: &LateContext, span: Span, source: MatchSource, op: &str, expr: &Expr) -> String { - let expr_snippet = snippet(cx, expr.span, ".."); +fn match_template(span: Span, source: MatchSource, expr: Sugg) -> String { match source { - MatchSource::Normal => format!("match {}{} {{ .. }}", op, expr_snippet), - MatchSource::IfLetDesugar { .. } => format!("if let .. = {}{} {{ .. }}", op, expr_snippet), - MatchSource::WhileLetDesugar => format!("while let .. = {}{} {{ .. }}", op, expr_snippet), + MatchSource::Normal => format!("match {} {{ .. }}", expr), + MatchSource::IfLetDesugar { .. } => format!("if let .. = {} {{ .. }}", expr), + MatchSource::WhileLetDesugar => format!("while let .. = {} {{ .. }}", expr), MatchSource::ForLoopDesugar => span_bug!(span, "for loop desugared to match with &-patterns!"), MatchSource::TryDesugar => span_bug!(span, "`?` operator desugared to match with &-patterns!"), } diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index bbfa8648c..f857c821e 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -134,6 +134,11 @@ impl<'a> Sugg<'a> { make_unop("&mut ", self) } + /// Convenience method to create the `*` suggestion. + pub fn deref(self) -> Sugg<'static> { + make_unop("*", self) + } + /// Convenience method to create the `..` or `...` suggestion. pub fn range(self, end: Self, limit: ast::RangeLimits) -> Sugg<'static> { match limit { diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index e49aeaa6e..f64cceb5c 100644 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -112,7 +112,7 @@ fn match_bool() { match test { //~^ ERROR you seem to be trying to match on a boolean expression - //~| HELP try + //~| HELP consider //~| SUGGESTION if test { 0 } else { 42 }; true => 0, false => 42, @@ -121,7 +121,7 @@ fn match_bool() { let option = 1; match option == 1 { //~^ ERROR you seem to be trying to match on a boolean expression - //~| HELP try + //~| HELP consider //~| SUGGESTION if option == 1 { 1 } else { 0 }; true => 1, false => 0, @@ -129,7 +129,7 @@ fn match_bool() { match test { //~^ ERROR you seem to be trying to match on a boolean expression - //~| HELP try + //~| HELP consider //~| SUGGESTION if !test { println!("Noooo!"); }; true => (), false => { println!("Noooo!"); } @@ -137,7 +137,7 @@ fn match_bool() { match test { //~^ ERROR you seem to be trying to match on a boolean expression - //~| HELP try + //~| HELP consider //~| SUGGESTION if !test { println!("Noooo!"); }; false => { println!("Noooo!"); } _ => (), @@ -145,7 +145,7 @@ fn match_bool() { match test && test { //~^ ERROR you seem to be trying to match on a boolean expression - //~| HELP try + //~| HELP consider //~| SUGGESTION if !(test && test) { println!("Noooo!"); }; //~| ERROR equal expressions as operands false => { println!("Noooo!"); } @@ -154,7 +154,7 @@ fn match_bool() { match test { //~^ ERROR you seem to be trying to match on a boolean expression - //~| HELP try + //~| HELP consider //~| SUGGESTION if test { println!("Yes!"); } else { println!("Noooo!"); }; false => { println!("Noooo!"); } true => { println!("Yes!"); }