Auto merge of #4039 - andrehjr:add-find-map-lints, r=flip1995

Add lints for find_map

changelog: adds lints for find_map and filter_map_next. Closes issue #3474

Hope I got everything correctly this time! Let me know if I missed something.

CHANGELOG.md

@@ -896,7 +896,9 @@ All notable changes to this project will be documented in this file.
 [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes
 [`fallible_impl_from`]: https://rust-lang.github.io/rust-clippy/master/index.html#fallible_impl_from
 [`filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map
+[`filter_map_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_next
 [`filter_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_next
+[`find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#find_map
 [`float_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_arithmetic
 [`float_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp
 [`float_cmp_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp_const

README.md

@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 299 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 301 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -622,6 +622,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         loops::EXPLICIT_ITER_LOOP,
         matches::SINGLE_MATCH_ELSE,
         methods::FILTER_MAP,
+        methods::FILTER_MAP_NEXT,
+        methods::FIND_MAP,
         methods::MAP_FLATTEN,
         methods::OPTION_MAP_UNWRAP_OR,
         methods::OPTION_MAP_UNWRAP_OR_ELSE,

clippy_lints/src/methods/mod.rs

@@ -288,6 +288,50 @@ declare_clippy_lint! {
     "using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.filter_map(_).next()`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely as a
+    /// single method call.
+    ///
+    /// **Known problems:** None
+    ///
+    /// **Example:**
+    /// ```rust
+    ///  (0..3).filter_map(|x| if x == 2 { Some(x) } else { None }).next();
+    /// ```
+    /// Can be written as
+    ///
+    /// ```rust
+    ///  (0..3).find_map(|x| if x == 2 { Some(x) } else { None });
+    /// ```
+    pub FILTER_MAP_NEXT,
+    pedantic,
+    "using combination of `filter_map` and `next` which can usually be written as a single method call"
+}
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.find(_).map(_)`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely as a
+    /// single method call.
+    ///
+    /// **Known problems:** Often requires a condition + Option/Iterator creation
+    /// inside the closure.
+    ///
+    /// **Example:**
+    /// ```rust
+    ///  (0..3).find(|x| x == 2).map(|x| x * 2);
+    /// ```
+    /// Can be written as
+    /// ```rust
+    ///  (0..3).find_map(|x| if x == 2 { Some(x * 2) } else { None });
+    /// ```
+    pub FIND_MAP,
+    pedantic,
+    "using a combination of `find` and `map` can usually be written as a single method call"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for an iterator search (such as `find()`,
     /// `position()`, or `rposition()`) followed by a call to `is_some()`.
@@ -798,6 +842,8 @@ declare_lint_pass!(Methods => [
     TEMPORARY_CSTRING_AS_PTR,
     FILTER_NEXT,
     FILTER_MAP,
+    FILTER_MAP_NEXT,
+    FIND_MAP,
     MAP_FLATTEN,
     ITER_NTH,
     ITER_SKIP_NEXT,
@@ -833,6 +879,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             ["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
             ["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]),
+            ["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1]),
+            ["map", "find"] => lint_find_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["flat_map", "filter"] => lint_filter_flat_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["flat_map", "filter_map"] => lint_filter_map_flat_map(cx, expr, arg_lists[1], arg_lists[0]),
             ["flatten", "map"] => lint_map_flatten(cx, expr, arg_lists[1]),
@@ -1911,6 +1959,42 @@ fn lint_filter_map<'a, 'tcx>(
     }
 }
 
+/// lint use of `filter_map().next()` for `Iterators`
+fn lint_filter_map_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, filter_args: &'tcx [hir::Expr]) {
+    if match_trait_method(cx, expr, &paths::ITERATOR) {
+        let msg = "called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling \
+                   `.find_map(p)` instead.";
+        let filter_snippet = snippet(cx, filter_args[1].span, "..");
+        if filter_snippet.lines().count() <= 1 {
+            span_note_and_lint(
+                cx,
+                FILTER_MAP_NEXT,
+                expr.span,
+                msg,
+                expr.span,
+                &format!("replace `filter_map({0}).next()` with `find_map({0})`", filter_snippet),
+            );
+        } else {
+            span_lint(cx, FILTER_MAP_NEXT, expr.span, msg);
+        }
+    }
+}
+
+/// lint use of `find().map()` for `Iterators`
+fn lint_find_map<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &'tcx hir::Expr,
+    _find_args: &'tcx [hir::Expr],
+    map_args: &'tcx [hir::Expr],
+) {
+    // lint if caller of `.filter().map()` is an Iterator
+    if match_trait_method(cx, &map_args[0], &paths::ITERATOR) {
+        let msg = "called `find(p).map(q)` on an `Iterator`. \
+                   This is more succinctly expressed by calling `.find_map(..)` instead.";
+        span_lint(cx, FIND_MAP, expr.span, msg);
+    }
+}
+
 /// lint use of `filter().map()` for `Iterators`
 fn lint_filter_map_map<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,

clippy_lints/src/shadow.rs

@@ -186,7 +186,9 @@ fn check_pat<'a, 'tcx>(
                 if let ExprKind::Struct(_, ref efields, _) = init_struct.node {
                     for field in pfields {
                         let name = field.node.ident.name;
-                        let efield = efields.iter().find(|f| f.ident.name == name).map(|f| &*f.expr);
+                        let efield = efields
+                            .iter()
+                            .find_map(|f| if f.ident.name == name { Some(&*f.expr) } else { None });
                         check_pat(cx, &field.node.pat, efield, span, bindings);
                     }
                 } else {

clippy_lints/src/utils/attrs.rs

@@ -58,10 +58,16 @@ pub fn get_attr<'a>(
     attrs.iter().filter(move |attr| {
         let attr_segments = &attr.path.segments;
         if attr_segments.len() == 2 && attr_segments[0].ident.to_string() == "clippy" {
-            if let Some(deprecation_status) = BUILTIN_ATTRIBUTES
-                .iter()
-                .find(|(builtin_name, _)| *builtin_name == attr_segments[1].ident.to_string())
-                .map(|(_, deprecation_status)| deprecation_status)
+            if let Some(deprecation_status) =
+                BUILTIN_ATTRIBUTES
+                    .iter()
+                    .find_map(|(builtin_name, deprecation_status)| {
+                        if *builtin_name == attr_segments[1].ident.to_string() {
+                            Some(deprecation_status)
+                        } else {
+                            None
+                        }
+                    })
             {
                 let mut db = sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute");
                 match deprecation_status {

tests/compile-test.rs

@@ -61,10 +61,13 @@ fn config(mode: &str, dir: PathBuf) -> compiletest::Config {
             let name = path.file_name()?.to_string_lossy();
             // NOTE: This only handles a single dep
             // https://github.com/laumann/compiletest-rs/issues/101
-            needs_disambiguation
-                .iter()
-                .find(|dep| name.starts_with(&format!("lib{}-", dep)))
-                .map(|dep| format!("--extern {}={}", dep, path.display()))
+            needs_disambiguation.iter().find_map(|dep| {
+                if name.starts_with(&format!("lib{}-", dep)) {
+                    Some(format!("--extern {}={}", dep, path.display()))
+                } else {
+                    None
+                }
+            })
         })
         .collect::<Vec<_>>();
 

tests/ui/filter_map_next.rs

@@ -0,0 +1,20 @@
+#![warn(clippy::all, clippy::pedantic)]
+
+fn main() {
+    let a = ["1", "lol", "3", "NaN", "5"];
+
+    let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
+    assert_eq!(element, Some(1));
+
+    #[rustfmt::skip]
+    let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
+        .into_iter()
+        .filter_map(|x| {
+            if x == 2 {
+                Some(x * 2)
+            } else {
+                None
+            }
+        })
+        .next();
+}

tests/ui/filter_map_next.stderr

@@ -0,0 +1,24 @@
+error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
+  --> $DIR/filter_map_next.rs:6:32
+   |
+LL |     let element: Option<i32> = a.iter().filter_map(|s| s.parse().ok()).next();
+   |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::filter-map-next` implied by `-D warnings`
+   = note: replace `filter_map(|s| s.parse().ok()).next()` with `find_map(|s| s.parse().ok())`
+
+error: called `filter_map(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find_map(p)` instead.
+  --> $DIR/filter_map_next.rs:10:26
+   |
+LL |       let _: Option<u32> = vec![1, 2, 3, 4, 5, 6]
+   |  __________________________^
+LL | |         .into_iter()
+LL | |         .filter_map(|x| {
+LL | |             if x == 2 {
+...  |
+LL | |         })
+LL | |         .next();
+   | |_______________^
+
+error: aborting due to 2 previous errors
+

tests/ui/find_map.rs

@@ -0,0 +1,32 @@
+#![warn(clippy::all, clippy::pedantic)]
+
+#[derive(Debug, Copy, Clone)]
+enum Flavor {
+    Chocolate,
+}
+
+#[derive(Debug, Copy, Clone)]
+enum Dessert {
+    Banana,
+    Pudding,
+    Cake(Flavor),
+}
+
+fn main() {
+    let desserts_of_the_week = vec![Dessert::Banana, Dessert::Cake(Flavor::Chocolate), Dessert::Pudding];
+
+    let a = ["lol", "NaN", "2", "5", "Xunda"];
+
+    let _: Option<i32> = a.iter().find(|s| s.parse::<i32>().is_ok()).map(|s| s.parse().unwrap());
+
+    let _: Option<Flavor> = desserts_of_the_week
+        .iter()
+        .find(|dessert| match *dessert {
+            Dessert::Cake(_) => true,
+            _ => false,
+        })
+        .map(|dessert| match *dessert {
+            Dessert::Cake(ref flavor) => *flavor,
+            _ => unreachable!(),
+        });
+}

tests/ui/find_map.stderr

@@ -0,0 +1,23 @@
+error: called `find(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
+  --> $DIR/find_map.rs:20:26
+   |
+LL |     let _: Option<i32> = a.iter().find(|s| s.parse::<i32>().is_ok()).map(|s| s.parse().unwrap());
+   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::find-map` implied by `-D warnings`
+
+error: called `find(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.find_map(..)` instead.
+  --> $DIR/find_map.rs:22:29
+   |
+LL |       let _: Option<Flavor> = desserts_of_the_week
+   |  _____________________________^
+LL | |         .iter()
+LL | |         .find(|dessert| match *dessert {
+LL | |             Dessert::Cake(_) => true,
+...  |
+LL | |             _ => unreachable!(),
+LL | |         });
+   | |__________^
+
+error: aborting due to 2 previous errors
+