From 40ee620e51c86c72e3c2b65df71f5f0a4a79797f Mon Sep 17 00:00:00 2001 From: Teddy_Wang Date: Mon, 8 Jun 2020 00:35:10 -0400 Subject: [PATCH 1/2] Added a lint for .map(|x| x) --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 5 ++ clippy_lints/src/map_identity.rs | 126 +++++++++++++++++++++++++++++++ src/lintlist/mod.rs | 7 ++ tests/ui/map_flatten.fixed | 1 + tests/ui/map_flatten.rs | 1 + tests/ui/map_flatten.stderr | 4 +- tests/ui/map_identity.fixed | 23 ++++++ tests/ui/map_identity.rs | 25 ++++++ tests/ui/map_identity.stderr | 37 +++++++++ 10 files changed, 228 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/map_identity.rs create mode 100644 tests/ui/map_identity.fixed create mode 100644 tests/ui/map_identity.rs create mode 100644 tests/ui/map_identity.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index adc945a6944..d6186c319d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1508,6 +1508,7 @@ Released 2018-09-13 [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone [`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry [`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten +[`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity [`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or [`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref [`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 021fbe932d8..8b5e0b84eb4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -252,6 +252,7 @@ macro_rules! declare_clippy_lint { mod manual_async_fn; mod manual_non_exhaustive; mod map_clone; +mod map_identity; mod map_unit_fn; mod match_on_vec_items; mod matches; @@ -631,6 +632,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &manual_async_fn::MANUAL_ASYNC_FN, &manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE, &map_clone::MAP_CLONE, + &map_identity::MAP_IDENTITY, &map_unit_fn::OPTION_MAP_UNIT_FN, &map_unit_fn::RESULT_MAP_UNIT_FN, &match_on_vec_items::MATCH_ON_VEC_ITEMS, @@ -1080,6 +1082,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_early_pass(|| box unnested_or_patterns::UnnestedOrPatterns); store.register_late_pass(|| box macro_use::MacroUseImports::default()); + store.register_late_pass(|| box map_identity::MapIdentity); store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![ LintId::of(&arithmetic::FLOAT_ARITHMETIC), @@ -1295,6 +1298,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&manual_async_fn::MANUAL_ASYNC_FN), LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE), LintId::of(&map_clone::MAP_CLONE), + LintId::of(&map_identity::MAP_IDENTITY), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH), @@ -1573,6 +1577,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::EXPLICIT_COUNTER_LOOP), LintId::of(&loops::MUT_RANGE_BOUND), LintId::of(&loops::WHILE_LET_LOOP), + LintId::of(&map_identity::MAP_IDENTITY), LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN), LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN), LintId::of(&matches::MATCH_AS_REF), diff --git a/clippy_lints/src/map_identity.rs b/clippy_lints/src/map_identity.rs new file mode 100644 index 00000000000..9bc8f1bec1b --- /dev/null +++ b/clippy_lints/src/map_identity.rs @@ -0,0 +1,126 @@ +use crate::utils::{ + is_adjusted, is_type_diagnostic_item, match_path, match_trait_method, match_var, paths, remove_blocks, + span_lint_and_sugg, +}; +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Body, Expr, ExprKind, Pat, PatKind, QPath, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks for instances of `map(f)` where `f` is the identity function. + /// + /// **Why is this bad?** It can be written more concisely without the call to `map`. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// let x = [1, 2, 3]; + /// let y: Vec<_> = x.iter().map(|x| x).map(|x| 2*x).collect(); + /// ``` + /// Use instead: + /// ```rust + /// let x = [1, 2, 3]; + /// let y: Vec<_> = x.iter().map(|x| 2*x).collect(); + /// ``` + pub MAP_IDENTITY, + complexity, + "using iterator.map(|x| x)" +} + +declare_lint_pass!(MapIdentity => [MAP_IDENTITY]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MapIdentity { + fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) { + if expr.span.from_expansion() { + return; + } + + if_chain! { + if let Some([caller, func]) = get_map_argument(cx, expr); + if is_expr_identity_function(cx, func); + then { + span_lint_and_sugg( + cx, + MAP_IDENTITY, + expr.span.trim_start(caller.span).unwrap(), + "unnecessary map of the identity function", + "remove the call to `map`", + String::new(), + Applicability::MachineApplicable + ) + } + } + } +} + +/// Returns the arguments passed into map() if the expression is a method call to +/// map(). Otherwise, returns None. +fn get_map_argument<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'a>) -> Option<&'a [Expr<'a>]> { + if_chain! { + if let ExprKind::MethodCall(ref method, _, ref args) = expr.kind; + if args.len() == 2 && method.ident.as_str() == "map"; + let caller_ty = cx.tables.expr_ty(&args[0]); + if match_trait_method(cx, expr, &paths::ITERATOR) + || is_type_diagnostic_item(cx, caller_ty, sym!(result_type)) + || is_type_diagnostic_item(cx, caller_ty, sym!(option_type)); + then { + Some(args) + } else { + None + } + } +} + +/// Checks if an expression represents the identity function +/// Only examines closures and `std::convert::identity` +fn is_expr_identity_function(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { + match expr.kind { + ExprKind::Closure(_, _, body_id, _, _) => is_body_identity_function(cx, cx.tcx.hir().body(body_id)), + ExprKind::Path(QPath::Resolved(_, ref path)) => match_path(path, &paths::STD_CONVERT_IDENTITY), + _ => false, + } +} + +/// Checks if a function's body represents the identity function +/// Looks for bodies of the form `|x| x`, `|x| return x`, `|x| { return x }` or `|x| { +/// return x; }` +fn is_body_identity_function(cx: &LateContext<'_, '_>, func: &Body<'_>) -> bool { + let params = func.params; + let body = remove_blocks(&func.value); + + // if there's less/more than one parameter, then it is not the identity function + if params.len() != 1 { + return false; + } + + match body.kind { + ExprKind::Path(QPath::Resolved(None, _)) => match_expr_param(cx, body, params[0].pat), + ExprKind::Ret(Some(ref ret_val)) => match_expr_param(cx, ret_val, params[0].pat), + ExprKind::Block(ref block, _) => { + if_chain! { + if block.stmts.len() == 1; + if let StmtKind::Semi(ref expr) | StmtKind::Expr(ref expr) = block.stmts[0].kind; + if let ExprKind::Ret(Some(ref ret_val)) = expr.kind; + then { + match_expr_param(cx, ret_val, params[0].pat) + } else { + false + } + } + }, + _ => false, + } +} + +/// Returns true iff an expression returns the same thing as a parameter's pattern +fn match_expr_param(cx: &LateContext<'_, '_>, expr: &Expr<'_>, pat: &Pat<'_>) -> bool { + if let PatKind::Binding(_, _, ident, _) = pat.kind { + match_var(expr, ident.name) && !(cx.tables.hir_owner == Some(expr.hir_id.owner) && is_adjusted(cx, expr)) + } else { + false + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index cac3cc6bdb3..a9cd5469048 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1144,6 +1144,13 @@ deprecation: None, module: "methods", }, + Lint { + name: "map_identity", + group: "complexity", + desc: "using iterator.map(|x| x)", + deprecation: None, + module: "map_identity", + }, Lint { name: "map_unwrap_or", group: "pedantic", diff --git a/tests/ui/map_flatten.fixed b/tests/ui/map_flatten.fixed index 7ac368878ab..4171d80f48a 100644 --- a/tests/ui/map_flatten.fixed +++ b/tests/ui/map_flatten.fixed @@ -2,6 +2,7 @@ #![warn(clippy::all, clippy::pedantic)] #![allow(clippy::missing_docs_in_private_items)] +#![allow(clippy::map_identity)] fn main() { let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect(); diff --git a/tests/ui/map_flatten.rs b/tests/ui/map_flatten.rs index a608601039c..16a0fd090ad 100644 --- a/tests/ui/map_flatten.rs +++ b/tests/ui/map_flatten.rs @@ -2,6 +2,7 @@ #![warn(clippy::all, clippy::pedantic)] #![allow(clippy::missing_docs_in_private_items)] +#![allow(clippy::map_identity)] 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 index 3cf2abd5b6d..00bc41c15e9 100644 --- a/tests/ui/map_flatten.stderr +++ b/tests/ui/map_flatten.stderr @@ -1,5 +1,5 @@ error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)` - --> $DIR/map_flatten.rs:7:21 + --> $DIR/map_flatten.rs:8:21 | LL | 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)` @@ -7,7 +7,7 @@ LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().colle = note: `-D clippy::map-flatten` implied by `-D warnings` error: called `map(..).flatten()` on an `Option`. This is more succinctly expressed by calling `.and_then(..)` - --> $DIR/map_flatten.rs:8:24 + --> $DIR/map_flatten.rs:9:24 | LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `(Some(Some(1))).and_then(|x| x)` diff --git a/tests/ui/map_identity.fixed b/tests/ui/map_identity.fixed new file mode 100644 index 00000000000..4a1452b25f3 --- /dev/null +++ b/tests/ui/map_identity.fixed @@ -0,0 +1,23 @@ +// run-rustfix +#![warn(clippy::map_identity)] +#![allow(clippy::needless_return)] + +fn main() { + let x: [u16; 3] = [1, 2, 3]; + // should lint + let _: Vec<_> = x.iter().map(not_identity).collect(); + let _: Vec<_> = x.iter().collect(); + let _: Option = Some(3); + let _: Result = Ok(-3); + // should not lint + let _: Vec<_> = x.iter().map(|x| 2 * x).collect(); + let _: Vec<_> = x.iter().map(not_identity).map(|x| return x - 4).collect(); + let _: Option = None.map(|x: u8| x - 1); + let _: Result = Err(2.3).map(|x: i8| { + return x + 3; + }); +} + +fn not_identity(x: &u16) -> u16 { + *x +} diff --git a/tests/ui/map_identity.rs b/tests/ui/map_identity.rs new file mode 100644 index 00000000000..65c7e6e1ea5 --- /dev/null +++ b/tests/ui/map_identity.rs @@ -0,0 +1,25 @@ +// run-rustfix +#![warn(clippy::map_identity)] +#![allow(clippy::needless_return)] + +fn main() { + let x: [u16; 3] = [1, 2, 3]; + // should lint + let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect(); + let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect(); + let _: Option = Some(3).map(|x| x); + let _: Result = Ok(-3).map(|x| { + return x; + }); + // should not lint + let _: Vec<_> = x.iter().map(|x| 2 * x).collect(); + let _: Vec<_> = x.iter().map(not_identity).map(|x| return x - 4).collect(); + let _: Option = None.map(|x: u8| x - 1); + let _: Result = Err(2.3).map(|x: i8| { + return x + 3; + }); +} + +fn not_identity(x: &u16) -> u16 { + *x +} diff --git a/tests/ui/map_identity.stderr b/tests/ui/map_identity.stderr new file mode 100644 index 00000000000..e4a0320cbda --- /dev/null +++ b/tests/ui/map_identity.stderr @@ -0,0 +1,37 @@ +error: unnecessary map of the identity function + --> $DIR/map_identity.rs:8:47 + | +LL | let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect(); + | ^^^^^^^^^^^^^^^^^^ help: remove the call to `map` + | + = note: `-D clippy::map-identity` implied by `-D warnings` + +error: unnecessary map of the identity function + --> $DIR/map_identity.rs:9:57 + | +LL | let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect(); + | ^^^^^^^^^^^ help: remove the call to `map` + +error: unnecessary map of the identity function + --> $DIR/map_identity.rs:9:29 + | +LL | let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map` + +error: unnecessary map of the identity function + --> $DIR/map_identity.rs:10:32 + | +LL | let _: Option = Some(3).map(|x| x); + | ^^^^^^^^^^^ help: remove the call to `map` + +error: unnecessary map of the identity function + --> $DIR/map_identity.rs:11:36 + | +LL | let _: Result = Ok(-3).map(|x| { + | ____________________________________^ +LL | | return x; +LL | | }); + | |______^ help: remove the call to `map` + +error: aborting due to 5 previous errors + From fb4f9a0ad7a4656beb01c85b02b3e6ef15d914ec Mon Sep 17 00:00:00 2001 From: Teddy_Wang Date: Tue, 23 Jun 2020 11:40:38 -0400 Subject: [PATCH 2/2] Fix pattern match of ExprKind::MethodCall --- clippy_lints/src/map_identity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/map_identity.rs b/clippy_lints/src/map_identity.rs index 9bc8f1bec1b..6607a26b130 100644 --- a/clippy_lints/src/map_identity.rs +++ b/clippy_lints/src/map_identity.rs @@ -61,7 +61,7 @@ fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) { /// map(). Otherwise, returns None. fn get_map_argument<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'a>) -> Option<&'a [Expr<'a>]> { if_chain! { - if let ExprKind::MethodCall(ref method, _, ref args) = expr.kind; + if let ExprKind::MethodCall(ref method, _, ref args, _) = expr.kind; if args.len() == 2 && method.ident.as_str() == "map"; let caller_ty = cx.tables.expr_ty(&args[0]); if match_trait_method(cx, expr, &paths::ITERATOR)