issue#3424 improve filter_map lint docs

CHANGELOG.md

@@ -944,6 +944,7 @@ Released 2018-09-13
 [`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_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#filter_map_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

README.md

@@ -6,7 +6,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 306 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

@@ -633,6 +633,7 @@ 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_MAP,
         methods::FILTER_MAP_NEXT,
         methods::FIND_MAP,
         methods::MAP_FLATTEN,

clippy_lints/src/methods/mod.rs

@@ -13,6 +13,7 @@ use rustc::hir::intravisit::{self, Visitor};
 use rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass};
 use rustc::ty::{self, Predicate, Ty};
 use rustc::{declare_lint_pass, declare_tool_lint};
+use crate::rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
 use syntax::ast;
 use syntax::source_map::{BytePos, Span};
@@ -271,6 +272,43 @@ declare_clippy_lint! {
     "using combinations of `flatten` and `map` which can usually be written as a single method call"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usage of `_.filter_map(_).map(_)`, which can be more concisely
+    /// written as `_.filter_map(_)`.
+    ///
+    /// **Why is this bad?** Readability, this can be written more concisely as a
+    /// single method call.
+    ///
+    /// ** Example:**
+    /// let ws: Vec<String> = ["is", "this", "shizzle", "for", "rizzle"]
+    ///             .iter()
+    ///             .map(|s| s.to_string())
+    ///             .collect();
+    ///
+    /// let correct: Vec<String> = ws.iter()
+    ///     .filter_map(|s| if s.contains("izzle"){
+    ///       Some(s)
+    ///     } else {
+    ///       None
+    ///     })
+    ///     .map(|s| format!("{}{}", s, s))
+    ///     .collect();
+    ///
+    /// let more_correct: Vec<String> = ws.iter()
+    ///     .filter_map(|s| if s.contains("izzle"){
+    ///       Some(format!("{}{}", s, s))
+    ///     } else {
+    ///       None
+    ///     })
+    ///     .collect();
+    ///
+    /// println!("{:?}", correct);
+    /// assert_eq!(correct, more_correct);
+    pub FILTER_MAP_MAP,
+    pedantic,
+    "using `filter_map` followed by `map` can more concisely be written as a single method call"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for usage of `_.filter(_).map(_)`,
     /// `_.filter(_).flat_map(_)`, `_.filter_map(_).flat_map(_)` and similar.
@@ -287,7 +325,7 @@ declare_clippy_lint! {
     /// ```
     pub FILTER_MAP,
     pedantic,
-    "using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call"
+    "using `filter` and `map` to destructure an iterator of Options or a Results instead of a single `filter_map` call"
 }
 
 declare_clippy_lint! {
@@ -842,6 +880,7 @@ declare_lint_pass!(Methods => [
     SEARCH_IS_SOME,
     TEMPORARY_CSTRING_AS_PTR,
     FILTER_NEXT,
+    FILTER_MAP_MAP,
     FILTER_MAP,
     FILTER_MAP_NEXT,
     FIND_MAP,
@@ -882,8 +921,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
             ["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]),
             ["is_some", "find"] => lint_search_is_some(cx, expr, "find", arg_lists[1], arg_lists[0]),
             ["is_some", "position"] => lint_search_is_some(cx, expr, "position", arg_lists[1], arg_lists[0]),
@@ -1998,14 +2035,35 @@ fn lint_filter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr,
 fn lint_filter_map<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
     expr: &'tcx hir::Expr,
-    _filter_args: &'tcx [hir::Expr],
-    _map_args: &'tcx [hir::Expr],
+    filter_args: &'tcx [hir::Expr],
+    map_args: &'tcx [hir::Expr],
 ) {
-    // lint if caller of `.filter().map()` is an Iterator
-    if match_trait_method(cx, expr, &paths::ITERATOR) {
-        let msg = "called `filter(p).map(q)` on an `Iterator`. \
-                   This is more succinctly expressed by calling `.filter_map(..)` instead.";
-        span_lint(cx, FILTER_MAP, expr.span, msg);
+    let mut responses: FxHashMap<String, &str> = FxHashMap::default();
+    responses.insert(
+        "is_some".to_string(),
+        "called `filter(p).map(q)` on an `Iterator<Option<_>>`. \
+         Consider calling `.filter_map(..)` instead.",
+    );
+    responses.insert(
+        "is_ok".to_string(),
+        "called `filter(p).map(q)` on an `Iterator<Result<_>>`. \
+         Consider calling `.filter_map(..)` instead.",
+    );
+
+    if_chain! {
+        if match_trait_method(cx, expr, &paths::ITERATOR);
+        if let hir::ExprKind::Closure(_, _, ref map_body_id, _, _) = map_args[1].node;
+        if let hir::ExprKind::MethodCall(ref map_ps, _, _) = cx.tcx.hir().body(*map_body_id).value.node;
+        if map_ps.ident.name == sym!(unwrap);
+        if let hir::ExprKind::Closure(_, _, ref filter_body_id, _, _) = filter_args[1].node;
+        if let hir::ExprKind::MethodCall(ref filter_ps, _, _) = cx.tcx.hir().body(*filter_body_id).value.node;
+        if responses.contains_key(&filter_ps.ident.name.to_string());
+        if let hir::ExprKind::MethodCall(_, _, ref expr_vec) = expr.node;
+        if let hir::ExprKind::MethodCall(_, ref filter_span, _) = expr_vec[0].node;
+        then {
+            let new_span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt());
+            span_lint(cx, FILTER_MAP, new_span, responses[&filter_ps.ident.name.to_string()]);
+        }
     }
 }
 
@@ -2052,43 +2110,18 @@ fn lint_filter_map_map<'a, 'tcx>(
     _filter_args: &'tcx [hir::Expr],
     _map_args: &'tcx [hir::Expr],
 ) {
-    // lint if caller of `.filter().map()` is an Iterator
-    if match_trait_method(cx, expr, &paths::ITERATOR) {
-        let msg = "called `filter_map(p).map(q)` on an `Iterator`. \
-                   This is more succinctly expressed by only calling `.filter_map(..)` instead.";
-        span_lint(cx, FILTER_MAP, expr.span, msg);
-    }
-}
-
-/// lint use of `filter().flat_map()` for `Iterators`
-fn lint_filter_flat_map<'a, 'tcx>(
-    cx: &LateContext<'a, 'tcx>,
-    expr: &'tcx hir::Expr,
-    _filter_args: &'tcx [hir::Expr],
-    _map_args: &'tcx [hir::Expr],
-) {
-    // lint if caller of `.filter().flat_map()` is an Iterator
-    if match_trait_method(cx, expr, &paths::ITERATOR) {
-        let msg = "called `filter(p).flat_map(q)` on an `Iterator`. \
-                   This is more succinctly expressed by calling `.flat_map(..)` \
-                   and filtering by returning an empty Iterator.";
-        span_lint(cx, FILTER_MAP, expr.span, msg);
+    let mut span = expr.span;
+    if let hir::ExprKind::MethodCall(_, _, ref expr_vec) = expr.node {
+        if let hir::ExprKind::MethodCall(_, ref filter_span, _) = expr_vec[0].node {
+            span = Span::new(filter_span.lo(), expr.span.hi(), expr.span.ctxt());
+        }
     }
-}
 
-/// lint use of `filter_map().flat_map()` for `Iterators`
-fn lint_filter_map_flat_map<'a, 'tcx>(
-    cx: &LateContext<'a, 'tcx>,
-    expr: &'tcx hir::Expr,
-    _filter_args: &'tcx [hir::Expr],
-    _map_args: &'tcx [hir::Expr],
-) {
-    // lint if caller of `.filter_map().flat_map()` is an Iterator
+    // lint if caller of `.filter_map().map()` is an Iterator
     if match_trait_method(cx, expr, &paths::ITERATOR) {
-        let msg = "called `filter_map(p).flat_map(q)` on an `Iterator`. \
-                   This is more succinctly expressed by calling `.flat_map(..)` \
-                   and filtering by returning an empty Iterator.";
-        span_lint(cx, FILTER_MAP, expr.span, msg);
+        let msg = "called `filter_map(p).map(q)` on an `Iterator`. \
+                   This is more succinctly expressed by only calling `.filter_map(..)` instead.";
+        span_lint(cx, FILTER_MAP_MAP, span, msg);
     }
 }
 

src/lintlist/mod.rs

@@ -6,7 +6,7 @@ pub use lint::Lint;
 pub use lint::LINT_LEVELS;
 
 // begin lint list, do not remove this comment, it’s used in `update_lints`
-pub const ALL_LINTS: [Lint; 305] = [
+pub const ALL_LINTS: [Lint; 306] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -528,7 +528,14 @@ pub const ALL_LINTS: [Lint; 305] = [
     Lint {
         name: "filter_map",
         group: "pedantic",
-        desc: "using combinations of `filter`, `map`, `filter_map` and `flat_map` which can usually be written as a single method call",
+        desc: "using `filter` and `map` to destructure an iterator of Options or a Results instead of a single `filter_map` call",
+        deprecation: None,
+        module: "methods",
+    },
+    Lint {
+        name: "filter_map_map",
+        group: "pedantic",
+        desc: "using `filter_map` followed by `map` can more concisely be written as a single method call",
         deprecation: None,
         module: "methods",
     },

tests/ui/filter_methods.rs

@@ -1,24 +1,48 @@
 #![warn(clippy::all, clippy::pedantic)]
 #![allow(clippy::missing_docs_in_private_items)]
+#![allow(clippy::redundant_closure)]
+#![allow(clippy::unnecessary_filter_map)]
 
 fn main() {
-    let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
+    let options: Vec<Option<i8>> = vec![Some(5), None, Some(6)];
+    let results = ["1", "lol", "3", "NaN", "5"];
 
-    let _: Vec<_> = vec![5_i8; 6]
-        .into_iter()
-        .filter(|&x| x == 0)
-        .flat_map(|x| x.checked_mul(2))
+    // validate iterator of values triggers filter_map + map lint
+    let _: Vec<_> = vec![5_i8; 6].into_iter()
+        .filter_map(|x| x.checked_mul(2))
+        .map(|x| x.checked_mul(2))
         .collect();
 
-    let _: Vec<_> = vec![5_i8; 6]
-        .into_iter()
-        .filter_map(|x| x.checked_mul(2))
-        .flat_map(|x| x.checked_mul(2))
+    // validate iterator of options triggers filter + map lint
+    let _: Vec<i8> = options.clone().into_iter()
+        .filter(|x| x.is_some())
+        .map(|x| x.unwrap())
         .collect();
 
-    let _: Vec<_> = vec![5_i8; 6]
-        .into_iter()
-        .filter_map(|x| x.checked_mul(2))
-        .map(|x| x.checked_mul(2))
+    // validate iterator of options triggers filter + flat_map lint
+    let _: Vec<Option<i8>> = std::iter::repeat(options.clone())
+        .take(5)
+        .map(|v| Some(v.into_iter()))
+        .filter(|x| x.is_some())
+        .flat_map(|x| x.unwrap())
+        .collect();
+
+    // validate iterator of results triggers filter + map lint
+    let _: Vec<i8> =  results.iter()
+        .map(|s| s.parse())
+        .filter(|s| s.is_ok())
+        .map(|s| s.unwrap())
+        .collect();
+
+    // validate iterator of values **does not** trigger filter + map lint
+    let _: Vec<String> =  results.iter()
+        .filter(|s| s.len() > 3)
+        .map(|s| format!("{}{}", s, s))
+        .collect();
+
+    // validate iterator of values **does not** trigger filter + flat_map lint
+    let _: String = results.iter()
+        .filter(|i| i.len() > 1)
+        .flat_map(|s| s.chars())
         .collect();
 }

tests/ui/filter_methods.stderr

@@ -1,40 +1,58 @@
-error: called `filter(p).map(q)` on an `Iterator`. This is more succinctly expressed by calling `.filter_map(..)` instead.
-  --> $DIR/filter_methods.rs:5:21
+error: redundant closure found
+  --> $DIR/filter_methods.rs:19:14
    |
-LL |     let _: Vec<_> = vec![5; 6].into_iter().filter(|&x| x == 0).map(|x| x * 2).collect();
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+LL |         .map(|x| x.unwrap())
+   |              ^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::unwrap`
+   |
+   = note: `-D clippy::redundant-closure-for-method-calls` implied by `-D warnings`
+
+error: called `filter(p).map(q)` on an `Iterator<Option<_>>`. Consider calling `.filter_map(..)` instead.
+  --> $DIR/filter_methods.rs:18:10
+   |
+LL |           .filter(|x| x.is_some())
+   |  __________^
+LL | |         .map(|x| x.unwrap())
+   | |____________________________^
    |
    = note: `-D clippy::filter-map` implied by `-D warnings`
 
-error: called `filter(p).flat_map(q)` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` and filtering by returning an empty Iterator.
-  --> $DIR/filter_methods.rs:7:21
-   |
-LL |       let _: Vec<_> = vec![5_i8; 6]
-   |  _____________________^
-LL | |         .into_iter()
-LL | |         .filter(|&x| x == 0)
-LL | |         .flat_map(|x| x.checked_mul(2))
-   | |_______________________________________^
-
-error: called `filter_map(p).flat_map(q)` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` and filtering by returning an empty Iterator.
-  --> $DIR/filter_methods.rs:13:21
-   |
-LL |       let _: Vec<_> = vec![5_i8; 6]
-   |  _____________________^
-LL | |         .into_iter()
-LL | |         .filter_map(|x| x.checked_mul(2))
-LL | |         .flat_map(|x| x.checked_mul(2))
-   | |_______________________________________^
-
-error: called `filter_map(p).map(q)` on an `Iterator`. This is more succinctly expressed by only calling `.filter_map(..)` instead.
-  --> $DIR/filter_methods.rs:19:21
-   |
-LL |       let _: Vec<_> = vec![5_i8; 6]
-   |  _____________________^
-LL | |         .into_iter()
-LL | |         .filter_map(|x| x.checked_mul(2))
-LL | |         .map(|x| x.checked_mul(2))
-   | |__________________________________^
-
-error: aborting due to 4 previous errors
+error: redundant closure found
+  --> $DIR/filter_methods.rs:18:17
+   |
+LL |         .filter(|x| x.is_some())
+   |                 ^^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::is_some`
+
+error: redundant closure found
+  --> $DIR/filter_methods.rs:27:19
+   |
+LL |         .flat_map(|x| x.unwrap())
+   |                   ^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::unwrap`
+
+error: redundant closure found
+  --> $DIR/filter_methods.rs:26:17
+   |
+LL |         .filter(|x| x.is_some())
+   |                 ^^^^^^^^^^^^^^^ help: remove closure as shown: `std::option::Option::is_some`
+
+error: redundant closure found
+  --> $DIR/filter_methods.rs:34:14
+   |
+LL |         .map(|s| s.unwrap())
+   |              ^^^^^^^^^^^^^^ help: remove closure as shown: `std::result::Result::unwrap`
+
+error: called `filter(p).map(q)` on an `Iterator<Result<_>>`. Consider calling `.filter_map(..)` instead.
+  --> $DIR/filter_methods.rs:33:10
+   |
+LL |           .filter(|s| s.is_ok())
+   |  __________^
+LL | |         .map(|s| s.unwrap())
+   | |____________________________^
+
+error: redundant closure found
+  --> $DIR/filter_methods.rs:33:17
+   |
+LL |         .filter(|s| s.is_ok())
+   |                 ^^^^^^^^^^^^^ help: remove closure as shown: `std::result::Result::is_ok`
+
+error: aborting due to 8 previous errors