Merge pull request #3203 from yaahallo/master

Lint for chaining flatten after map

Author: flip1995

CHANGELOG.md

@@ -736,6 +736,7 @@ All notable changes to this project will be documented in this file.
 [`many_single_char_names`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#many_single_char_names
 [`map_clone`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#map_clone
 [`map_entry`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#map_entry
+[`map_flatten`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#map_flatten
 [`match_as_ref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#match_as_ref
 [`match_bool`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#match_bool
 [`match_overlapping_arm`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#match_overlapping_arm

README.md

@@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 277 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
+[There are 278 lints included in this crate!](https://rust-lang-nursery.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

@@ -473,6 +473,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         loops::EXPLICIT_ITER_LOOP,
         matches::SINGLE_MATCH_ELSE,
         methods::FILTER_MAP,
+        methods::MAP_FLATTEN,
         methods::OPTION_MAP_UNWRAP_OR,
         methods::OPTION_MAP_UNWRAP_OR_ELSE,
         methods::RESULT_MAP_UNWRAP_OR_ELSE,

clippy_lints/src/methods.rs

@@ -1,22 +1,24 @@
-use matches::matches;
 use crate::rustc::hir;
-use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass, in_external_macro, Lint, LintContext};
+use crate::rustc::hir::def::Def;
+use crate::rustc::lint::{in_external_macro, LateContext, LateLintPass, Lint, LintArray, LintContext, LintPass};
+use crate::rustc::ty::{self, Ty};
 use crate::rustc::{declare_tool_lint, lint_array};
+use crate::rustc_errors::Applicability;
+use crate::syntax::ast;
+use crate::syntax::source_map::{BytePos, Span};
+use crate::utils::paths;
+use crate::utils::sugg;
+use crate::utils::{
+    get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self, is_self_ty,
+    iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type,
+    match_var, method_chain_args, remove_blocks, return_ty, same_tys, single_segment_path, snippet, span_lint,
+    span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq,
+};
 use if_chain::if_chain;
-use crate::rustc::ty::{self, Ty};
-use crate::rustc::hir::def::Def;
+use matches::matches;
 use std::borrow::Cow;
 use std::fmt;
 use std::iter;
-use crate::syntax::ast;
-use crate::syntax::source_map::{Span, BytePos};
-use crate::utils::{get_arg_name, get_trait_def_id, implements_trait, in_macro, is_copy, is_expn_of, is_self,
-            is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method,
-            match_type, method_chain_args, match_var, return_ty, remove_blocks, same_tys, single_segment_path, snippet,
-            span_lint, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth, SpanlessEq};
-use crate::utils::paths;
-use crate::utils::sugg;
-use crate::rustc_errors::Applicability;
 
 #[derive(Clone)]
 pub struct Pass;
@@ -247,6 +249,24 @@ declare_clippy_lint! {
     "using `filter(p).next()`, which is more succinctly expressed as `.find(p)`"
 }
 
+/// **What it does:** Checks for usage of `_.map(_).flatten(_)`,
+///
+/// **Why is this bad?** Readability, this can be written more concisely as a
+/// single method call.
+///
+/// **Known problems:**
+///
+/// **Example:**
+/// ```rust
+/// iter.map(|x| x.iter()).flatten()
+/// ```
+declare_clippy_lint! {
+    pub MAP_FLATTEN,
+    pedantic,
+    "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.
 ///
@@ -698,6 +718,7 @@ impl LintPass for Pass {
             TEMPORARY_CSTRING_AS_PTR,
             FILTER_NEXT,
             FILTER_MAP,
+            MAP_FLATTEN,
             ITER_NTH,
             ITER_SKIP_NEXT,
             GET_UNWRAP,
@@ -744,6 +765,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                     lint_filter_flat_map(cx, expr, arglists[0], arglists[1]);
                 } else if let Some(arglists) = method_chain_args(expr, &["filter_map", "flat_map"]) {
                     lint_filter_map_flat_map(cx, expr, arglists[0], arglists[1]);
+                } else if let Some(arglists) = method_chain_args(expr, &["map", "flatten"]) {
+                    lint_map_flatten(cx, expr, arglists[0]);
                 } else if let Some(arglists) = method_chain_args(expr, &["find", "is_some"]) {
                     lint_search_is_some(cx, expr, "find", arglists[0], arglists[1]);
                 } else if let Some(arglists) = method_chain_args(expr, &["position", "is_some"]) {
@@ -1577,6 +1600,30 @@ fn lint_map_unwrap_or(cx: &LateContext<'_, '_>, expr: &hir::Expr, map_args: &[hi
     }
 }
 
+/// lint use of `map().flatten()` for `Iterators`
+fn lint_map_flatten<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &'tcx hir::Expr,
+    map_args: &'tcx [hir::Expr],
+) {
+    // lint if caller of `.map().flatten()` is an Iterator
+    if match_trait_method(cx, expr, &paths::ITERATOR) {
+        let msg = "called `map(..).flatten()` on an `Iterator`. \
+                   This is more succinctly expressed by calling `.flat_map(..)`";
+        let self_snippet = snippet(cx, map_args[0].span, "..");
+        let func_snippet = snippet(cx, map_args[1].span, "..");
+        let hint = format!("{0}.flat_map({1})", self_snippet, func_snippet);
+        span_lint_and_then(cx, MAP_FLATTEN, expr.span, msg, |db| {
+            db.span_suggestion_with_applicability(
+                expr.span,
+                "try using flat_map instead",
+                hint,
+                Applicability::MachineApplicable,
+            );
+        });
+    }
+}
+
 /// lint use of `map().unwrap_or_else()` for `Option`s and `Result`s
 fn lint_map_unwrap_or_else<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,

tests/ui/map_flatten.rs

@@ -0,0 +1,7 @@
+#![feature(tool_lints)]
+#![warn(clippy::all, clippy::pedantic)]
+#![allow(clippy::missing_docs_in_private_items)]
+
+fn main() {
+    let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
+}

tests/ui/map_flatten.stderr

@@ -0,0 +1,10 @@
+error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)`
+ --> $DIR/map_flatten.rs:6:21
+  |
+6 |     let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
+  |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using flat_map instead: `vec![5_i8; 6].into_iter().flat_map(|x| 0..x)`
+  |
+  = note: `-D clippy::map-flatten` implied by `-D warnings`
+
+error: aborting due to previous error
+