From 393014805986a1360d9b2ec4fd54137839777dbe Mon Sep 17 00:00:00 2001 From: Lucas Lois Date: Fri, 30 Nov 2018 10:21:11 -0300 Subject: [PATCH] Adds inequality cases to bool comparison lint The lint now checks cases like `y != true` --- clippy_lints/src/needless_bool.rs | 130 +++++++++++++++--------------- tests/ui/bool_comparison.rs | 4 + tests/ui/bool_comparison.stderr | 26 +++++- 3 files changed, 95 insertions(+), 65 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index e3db1f79ed7..3bb87fbf5e9 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -18,7 +18,7 @@ use crate::rustc_errors::Applicability; use crate::syntax::ast::LitKind; use crate::syntax::source_map::Spanned; use crate::utils::sugg::Sugg; -use crate::utils::{in_macro, snippet_with_applicability, span_lint, span_lint_and_sugg}; +use crate::utils::{in_macro, span_lint, span_lint_and_sugg}; /// **What it does:** Checks for expressions of the form `if c { true } else { /// false }` @@ -45,8 +45,8 @@ declare_clippy_lint! { "if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }`" } -/// **What it does:** Checks for expressions of the form `x == true` (or vice -/// versa) and suggest using the variable directly. +/// **What it does:** Checks for expressions of the form `x == true` and +/// `x != true` (or vice versa) and suggest using the variable directly. /// /// **Why is this bad?** Unnecessary code. /// @@ -59,7 +59,7 @@ declare_clippy_lint! { declare_clippy_lint! { pub BOOL_COMPARISON, complexity, - "comparing a variable to a boolean, e.g. `if x == true`" + "comparing a variable to a boolean, e.g. `if x == true` or `if x != true`" } #[derive(Copy, Clone)] @@ -138,76 +138,78 @@ impl LintPass for BoolComparison { impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { - use self::Expression::*; - if in_macro(e.span) { return; } - if let ExprKind::Binary( - Spanned { - node: BinOpKind::Eq, .. - }, - ref left_side, - ref right_side, - ) = e.node - { - let mut applicability = Applicability::MachineApplicable; - match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { - (Bool(true), Other) => { - let hint = snippet_with_applicability(cx, right_side.span, "..", &mut applicability); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint.to_string(), - applicability, - ); - }, - (Other, Bool(true)) => { - let hint = snippet_with_applicability(cx, left_side.span, "..", &mut applicability); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint.to_string(), - applicability, - ); - }, - (Bool(false), Other) => { - let hint = Sugg::hir_with_applicability(cx, right_side, "..", &mut applicability); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against false can be replaced by a negation", - "try simplifying it as shown", - (!hint).to_string(), - applicability, - ); - }, - (Other, Bool(false)) => { - let hint = Sugg::hir_with_applicability(cx, left_side, "..", &mut applicability); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against false can be replaced by a negation", - "try simplifying it as shown", - (!hint).to_string(), - applicability, - ); - }, + if let ExprKind::Binary(Spanned { node, .. }, ..) = e.node { + match node { + BinOpKind::Eq => check_comparison( + cx, + e, + "equality checks against true are unnecessary", + "equality checks against false can be replaced by a negation", + |h| h, + |h| !h, + ), + BinOpKind::Ne => check_comparison( + cx, + e, + "inequality checks against true can be replaced by a negation", + "inequality checks against false are unnecessary", + |h| !h, + |h| h, + ), _ => (), } } } } +fn check_comparison<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + e: &'tcx Expr, + true_message: &str, + false_message: &str, + true_hint: impl FnOnce(Sugg<'_>) -> Sugg<'_>, + false_hint: impl FnOnce(Sugg<'_>) -> Sugg<'_>, +) { + use self::Expression::*; + + if let ExprKind::Binary(_, ref left_side, ref right_side) = e.node { + let applicability = Applicability::MachineApplicable; + match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { + (Bool(true), Other) => suggest_bool_comparison(cx, e, right_side, applicability, true_message, true_hint), + (Other, Bool(true)) => suggest_bool_comparison(cx, e, left_side, applicability, true_message, true_hint), + (Bool(false), Other) => { + suggest_bool_comparison(cx, e, right_side, applicability, false_message, false_hint) + }, + (Other, Bool(false)) => suggest_bool_comparison(cx, e, left_side, applicability, false_message, false_hint), + _ => (), + } + } +} + +fn suggest_bool_comparison<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + e: &'tcx Expr, + expr: &Expr, + mut applicability: Applicability, + message: &str, + conv_hint: impl FnOnce(Sugg<'_>) -> Sugg<'_>, +) { + let hint = Sugg::hir_with_applicability(cx, expr, "..", &mut applicability); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + message, + "try simplifying it as shown", + conv_hint(hint).to_string(), + applicability, + ); +} + enum Expression { Bool(bool), RetBool(bool), diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index c213414a63d..8ab8b3f9281 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -18,4 +18,8 @@ fn main() { if x == false { "yes" } else { "no" }; if true == x { "yes" } else { "no" }; if false == x { "yes" } else { "no" }; + if x != true { "yes" } else { "no" }; + if x != false { "yes" } else { "no" }; + if true != x { "yes" } else { "no" }; + if false != x { "yes" } else { "no" }; } diff --git a/tests/ui/bool_comparison.stderr b/tests/ui/bool_comparison.stderr index f1bb50fae9e..b4a1545b49e 100644 --- a/tests/ui/bool_comparison.stderr +++ b/tests/ui/bool_comparison.stderr @@ -24,5 +24,29 @@ error: equality checks against false can be replaced by a negation 20 | if false == x { "yes" } else { "no" }; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` -error: aborting due to 4 previous errors +error: inequality checks against true can be replaced by a negation + --> $DIR/bool_comparison.rs:21:8 + | +21 | if x != true { "yes" } else { "no" }; + | ^^^^^^^^^ help: try simplifying it as shown: `!x` + +error: inequality checks against false are unnecessary + --> $DIR/bool_comparison.rs:22:8 + | +22 | if x != false { "yes" } else { "no" }; + | ^^^^^^^^^^ help: try simplifying it as shown: `x` + +error: inequality checks against true can be replaced by a negation + --> $DIR/bool_comparison.rs:23:8 + | +23 | if true != x { "yes" } else { "no" }; + | ^^^^^^^^^ help: try simplifying it as shown: `!x` + +error: inequality checks against false are unnecessary + --> $DIR/bool_comparison.rs:24:8 + | +24 | if false != x { "yes" } else { "no" }; + | ^^^^^^^^^^ help: try simplifying it as shown: `x` + +error: aborting due to 8 previous errors