From 12796cd6886989c449e62818fb8f0bb40b9ce41e Mon Sep 17 00:00:00 2001 From: mgr-inz-rafal Date: Mon, 23 Mar 2020 20:29:12 +0100 Subject: [PATCH 1/4] Initial lint without suggestion --- clippy_lints/src/needless_bool.rs | 29 ++++++++++++++++++++++++++--- tests/ui/bool_comparison.fixed | 10 ++++++++++ tests/ui/bool_comparison.rs | 10 ++++++++++ tests/ui/bool_comparison.stderr | 18 +++++++++++++++++- 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 19c22d2b791..8de2fe2f3ba 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -3,10 +3,11 @@ //! This lint is **warn** by default use crate::utils::sugg::Sugg; -use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_sugg}; +use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_help, span_lint_and_sugg}; +use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Spanned; @@ -188,6 +189,21 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { } } +fn is_unary_not<'tcx>(e: &'tcx Expr<'_>) -> bool { + if_chain! { + if let ExprKind::Unary(unop, _) = e.kind; + if let UnOp::UnNot = unop; + then { + return true; + } + }; + false +} + +fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> bool { + is_unary_not(left_side) ^ is_unary_not(right_side) +} + fn check_comparison<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, e: &'tcx Expr<'_>, @@ -199,9 +215,16 @@ fn check_comparison<'a, 'tcx>( ) { use self::Expression::{Bool, Other}; - if let ExprKind::Binary(_, ref left_side, ref right_side) = e.kind { + if let ExprKind::Binary(op, ref left_side, ref right_side) = e.kind { let (l_ty, r_ty) = (cx.tables.expr_ty(left_side), cx.tables.expr_ty(right_side)); if l_ty.is_bool() && r_ty.is_bool() { + if_chain! { + if let BinOpKind::Eq = op.node; + if one_side_is_unary_not(&left_side, &right_side); + then { + span_lint_and_help(cx, BOOL_COMPARISON, e.span, "Here comes", "the suggestion"); + } + }; let mut applicability = Applicability::MachineApplicable; match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { (Bool(true), Other) => left_true.map_or((), |(h, m)| { diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index 0bd73ec2c10..d217d03ead4 100644 --- a/tests/ui/bool_comparison.fixed +++ b/tests/ui/bool_comparison.fixed @@ -111,3 +111,13 @@ fn issue3703() { if Foo < false {} if false < Foo {} } + +fn issue4983() { + let a = true; + let b = false; + + if a == !b {}; + if !a == b {}; + if a == b {}; + if !a == !b {}; +} diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index 74f504edfd0..c13575eae71 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -111,3 +111,13 @@ fn issue3703() { if Foo < false {} if false < Foo {} } + +fn issue4983() { + let a = true; + let b = false; + + if a == !b {}; + if !a == b {}; + if a == b {}; + if !a == !b {}; +} diff --git a/tests/ui/bool_comparison.stderr b/tests/ui/bool_comparison.stderr index 2aa070a00f3..cec7b196a23 100644 --- a/tests/ui/bool_comparison.stderr +++ b/tests/ui/bool_comparison.stderr @@ -84,5 +84,21 @@ error: order comparisons between booleans can be simplified LL | if x > y { | ^^^^^ help: try simplifying it as shown: `x & !y` -error: aborting due to 14 previous errors +error: Here comes + --> $DIR/bool_comparison.rs:119:8 + | +LL | if a == !b {}; + | ^^^^^^^ + | + = help: the suggestion + +error: Here comes + --> $DIR/bool_comparison.rs:120:8 + | +LL | if !a == b {}; + | ^^^^^^^ + | + = help: the suggestion + +error: aborting due to 16 previous errors From 3d3af078454bcd10980af28a04e172cc1f6bc9c8 Mon Sep 17 00:00:00 2001 From: mgr-inz-rafal Date: Mon, 23 Mar 2020 21:00:02 +0100 Subject: [PATCH 2/4] Provide appropriate suggestion --- clippy_lints/src/needless_bool.rs | 45 +++++++++++++++++++++---------- tests/ui/bool_comparison.fixed | 4 +-- tests/ui/bool_comparison.rs | 5 ++++ tests/ui/bool_comparison.stderr | 12 +++------ 4 files changed, 42 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 8de2fe2f3ba..a93c55f2d59 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -3,7 +3,7 @@ //! This lint is **warn** by default use crate::utils::sugg::Sugg; -use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_help, span_lint_and_sugg}; +use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_help, span_lint_and_sugg, snippet_with_applicability}; use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; @@ -189,19 +189,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { } } -fn is_unary_not<'tcx>(e: &'tcx Expr<'_>) -> bool { +fn is_unary_not<'tcx>(e: &'tcx Expr<'_>) -> (bool, rustc_span::Span) { if_chain! { - if let ExprKind::Unary(unop, _) = e.kind; + if let ExprKind::Unary(unop, operand) = e.kind; if let UnOp::UnNot = unop; then { - return true; + return (true, operand.span); } }; - false + (false, e.span) } -fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> bool { - is_unary_not(left_side) ^ is_unary_not(right_side) +fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> (bool, rustc_span::Span, rustc_span::Span) { + let left = is_unary_not(left_side); + let right = is_unary_not(right_side); + + let retval = left.0 ^ right.0; + (retval, left.1, right.1) } fn check_comparison<'a, 'tcx>( @@ -218,14 +222,27 @@ fn check_comparison<'a, 'tcx>( if let ExprKind::Binary(op, ref left_side, ref right_side) = e.kind { let (l_ty, r_ty) = (cx.tables.expr_ty(left_side), cx.tables.expr_ty(right_side)); if l_ty.is_bool() && r_ty.is_bool() { - if_chain! { - if let BinOpKind::Eq = op.node; - if one_side_is_unary_not(&left_side, &right_side); - then { - span_lint_and_help(cx, BOOL_COMPARISON, e.span, "Here comes", "the suggestion"); - } - }; let mut applicability = Applicability::MachineApplicable; + + if let BinOpKind::Eq = op.node + { + let xxx = one_side_is_unary_not(&left_side, &right_side); + if xxx.0 + { + span_lint_and_sugg( + cx, + BOOL_COMPARISON, + e.span, + "This comparison might be written more concisely", + "try simplifying it as shown", + format!("{} != {}", + snippet_with_applicability(cx, xxx.1, "..", &mut applicability), + snippet_with_applicability(cx, xxx.2, "..", &mut applicability)), + applicability, + ) + } + } + match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) { (Bool(true), Other) => left_true.map_or((), |(h, m)| { suggest_bool_comparison(cx, e, right_side, applicability, m, h) diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index d217d03ead4..2081dea36a7 100644 --- a/tests/ui/bool_comparison.fixed +++ b/tests/ui/bool_comparison.fixed @@ -116,8 +116,8 @@ fn issue4983() { let a = true; let b = false; - if a == !b {}; - if !a == b {}; + if a != b {}; + if a != b {}; if a == b {}; if !a == !b {}; } diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index c13575eae71..f463ac1c883 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -120,4 +120,9 @@ fn issue4983() { if !a == b {}; if a == b {}; if !a == !b {}; + + if b == !a {}; + if !b == a {}; + if b == a {}; + if !b == !a {}; } diff --git a/tests/ui/bool_comparison.stderr b/tests/ui/bool_comparison.stderr index cec7b196a23..a2a89fc3287 100644 --- a/tests/ui/bool_comparison.stderr +++ b/tests/ui/bool_comparison.stderr @@ -84,21 +84,17 @@ error: order comparisons between booleans can be simplified LL | if x > y { | ^^^^^ help: try simplifying it as shown: `x & !y` -error: Here comes +error: This comparison might be written more concisely --> $DIR/bool_comparison.rs:119:8 | LL | if a == !b {}; - | ^^^^^^^ - | - = help: the suggestion + | ^^^^^^^ help: try simplifying it as shown: `a != b` -error: Here comes +error: This comparison might be written more concisely --> $DIR/bool_comparison.rs:120:8 | LL | if !a == b {}; - | ^^^^^^^ - | - = help: the suggestion + | ^^^^^^^ help: try simplifying it as shown: `a != b` error: aborting due to 16 previous errors From ff9602515e4a6ca0792cead4102b8aa1f5cb8a34 Mon Sep 17 00:00:00 2001 From: mgr-inz-rafal Date: Mon, 23 Mar 2020 21:21:18 +0100 Subject: [PATCH 3/4] Code clean-up and formatting --- clippy_lints/src/needless_bool.rs | 36 ++++++++++++++++++++----------- tests/ui/bool_comparison.fixed | 6 ++++++ tests/ui/bool_comparison.rs | 1 + tests/ui/bool_comparison.stderr | 18 +++++++++++++--- 4 files changed, 45 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index a93c55f2d59..3060d6496a8 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -3,7 +3,7 @@ //! This lint is **warn** by default use crate::utils::sugg::Sugg; -use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_help, span_lint_and_sugg, snippet_with_applicability}; +use crate::utils::{higher, parent_node_is_if_expr, snippet_with_applicability, span_lint, span_lint_and_sugg}; use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; @@ -11,6 +11,7 @@ use rustc_hir::{BinOpKind, Block, Expr, ExprKind, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Spanned; +use rustc_span::Span; declare_clippy_lint! { /// **What it does:** Checks for expressions of the form `if c { true } else { @@ -189,7 +190,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { } } -fn is_unary_not<'tcx>(e: &'tcx Expr<'_>) -> (bool, rustc_span::Span) { +struct ExpressionInfoWithSpan { + one_side_is_unary_not: bool, + left_span: Span, + right_span: Span, +} + +fn is_unary_not(e: &Expr<'_>) -> (bool, Span) { if_chain! { if let ExprKind::Unary(unop, operand) = e.kind; if let UnOp::UnNot = unop; @@ -200,12 +207,15 @@ fn is_unary_not<'tcx>(e: &'tcx Expr<'_>) -> (bool, rustc_span::Span) { (false, e.span) } -fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> (bool, rustc_span::Span, rustc_span::Span) { +fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr<'_>) -> ExpressionInfoWithSpan { let left = is_unary_not(left_side); let right = is_unary_not(right_side); - let retval = left.0 ^ right.0; - (retval, left.1, right.1) + ExpressionInfoWithSpan { + one_side_is_unary_not: left.0 ^ right.0, + left_span: left.1, + right_span: right.1, + } } fn check_comparison<'a, 'tcx>( @@ -224,20 +234,20 @@ fn check_comparison<'a, 'tcx>( if l_ty.is_bool() && r_ty.is_bool() { let mut applicability = Applicability::MachineApplicable; - if let BinOpKind::Eq = op.node - { - let xxx = one_side_is_unary_not(&left_side, &right_side); - if xxx.0 - { + if let BinOpKind::Eq = op.node { + let expression_info = one_side_is_unary_not(&left_side, &right_side); + if expression_info.one_side_is_unary_not { span_lint_and_sugg( cx, BOOL_COMPARISON, e.span, "This comparison might be written more concisely", "try simplifying it as shown", - format!("{} != {}", - snippet_with_applicability(cx, xxx.1, "..", &mut applicability), - snippet_with_applicability(cx, xxx.2, "..", &mut applicability)), + format!( + "{} != {}", + snippet_with_applicability(cx, expression_info.left_span, "..", &mut applicability), + snippet_with_applicability(cx, expression_info.right_span, "..", &mut applicability) + ), applicability, ) } diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index 2081dea36a7..91211764759 100644 --- a/tests/ui/bool_comparison.fixed +++ b/tests/ui/bool_comparison.fixed @@ -112,6 +112,7 @@ fn issue3703() { if false < Foo {} } +#[allow(dead_code)] fn issue4983() { let a = true; let b = false; @@ -120,4 +121,9 @@ fn issue4983() { if a != b {}; if a == b {}; if !a == !b {}; + + if b != a {}; + if b != a {}; + if b == a {}; + if !b == !a {}; } diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index f463ac1c883..01ee35859f0 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -112,6 +112,7 @@ fn issue3703() { if false < Foo {} } +#[allow(dead_code)] fn issue4983() { let a = true; let b = false; diff --git a/tests/ui/bool_comparison.stderr b/tests/ui/bool_comparison.stderr index a2a89fc3287..eeb1f20ee89 100644 --- a/tests/ui/bool_comparison.stderr +++ b/tests/ui/bool_comparison.stderr @@ -85,16 +85,28 @@ LL | if x > y { | ^^^^^ help: try simplifying it as shown: `x & !y` error: This comparison might be written more concisely - --> $DIR/bool_comparison.rs:119:8 + --> $DIR/bool_comparison.rs:120:8 | LL | if a == !b {}; | ^^^^^^^ help: try simplifying it as shown: `a != b` error: This comparison might be written more concisely - --> $DIR/bool_comparison.rs:120:8 + --> $DIR/bool_comparison.rs:121:8 | LL | if !a == b {}; | ^^^^^^^ help: try simplifying it as shown: `a != b` -error: aborting due to 16 previous errors +error: This comparison might be written more concisely + --> $DIR/bool_comparison.rs:125:8 + | +LL | if b == !a {}; + | ^^^^^^^ help: try simplifying it as shown: `b != a` + +error: This comparison might be written more concisely + --> $DIR/bool_comparison.rs:126:8 + | +LL | if !b == a {}; + | ^^^^^^^ help: try simplifying it as shown: `b != a` + +error: aborting due to 18 previous errors From c8f32411777f8287cccb8fa9ab69df17496831af Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Mon, 30 Mar 2020 12:19:30 -0700 Subject: [PATCH 4/4] Update clippy_lints/src/needless_bool.rs --- clippy_lints/src/needless_bool.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index 3060d6496a8..efa77db822d 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -212,7 +212,7 @@ fn one_side_is_unary_not<'tcx>(left_side: &'tcx Expr<'_>, right_side: &'tcx Expr let right = is_unary_not(right_side); ExpressionInfoWithSpan { - one_side_is_unary_not: left.0 ^ right.0, + one_side_is_unary_not: left.0 != right.0, left_span: left.1, right_span: right.1, }