Suggest flatten instead of is_some -> unwrap

CHANGELOG.md

@@ -2389,6 +2389,7 @@ Released 2018-09-13
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
 [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
 [`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap
+[`option_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_filter_map
 [`option_if_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else
 [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none
 [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 400 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 450 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
 You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.

clippy_lints/src/lib.rs

@@ -804,6 +804,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &methods::NEW_RET_NO_SELF,
         &methods::OK_EXPECT,
         &methods::OPTION_AS_REF_DEREF,
+        &methods::OPTION_FILTER_MAP,
         &methods::OPTION_MAP_OR_NONE,
         &methods::OR_FUN_CALL,
         &methods::RESULT_MAP_OR_INTO_OPTION,
@@ -1596,6 +1597,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::NEW_RET_NO_SELF),
         LintId::of(&methods::OK_EXPECT),
         LintId::of(&methods::OPTION_AS_REF_DEREF),
+        LintId::of(&methods::OPTION_FILTER_MAP),
         LintId::of(&methods::OPTION_MAP_OR_NONE),
         LintId::of(&methods::OR_FUN_CALL),
         LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
@@ -1891,6 +1893,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&methods::MANUAL_FILTER_MAP),
         LintId::of(&methods::MANUAL_FIND_MAP),
         LintId::of(&methods::OPTION_AS_REF_DEREF),
+        LintId::of(&methods::OPTION_FILTER_MAP),
         LintId::of(&methods::SEARCH_IS_SOME),
         LintId::of(&methods::SKIP_WHILE_NEXT),
         LintId::of(&methods::SUSPICIOUS_MAP),

clippy_lints/src/methods/filter_map.rs

@@ -1,22 +1,99 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::snippet;
+use clippy_utils::source::{indent_of, reindent_multiline, snippet};
-use clippy_utils::{is_trait_method, path_to_local_id, SpanlessEq};
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::{is_trait_method, path_to_local_id, remove_blocks, SpanlessEq};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::{Expr, ExprKind, PatKind, UnOp};
+use rustc_hir::def::Res;
+use rustc_hir::{Expr, ExprKind, PatKind, QPath, UnOp};
 use rustc_lint::LateContext;
 use rustc_middle::ty::TyS;
-use rustc_span::symbol::sym;
+use rustc_span::source_map::Span;
+use rustc_span::symbol::{sym, Symbol};
+use std::borrow::Cow;
 
 use super::MANUAL_FILTER_MAP;
 use super::MANUAL_FIND_MAP;
+use super::OPTION_FILTER_MAP;
+
+fn is_method<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>, method_name: Symbol) -> bool {
+    match &expr.kind {
+        hir::ExprKind::Path(QPath::TypeRelative(_, ref mname)) => mname.ident.name == method_name,
+        hir::ExprKind::Path(QPath::Resolved(_, segments)) => {
+            segments.segments.last().unwrap().ident.name == method_name
+        },
+        hir::ExprKind::Closure(_, _, c, _, _) => {
+            let body = cx.tcx.hir().body(*c);
+            let closure_expr = remove_blocks(&body.value);
+            let arg_id = body.params[0].pat.hir_id;
+            match closure_expr.kind {
+                hir::ExprKind::MethodCall(hir::PathSegment { ident, .. }, _, ref args, _) => {
+                    if_chain! {
+                    if ident.name == method_name;
+                    if let hir::ExprKind::Path(path) = &args[0].kind;
+                    if let Res::Local(ref local) = cx.qpath_res(path, args[0].hir_id);
+                    then {
+                        return arg_id == *local
+                    }
+                    }
+                    false
+                },
+                _ => false,
+            }
+        },
+        _ => false,
+    }
+}
+
+fn is_option_filter_map<'tcx>(
+    cx: &LateContext<'tcx>,
+    filter_arg: &'tcx hir::Expr<'_>,
+    map_arg: &'tcx hir::Expr<'_>,
+) -> bool {
+    is_method(cx, map_arg, sym::unwrap) && is_method(cx, filter_arg, sym!(is_some))
+}
+
+/// lint use of `filter().map()` for `Iterators`
+fn lint_filter_some_map_unwrap<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'_>,
+    filter_recv: &'tcx hir::Expr<'_>,
+    filter_arg: &'tcx hir::Expr<'_>,
+    map_arg: &'tcx hir::Expr<'_>,
+    target_span: Span,
+    methods_span: Span,
+) {
+    let iterator = is_trait_method(cx, expr, sym::Iterator);
+    let option = is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(&filter_recv), sym::option_type);
+    if (iterator || option) && is_option_filter_map(cx, filter_arg, map_arg) {
+        let msg = "`filter` for `Some` followed by `unwrap`";
+        let help = "consider using `flatten` instead";
+        let sugg = format!(
+            "{}",
+            reindent_multiline(Cow::Borrowed("flatten()"), true, indent_of(cx, target_span),)
+        );
+        span_lint_and_sugg(
+            cx,
+            OPTION_FILTER_MAP,
+            methods_span,
+            msg,
+            help,
+            sugg,
+            Applicability::MachineApplicable,
+        );
+    }
+}
 
 /// lint use of `filter().map()` or `find().map()` for `Iterators`
-pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, is_find: bool) {
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, is_find: bool, target_span: Span) {
     if_chain! {
             if let ExprKind::MethodCall(_, _, [map_recv, map_arg], map_span) = expr.kind;
-        if let ExprKind::MethodCall(_, _, [_, filter_arg], filter_span) = map_recv.kind;
+            if let ExprKind::MethodCall(_, _, [filter_recv, filter_arg], filter_span) = map_recv.kind;
+            then {
+              lint_filter_some_map_unwrap(cx, expr, filter_recv, filter_arg,
+                map_arg, target_span, filter_span.to(map_span));
+              if_chain! {
             if is_trait_method(cx, map_recv, sym::Iterator);
 
             // filter(|x| ...is_some())...
@@ -85,3 +162,5 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, is_
             }
         }
         }
+    }
+}

clippy_lints/src/methods/mod.rs

@@ -896,6 +896,28 @@ declare_clippy_lint! {
     "using `Iterator::step_by(0)`, which will panic at runtime"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for indirect collection of populated `Option`
+    ///
+    /// **Why is this bad?** `Option` is like a collection of 0-1 things, so `flatten`
+    /// automatically does this without suspicious-looking `unwrap` calls.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let _ = std::iter::empty::<Option<i32>>().filter(Option::is_some).map(Option::unwrap);
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let _ = std::iter::empty::<Option<i32>>().flatten();
+    /// ```
+    pub OPTION_FILTER_MAP,
+    complexity,
+    "filtering `Option` for `Some` then force-unwrapping, which can be one type-safe operation"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for the use of `iter.nth(0)`.
     ///
@@ -1651,6 +1673,7 @@ impl_lint_pass!(Methods => [
     FILTER_MAP_IDENTITY,
     MANUAL_FILTER_MAP,
     MANUAL_FIND_MAP,
+    OPTION_FILTER_MAP,
     FILTER_MAP_NEXT,
     FLAT_MAP_IDENTITY,
     MAP_FLATTEN,
@@ -1720,10 +1743,10 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             ["next", "filter"] => filter_next::check(cx, expr, arg_lists[1]),
             ["next", "skip_while"] => skip_while_next::check(cx, expr, arg_lists[1]),
             ["next", "iter"] => iter_next_slice::check(cx, expr, arg_lists[1]),
-            ["map", "filter"] => filter_map::check(cx, expr, false),
+            ["map", "filter"] => filter_map::check(cx, expr, false, method_spans[0]),
             ["map", "filter_map"] => filter_map_map::check(cx, expr),
             ["next", "filter_map"] => filter_map_next::check(cx, expr, arg_lists[1], self.msrv.as_ref()),
-            ["map", "find"] => filter_map::check(cx, expr, true),
+            ["map", "find"] => filter_map::check(cx, expr, true, method_spans[0]),
             ["flat_map", "filter"] => filter_flat_map::check(cx, expr),
             ["flat_map", "filter_map"] => filter_map_flat_map::check(cx, expr),
             ["flat_map", ..] => flat_map_identity::check(cx, expr, arg_lists[0], method_spans[0]),

tests/ui/option_filter_map.fixed

@@ -0,0 +1,23 @@
+#![warn(clippy::option_filter_map)]
+// run-rustfix
+fn odds_out(x: i32) -> Option<i32> {
+    if x % 2 == 0 { Some(x) } else { None }
+}
+
+fn main() {
+    let _ = Some(Some(1)).flatten();
+    let _ = Some(Some(1)).flatten();
+    let _ = Some(1).map(odds_out).flatten();
+    let _ = Some(1).map(odds_out).flatten();
+
+    let _ = vec![Some(1)].into_iter().flatten();
+    let _ = vec![Some(1)].into_iter().flatten();
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .flatten();
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .flatten();
+}

tests/ui/option_filter_map.rs

@@ -0,0 +1,25 @@
+#![warn(clippy::option_filter_map)]
+// run-rustfix
+fn odds_out(x: i32) -> Option<i32> {
+    if x % 2 == 0 { Some(x) } else { None }
+}
+
+fn main() {
+    let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
+    let _ = Some(Some(1)).filter(|o| o.is_some()).map(|o| o.unwrap());
+    let _ = Some(1).map(odds_out).filter(Option::is_some).map(Option::unwrap);
+    let _ = Some(1).map(odds_out).filter(|o| o.is_some()).map(|o| o.unwrap());
+
+    let _ = vec![Some(1)].into_iter().filter(Option::is_some).map(Option::unwrap);
+    let _ = vec![Some(1)].into_iter().filter(|o| o.is_some()).map(|o| o.unwrap());
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .filter(Option::is_some)
+        .map(Option::unwrap);
+    let _ = vec![1]
+        .into_iter()
+        .map(odds_out)
+        .filter(|o| o.is_some())
+        .map(|o| o.unwrap());
+}

tests/ui/option_filter_map.stderr

@@ -0,0 +1,56 @@
+error: `filter` for `Some` followed by `unwrap`
+  --> $DIR/option_filter_map.rs:8:27
+   |
+LL |     let _ = Some(Some(1)).filter(Option::is_some).map(Option::unwrap);
+   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `flatten` instead: `flatten()`
+   |
+   = note: `-D clippy::option-filter-map` implied by `-D warnings`
+
+error: `filter` for `Some` followed by `unwrap`
+  --> $DIR/option_filter_map.rs:9: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:10: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: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:13: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:14: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:18:10
+   |
+LL |           .filter(Option::is_some)
+   |  __________^
+LL | |         .map(Option::unwrap);
+   | |____________________________^ help: consider using `flatten` instead: `flatten()`
+
+error: `filter` for `Some` followed by `unwrap`
+  --> $DIR/option_filter_map.rs:23:10
+   |
+LL |           .filter(|o| o.is_some())
+   |  __________^
+LL | |         .map(|o| o.unwrap());
+   | |____________________________^ help: consider using `flatten` instead: `flatten()`
+
+error: aborting due to 8 previous errors
+