Auto merge of rust-lang#11869 - PartiallyTyped:result-filter-map, r=A…

…lexendoo

New Lint: `result_filter_map` / Mirror of `option_filter_map`

Added the `Result` mirror of `option_filter_map`.

changelog: New Lint: [`result_filter_map`]

I had to move around some code because the function def was too long 🙃.

I have also added some pattern checks on `option_filter_map`

CHANGELOG.md

@@ -5470,6 +5470,7 @@ Released 2018-09-13
 [`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization
 [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
 [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used
+[`result_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_filter_map
 [`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
 [`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn

clippy_lints/src/declared_lints.rs

@@ -419,6 +419,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::READ_LINE_WITHOUT_TRIM_INFO,
     crate::methods::REDUNDANT_AS_STR_INFO,
     crate::methods::REPEAT_ONCE_INFO,
+    crate::methods::RESULT_FILTER_MAP_INFO,
     crate::methods::RESULT_MAP_OR_INTO_OPTION_INFO,
     crate::methods::SEARCH_IS_SOME_INFO,
     crate::methods::SEEK_FROM_CURRENT_INFO,

clippy_lints/src/methods/filter_map.rs

@@ -14,7 +14,7 @@ use rustc_span::symbol::{sym, Ident, Symbol};
 use rustc_span::Span;
 use std::borrow::Cow;
 
-use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP};
+use super::{MANUAL_FILTER_MAP, MANUAL_FIND_MAP, OPTION_FILTER_MAP, RESULT_FILTER_MAP};
 
 fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool {
     match &expr.kind {
@@ -46,6 +46,9 @@ fn is_method(cx: &LateContext<'_>, expr: &hir::Expr<'_>, method_name: Symbol) ->
 fn is_option_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool {
     is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
 }
+fn is_ok_filter_map(cx: &LateContext<'_>, filter_arg: &hir::Expr<'_>, map_arg: &hir::Expr<'_>) -> bool {
+    is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_ok))
+}
 
 #[derive(Debug, Copy, Clone)]
 enum OffendingFilterExpr<'tcx> {
@@ -273,6 +276,18 @@ fn is_filter_some_map_unwrap(
     (iterator || option) && is_option_filter_map(cx, filter_arg, map_arg)
 }
 
+/// is `filter(|x| x.is_ok()).map(|x| x.unwrap())`
+fn is_filter_ok_map_unwrap(
+    cx: &LateContext<'_>,
+    expr: &hir::Expr<'_>,
+    filter_arg: &hir::Expr<'_>,
+    map_arg: &hir::Expr<'_>,
+) -> bool {
+    // result has no filter, so we only check for iterators
+    let iterator = is_trait_method(cx, expr, sym::Iterator);
+    iterator && is_ok_filter_map(cx, filter_arg, map_arg)
+}
+
 /// lint use of `filter().map()` or `find().map()` for `Iterators`
 #[allow(clippy::too_many_arguments)]
 pub(super) fn check(
@@ -300,30 +315,21 @@ pub(super) fn check(
         return;
     }
 
-    if is_trait_method(cx, map_recv, sym::Iterator)
-
-        // filter(|x| ...is_some())...
-        && let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind
-        && let filter_body = cx.tcx.hir().body(filter_body_id)
-        && let [filter_param] = filter_body.params
-        // optional ref pattern: `filter(|&x| ..)`
-        && let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
-            (ref_pat, true)
-        } else {
-            (filter_param.pat, false)
-        }
-
-        && let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind
-        && let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id)
+    if is_filter_ok_map_unwrap(cx, expr, filter_arg, map_arg) {
+        span_lint_and_sugg(
+            cx,
+            RESULT_FILTER_MAP,
+            filter_span.with_hi(expr.span.hi()),
+            "`filter` for `Ok` followed by `unwrap`",
+            "consider using `flatten` instead",
+            reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, map_span)).into_owned(),
+            Applicability::MachineApplicable,
+        );
 
-        && let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind
-        && let map_body = cx.tcx.hir().body(map_body_id)
-        && let [map_param] = map_body.params
-        && let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind
+        return;
+    }
 
-        && let Some(check_result) =
-            offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref)
-    {
+    if let Some((map_param_ident, check_result)) = is_find_or_filter(cx, map_recv, filter_arg, map_arg) {
         let span = filter_span.with_hi(expr.span.hi());
         let (filter_name, lint) = if is_find {
             ("find", MANUAL_FIND_MAP)
@@ -395,6 +401,40 @@ pub(super) fn check(
     }
 }
 
+fn is_find_or_filter<'a>(
+    cx: &LateContext<'a>,
+    map_recv: &hir::Expr<'_>,
+    filter_arg: &hir::Expr<'_>,
+    map_arg: &hir::Expr<'_>,
+) -> Option<(Ident, CheckResult<'a>)> {
+    if is_trait_method(cx, map_recv, sym::Iterator)
+        // filter(|x| ...is_some())...
+        && let ExprKind::Closure(&Closure { body: filter_body_id, .. }) = filter_arg.kind
+        && let filter_body = cx.tcx.hir().body(filter_body_id)
+        && let [filter_param] = filter_body.params
+        // optional ref pattern: `filter(|&x| ..)`
+        && let (filter_pat, is_filter_param_ref) = if let PatKind::Ref(ref_pat, _) = filter_param.pat.kind {
+            (ref_pat, true)
+        } else {
+            (filter_param.pat, false)
+        }
+
+        && let PatKind::Binding(_, filter_param_id, _, None) = filter_pat.kind
+        && let Some(mut offending_expr) = OffendingFilterExpr::hir(cx, filter_body.value, filter_param_id)
+
+        && let ExprKind::Closure(&Closure { body: map_body_id, .. }) = map_arg.kind
+        && let map_body = cx.tcx.hir().body(map_body_id)
+        && let [map_param] = map_body.params
+        && let PatKind::Binding(_, map_param_id, map_param_ident, None) = map_param.pat.kind
+
+        && let Some(check_result) =
+            offending_expr.check_map_call(cx, map_body, map_param_id, filter_param_id, is_filter_param_ref)
+    {
+        return Some((map_param_ident, check_result));
+    }
+    None
+}
+
 fn acceptable_methods(method: &PathSegment<'_>) -> bool {
     let methods: [Symbol; 8] = [
         sym::clone,

clippy_lints/src/methods/mod.rs

@@ -1175,7 +1175,8 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for indirect collection of populated `Option`
+    /// Checks for iterators of `Option`s using ``.filter(Option::is_some).map(Option::unwrap)` that may
+    /// be replaced with a `.flatten()` call.
     ///
     /// ### Why is this bad?
     /// `Option` is like a collection of 0-1 things, so `flatten`
@@ -3752,6 +3753,29 @@ declare_clippy_lint! {
     "using `Option.map_or(Err(_), Ok)`, which is more succinctly expressed as `Option.ok_or(_)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for iterators of `Result`s using ``.filter(Result::is_ok).map(Result::unwrap)` that may
+    /// be replaced with a `.flatten()` call.
+    ///
+    /// ### Why is this bad?
+    /// `Result` implements `IntoIterator<Item = T>`. This means that `Result` can be flattened
+    /// automatically without suspicious-looking `unwrap` calls.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let _ = std::iter::empty::<Result<i32, ()>>().filter(Result::is_ok).map(Result::unwrap);
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let _ = std::iter::empty::<Result<i32, ()>>().flatten();
+    /// ```
+    #[clippy::version = "1.76.0"]
+    pub RESULT_FILTER_MAP,
+    complexity,
+    "filtering `Result` for `Ok` then force-unwrapping, which can be one type-safe operation"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -3903,6 +3927,7 @@ impl_lint_pass!(Methods => [
     UNNECESSARY_FALLIBLE_CONVERSIONS,
     JOIN_ABSOLUTE_PATHS,
     OPTION_MAP_OR_ERR_OK,
+    RESULT_FILTER_MAP,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.

tests/ui/option_filter_map.fixed

@@ -3,12 +3,18 @@
 
 fn main() {
     let _ = Some(Some(1)).flatten();
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = Some(Some(1)).flatten();
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = Some(1).map(odds_out).flatten();
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = Some(1).map(odds_out).flatten();
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
 
     let _ = vec![Some(1)].into_iter().flatten();
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = vec![Some(1)].into_iter().flatten();
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = vec![1]
         .into_iter()
         .map(odds_out)

tests/ui/option_filter_map.rs

@@ -3,21 +3,29 @@
 
 fn main() {
     let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
 
     let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
+    //~^ ERROR: `filter` for `Some` followed by `unwrap`
     let _ = vec![1]
         .into_iter()
         .map(odds_out)
         .filter(Option::is_some)
+        //~^ ERROR: `filter` for `Some` followed by `unwrap`
         .map(Option::unwrap);
     let _ = vec![1]
         .into_iter()
         .map(odds_out)
         .filter(|o| o.is_some())
+        //~^ ERROR: `filter` for `Some` followed by `unwrap`
         .map(|o| o.unwrap());
 }
 

tests/ui/option_filter_map.stderr

@@ -8,48 +8,50 @@ LL |     let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
    = help: to override `-D warnings` add `#[allow(clippy::option_filter_map)]`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:6:27
+  --> $DIR/option_filter_map.rs:7:27
    |
 LL |     let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:7:35
+  --> $DIR/option_filter_map.rs:9:35
    |
 LL |     let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:8:35
+  --> $DIR/option_filter_map.rs:11:35
    |
 LL |     let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
    |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:10:39
+  --> $DIR/option_filter_map.rs:14:39
    |
 LL |     let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
    |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:11:39
+  --> $DIR/option_filter_map.rs:16:39
    |
 LL |     let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
    |                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:15:10
+  --> $DIR/option_filter_map.rs:21:10
    |
 LL |           .filter(Option::is_some)
    |  __________^
+LL | |
 LL | |         .map(Option::unwrap);
    | |____________________________^ help: consider using `flatten` instead: `flatten()`
 
 error: `filter` for `Some` followed by `unwrap`
-  --> $DIR/option_filter_map.rs:20:10
+  --> $DIR/option_filter_map.rs:27:10
    |
 LL |           .filter(|o| o.is_some())
    |  __________^
+LL | |
 LL | |         .map(|o| o.unwrap());
    | |____________________________^ help: consider using `flatten` instead: `flatten()`
 

tests/ui/result_filter_map.fixed

@@ -0,0 +1,27 @@
+#![warn(clippy::result_filter_map)]
+#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
+
+fn odds_out(x: i32) -> Result<i32, ()> {
+    if x % 2 == 0 { Ok(x) } else { Err(()) }
+}
+
+fn main() {
+    // Unlike the Option version, Result does not have a filter operation => we will check for iterators
+    // only
+    let _ = vec![Ok(1) as Result<i32, ()>]
+        .into_iter()
+        .flatten();
+
+    let _ = vec![Ok(1) as Result<i32, ()>]
+        .into_iter()
+        .flatten();
+
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .flatten();
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .flatten();
+}

tests/ui/result_filter_map.rs

@@ -0,0 +1,35 @@
+#![warn(clippy::result_filter_map)]
+#![allow(clippy::map_flatten, clippy::unnecessary_map_on_constructor)]
+
+fn odds_out(x: i32) -> Result<i32, ()> {
+    if x % 2 == 0 { Ok(x) } else { Err(()) }
+}
+
+fn main() {
+    // Unlike the Option version, Result does not have a filter operation => we will check for iterators
+    // only
+    let _ = vec![Ok(1) as Result<i32, ()>]
+        .into_iter()
+        .filter(Result::is_ok)
+        //~^ ERROR: `filter` for `Ok` followed by `unwrap`
+        .map(Result::unwrap);
+
+    let _ = vec![Ok(1) as Result<i32, ()>]
+        .into_iter()
+        .filter(|o| o.is_ok())
+        //~^ ERROR: `filter` for `Ok` followed by `unwrap`
+        .map(|o| o.unwrap());
+
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .filter(Result::is_ok)
+        //~^ ERROR: `filter` for `Ok` followed by `unwrap`
+        .map(Result::unwrap);
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .filter(|o| o.is_ok())
+        //~^ ERROR: `filter` for `Ok` followed by `unwrap`
+        .map(|o| o.unwrap());
+}

tests/ui/result_filter_map.stderr

@@ -0,0 +1,41 @@
+error: `filter` for `Ok` followed by `unwrap`
+  --> $DIR/result_filter_map.rs:13:10
+   |
+LL |           .filter(Result::is_ok)
+   |  __________^
+LL | |
+LL | |         .map(Result::unwrap);
+   | |____________________________^ help: consider using `flatten` instead: `flatten()`
+   |
+   = note: `-D clippy::result-filter-map` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::result_filter_map)]`
+
+error: `filter` for `Ok` followed by `unwrap`
+  --> $DIR/result_filter_map.rs:19:10
+   |
+LL |           .filter(|o| o.is_ok())
+   |  __________^
+LL | |
+LL | |         .map(|o| o.unwrap());
+   | |____________________________^ help: consider using `flatten` instead: `flatten()`
+
+error: `filter` for `Ok` followed by `unwrap`
+  --> $DIR/result_filter_map.rs:26:10
+   |
+LL |           .filter(Result::is_ok)
+   |  __________^
+LL | |
+LL | |         .map(Result::unwrap);
+   | |____________________________^ help: consider using `flatten` instead: `flatten()`
+
+error: `filter` for `Ok` followed by `unwrap`
+  --> $DIR/result_filter_map.rs:32:10
+   |
+LL |           .filter(|o| o.is_ok())
+   |  __________^
+LL | |
+LL | |         .map(|o| o.unwrap());
+   | |____________________________^ help: consider using `flatten` instead: `flatten()`
+
+error: aborting due to 4 previous errors
+