Remove filter + flat_map lint

Author: waynr

clippy_lints/src/methods/mod.rs

@@ -280,32 +280,13 @@ declare_clippy_lint! {
     "using combinations of `flatten` and `map` which can usually be written as a single method call"
 }
 
-/// **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.
 ///
-/// **Known problems:** Often requires a condition + Option/Iterator creation
-/// inside the closure.
-///
-/// ** filter + map Example:**
-/// let ns = [-9, 2, 3, 4, 5, 6];
-/// let correct: Vec<i32> = ns.iter()
-///     .cloned()
-///     .filter(|&x| x > 0)
-///     .map(|x| x * 2)
-///     .collect();
-///
-/// let more_correct: Vec<i32> = ns.iter()
-///     .cloned()
-///     .filter_map(|x| if x > 0 { Some(x * 2) } else { None })
-///     .collect();
-///
-/// println!("{:?}", more_correct);
-/// assert_eq!(correct, more_correct);
-///
-/// ** filter_map + map Example:**
+/// ** Example:**
 /// let ws: Vec<String> = ["is", "this", "shizzle", "for", "rizzle"]
 ///             .iter()
 ///             .map(|s| s.to_string())
@@ -331,57 +312,36 @@ declare_clippy_lint! {
 /// println!("{:?}", correct);
 /// assert_eq!(correct, more_correct);
 ///
-/// ** filter + flat_map Example:**
-/// let ws: Vec<String> = ["is", "this", "shizzle", "for", "rizzle"]
-///             .iter()
-///             .map(|s| s.to_string())
-///             .collect();
-///
-/// let correct: String = ws.iter()
-///     .filter(|s| s.contains("izzle"))
-///     .flat_map(|s| s.chars())
-///     .collect();
-///
-/// let more_correct: String = ws.iter()
-///     .flat_map(|s| if s.contains("izzle") {
-///         return s.chars()
-///     } else {
-///         "".chars()
-///     })
-///     .collect();
-///
-/// println!("{:?}", correct);
-/// assert_eq!(correct, more_correct);
+declare_clippy_lint! {
+    pub FILTER_MAP_MAP,
+    pedantic,
+    "using `filter_map` followed by `map` can more concisely be written as a single method call"
+}
+
+/// **What it does:** Checks for usage of `_.filter(_).map(_)` used to destructure
+/// `Iterator<Option<_>>` and `Iterator<Result<_>>` into `Iterator<_>` (an iterator of values).
 ///
-/// ** filter_map + flat_map Example:**
-/// let ws: Vec<String> = ["is", "this", "shizzle", "for", "rizzle"]
-///             .iter()
-///             .map(|s| s.to_string())
-///             .collect();
+/// **Why is this bad?** Readability, this can be written more concisely as `_.filter_map(|x| x)`.
 ///
-/// let correct: String = ws.iter()
-///     .filter_map(|s| if s.contains("izzle"){
-///         Some(s)
-///     } else {
-///         None
-///     })
-///     .flat_map(|s| s.chars())
+/// ** Example:**
+/// let ns = [Some(-9), Some(2), None, Some(3), Some(4), None, Some(5), Some(6)];
+/// let correct: Vec<i32> = ns.clone().iter()
+///     .filter(|x| x.is_some())
+///     .map(|x| x.unwrap() )
 ///     .collect();
 ///
-/// let more_correct: String = ws.iter()
-///     .flat_map(|s| if s.contains("izzle") {
-///         return s.chars()
-///     } else {
-///         "".chars()
-///     })
+/// let more_correct: Vec<i32> = ns.iter()
+///     .cloned()
+///     .filter_map(|x| x)
 ///     .collect();
 ///
-/// println!("{:?}", correct);
+/// println!("{:?}", more_correct);
 /// assert_eq!(correct, more_correct);
+///
 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"
 }
 
 /// **What it does:** Checks for an iterator search (such as `find()`,
@@ -933,7 +893,6 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
             ["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]),
-            ["flat_map", "filter"] => lint_filter_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]),
@@ -1984,14 +1943,25 @@ fn lint_filter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr,
     }
 }
 
-/// lint filter_map variants that iterate over Result or Option values
-fn lint_filter_map_variant<'a, 'tcx>(
+/// lint use of `filter().map()` for `Iterators`
+fn lint_filter_map<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
     expr: &'tcx hir::Expr,
     filter_args: &'tcx [hir::Expr],
     map_args: &'tcx [hir::Expr],
-    responses: &HashMap<String, &str>,
 ) {
+    let mut responses: HashMap<String, &str> = HashMap::new();
+    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;
@@ -2009,28 +1979,6 @@ fn lint_filter_map_variant<'a, 'tcx>(
     }
 }
 
-/// lint use of `filter().map()` for `Iterators`
-fn lint_filter_map<'a, 'tcx>(
-    cx: &LateContext<'a, 'tcx>,
-    expr: &'tcx hir::Expr,
-    filter_args: &'tcx [hir::Expr],
-    map_args: &'tcx [hir::Expr],
-) {
-    let mut responses: HashMap<String, &str> = HashMap::new();
-    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."
-    );
-
-    lint_filter_map_variant(cx, expr, filter_args, map_args, &responses);
-}
-
 /// lint use of `filter().map()` for `Iterators`
 fn lint_filter_map_map<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
@@ -2049,27 +1997,10 @@ fn lint_filter_map_map<'a, 'tcx>(
     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, span, msg);
+        span_lint(cx, FILTER_MAP_MAP, 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],
-) {
-    let mut responses: HashMap<String, &str> = HashMap::new();
-    responses.insert(
-        "is_some".to_string(),
-        "called `filter(p).flat_map(q)` on an `Iterator<Option<_>>`. \
-         Consider calling `.filter_map(..)` instead."
-    );
-
-    lint_filter_map_variant(cx, expr, filter_args, map_args, &responses);
-}
-
 /// lint searching an Iterator followed by `is_some()`
 fn lint_search_is_some<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,

tests/ui/filter_methods.stderr

@@ -1,28 +1,12 @@
-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:25:10
-   |
-25 |           .filter_map(|x| x.checked_mul(2))
-   |  __________^
-26 | |         .map(|x| x.checked_mul(2))
-   | |__________________________________^
-   |
-   = note: `-D clippy::filter-map` implied by `-D warnings`
-
 error: called `filter(p).map(q)` on an `Iterator<Option<_>>`. Consider calling `.filter_map(..)` instead.
   --> $DIR/filter_methods.rs:31:10
    |
 31 |           .filter(|x| x.is_some())
    |  __________^
 32 | |         .map(|x| x.unwrap())
    | |____________________________^
-
-error: called `filter(p).flat_map(q)` on an `Iterator<Option<_>>`. Consider calling `.filter_map(..)` instead.
-  --> $DIR/filter_methods.rs:39:10
    |
-39 |           .filter(|x| x.is_some())
-   |  __________^
-40 | |         .flat_map(|x| x.unwrap())
-   | |_________________________________^
+   = note: `-D clippy::filter-map` implied by `-D warnings`
 
 error: called `filter(p).map(q)` on an `Iterator<Result<_>>`. Consider calling `.filter_map(..)` instead.
   --> $DIR/filter_methods.rs:46:10
@@ -32,5 +16,5 @@ error: called `filter(p).map(q)` on an `Iterator<Result<_>>`. Consider calling `
 47 | |         .map(|s| s.unwrap())
    | |____________________________^
 
-error: aborting due to 4 previous errors
+error: aborting due to 2 previous errors