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};
@@ -272,8 +273,70 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for usage of `_.filter(_).map(_)`,
-    /// `_.filter(_).flat_map(_)`, `_.filter_map(_).flat_map(_)` and similar.
+    /// **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:**
+    /// ```rust
+    /// 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);
+    /// ```
+    ///             .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(_)` to destructure an iterator of
+    /// Options or Results instead of a single `_.filter_map(_)` call.
     ///
     /// **Why is this bad?** Readability, this can be written more concisely as a
     /// single method call.
@@ -287,7 +350,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 Results instead of a single `filter_map` call"
 }
 
 declare_clippy_lint! {
@@ -842,6 +905,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 +946,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 +2060,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 +2135,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,55 @@
 #![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();
 
+    // 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();
+
+    // validate filter_map + flat_map **does not** trigger linter
     let _: Vec<_> = vec![5_i8; 6]
         .into_iter()
         .filter_map(|x| x.checked_mul(2))
-        .map(|x| x.checked_mul(2))
+        .flat_map(|x| x.checked_mul(2))
         .collect();
 }