From 7cfde9cfa95b0c466efe867e064d05fd8adea568 Mon Sep 17 00:00:00 2001 From: Giorgio Gambino Date: Sun, 28 Oct 2018 15:37:39 +0100 Subject: [PATCH] Fix #3335: bool_comparison triggers 3 times on same code --- clippy_lints/src/needless_bool.rs | 100 +++++++++++++++--------------- tests/ui/needless_bool.rs | 49 ++++++++++++++- tests/ui/needless_bool.stderr | 94 ++++++++++++++++++---------- 3 files changed, 158 insertions(+), 85 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index f102b49d785..3afccf9f984 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -17,7 +17,7 @@ use crate::rustc::{declare_tool_lint, lint_array}; use crate::rustc::hir::*; use crate::syntax::ast::LitKind; use crate::syntax::source_map::Spanned; -use crate::utils::{snippet, span_lint, span_lint_and_sugg}; +use crate::utils::{in_macro, snippet, span_lint, span_lint_and_sugg}; use crate::utils::sugg::Sugg; /// **What it does:** Checks for expressions of the form `if c { true } else { @@ -133,54 +133,56 @@ 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 let ExprKind::Binary(Spanned { node: BinOpKind::Eq, .. }, ref left_side, ref right_side) = e.node { - match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { - (Bool(true), Other) => { - let hint = snippet(cx, right_side.span, "..").into_owned(); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint, - ); - }, - (Other, Bool(true)) => { - let hint = snippet(cx, left_side.span, "..").into_owned(); - span_lint_and_sugg( - cx, - BOOL_COMPARISON, - e.span, - "equality checks against true are unnecessary", - "try simplifying it as shown", - hint, - ); - }, - (Bool(false), Other) => { - let hint = Sugg::hir(cx, right_side, ".."); - 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(), - ); - }, - (Other, Bool(false)) => { - let hint = Sugg::hir(cx, left_side, ".."); - 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(), - ); - }, - _ => (), + if !in_macro(e.span) { + use self::Expression::*; + if let ExprKind::Binary(Spanned { node: BinOpKind::Eq, .. }, ref left_side, ref right_side) = e.node { + match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { + (Bool(true), Other) => { + let hint = snippet(cx, right_side.span, "..").into_owned(); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against true are unnecessary", + "try simplifying it as shown", + hint, + ); + }, + (Other, Bool(true)) => { + let hint = snippet(cx, left_side.span, "..").into_owned(); + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "equality checks against true are unnecessary", + "try simplifying it as shown", + hint, + ); + }, + (Bool(false), Other) => { + let hint = Sugg::hir(cx, right_side, ".."); + 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(), + ); + }, + (Other, Bool(false)) => { + let hint = Sugg::hir(cx, left_side, ".."); + 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(), + ); + }, + _ => (), + } } } } diff --git a/tests/ui/needless_bool.rs b/tests/ui/needless_bool.rs index a9a2e3709f1..aca4ccabf0e 100644 --- a/tests/ui/needless_bool.rs +++ b/tests/ui/needless_bool.rs @@ -8,10 +8,32 @@ // except according to those terms. - - #![warn(clippy::needless_bool)] +use std::cell::Cell; + +macro_rules! bool_comparison_trigger { + ($($i:ident: $def:expr, $stb:expr );+ $(;)*) => ( + + #[derive(Clone)] + pub struct Trigger { + $($i: (Cell, bool, bool)),+ + } + + #[allow(dead_code)] + impl Trigger { + pub fn trigger(&self, key: &str) -> bool { + $( + if let stringify!($i) = key { + return self.$i.1 && self.$i.2 == $def; + } + )+ + false + } + } + ) +} + #[allow(clippy::if_same_then_else)] fn main() { let x = true; @@ -28,6 +50,9 @@ fn main() { bool_ret5(x, x); bool_ret4(x); bool_ret6(x, x); + needless_bool(x); + needless_bool2(x); + needless_bool3(x); } #[allow(clippy::if_same_then_else, clippy::needless_return)] @@ -59,3 +84,23 @@ fn bool_ret4(x: bool) -> bool { fn bool_ret6(x: bool, y: bool) -> bool { if x && y { return false } else { return true }; } + +fn needless_bool(x: bool) { + if x == true { }; +} + +fn needless_bool2(x: bool) { + if x == false { }; +} + +fn needless_bool3(x: bool) { + + bool_comparison_trigger! { + test_one: false, false; + test_three: false, false; + test_two: true, true; + } + + if x == true { }; + if x == false { }; +} \ No newline at end of file diff --git a/tests/ui/needless_bool.stderr b/tests/ui/needless_bool.stderr index 13af6fc3564..638a3f56f0f 100644 --- a/tests/ui/needless_bool.stderr +++ b/tests/ui/needless_bool.stderr @@ -1,70 +1,96 @@ error: this if-then-else expression will always return true - --> $DIR/needless_bool.rs:19:5 + --> $DIR/needless_bool.rs:41:5 | -19 | if x { true } else { true }; +41 | if x { true } else { true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::needless-bool` implied by `-D warnings` error: this if-then-else expression will always return false - --> $DIR/needless_bool.rs:20:5 + --> $DIR/needless_bool.rs:42:5 | -20 | if x { false } else { false }; +42 | if x { false } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:21:5 + --> $DIR/needless_bool.rs:43:5 | -21 | if x { true } else { false }; +43 | if x { true } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:22:5 + --> $DIR/needless_bool.rs:44:5 | -22 | if x { false } else { true }; +44 | if x { false } else { true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `!x` -error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:23:5 - | -23 | if x && y { false } else { true }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `!(x && y)` - -error: this if-then-else expression will always return true - --> $DIR/needless_bool.rs:35:5 - | -35 | if x { return true } else { return true }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: this if-then-else expression will always return false - --> $DIR/needless_bool.rs:40:5 - | -40 | if x { return false } else { return false }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - error: this if-then-else expression returns a bool literal --> $DIR/needless_bool.rs:45:5 | -45 | if x { return true } else { return false }; +45 | if x && y { false } else { true }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `!(x && y)` + +error: this if-then-else expression will always return true + --> $DIR/needless_bool.rs:60:5 + | +60 | if x { return true } else { return true }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this if-then-else expression will always return false + --> $DIR/needless_bool.rs:65:5 + | +65 | if x { return false } else { return false }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this if-then-else expression returns a bool literal + --> $DIR/needless_bool.rs:70:5 + | +70 | if x { return true } else { return false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:50:5 + --> $DIR/needless_bool.rs:75:5 | -50 | if x && y { return true } else { return false }; +75 | if x && y { return true } else { return false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return x && y` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:55:5 + --> $DIR/needless_bool.rs:80:5 | -55 | if x { return false } else { return true }; +80 | if x { return false } else { return true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return !x` error: this if-then-else expression returns a bool literal - --> $DIR/needless_bool.rs:60:5 + --> $DIR/needless_bool.rs:85:5 | -60 | if x && y { return false } else { return true }; +85 | if x && y { return false } else { return true }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `return !(x && y)` -error: aborting due to 11 previous errors +error: equality checks against true are unnecessary + --> $DIR/needless_bool.rs:89:7 + | +89 | if x == true { }; + | ^^^^^^^^^^ help: try simplifying it as shown: `x` + | + = note: `-D clippy::bool-comparison` implied by `-D warnings` + +error: equality checks against false can be replaced by a negation + --> $DIR/needless_bool.rs:93:7 + | +93 | if x == false { }; + | ^^^^^^^^^^^ help: try simplifying it as shown: `!x` + +error: equality checks against true are unnecessary + --> $DIR/needless_bool.rs:104:8 + | +104 | if x == true { }; + | ^^^^^^^^^ help: try simplifying it as shown: `x` + +error: equality checks against false can be replaced by a negation + --> $DIR/needless_bool.rs:105:8 + | +105 | if x == false { }; + | ^^^^^^^^^^ help: try simplifying it as shown: `!x` + +error: aborting due to 15 previous errors