From 56b3e7b4c2308be84ce4eed18f03743cc592b780 Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Tue, 9 Feb 2016 14:10:22 -0500 Subject: [PATCH 1/4] lint comparison to bool (e.g. `y == true`) Addresses #630 --- src/lib.rs | 2 + src/needless_bool.rs | 66 +++++++++++++++++++++++++++ tests/compile-fail/bool_comparison.rs | 12 +++++ 3 files changed, 80 insertions(+) create mode 100644 tests/compile-fail/bool_comparison.rs diff --git a/src/lib.rs b/src/lib.rs index 728aa124a18..c1e8c3c9f01 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -105,6 +105,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box bit_mask::BitMask); reg.register_late_lint_pass(box ptr_arg::PtrArg); reg.register_late_lint_pass(box needless_bool::NeedlessBool); + reg.register_late_lint_pass(box needless_bool::BoolComparison); reg.register_late_lint_pass(box approx_const::ApproxConstant); reg.register_late_lint_pass(box misc::FloatCmp); reg.register_early_lint_pass(box precedence::Precedence); @@ -253,6 +254,7 @@ pub fn plugin_registrar(reg: &mut Registry) { mut_reference::UNNECESSARY_MUT_PASSED, mutex_atomic::MUTEX_ATOMIC, needless_bool::NEEDLESS_BOOL, + needless_bool::BOOL_COMPARISON, needless_features::UNSTABLE_AS_MUT_SLICE, needless_features::UNSTABLE_AS_SLICE, needless_update::NEEDLESS_UPDATE, diff --git a/src/needless_bool.rs b/src/needless_bool.rs index bfd819edcb6..a2a0d13e17c 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -6,6 +6,7 @@ use rustc::lint::*; use rustc_front::hir::*; use syntax::ast::Lit_; +use syntax::codemap::Spanned; use utils::{span_lint, snippet}; @@ -23,6 +24,20 @@ declare_lint! { `if p { true } else { false }`" } +/// **What it does:** This lint checks for expressions of the form `x == true` (or vice versa) and suggest using the variable directly. +/// +/// **Why is this bad?** Unnecessary code. +/// +/// **Known problems:** None. +/// +/// **Example:** `if x == true { }` could be `if x { }` +declare_lint! { + pub BOOL_COMPARISON, + Warn, + "comparing a variable to a boolean, e.g. \ + `if x == true`" +} + #[derive(Copy,Clone)] pub struct NeedlessBool; @@ -78,6 +93,57 @@ impl LateLintPass for NeedlessBool { } } +#[derive(Copy,Clone)] +pub struct BoolComparison; + +impl LintPass for BoolComparison { + fn get_lints(&self) -> LintArray { + lint_array!(BOOL_COMPARISON) + } +} + +impl LateLintPass for BoolComparison { + fn check_expr(&mut self, cx: &LateContext, e: &Expr) { + if let ExprBinary(Spanned{ node: BiEq, .. }, ref left_side, ref right_side) = e.node { + match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { + (Some(true), None) => { + let side_snip = snippet(cx, right_side.span, ".."); + let hint = format!("`{}`", side_snip); + span_lint(cx, + BOOL_COMPARISON, + e.span, + &format!("you can simplify this boolean comparison to {}", hint)); + } + (None, Some(true)) => { + let side_snip = snippet(cx, left_side.span, ".."); + let hint = format!("`{}`", side_snip); + span_lint(cx, + BOOL_COMPARISON, + e.span, + &format!("you can simplify this boolean comparison to {}", hint)); + } + (Some(false), None) => { + let side_snip = snippet(cx, right_side.span, ".."); + let hint = format!("`!{}`", side_snip); + span_lint(cx, + BOOL_COMPARISON, + e.span, + &format!("you can simplify this boolean comparison to {}", hint)); + } + (None, Some(false)) => { + let side_snip = snippet(cx, left_side.span, ".."); + let hint = format!("`!{}`", side_snip); + span_lint(cx, + BOOL_COMPARISON, + e.span, + &format!("you can simplify this boolean comparison to {}", hint)); + } + _ => (), + } + } + } +} + fn fetch_bool_block(block: &Block) -> Option { if block.stmts.is_empty() { block.expr.as_ref().and_then(|e| fetch_bool_expr(e)) diff --git a/tests/compile-fail/bool_comparison.rs b/tests/compile-fail/bool_comparison.rs new file mode 100644 index 00000000000..d2a362af2f4 --- /dev/null +++ b/tests/compile-fail/bool_comparison.rs @@ -0,0 +1,12 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[allow(needless_bool)] +#[deny(bool_comparison)] +fn main() { + let x = true; + if x == true { true } else { false }; //~ERROR you can simplify this boolean comparison to `x` + if x == false { true } else { false }; //~ERROR you can simplify this boolean comparison to `!x` + if true == x { true } else { false }; //~ERROR you can simplify this boolean comparison to `x` + if false == x { true } else { false }; //~ERROR you can simplify this boolean comparison to `!x` +} From 14292674b0cc828ced88731d15c86035045fa206 Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Tue, 9 Feb 2016 14:44:42 -0500 Subject: [PATCH 2/4] display suggestion separately from lint --- src/needless_bool.rs | 46 +++++++++++++++++---------- tests/compile-fail/bool_comparison.rs | 21 +++++++++--- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/needless_bool.rs b/src/needless_bool.rs index a2a0d13e17c..a40b4b3a252 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -8,7 +8,7 @@ use rustc_front::hir::*; use syntax::ast::Lit_; use syntax::codemap::Spanned; -use utils::{span_lint, snippet}; +use utils::{span_lint, span_lint_and_then, snippet}; /// **What it does:** This lint checks for expressions of the form `if c { true } else { false }` (or vice versa) and suggest using the condition directly. /// @@ -109,34 +109,46 @@ impl LateLintPass for BoolComparison { (Some(true), None) => { let side_snip = snippet(cx, right_side.span, ".."); let hint = format!("`{}`", side_snip); - span_lint(cx, - BOOL_COMPARISON, - e.span, - &format!("you can simplify this boolean comparison to {}", hint)); + span_lint_and_then(cx, + BOOL_COMPARISON, + e.span, + "equality checks against booleans are unnecesary", + |db| { + db.span_suggestion(e.span, "try simplifying it:", hint); + }); } (None, Some(true)) => { let side_snip = snippet(cx, left_side.span, ".."); let hint = format!("`{}`", side_snip); - span_lint(cx, - BOOL_COMPARISON, - e.span, - &format!("you can simplify this boolean comparison to {}", hint)); + span_lint_and_then(cx, + BOOL_COMPARISON, + e.span, + "equality checks against booleans are unnecesary", + |db| { + db.span_suggestion(e.span, "try simplifying it:", hint); + }); } (Some(false), None) => { let side_snip = snippet(cx, right_side.span, ".."); let hint = format!("`!{}`", side_snip); - span_lint(cx, - BOOL_COMPARISON, - e.span, - &format!("you can simplify this boolean comparison to {}", hint)); + span_lint_and_then(cx, + BOOL_COMPARISON, + e.span, + "equality checks against booleans are unnecesary", + |db| { + db.span_suggestion(e.span, "try simplifying it:", hint); + }); } (None, Some(false)) => { let side_snip = snippet(cx, left_side.span, ".."); let hint = format!("`!{}`", side_snip); - span_lint(cx, - BOOL_COMPARISON, - e.span, - &format!("you can simplify this boolean comparison to {}", hint)); + span_lint_and_then(cx, + BOOL_COMPARISON, + e.span, + "equality checks against booleans are unnecesary", + |db| { + db.span_suggestion(e.span, "try simplifying it:", hint); + }); } _ => (), } diff --git a/tests/compile-fail/bool_comparison.rs b/tests/compile-fail/bool_comparison.rs index d2a362af2f4..468b8382e94 100644 --- a/tests/compile-fail/bool_comparison.rs +++ b/tests/compile-fail/bool_comparison.rs @@ -1,12 +1,23 @@ #![feature(plugin)] #![plugin(clippy)] -#[allow(needless_bool)] #[deny(bool_comparison)] fn main() { let x = true; - if x == true { true } else { false }; //~ERROR you can simplify this boolean comparison to `x` - if x == false { true } else { false }; //~ERROR you can simplify this boolean comparison to `!x` - if true == x { true } else { false }; //~ERROR you can simplify this boolean comparison to `x` - if false == x { true } else { false }; //~ERROR you can simplify this boolean comparison to `!x` + if x == true { "yes" } else { "no" }; + //~^ ERROR equality checks against booleans are unnecesary + //~| HELP try simplifying it: + //~| SUGGESTION x + if x == false { "yes" } else { "no" }; + //~^ ERROR equality checks against booleans are unnecesary + //~| HELP try simplifying it: + //~| SUGGESTION !x + if true == x { "yes" } else { "no" }; + //~^ ERROR equality checks against booleans are unnecesary + //~| HELP try simplifying it: + //~| SUGGESTION x + if false == x { "yes" } else { "no" }; + //~^ ERROR equality checks against booleans are unnecesary + //~| HELP try simplifying it: + //~| SUGGESTION !x } From 2687a3f6b590f7175d87a95dd49ccf2f11c9d2e3 Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Tue, 9 Feb 2016 14:52:20 -0500 Subject: [PATCH 3/4] Update lints --- README.md | 3 ++- src/lib.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index cbb8646c672..4f5cdf30606 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 117 lints included in this crate: +There are 118 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -15,6 +15,7 @@ name [bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask) | warn | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have) [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr) | warn | braces can be eliminated in conditions that are expressions, e.g `if { true } ...` [block_in_if_condition_stmt](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_stmt) | warn | avoid complex blocks in conditions, instead move the block higher and bind it with 'let'; e.g: `if { let x = true; x } ...` +[bool_comparison](https://github.com/Manishearth/rust-clippy/wiki#bool_comparison) | warn | comparing a variable to a boolean, e.g. `if x == true` [box_vec](https://github.com/Manishearth/rust-clippy/wiki#box_vec) | warn | usage of `Box>`, vector elements are already on the heap [boxed_local](https://github.com/Manishearth/rust-clippy/wiki#boxed_local) | warn | using Box where unnecessary [cast_possible_truncation](https://github.com/Manishearth/rust-clippy/wiki#cast_possible_truncation) | allow | casts that may cause truncation of the value, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32` diff --git a/src/lib.rs b/src/lib.rs index c1e8c3c9f01..3485b83a18b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -253,8 +253,8 @@ pub fn plugin_registrar(reg: &mut Registry) { misc_early::UNNEEDED_FIELD_PATTERN, mut_reference::UNNECESSARY_MUT_PASSED, mutex_atomic::MUTEX_ATOMIC, - needless_bool::NEEDLESS_BOOL, needless_bool::BOOL_COMPARISON, + needless_bool::NEEDLESS_BOOL, needless_features::UNSTABLE_AS_MUT_SLICE, needless_features::UNSTABLE_AS_SLICE, needless_update::NEEDLESS_UPDATE, From 7e06737d6f54ed6ecae339c625a5a2e2d679ad7e Mon Sep 17 00:00:00 2001 From: Joshua Holmer Date: Tue, 9 Feb 2016 15:44:07 -0500 Subject: [PATCH 4/4] Improve testing and suggestion messages on bool_comparison --- src/needless_bool.rs | 28 ++++++++++++--------------- tests/compile-fail/bool_comparison.rs | 24 +++++++++++------------ 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/needless_bool.rs b/src/needless_bool.rs index a40b4b3a252..5382b8f0f04 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -107,47 +107,43 @@ impl LateLintPass for BoolComparison { if let ExprBinary(Spanned{ node: BiEq, .. }, ref left_side, ref right_side) = e.node { match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { (Some(true), None) => { - let side_snip = snippet(cx, right_side.span, ".."); - let hint = format!("`{}`", side_snip); + let hint = format!("{}", snippet(cx, right_side.span, "..")); span_lint_and_then(cx, BOOL_COMPARISON, e.span, - "equality checks against booleans are unnecesary", + "equality checks against true are unnecesary", |db| { - db.span_suggestion(e.span, "try simplifying it:", hint); + db.span_suggestion(e.span, "try simplifying it as shown:", hint); }); } (None, Some(true)) => { - let side_snip = snippet(cx, left_side.span, ".."); - let hint = format!("`{}`", side_snip); + let hint = format!("{}", snippet(cx, left_side.span, "..")); span_lint_and_then(cx, BOOL_COMPARISON, e.span, - "equality checks against booleans are unnecesary", + "equality checks against true are unnecesary", |db| { - db.span_suggestion(e.span, "try simplifying it:", hint); + db.span_suggestion(e.span, "try simplifying it as shown:", hint); }); } (Some(false), None) => { - let side_snip = snippet(cx, right_side.span, ".."); - let hint = format!("`!{}`", side_snip); + let hint = format!("!{}", snippet(cx, right_side.span, "..")); span_lint_and_then(cx, BOOL_COMPARISON, e.span, - "equality checks against booleans are unnecesary", + "equality checks against false can be replaced by a negation", |db| { - db.span_suggestion(e.span, "try simplifying it:", hint); + db.span_suggestion(e.span, "try simplifying it as shown:", hint); }); } (None, Some(false)) => { - let side_snip = snippet(cx, left_side.span, ".."); - let hint = format!("`!{}`", side_snip); + let hint = format!("!{}", snippet(cx, left_side.span, "..")); span_lint_and_then(cx, BOOL_COMPARISON, e.span, - "equality checks against booleans are unnecesary", + "equality checks against false can be replaced by a negation", |db| { - db.span_suggestion(e.span, "try simplifying it:", hint); + db.span_suggestion(e.span, "try simplifying it as shown:", hint); }); } _ => (), diff --git a/tests/compile-fail/bool_comparison.rs b/tests/compile-fail/bool_comparison.rs index 468b8382e94..8a792931d02 100644 --- a/tests/compile-fail/bool_comparison.rs +++ b/tests/compile-fail/bool_comparison.rs @@ -5,19 +5,19 @@ fn main() { let x = true; if x == true { "yes" } else { "no" }; - //~^ ERROR equality checks against booleans are unnecesary - //~| HELP try simplifying it: - //~| SUGGESTION x + //~^ ERROR equality checks against true are unnecesary + //~| HELP try simplifying it as shown: + //~| SUGGESTION if x { "yes" } else { "no" }; if x == false { "yes" } else { "no" }; - //~^ ERROR equality checks against booleans are unnecesary - //~| HELP try simplifying it: - //~| SUGGESTION !x + //~^ ERROR equality checks against false can be replaced by a negation + //~| HELP try simplifying it as shown: + //~| SUGGESTION if !x { "yes" } else { "no" }; if true == x { "yes" } else { "no" }; - //~^ ERROR equality checks against booleans are unnecesary - //~| HELP try simplifying it: - //~| SUGGESTION x + //~^ ERROR equality checks against true are unnecesary + //~| HELP try simplifying it as shown: + //~| SUGGESTION if x { "yes" } else { "no" }; if false == x { "yes" } else { "no" }; - //~^ ERROR equality checks against booleans are unnecesary - //~| HELP try simplifying it: - //~| SUGGESTION !x + //~^ ERROR equality checks against false can be replaced by a negation + //~| HELP try simplifying it as shown: + //~| SUGGESTION if !x { "yes" } else { "no" }; }