From 14feb3670f4e1e5ff759253b9dc86cfbb29ff8a5 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Fri, 21 Sep 2018 00:26:38 -0700 Subject: [PATCH] Lint for chaining flatten after map This change adds a lint to check for instances of `map(..).flatten()` that can be trivially shortened to `flat_map(..)` Closes #3196 --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 1 + clippy_lints/src/methods.rs | 75 ++++++++++++++++++++++++++++++------- tests/ui/map_flatten.rs | 7 ++++ tests/ui/map_flatten.stderr | 10 +++++ 6 files changed, 81 insertions(+), 15 deletions(-) create mode 100644 tests/ui/map_flatten.rs create mode 100644 tests/ui/map_flatten.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index a7b4c5921e8..2bc4a8a8f91 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index f7babe36ad9..eb1de06cce0 100644 --- a/README.md +++ b/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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 90094429a36..3e25c79a7fd 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 54df9809a3e..5a9818a6b63 100644 --- a/clippy_lints/src/methods.rs +++ b/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::{declare_tool_lint, lint_array}; -use if_chain::if_chain; -use crate::rustc::ty::{self, Ty}; 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 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>, diff --git a/tests/ui/map_flatten.rs b/tests/ui/map_flatten.rs new file mode 100644 index 00000000000..c5cf24d9bb0 --- /dev/null +++ b/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(); +} diff --git a/tests/ui/map_flatten.stderr b/tests/ui/map_flatten.stderr new file mode 100644 index 00000000000..d4ce44490d1 --- /dev/null +++ b/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 +