From 2951b70d1575f62771ac2da007ef426f9ac600c6 Mon Sep 17 00:00:00 2001 From: Vikas Kumar Date: Tue, 20 Oct 2015 10:18:48 -0700 Subject: [PATCH 1/3] Match on bool should be replaced with if..else block 1. Added another conditional in `check_expr` impl to lint if match expr is a bool. 2. Test cases. --- src/lib.rs | 1 + src/matches.rs | 19 ++++++++++++++++++- tests/compile-fail/matches.rs | 22 ++++++++++++++++++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index a3657460fe99..6d6c6dfeeb0c 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -138,6 +138,7 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::WHILE_LET_LOOP, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, + matches::MATCH_BOOL, methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, diff --git a/src/matches.rs b/src/matches.rs index e935a6aa6e16..0606c2447476 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -1,5 +1,6 @@ use rustc::lint::*; use rustc_front::hir::*; +use rustc::middle::ty; use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block}; @@ -9,13 +10,15 @@ declare_lint!(pub SINGLE_MATCH, Warn, declare_lint!(pub MATCH_REF_PATS, Warn, "a match has all arms prefixed with `&`; the match expression can be \ dereferenced instead"); +declare_lint!(pub MATCH_BOOL, Warn, + "a match on boolean expression; recommends `if..else` block instead"); #[allow(missing_copy_implementations)] pub struct MatchPass; impl LintPass for MatchPass { fn get_lints(&self) -> LintArray { - lint_array!(SINGLE_MATCH, MATCH_REF_PATS) + lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL) } } @@ -59,6 +62,16 @@ impl LateLintPass for MatchPass { expression to match: `match *{} {{ ...`", snippet(cx, ex.span, ".."))); } } + + // check preconditions for MATCH_BOOL + // type of expression == bool + if is_bool_expr(cx, ex) { + if in_external_macro(cx, expr.span) { return; } + + span_lint(cx, MATCH_BOOL, expr.span, + "you seem to be trying to match on a boolean expression. \ + Consider using if..else block"); + } } } } @@ -71,6 +84,10 @@ fn is_unit_expr(expr: &Expr) -> bool { } } +fn is_bool_expr(cx: &LateContext, ex: &Expr ) -> bool { + cx.tcx.expr_ty(ex).sty == ty::TyBool +} + fn has_only_ref_pats(arms: &[Arm]) -> bool { let mapped = arms.iter().flat_map(|a| &a.pats).map(|p| match p.node { PatRegion(..) => Some(true), // &-patterns diff --git a/tests/compile-fail/matches.rs b/tests/compile-fail/matches.rs index f25a5fa3fa4a..1cdf813bc00b 100755 --- a/tests/compile-fail/matches.rs +++ b/tests/compile-fail/matches.rs @@ -38,6 +38,28 @@ fn single_match(){ } } +fn match_bool() { + let test: bool = true; + + match test { //~ ERROR you seem to be trying to match on a boolean expression + true => (), + false => (), + }; + + let option = 1; + match option == 1 { //~ ERROR you seem to be trying to match on a boolean expression + true => (), + false => (), + }; + + // Not linted + match option { + 1 ... 10 => (), + 10 ... 20 => (), + _ => (), + }; +} + fn ref_pats() { { let v = &Some(0); From 675c532eabadeb5e203e71336a50fbf70f7decde Mon Sep 17 00:00:00 2001 From: Vikas Kumar Date: Tue, 20 Oct 2015 10:25:37 -0700 Subject: [PATCH 2/3] Ran util/update_lints.py to auto gen doc and lib.rs --- README.md | 3 ++- src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e50407eaaaa3..990db2e54608 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 66 lints included in this crate: +There are 67 lints included in this crate: name | default | meaning -------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -34,6 +34,7 @@ name [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return) | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value) | warn | creating a let binding to a value of unit type, which usually can't be used afterwards [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist) | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque +[match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool) | warn | a match on boolean expression; recommends `if..else` block instead [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match has all arms prefixed with `&`; the match expression can be dereferenced instead [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max) | warn | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 diff --git a/src/lib.rs b/src/lib.rs index 6d6c6dfeeb0c..6f06641ef45b 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -136,9 +136,9 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::REVERSE_RANGE_LOOP, loops::UNUSED_COLLECT, loops::WHILE_LET_LOOP, + matches::MATCH_BOOL, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, - matches::MATCH_BOOL, methods::SHOULD_IMPLEMENT_TRAIT, methods::STR_TO_STRING, methods::STRING_TO_STRING, From 5e78fbbf57e5bf5891c116a3c1bf287212744efa Mon Sep 17 00:00:00 2001 From: Vikas Kumar Date: Tue, 20 Oct 2015 11:26:54 -0700 Subject: [PATCH 3/3] Fixups from review comments 1. Moved common check `in_external_macro` to the top of function from inside each conditionals. 2. Inlined `is_bool_expr` call --- src/matches.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/matches.rs b/src/matches.rs index 0606c2447476..da770c6f4848 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -25,6 +25,8 @@ impl LintPass for MatchPass { impl LateLintPass for MatchPass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { if let ExprMatch(ref ex, ref arms, MatchSource::Normal) = expr.node { + if in_external_macro(cx, expr.span) { return; } + // check preconditions for SINGLE_MATCH // only two arms if arms.len() == 2 && @@ -39,7 +41,6 @@ impl LateLintPass for MatchPass { // finally, we don't want any content in the second arm (unit or empty block) is_unit_expr(&arms[1].body) { - if in_external_macro(cx, expr.span) {return;} span_help_and_lint(cx, SINGLE_MATCH, expr.span, "you seem to be trying to use match for destructuring a \ single pattern. Consider using `if let`", @@ -51,7 +52,6 @@ impl LateLintPass for MatchPass { // check preconditions for MATCH_REF_PATS if has_only_ref_pats(arms) { - if in_external_macro(cx, expr.span) { return; } if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node { span_lint(cx, MATCH_REF_PATS, expr.span, &format!( "you don't need to add `&` to both the expression to match \ @@ -65,12 +65,11 @@ impl LateLintPass for MatchPass { // check preconditions for MATCH_BOOL // type of expression == bool - if is_bool_expr(cx, ex) { - if in_external_macro(cx, expr.span) { return; } + if cx.tcx.expr_ty(ex).sty == ty::TyBool { span_lint(cx, MATCH_BOOL, expr.span, "you seem to be trying to match on a boolean expression. \ - Consider using if..else block"); + Consider using an if..else block"); } } } @@ -84,10 +83,6 @@ fn is_unit_expr(expr: &Expr) -> bool { } } -fn is_bool_expr(cx: &LateContext, ex: &Expr ) -> bool { - cx.tcx.expr_ty(ex).sty == ty::TyBool -} - fn has_only_ref_pats(arms: &[Arm]) -> bool { let mapped = arms.iter().flat_map(|a| &a.pats).map(|p| match p.node { PatRegion(..) => Some(true), // &-patterns