Auto merge of rust-lang#7101 - camsteffen:flat-map-option, r=giraffate

Add flat_map_option lint

changelog: Add flat_map_option lint

Closes rust-lang#2241

Author: bors

CHANGELOG.md

@@ -2216,6 +2216,7 @@ Released 2018-09-13
 [`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
 [`flat_map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#flat_map_identity
+[`flat_map_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#flat_map_option
 [`float_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_arithmetic
 [`float_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp
 [`float_cmp_const`]: https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp_const

clippy_lints/src/lib.rs

@@ -770,6 +770,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         methods::FILTER_MAP_NEXT,
         methods::FILTER_NEXT,
         methods::FLAT_MAP_IDENTITY,
+        methods::FLAT_MAP_OPTION,
         methods::FROM_ITER_INSTEAD_OF_COLLECT,
         methods::GET_UNWRAP,
         methods::IMPLICIT_CLONE,
@@ -1383,6 +1384,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(matches::SINGLE_MATCH_ELSE),
         LintId::of(methods::CLONED_INSTEAD_OF_COPIED),
         LintId::of(methods::FILTER_MAP_NEXT),
+        LintId::of(methods::FLAT_MAP_OPTION),
         LintId::of(methods::IMPLICIT_CLONE),
         LintId::of(methods::INEFFICIENT_TO_STRING),
         LintId::of(methods::MAP_FLATTEN),

clippy_lints/src/matches.rs

@@ -1509,7 +1509,7 @@ fn opt_parent_let<'a>(cx: &LateContext<'a>, ex: &Expr<'a>) -> Option<&'a Local<'
 /// Gets all arms that are unbounded `PatRange`s.
 fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) -> Vec<SpannedRange<Constant>> {
     arms.iter()
-        .flat_map(|arm| {
+        .filter_map(|arm| {
             if let Arm { pat, guard: None, .. } = *arm {
                 if let PatKind::Range(ref lhs, ref rhs, range_end) = pat.kind {
                     let lhs = match lhs {

clippy_lints/src/methods/flat_map_option.rs

@@ -0,0 +1,34 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::is_trait_method;
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use rustc_span::{source_map::Span, sym};
+
+use super::FLAT_MAP_OPTION;
+use clippy_utils::ty::is_type_diagnostic_item;
+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, arg: &'tcx hir::Expr<'_>, span: Span) {
+    if !is_trait_method(cx, expr, sym::Iterator) {
+        return;
+    }
+    let arg_ty = cx.typeck_results().expr_ty_adjusted(arg);
+    let sig = match arg_ty.kind() {
+        ty::Closure(_, substs) => substs.as_closure().sig(),
+        _ if arg_ty.is_fn() => arg_ty.fn_sig(cx.tcx),
+        _ => return,
+    };
+    if !is_type_diagnostic_item(cx, sig.output().skip_binder(), sym::option_type) {
+        return;
+    }
+    span_lint_and_sugg(
+        cx,
+        FLAT_MAP_OPTION,
+        span,
+        "used `flat_map` where `filter_map` could be used instead",
+        "try",
+        "filter_map".into(),
+        Applicability::MachineApplicable,
+    )
+}

clippy_lints/src/methods/mod.rs

@@ -17,6 +17,7 @@ mod filter_map_identity;
 mod filter_map_next;
 mod filter_next;
 mod flat_map_identity;
+mod flat_map_option;
 mod from_iter_instead_of_collect;
 mod get_unwrap;
 mod implicit_clone;
@@ -97,6 +98,29 @@ declare_clippy_lint! {
     "used `cloned` where `copied` could be used instead"
 }
 
+declare_clippy_lint! {
+    /// **What it does:** Checks for usages of `Iterator::flat_map()` where `filter_map()` could be
+    /// used instead.
+    ///
+    /// **Why is this bad?** When applicable, `filter_map()` is more clear since it shows that
+    /// `Option` is used to produce 0 or 1 items.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    ///
+    /// ```rust
+    /// let nums: Vec<i32> = ["1", "2", "whee!"].iter().flat_map(|x| x.parse().ok()).collect();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let nums: Vec<i32> = ["1", "2", "whee!"].iter().filter_map(|x| x.parse().ok()).collect();
+    /// ```
+    pub FLAT_MAP_OPTION,
+    pedantic,
+    "used `flat_map` where `filter_map` could be used instead"
+}
+
 declare_clippy_lint! {
     /// **What it does:** Checks for `.unwrap()` calls on `Option`s and on `Result`s.
     ///
@@ -1663,6 +1687,7 @@ impl_lint_pass!(Methods => [
     CLONE_ON_REF_PTR,
     CLONE_DOUBLE_REF,
     CLONED_INSTEAD_OF_COPIED,
+    FLAT_MAP_OPTION,
     INEFFICIENT_TO_STRING,
     NEW_RET_NO_SELF,
     SINGLE_CHAR_PATTERN,
@@ -1958,7 +1983,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 unnecessary_filter_map::check(cx, expr, arg);
                 filter_map_identity::check(cx, expr, arg, span);
             },
-            ("flat_map", [flm_arg]) => flat_map_identity::check(cx, expr, flm_arg, span),
+            ("flat_map", [arg]) => {
+                flat_map_identity::check(cx, expr, arg, span);
+                flat_map_option::check(cx, expr, arg, span);
+            },
             ("flatten", []) => {
                 if let Some(("map", [recv, map_arg], _)) = method_call!(recv) {
                     map_flatten::check(cx, expr, recv, map_arg);

clippy_utils/src/attrs.rs

@@ -154,6 +154,6 @@ pub fn is_doc_hidden(attrs: &[ast::Attribute]) -> bool {
     attrs
         .iter()
         .filter(|attr| attr.has_name(sym::doc))
-        .flat_map(ast::Attribute::meta_item_list)
+        .filter_map(ast::Attribute::meta_item_list)
         .any(|l| attr::list_contains_name(&l, sym::hidden))
 }

tests/ui/flat_map_option.fixed

@@ -0,0 +1,13 @@
+// run-rustfix
+#![warn(clippy::flat_map_option)]
+#![allow(clippy::redundant_closure, clippy::unnecessary_filter_map)]
+
+fn main() {
+    // yay
+    let c = |x| Some(x);
+    let _ = [1].iter().filter_map(c);
+    let _ = [1].iter().filter_map(Some);
+
+    // nay
+    let _ = [1].iter().flat_map(|_| &Some(1));
+}

tests/ui/flat_map_option.rs

@@ -0,0 +1,13 @@
+// run-rustfix
+#![warn(clippy::flat_map_option)]
+#![allow(clippy::redundant_closure, clippy::unnecessary_filter_map)]
+
+fn main() {
+    // yay
+    let c = |x| Some(x);
+    let _ = [1].iter().flat_map(c);
+    let _ = [1].iter().flat_map(Some);
+
+    // nay
+    let _ = [1].iter().flat_map(|_| &Some(1));
+}

tests/ui/flat_map_option.stderr

@@ -0,0 +1,16 @@
+error: used `flat_map` where `filter_map` could be used instead
+  --> $DIR/flat_map_option.rs:8:24
+   |
+LL |     let _ = [1].iter().flat_map(c);
+   |                        ^^^^^^^^ help: try: `filter_map`
+   |
+   = note: `-D clippy::flat-map-option` implied by `-D warnings`
+
+error: used `flat_map` where `filter_map` could be used instead
+  --> $DIR/flat_map_option.rs:9:24
+   |
+LL |     let _ = [1].iter().flat_map(Some);
+   |                        ^^^^^^^^ help: try: `filter_map`
+
+error: aborting due to 2 previous errors
+