Auto merge of #4568 - mikerite:fix-4548, r=flip1995

Fix `nonminimal-bool` false positive

Closes #4548
Closes #3847

changelog: Fix `nonminimal-bool` false positive

Author: bors

clippy_lints/src/booleans.rs

@@ -1,5 +1,6 @@
 use crate::utils::{
-    get_trait_def_id, implements_trait, in_macro, match_type, paths, snippet_opt, span_lint_and_then, SpanlessEq,
+    get_trait_def_id, implements_trait, in_macro, match_type, paths, snippet_opt, span_lint_and_sugg,
+    span_lint_and_then, SpanlessEq,
 };
 use if_chain::if_chain;
 use rustc::hir::intravisit::*;
@@ -159,46 +160,6 @@ struct SuggestContext<'a, 'tcx, 'v> {
 }
 
 impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
-    fn snip(&self, e: &Expr) -> Option<String> {
-        snippet_opt(self.cx, e.span)
-    }
-
-    fn simplify_not(&self, expr: &Expr) -> Option<String> {
-        match &expr.node {
-            ExprKind::Binary(binop, lhs, rhs) => {
-                if !implements_ord(self.cx, lhs) {
-                    return None;
-                }
-
-                match binop.node {
-                    BinOpKind::Eq => Some(" != "),
-                    BinOpKind::Ne => Some(" == "),
-                    BinOpKind::Lt => Some(" >= "),
-                    BinOpKind::Gt => Some(" <= "),
-                    BinOpKind::Le => Some(" > "),
-                    BinOpKind::Ge => Some(" < "),
-                    _ => None,
-                }
-                .and_then(|op| Some(format!("{}{}{}", self.snip(lhs)?, op, self.snip(rhs)?)))
-            },
-            ExprKind::MethodCall(path, _, args) if args.len() == 1 => {
-                let type_of_receiver = self.cx.tables.expr_ty(&args[0]);
-                if !match_type(self.cx, type_of_receiver, &paths::OPTION)
-                    && !match_type(self.cx, type_of_receiver, &paths::RESULT)
-                {
-                    return None;
-                }
-                METHODS_WITH_NEGATION
-                    .iter()
-                    .cloned()
-                    .flat_map(|(a, b)| vec![(a, b), (b, a)])
-                    .find(|&(a, _)| a == path.ident.name.as_str())
-                    .and_then(|(_, neg_method)| Some(format!("{}.{}()", self.snip(&args[0])?, neg_method)))
-            },
-            _ => None,
-        }
-    }
-
     fn recurse(&mut self, suggestion: &Bool) -> Option<()> {
         use quine_mc_cluskey::Bool::*;
         match suggestion {
@@ -217,12 +178,12 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
                 },
                 Term(n) => {
                     let terminal = self.terminals[n as usize];
-                    if let Some(str) = self.simplify_not(terminal) {
+                    if let Some(str) = simplify_not(self.cx, terminal) {
                         self.simplified = true;
                         self.output.push_str(&str)
                     } else {
                         self.output.push('!');
-                        let snip = self.snip(terminal)?;
+                        let snip = snippet_opt(self.cx, terminal.span)?;
                         self.output.push_str(&snip);
                     }
                 },
@@ -254,14 +215,55 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
                 }
             },
             &Term(n) => {
-                let snip = self.snip(self.terminals[n as usize])?;
+                let snip = snippet_opt(self.cx, self.terminals[n as usize].span)?;
                 self.output.push_str(&snip);
             },
         }
         Some(())
     }
 }
 
+fn simplify_not(cx: &LateContext<'_, '_>, expr: &Expr) -> Option<String> {
+    match &expr.node {
+        ExprKind::Binary(binop, lhs, rhs) => {
+            if !implements_ord(cx, lhs) {
+                return None;
+            }
+
+            match binop.node {
+                BinOpKind::Eq => Some(" != "),
+                BinOpKind::Ne => Some(" == "),
+                BinOpKind::Lt => Some(" >= "),
+                BinOpKind::Gt => Some(" <= "),
+                BinOpKind::Le => Some(" > "),
+                BinOpKind::Ge => Some(" < "),
+                _ => None,
+            }
+            .and_then(|op| {
+                Some(format!(
+                    "{}{}{}",
+                    snippet_opt(cx, lhs.span)?,
+                    op,
+                    snippet_opt(cx, rhs.span)?
+                ))
+            })
+        },
+        ExprKind::MethodCall(path, _, args) if args.len() == 1 => {
+            let type_of_receiver = cx.tables.expr_ty(&args[0]);
+            if !match_type(cx, type_of_receiver, &paths::OPTION) && !match_type(cx, type_of_receiver, &paths::RESULT) {
+                return None;
+            }
+            METHODS_WITH_NEGATION
+                .iter()
+                .cloned()
+                .flat_map(|(a, b)| vec![(a, b), (b, a)])
+                .find(|&(a, _)| a == path.ident.name.as_str())
+                .and_then(|(_, neg_method)| Some(format!("{}.{}()", snippet_opt(cx, args[0].span)?, neg_method)))
+        },
+        _ => None,
+    }
+}
+
 // The boolean part of the return indicates whether some simplifications have been applied.
 fn suggest(cx: &LateContext<'_, '_>, suggestion: &Bool, terminals: &[&Expr]) -> (String, bool) {
     let mut suggest_context = SuggestContext {
@@ -330,7 +332,7 @@ fn terminal_stats(b: &Bool) -> Stats {
 }
 
 impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
-    fn bool_expr(&self, e: &Expr) {
+    fn bool_expr(&self, e: &'tcx Expr) {
         let mut h2q = Hir2Qmm {
             terminals: Vec::new(),
             cx: self.cx,
@@ -420,10 +422,8 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
                 );
             };
             if improvements.is_empty() {
-                let suggest = suggest(self.cx, &expr, &h2q.terminals);
-                if suggest.1 {
-                    nonminimal_bool_lint(vec![suggest.0])
-                }
+                let mut visitor = NotSimplificationVisitor { cx: self.cx };
+                visitor.visit_expr(e);
             } else {
                 nonminimal_bool_lint(
                     improvements
@@ -464,3 +464,30 @@ fn implements_ord<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, expr: &Expr) -> bool
     let ty = cx.tables.expr_ty(expr);
     get_trait_def_id(cx, &paths::ORD).map_or(false, |id| implements_trait(cx, ty, id, &[]))
 }
+
+struct NotSimplificationVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'a, 'tcx>,
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for NotSimplificationVisitor<'a, 'tcx> {
+    fn visit_expr(&mut self, expr: &'tcx Expr) {
+        if let ExprKind::Unary(UnNot, inner) = &expr.node {
+            if let Some(suggestion) = simplify_not(self.cx, inner) {
+                span_lint_and_sugg(
+                    self.cx,
+                    NONMINIMAL_BOOL,
+                    expr.span,
+                    "this boolean expression can be simplified",
+                    "try",
+                    suggestion,
+                    Applicability::MachineApplicable,
+                );
+            }
+        }
+
+        walk_expr(self, expr);
+    }
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::None
+    }
+}

tests/ui/booleans.rs

@@ -142,3 +142,23 @@ fn dont_warn_for_negated_partial_ord_comparison() {
     let _ = !(a > b);
     let _ = !(a >= b);
 }
+
+fn issue3847(a: u32, b: u32) -> bool {
+    const THRESHOLD: u32 = 1_000;
+
+    if a < THRESHOLD && b >= THRESHOLD || a >= THRESHOLD && b < THRESHOLD {
+        return false;
+    }
+    true
+}
+
+fn issue4548() {
+    fn f(_i: u32, _j: u32) -> u32 {
+        unimplemented!();
+    }
+
+    let i = 0;
+    let j = 0;
+
+    if i != j && f(i, j) != 0 || i == j && f(i, j) != 1 {}
+}

tests/ui/booleans.stderr

@@ -158,22 +158,22 @@ LL |     let _ = !(a.is_some() && !c);
    |             ^^^^^^^^^^^^^^^^^^^^ help: try: `c || a.is_none()`
 
 error: this boolean expression can be simplified
-  --> $DIR/booleans.rs:54:13
+  --> $DIR/booleans.rs:54:26
    |
 LL |     let _ = !(!c ^ c) || !a.is_some();
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!(!c ^ c) || a.is_none()`
+   |                          ^^^^^^^^^^^^ help: try: `a.is_none()`
 
 error: this boolean expression can be simplified
-  --> $DIR/booleans.rs:55:13
+  --> $DIR/booleans.rs:55:25
    |
 LL |     let _ = (!c ^ c) || !a.is_some();
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(!c ^ c) || a.is_none()`
+   |                         ^^^^^^^^^^^^ help: try: `a.is_none()`
 
 error: this boolean expression can be simplified
-  --> $DIR/booleans.rs:56:13
+  --> $DIR/booleans.rs:56:23
    |
 LL |     let _ = !c ^ c || !a.is_some();
-   |             ^^^^^^^^^^^^^^^^^^^^^^ help: try: `!c ^ c || a.is_none()`
+   |                       ^^^^^^^^^^^^ help: try: `a.is_none()`
 
 error: this boolean expression can be simplified
   --> $DIR/booleans.rs:128:8