From a18e0a11f7a0a62a7fcaec178aad34f880b659dc Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Thu, 8 Feb 2024 22:36:41 +0100 Subject: [PATCH 1/2] Extend `NONMINIMAL_BOOL` lint --- clippy_lints/src/booleans.rs | 26 ++++++++++++++++++++++++++ tests/ui/nonminimal_bool.rs | 8 ++++++++ tests/ui/nonminimal_bool.stderr | 26 +++++++++++++++++++++++++- 3 files changed, 59 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 2d1c250ace90..3c17f65f0d3c 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -85,6 +85,32 @@ impl<'tcx> LateLintPass<'tcx> for NonminimalBool { ) { NonminimalBoolVisitor { cx }.visit_body(body); } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if let ExprKind::Unary(UnOp::Not, sub) = expr.kind + && !expr.span.from_expansion() + && let ExprKind::Binary(op, left, right) = sub.kind + { + let new_op = match op.node { + BinOpKind::Eq => "!=", + BinOpKind::Ne => "==", + _ => return, + }; + let Some(left) = snippet_opt(cx, left.span) else { return }; + let Some(right) = snippet_opt(cx, right.span) else { + return; + }; + span_lint_and_sugg( + cx, + NONMINIMAL_BOOL, + expr.span, + "this boolean expression can be simplified", + "try", + format!("{left} {new_op} {right}"), + Applicability::MachineApplicable, + ); + } + } } struct NonminimalBoolVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, diff --git a/tests/ui/nonminimal_bool.rs b/tests/ui/nonminimal_bool.rs index f7c3df7066f7..ee092b9aca69 100644 --- a/tests/ui/nonminimal_bool.rs +++ b/tests/ui/nonminimal_bool.rs @@ -156,3 +156,11 @@ fn issue11932() { x % 3 == 0 }; } + +fn issue_5794() { + let a = 0; + if !(12 == a) {} //~ ERROR: this boolean expression can be simplified + if !(a == 12) {} //~ ERROR: this boolean expression can be simplified + if !(12 != a) {} //~ ERROR: this boolean expression can be simplified + if !(a != 12) {} //~ ERROR: this boolean expression can be simplified +} diff --git a/tests/ui/nonminimal_bool.stderr b/tests/ui/nonminimal_bool.stderr index fd1568d94e3e..856b5cd08691 100644 --- a/tests/ui/nonminimal_bool.stderr +++ b/tests/ui/nonminimal_bool.stderr @@ -114,5 +114,29 @@ error: this boolean expression can be simplified LL | if matches!(true, true) && true { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `matches!(true, true)` -error: aborting due to 13 previous errors +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:162:8 + | +LL | if !(12 == a) {} + | ^^^^^^^^^^ help: try: `12 != a` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:163:8 + | +LL | if !(a == 12) {} + | ^^^^^^^^^^ help: try: `a != 12` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:164:8 + | +LL | if !(12 != a) {} + | ^^^^^^^^^^ help: try: `12 == a` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:165:8 + | +LL | if !(a != 12) {} + | ^^^^^^^^^^ help: try: `a == 12` + +error: aborting due to 17 previous errors From 5e7c437d415871052fdd92f0d402ac9fd050fefa Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 9 Feb 2024 20:47:31 +0100 Subject: [PATCH 2/2] Extend `NONMINIMAL_BOOL` to check inverted boolean values --- clippy_lints/src/booleans.rs | 128 ++++++++++++++++++++++++++------ tests/ui/bool_comparison.fixed | 2 +- tests/ui/bool_comparison.rs | 2 +- tests/ui/nonminimal_bool.rs | 9 +++ tests/ui/nonminimal_bool.stderr | 77 ++++++++++++++++++- 5 files changed, 193 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 3c17f65f0d3c..0d66f2d644d9 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -87,31 +87,115 @@ impl<'tcx> LateLintPass<'tcx> for NonminimalBool { } fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if let ExprKind::Unary(UnOp::Not, sub) = expr.kind - && !expr.span.from_expansion() - && let ExprKind::Binary(op, left, right) = sub.kind - { - let new_op = match op.node { - BinOpKind::Eq => "!=", - BinOpKind::Ne => "==", - _ => return, - }; - let Some(left) = snippet_opt(cx, left.span) else { return }; - let Some(right) = snippet_opt(cx, right.span) else { - return; - }; - span_lint_and_sugg( - cx, - NONMINIMAL_BOOL, - expr.span, - "this boolean expression can be simplified", - "try", - format!("{left} {new_op} {right}"), - Applicability::MachineApplicable, - ); + match expr.kind { + ExprKind::Unary(UnOp::Not, sub) => check_inverted_condition(cx, expr.span, sub), + // This check the case where an element in a boolean comparison is inverted, like: + // + // ``` + // let a = true; + // !a == false; + // ``` + ExprKind::Binary(op, left, right) if matches!(op.node, BinOpKind::Eq | BinOpKind::Ne) => { + check_inverted_bool_in_condition(cx, expr.span, op.node, left, right); + }, + _ => {}, } } } + +fn inverted_bin_op_eq_str(op: BinOpKind) -> Option<&'static str> { + match op { + BinOpKind::Eq => Some("!="), + BinOpKind::Ne => Some("=="), + _ => None, + } +} + +fn bin_op_eq_str(op: BinOpKind) -> Option<&'static str> { + match op { + BinOpKind::Eq => Some("=="), + BinOpKind::Ne => Some("!="), + _ => None, + } +} + +fn check_inverted_condition(cx: &LateContext<'_>, expr_span: Span, sub_expr: &Expr<'_>) { + if !expr_span.from_expansion() + && let ExprKind::Binary(op, left, right) = sub_expr.kind + && let Some(left) = snippet_opt(cx, left.span) + && let Some(right) = snippet_opt(cx, right.span) + { + let Some(op) = inverted_bin_op_eq_str(op.node) else { + return; + }; + span_lint_and_sugg( + cx, + NONMINIMAL_BOOL, + expr_span, + "this boolean expression can be simplified", + "try", + format!("{left} {op} {right}",), + Applicability::MachineApplicable, + ); + } +} + +fn check_inverted_bool_in_condition( + cx: &LateContext<'_>, + expr_span: Span, + op: BinOpKind, + left: &Expr<'_>, + right: &Expr<'_>, +) { + if expr_span.from_expansion() + && (!cx.typeck_results().node_types()[left.hir_id].is_bool() + || !cx.typeck_results().node_types()[right.hir_id].is_bool()) + { + return; + } + + let suggestion = match (left.kind, right.kind) { + (ExprKind::Unary(UnOp::Not, left_sub), ExprKind::Unary(UnOp::Not, right_sub)) => { + let Some(left) = snippet_opt(cx, left_sub.span) else { + return; + }; + let Some(right) = snippet_opt(cx, right_sub.span) else { + return; + }; + let Some(op) = bin_op_eq_str(op) else { return }; + format!("{left} {op} {right}") + }, + (ExprKind::Unary(UnOp::Not, left_sub), _) => { + let Some(left) = snippet_opt(cx, left_sub.span) else { + return; + }; + let Some(right) = snippet_opt(cx, right.span) else { + return; + }; + let Some(op) = inverted_bin_op_eq_str(op) else { return }; + format!("{left} {op} {right}") + }, + (_, ExprKind::Unary(UnOp::Not, right_sub)) => { + let Some(left) = snippet_opt(cx, left.span) else { return }; + let Some(right) = snippet_opt(cx, right_sub.span) else { + return; + }; + let Some(op) = inverted_bin_op_eq_str(op) else { return }; + format!("{left} {op} {right}") + }, + _ => return, + }; + span_lint_and_sugg( + cx, + NONMINIMAL_BOOL, + expr_span, + "this boolean expression can be simplified", + "try", + suggestion, + Applicability::MachineApplicable, + ); +} + struct NonminimalBoolVisitor<'a, 'tcx> { cx: &'a LateContext<'tcx>, } diff --git a/tests/ui/bool_comparison.fixed b/tests/ui/bool_comparison.fixed index 02f1d09b8339..54bdf7f5d70a 100644 --- a/tests/ui/bool_comparison.fixed +++ b/tests/ui/bool_comparison.fixed @@ -1,6 +1,6 @@ #![allow(clippy::needless_if)] #![warn(clippy::bool_comparison)] -#![allow(clippy::non_canonical_partial_ord_impl)] +#![allow(clippy::non_canonical_partial_ord_impl, clippy::nonminimal_bool)] fn main() { let x = true; diff --git a/tests/ui/bool_comparison.rs b/tests/ui/bool_comparison.rs index 5ef696d855ec..4fdf23052425 100644 --- a/tests/ui/bool_comparison.rs +++ b/tests/ui/bool_comparison.rs @@ -1,6 +1,6 @@ #![allow(clippy::needless_if)] #![warn(clippy::bool_comparison)] -#![allow(clippy::non_canonical_partial_ord_impl)] +#![allow(clippy::non_canonical_partial_ord_impl, clippy::nonminimal_bool)] fn main() { let x = true; diff --git a/tests/ui/nonminimal_bool.rs b/tests/ui/nonminimal_bool.rs index ee092b9aca69..908ef6cb2a02 100644 --- a/tests/ui/nonminimal_bool.rs +++ b/tests/ui/nonminimal_bool.rs @@ -163,4 +163,13 @@ fn issue_5794() { if !(a == 12) {} //~ ERROR: this boolean expression can be simplified if !(12 != a) {} //~ ERROR: this boolean expression can be simplified if !(a != 12) {} //~ ERROR: this boolean expression can be simplified + + let b = true; + let c = false; + if !b == true {} //~ ERROR: this boolean expression can be simplified + if !b != true {} //~ ERROR: this boolean expression can be simplified + if true == !b {} //~ ERROR: this boolean expression can be simplified + if true != !b {} //~ ERROR: this boolean expression can be simplified + if !b == !c {} //~ ERROR: this boolean expression can be simplified + if !b != !c {} //~ ERROR: this boolean expression can be simplified } diff --git a/tests/ui/nonminimal_bool.stderr b/tests/ui/nonminimal_bool.stderr index 856b5cd08691..a317c8328d94 100644 --- a/tests/ui/nonminimal_bool.stderr +++ b/tests/ui/nonminimal_bool.stderr @@ -138,5 +138,80 @@ error: this boolean expression can be simplified LL | if !(a != 12) {} | ^^^^^^^^^^ help: try: `a == 12` -error: aborting due to 17 previous errors +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:169:8 + | +LL | if !b == true {} + | ^^^^^^^^^^ help: try: `b != true` + +error: this comparison might be written more concisely + --> $DIR/nonminimal_bool.rs:169:8 + | +LL | if !b == true {} + | ^^^^^^^^^^ help: try simplifying it as shown: `b != true` + | + = note: `-D clippy::bool-comparison` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::bool_comparison)]` + +error: equality checks against true are unnecessary + --> $DIR/nonminimal_bool.rs:169:8 + | +LL | if !b == true {} + | ^^^^^^^^^^ help: try simplifying it as shown: `!b` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:170:8 + | +LL | if !b != true {} + | ^^^^^^^^^^ help: try: `b == true` + +error: inequality checks against true can be replaced by a negation + --> $DIR/nonminimal_bool.rs:170:8 + | +LL | if !b != true {} + | ^^^^^^^^^^ help: try simplifying it as shown: `!(!b)` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:171:8 + | +LL | if true == !b {} + | ^^^^^^^^^^ help: try: `true != b` + +error: this comparison might be written more concisely + --> $DIR/nonminimal_bool.rs:171:8 + | +LL | if true == !b {} + | ^^^^^^^^^^ help: try simplifying it as shown: `true != b` + +error: equality checks against true are unnecessary + --> $DIR/nonminimal_bool.rs:171:8 + | +LL | if true == !b {} + | ^^^^^^^^^^ help: try simplifying it as shown: `!b` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:172:8 + | +LL | if true != !b {} + | ^^^^^^^^^^ help: try: `true == b` + +error: inequality checks against true can be replaced by a negation + --> $DIR/nonminimal_bool.rs:172:8 + | +LL | if true != !b {} + | ^^^^^^^^^^ help: try simplifying it as shown: `!(!b)` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:173:8 + | +LL | if !b == !c {} + | ^^^^^^^^ help: try: `b == c` + +error: this boolean expression can be simplified + --> $DIR/nonminimal_bool.rs:174:8 + | +LL | if !b != !c {} + | ^^^^^^^^ help: try: `b != c` + +error: aborting due to 29 previous errors