From 5e7c437d415871052fdd92f0d402ac9fd050fefa Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Fri, 9 Feb 2024 20:47:31 +0100 Subject: [PATCH] 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 3c17f65f0..0d66f2d64 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 02f1d09b8..54bdf7f5d 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 5ef696d85..4fdf23052 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 ee092b9ac..908ef6cb2 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 856b5cd08..a317c8328 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