From 90e7d93d6cb333fea13383aa34d93b4fde7fbffc Mon Sep 17 00:00:00 2001
From: Philipp Hansch <dev@phansch.net>
Date: Sat, 7 Apr 2018 12:52:18 +0200
Subject: [PATCH] Fix nonminimal_bool false positive

It was checking any is_ok, is_err, is_some, is_none method for negation
but it should only perform the check for the built-in types, not custom
types.
---
 clippy_lints/src/booleans.rs |  7 ++++-
 tests/ui/booleans.rs         | 57 ++++++++++++++++++++++++++++++++++++
 tests/ui/booleans.stderr     | 26 +++++++++++++++-
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs
index 0fa2c2ac9..c8478c47a 100644
--- a/clippy_lints/src/booleans.rs
+++ b/clippy_lints/src/booleans.rs
@@ -4,7 +4,7 @@ use rustc::hir::intravisit::*;
 use syntax::ast::{LitKind, NodeId, DUMMY_NODE_ID};
 use syntax::codemap::{dummy_spanned, Span, DUMMY_SP};
 use syntax::util::ThinVec;
-use utils::{in_macro, snippet_opt, span_lint_and_then, SpanlessEq};
+use utils::{in_macro, paths, match_type, snippet_opt, span_lint_and_then, SpanlessEq};
 
 /// **What it does:** Checks for boolean expressions that can be written more
 /// concisely.
@@ -185,6 +185,11 @@ impl<'a, 'tcx, 'v> SuggestContext<'a, 'tcx, 'v> {
                 }.and_then(|op| Some(format!("{}{}{}", self.snip(lhs)?, op, self.snip(rhs)?)))
             },
             ExprMethodCall(ref path, _, ref 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)])
diff --git a/tests/ui/booleans.rs b/tests/ui/booleans.rs
index 0898de105..78e876e51 100644
--- a/tests/ui/booleans.rs
+++ b/tests/ui/booleans.rs
@@ -57,3 +57,60 @@ fn methods_with_negation() {
     let _ = (!c ^ c) || !a.is_some();
     let _ = !c ^ c || !a.is_some();
 }
+
+// Simplified versions of https://github.com/rust-lang-nursery/rust-clippy/issues/2638
+// nonminimal_bool should only check the built-in Result and Some type, not
+// any other types like the following.
+enum CustomResultOk<E> { Ok, Err(E) }
+enum CustomResultErr<E> { Ok, Err(E) }
+enum CustomSomeSome<T> { Some(T), None }
+enum CustomSomeNone<T> { Some(T), None }
+
+impl<E> CustomResultOk<E> {
+    pub fn is_ok(&self) -> bool { true }
+}
+
+impl<E> CustomResultErr<E> {
+    pub fn is_err(&self) -> bool { true }
+}
+
+impl<T> CustomSomeSome<T> {
+    pub fn is_some(&self) -> bool { true }
+}
+
+impl<T> CustomSomeNone<T> {
+    pub fn is_none(&self) -> bool { true }
+}
+
+fn dont_warn_for_custom_methods_with_negation() {
+    let res = CustomResultOk::Err("Error");
+    // Should not warn and suggest 'is_err()' because the type does not
+    // implement is_err().
+    if !res.is_ok() { }
+
+    let res = CustomResultErr::Err("Error");
+    // Should not warn and suggest 'is_ok()' because the type does not
+    // implement is_ok().
+    if !res.is_err() { }
+
+    let res = CustomSomeSome::Some("thing");
+    // Should not warn and suggest 'is_none()' because the type does not
+    // implement is_none().
+    if !res.is_some() { }
+
+    let res = CustomSomeNone::Some("thing");
+    // Should not warn and suggest 'is_some()' because the type does not
+    // implement is_some().
+    if !res.is_none() { }
+}
+
+// Only Built-in Result and Some types should suggest the negated alternative
+fn warn_for_built_in_methods_with_negation() {
+    let res: Result<usize, usize> = Ok(1);
+    if !res.is_ok() { }
+    if !res.is_err() { }
+
+    let res = Some(1);
+    if !res.is_some() { }
+    if !res.is_none() { }
+}
diff --git a/tests/ui/booleans.stderr b/tests/ui/booleans.stderr
index c88a7a7be..f1996e8a2 100644
--- a/tests/ui/booleans.stderr
+++ b/tests/ui/booleans.stderr
@@ -175,5 +175,29 @@ error: this boolean expression can be simplified
 58 |     let _ = !c ^ c || !a.is_some();
    |             ^^^^^^^^^^^^^^^^^^^^^^ help: try: `!c ^ c || a.is_none()`
 
-error: aborting due to 21 previous errors
+error: this boolean expression can be simplified
+   --> $DIR/booleans.rs:110:8
+    |
+110 |     if !res.is_ok() { }
+    |        ^^^^^^^^^^^^ help: try: `res.is_err()`
+
+error: this boolean expression can be simplified
+   --> $DIR/booleans.rs:111:8
+    |
+111 |     if !res.is_err() { }
+    |        ^^^^^^^^^^^^^ help: try: `res.is_ok()`
+
+error: this boolean expression can be simplified
+   --> $DIR/booleans.rs:114:8
+    |
+114 |     if !res.is_some() { }
+    |        ^^^^^^^^^^^^^^ help: try: `res.is_none()`
+
+error: this boolean expression can be simplified
+   --> $DIR/booleans.rs:115:8
+    |
+115 |     if !res.is_none() { }
+    |        ^^^^^^^^^^^^^^ help: try: `res.is_some()`
+
+error: aborting due to 25 previous errors