Auto merge of rust-lang#6119 - rsulli55:find_is_some_on_strs, r=flip1995

Add a case to `lint_search_is_some` to handle searching strings

Fixes: rust-lang#6010

This adds a lint which recommends using `contains()` instead of `find()` followed by `is_some()` on strings as suggested in rust-lang#6010.

This was added as an additional case to
https://github.com/rust-lang/rust-clippy/blob/5af88e3c2d8cc4fb74a0e455381669930ee3a31a/clippy_lints/src/methods/mod.rs#L3037

I would really appreciate any comments/suggestions for my code!

changelog: Added case to `lint_search_is_some` to handle searching strings

Author: bors

clippy_lints/src/methods/mod.rs

@@ -515,11 +515,11 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for an iterator search (such as `find()`,
+    /// **What it does:** Checks for an iterator or string search (such as `find()`,
     /// `position()`, or `rposition()`) followed by a call to `is_some()`.
     ///
     /// **Why is this bad?** Readability, this can be written more concisely as
-    /// `_.any(_)`.
+    /// `_.any(_)` or `_.contains(_)`.
     ///
     /// **Known problems:** None.
     ///
@@ -535,7 +535,7 @@ declare_clippy_lint! {
     /// ```
     pub SEARCH_IS_SOME,
     complexity,
-    "using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()`"
+    "using an iterator or string search followed by `is_some()`, which is more succinctly expressed as a call to `any()` or `contains()`"
 }
 
 declare_clippy_lint! {
@@ -3041,6 +3041,7 @@ fn lint_flat_map_identity<'tcx>(
 }
 
 /// lint searching an Iterator followed by `is_some()`
+/// or calling `find()` on a string followed by `is_some()`
 fn lint_search_is_some<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &'tcx hir::Expr<'_>,
@@ -3052,10 +3053,10 @@ fn lint_search_is_some<'tcx>(
     // lint if caller of search is an Iterator
     if match_trait_method(cx, &is_some_args[0], &paths::ITERATOR) {
         let msg = format!(
-            "called `is_some()` after searching an `Iterator` with {}. This is more succinctly \
-             expressed by calling `any()`.",
+            "called `is_some()` after searching an `Iterator` with `{}`",
             search_method
         );
+        let hint = "this is more succinctly expressed by calling `any()`";
         let search_snippet = snippet(cx, search_args[1].span, "..");
         if search_snippet.lines().count() <= 1 {
             // suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()`
@@ -3083,15 +3084,44 @@ fn lint_search_is_some<'tcx>(
                 SEARCH_IS_SOME,
                 method_span.with_hi(expr.span.hi()),
                 &msg,
-                "try this",
+                "use `any()` instead",
                 format!(
                     "any({})",
                     any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str)
                 ),
                 Applicability::MachineApplicable,
             );
         } else {
-            span_lint(cx, SEARCH_IS_SOME, expr.span, &msg);
+            span_lint_and_help(cx, SEARCH_IS_SOME, expr.span, &msg, None, hint);
+        }
+    }
+    // lint if `find()` is called by `String` or `&str`
+    else if search_method == "find" {
+        let is_string_or_str_slice = |e| {
+            let self_ty = cx.typeck_results().expr_ty(e).peel_refs();
+            if is_type_diagnostic_item(cx, self_ty, sym!(string_type)) {
+                true
+            } else {
+                *self_ty.kind() == ty::Str
+            }
+        };
+        if_chain! {
+            if is_string_or_str_slice(&search_args[0]);
+            if is_string_or_str_slice(&search_args[1]);
+            then {
+                let msg = "called `is_some()` after calling `find()` on a string";
+                let mut applicability = Applicability::MachineApplicable;
+                let find_arg = snippet_with_applicability(cx, search_args[1].span, "..", &mut applicability);
+                span_lint_and_sugg(
+                    cx,
+                    SEARCH_IS_SOME,
+                    method_span.with_hi(expr.span.hi()),
+                    msg,
+                    "use `contains()` instead",
+                    format!("contains({})", find_arg),
+                    applicability,
+                );
+            }
         }
     }
 }

src/lintlist/mod.rs

@@ -2121,7 +2121,7 @@ vec![
     Lint {
         name: "search_is_some",
         group: "complexity",
-        desc: "using an iterator search followed by `is_some()`, which is more succinctly expressed as a call to `any()`",
+        desc: "using an iterator or string search followed by `is_some()`, which is more succinctly expressed as a call to `any()` or `contains()`",
         deprecation: None,
         module: "methods",
     },

tests/ui/methods.rs

@@ -133,50 +133,6 @@ fn filter_next() {
     let _ = foo.filter().next();
 }
 
-/// Checks implementation of `SEARCH_IS_SOME` lint.
-#[rustfmt::skip]
-fn search_is_some() {
-    let v = vec![3, 2, 1, 0, -1, -2, -3];
-    let y = &&42;
-
-    // Check `find().is_some()`, single-line case.
-    let _ = v.iter().find(|&x| *x < 0).is_some();
-    let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
-    let _ = (0..1).find(|x| *x == 0).is_some();
-    let _ = v.iter().find(|x| **x == 0).is_some();
-
-    // Check `find().is_some()`, multi-line case.
-    let _ = v.iter().find(|&x| {
-                              *x < 0
-                          }
-                   ).is_some();
-
-    // Check `position().is_some()`, single-line case.
-    let _ = v.iter().position(|&x| x < 0).is_some();
-
-    // Check `position().is_some()`, multi-line case.
-    let _ = v.iter().position(|&x| {
-                                  x < 0
-                              }
-                   ).is_some();
-
-    // Check `rposition().is_some()`, single-line case.
-    let _ = v.iter().rposition(|&x| x < 0).is_some();
-
-    // Check `rposition().is_some()`, multi-line case.
-    let _ = v.iter().rposition(|&x| {
-                                   x < 0
-                               }
-                   ).is_some();
-
-    // Check that we don't lint if the caller is not an `Iterator`.
-    let foo = IteratorFalsePositives { foo: 0 };
-    let _ = foo.find().is_some();
-    let _ = foo.position().is_some();
-    let _ = foo.rposition().is_some();
-}
-
 fn main() {
     filter_next();
-    search_is_some();
 }

tests/ui/methods.stderr

@@ -20,73 +20,5 @@ LL | |                    ).next();
    |
    = note: `-D clippy::filter-next` implied by `-D warnings`
 
-error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:143:22
-   |
-LL |     let _ = v.iter().find(|&x| *x < 0).is_some();
-   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)`
-   |
-   = note: `-D clippy::search-is-some` implied by `-D warnings`
-
-error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:144:20
-   |
-LL |     let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
-   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)`
-
-error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:145:20
-   |
-LL |     let _ = (0..1).find(|x| *x == 0).is_some();
-   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)`
-
-error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:146:22
-   |
-LL |     let _ = v.iter().find(|x| **x == 0).is_some();
-   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)`
-
-error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:149:13
-   |
-LL |       let _ = v.iter().find(|&x| {
-   |  _____________^
-LL | |                               *x < 0
-LL | |                           }
-LL | |                    ).is_some();
-   | |______________________________^
-
-error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:155:22
-   |
-LL |     let _ = v.iter().position(|&x| x < 0).is_some();
-   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
-
-error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:158:13
-   |
-LL |       let _ = v.iter().position(|&x| {
-   |  _____________^
-LL | |                                   x < 0
-LL | |                               }
-LL | |                    ).is_some();
-   | |______________________________^
-
-error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:164:22
-   |
-LL |     let _ = v.iter().rposition(|&x| x < 0).is_some();
-   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)`
-
-error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`.
-  --> $DIR/methods.rs:167:13
-   |
-LL |       let _ = v.iter().rposition(|&x| {
-   |  _____________^
-LL | |                                    x < 0
-LL | |                                }
-LL | |                    ).is_some();
-   | |______________________________^
-
-error: aborting due to 11 previous errors
+error: aborting due to 2 previous errors
 

tests/ui/search_is_some.rs

@@ -0,0 +1,38 @@
+// aux-build:option_helpers.rs
+extern crate option_helpers;
+use option_helpers::IteratorFalsePositives;
+
+#[warn(clippy::search_is_some)]
+#[rustfmt::skip]
+fn main() {
+    let v = vec![3, 2, 1, 0, -1, -2, -3];
+    let y = &&42;
+
+
+    // Check `find().is_some()`, multi-line case.
+    let _ = v.iter().find(|&x| {
+                              *x < 0
+                          }
+                   ).is_some();
+
+    // Check `position().is_some()`, multi-line case.
+    let _ = v.iter().position(|&x| {
+                                  x < 0
+                              }
+                   ).is_some();
+
+    // Check `rposition().is_some()`, multi-line case.
+    let _ = v.iter().rposition(|&x| {
+                                   x < 0
+                               }
+                   ).is_some();
+
+    // Check that we don't lint if the caller is not an `Iterator` or string
+    let falsepos = IteratorFalsePositives { foo: 0 };
+    let _ = falsepos.find().is_some();
+    let _ = falsepos.position().is_some();
+    let _ = falsepos.rposition().is_some();
+    // check that we don't lint if `find()` is called with
+    // `Pattern` that is not a string
+    let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_some();
+}

tests/ui/search_is_some.stderr

@@ -0,0 +1,39 @@
+error: called `is_some()` after searching an `Iterator` with `find`
+  --> $DIR/search_is_some.rs:13:13
+   |
+LL |       let _ = v.iter().find(|&x| {
+   |  _____________^
+LL | |                               *x < 0
+LL | |                           }
+LL | |                    ).is_some();
+   | |______________________________^
+   |
+   = note: `-D clippy::search-is-some` implied by `-D warnings`
+   = help: this is more succinctly expressed by calling `any()`
+
+error: called `is_some()` after searching an `Iterator` with `position`
+  --> $DIR/search_is_some.rs:19:13
+   |
+LL |       let _ = v.iter().position(|&x| {
+   |  _____________^
+LL | |                                   x < 0
+LL | |                               }
+LL | |                    ).is_some();
+   | |______________________________^
+   |
+   = help: this is more succinctly expressed by calling `any()`
+
+error: called `is_some()` after searching an `Iterator` with `rposition`
+  --> $DIR/search_is_some.rs:25:13
+   |
+LL |       let _ = v.iter().rposition(|&x| {
+   |  _____________^
+LL | |                                    x < 0
+LL | |                                }
+LL | |                    ).is_some();
+   | |______________________________^
+   |
+   = help: this is more succinctly expressed by calling `any()`
+
+error: aborting due to 3 previous errors
+

tests/ui/search_is_some_fixable.fixed

@@ -0,0 +1,35 @@
+// run-rustfix
+
+#![warn(clippy::search_is_some)]
+
+fn main() {
+    let v = vec![3, 2, 1, 0, -1, -2, -3];
+    let y = &&42;
+
+    // Check `find().is_some()`, single-line case.
+    let _ = v.iter().any(|x| *x < 0);
+    let _ = (0..1).any(|x| **y == x); // one dereference less
+    let _ = (0..1).any(|x| x == 0);
+    let _ = v.iter().any(|x| *x == 0);
+
+    // Check `position().is_some()`, single-line case.
+    let _ = v.iter().any(|&x| x < 0);
+
+    // Check `rposition().is_some()`, single-line case.
+    let _ = v.iter().any(|&x| x < 0);
+
+    let s1 = String::from("hello world");
+    let s2 = String::from("world");
+    // caller of `find()` is a `&`static str`
+    let _ = "hello world".contains("world");
+    let _ = "hello world".contains(&s2);
+    let _ = "hello world".contains(&s2[2..]);
+    // caller of `find()` is a `String`
+    let _ = s1.contains("world");
+    let _ = s1.contains(&s2);
+    let _ = s1.contains(&s2[2..]);
+    // caller of `find()` is slice of `String`
+    let _ = s1[2..].contains("world");
+    let _ = s1[2..].contains(&s2);
+    let _ = s1[2..].contains(&s2[2..]);
+}

tests/ui/search_is_some_fixable.rs

@@ -0,0 +1,35 @@
+// run-rustfix
+
+#![warn(clippy::search_is_some)]
+
+fn main() {
+    let v = vec![3, 2, 1, 0, -1, -2, -3];
+    let y = &&42;
+
+    // Check `find().is_some()`, single-line case.
+    let _ = v.iter().find(|&x| *x < 0).is_some();
+    let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less
+    let _ = (0..1).find(|x| *x == 0).is_some();
+    let _ = v.iter().find(|x| **x == 0).is_some();
+
+    // Check `position().is_some()`, single-line case.
+    let _ = v.iter().position(|&x| x < 0).is_some();
+
+    // Check `rposition().is_some()`, single-line case.
+    let _ = v.iter().rposition(|&x| x < 0).is_some();
+
+    let s1 = String::from("hello world");
+    let s2 = String::from("world");
+    // caller of `find()` is a `&`static str`
+    let _ = "hello world".find("world").is_some();
+    let _ = "hello world".find(&s2).is_some();
+    let _ = "hello world".find(&s2[2..]).is_some();
+    // caller of `find()` is a `String`
+    let _ = s1.find("world").is_some();
+    let _ = s1.find(&s2).is_some();
+    let _ = s1.find(&s2[2..]).is_some();
+    // caller of `find()` is slice of `String`
+    let _ = s1[2..].find("world").is_some();
+    let _ = s1[2..].find(&s2).is_some();
+    let _ = s1[2..].find(&s2[2..]).is_some();
+}