bool_comparison: no longer lint on !x != y (rust-lang#15498)

The issue that this PR was opened to fix
(rust-lang#15367), talked about
how, for `!x == false`, this lint suggests both:
1. `x != false`
2. `x`

I initially wanted to make the lint short-circuit after emitting the
first suggestion (since that was the easier change code-wise), but that
was not optimal for two reasons:
1. suggestion 2 is obviously better (the other one requires one more
round of Clippy)
2. suggestion 1 is actually already covered by `nonminimal_bool`

And so I instead decided to completely remove the code path that emits
suggestion 1.

Fixes rust-lang#15367

changelog: [`bool_comparison`]: no longer lint on `!x != y`

Author: samueltardieu

clippy_lints/src/needless_bool.rs

@@ -7,7 +7,7 @@ use clippy_utils::{
 };
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOpKind, Expr, ExprKind, UnOp};
+use rustc_hir::{BinOpKind, Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 use rustc_span::Span;
@@ -232,7 +232,12 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison {
             return;
         }
 
-        if let ExprKind::Binary(Spanned { node, .. }, ..) = e.kind {
+        if let ExprKind::Binary(Spanned { node, .. }, left_side, right_side) = e.kind
+            && is_expn_of(left_side.span, sym::cfg).is_none()
+            && is_expn_of(right_side.span, sym::cfg).is_none()
+            && cx.typeck_results().expr_ty(left_side).is_bool()
+            && cx.typeck_results().expr_ty(right_side).is_bool()
+        {
             let ignore_case = None::<(fn(_) -> _, &str)>;
             let ignore_no_literal = None::<(fn(_, _) -> _, &str)>;
             match node {
@@ -288,30 +293,6 @@ impl<'tcx> LateLintPass<'tcx> for BoolComparison {
     }
 }
 
-struct ExpressionInfoWithSpan {
-    one_side_is_unary_not: bool,
-    left_span: Span,
-    right_span: Span,
-}
-
-fn is_unary_not(e: &Expr<'_>) -> (bool, Span) {
-    if let ExprKind::Unary(UnOp::Not, operand) = e.kind {
-        return (true, operand.span);
-    }
-    (false, e.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);
-
-    ExpressionInfoWithSpan {
-        one_side_is_unary_not: left.0 != right.0,
-        left_span: left.1,
-        right_span: right.1,
-    }
-}
-
 fn check_comparison<'a, 'tcx>(
     cx: &LateContext<'tcx>,
     e: &'tcx Expr<'_>,
@@ -321,81 +302,39 @@ fn check_comparison<'a, 'tcx>(
     right_false: Option<(impl FnOnce(Sugg<'a>) -> Sugg<'a>, &'static str)>,
     no_literal: Option<(impl FnOnce(Sugg<'a>, Sugg<'a>) -> Sugg<'a>, &'static str)>,
 ) {
-    if let ExprKind::Binary(op, left_side, right_side) = e.kind {
-        let (l_ty, r_ty) = (
-            cx.typeck_results().expr_ty(left_side),
-            cx.typeck_results().expr_ty(right_side),
-        );
-        if is_expn_of(left_side.span, sym::cfg).is_some() || is_expn_of(right_side.span, sym::cfg).is_some() {
-            return;
-        }
-        if l_ty.is_bool() && r_ty.is_bool() {
-            let mut applicability = Applicability::MachineApplicable;
-            // Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs,
-            // calling `source_callsite` make sure macros are handled correctly, see issue #9907
-            let binop_span = left_side
-                .span
-                .source_callsite()
-                .with_hi(right_side.span.source_callsite().hi());
+    if let ExprKind::Binary(_, left_side, right_side) = e.kind {
+        let mut applicability = Applicability::MachineApplicable;
+        // Eliminate parentheses in `e` by using the lo pos of lhs and hi pos of rhs,
+        // calling `source_callsite` make sure macros are handled correctly, see issue #9907
+        let binop_span = left_side.span.source_callsite().to(right_side.span.source_callsite());
 
-            if op.node == BinOpKind::Eq {
-                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,
-                        binop_span,
-                        "this comparison might be written more concisely",
-                        "try simplifying it as shown",
-                        format!(
-                            "{} != {}",
-                            snippet_with_applicability(
-                                cx,
-                                expression_info.left_span.source_callsite(),
-                                "..",
-                                &mut applicability
-                            ),
-                            snippet_with_applicability(
-                                cx,
-                                expression_info.right_span.source_callsite(),
-                                "..",
-                                &mut applicability
-                            )
-                        ),
-                        applicability,
-                    );
-                }
-            }
-
-            match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
-                (Some(true), None) => left_true.map_or((), |(h, m)| {
-                    suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
-                }),
-                (None, Some(true)) => right_true.map_or((), |(h, m)| {
-                    suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
-                }),
-                (Some(false), None) => left_false.map_or((), |(h, m)| {
-                    suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
-                }),
-                (None, Some(false)) => right_false.map_or((), |(h, m)| {
-                    suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
-                }),
-                (None, None) => no_literal.map_or((), |(h, m)| {
-                    let left_side = Sugg::hir_with_context(cx, left_side, binop_span.ctxt(), "..", &mut applicability);
-                    let right_side =
-                        Sugg::hir_with_context(cx, right_side, binop_span.ctxt(), "..", &mut applicability);
-                    span_lint_and_sugg(
-                        cx,
-                        BOOL_COMPARISON,
-                        binop_span,
-                        m,
-                        "try simplifying it as shown",
-                        h(left_side, right_side).into_string(),
-                        applicability,
-                    );
-                }),
-                _ => (),
-            }
+        match (fetch_bool_expr(left_side), fetch_bool_expr(right_side)) {
+            (Some(true), None) => left_true.map_or((), |(h, m)| {
+                suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
+            }),
+            (None, Some(true)) => right_true.map_or((), |(h, m)| {
+                suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
+            }),
+            (Some(false), None) => left_false.map_or((), |(h, m)| {
+                suggest_bool_comparison(cx, binop_span, right_side, applicability, m, h);
+            }),
+            (None, Some(false)) => right_false.map_or((), |(h, m)| {
+                suggest_bool_comparison(cx, binop_span, left_side, applicability, m, h);
+            }),
+            (None, None) => no_literal.map_or((), |(h, m)| {
+                let left_side = Sugg::hir_with_context(cx, left_side, binop_span.ctxt(), "..", &mut applicability);
+                let right_side = Sugg::hir_with_context(cx, right_side, binop_span.ctxt(), "..", &mut applicability);
+                span_lint_and_sugg(
+                    cx,
+                    BOOL_COMPARISON,
+                    binop_span,
+                    m,
+                    "try",
+                    h(left_side, right_side).into_string(),
+                    applicability,
+                );
+            }),
+            _ => (),
         }
     }
 }
@@ -414,7 +353,7 @@ fn suggest_bool_comparison<'a, 'tcx>(
         BOOL_COMPARISON,
         span,
         message,
-        "try simplifying it as shown",
+        "try",
         conv_hint(hint).into_string(),
         app,
     );

tests/ui/bool_comparison.fixed

@@ -1,94 +1,39 @@
 #![allow(non_local_definitions, clippy::needless_if)]
 #![warn(clippy::bool_comparison)]
-#![allow(clippy::non_canonical_partial_ord_impl, clippy::nonminimal_bool)]
+#![allow(clippy::non_canonical_partial_ord_impl)]
 
 fn main() {
     let x = true;
-    if x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if !x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if !x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if !x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if !x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if !x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if !x {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
+    let _ = if x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if !x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if !x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if !x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if !x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if !x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if x { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if !x { "yes" } else { "no" };
+    //~^ bool_comparison
+
     let y = true;
-    if !x & y {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
-    if x & !y {
-        //~^ bool_comparison
-        "yes"
-    } else {
-        "no"
-    };
+    let _ = if !x & y { "yes" } else { "no" };
+    //~^ bool_comparison
+    let _ = if x & !y { "yes" } else { "no" };
+    //~^ bool_comparison
 }
 
 fn issue3703() {
@@ -126,25 +71,6 @@ fn issue3703() {
     if false < Foo {}
 }
 
-fn issue4983() {
-    let a = true;
-    let b = false;
-
-    if a != b {};
-    //~^ bool_comparison
-    if a != b {};
-    //~^ bool_comparison
-    if a == b {};
-    if !a == !b {};
-
-    if b != a {};
-    //~^ bool_comparison
-    if b != a {};
-    //~^ bool_comparison
-    if b == a {};
-    if !b == !a {};
-}
-
 macro_rules! m {
     ($func:ident) => {
         $func()
@@ -193,10 +119,22 @@ fn issue9907() {
     //~^ bool_comparison
     // This is not part of the issue, but an unexpected found when fixing the issue,
     // the provided span was inside of macro rather than the macro callsite.
-    let _ = ((1 < 2) != m!(func)) as usize;
+    let _ = ((1 < 2) & !m!(func)) as usize;
     //~^ bool_comparison
 }
 
+#[allow(clippy::nonminimal_bool)]
+fn issue15367() {
+    let a = true;
+    let b = false;
+
+    // these cases are handled by `nonminimal_bool`, so don't double-lint
+    if a == !b {};
+    if !a == b {};
+    if b == !a {};
+    if !b == a {};
+}
+
 fn issue15497() {
     fn func() -> bool {
         true