Skip to content

Commit

Permalink
Added a lint for .map(|x| x)
Browse files Browse the repository at this point in the history
  • Loading branch information
theo-lw committed Jun 13, 2020
1 parent 7427065 commit 40ee620
Show file tree
Hide file tree
Showing 10 changed files with 228 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ mod main_recursion;
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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
126 changes: 126 additions & 0 deletions clippy_lints/src/map_identity.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1144,6 +1144,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
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",
Expand Down
1 change: 1 addition & 0 deletions tests/ui/map_flatten.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions tests/ui/map_flatten.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/map_flatten.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
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)`
|
= 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)`
Expand Down
23 changes: 23 additions & 0 deletions tests/ui/map_identity.fixed
Original file line number Diff line number Diff line change
@@ -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<u8> = Some(3);
let _: Result<i8, f32> = 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<u8> = None.map(|x: u8| x - 1);
let _: Result<i8, f32> = Err(2.3).map(|x: i8| {
return x + 3;
});
}

fn not_identity(x: &u16) -> u16 {
*x
}
25 changes: 25 additions & 0 deletions tests/ui/map_identity.rs
Original file line number Diff line number Diff line change
@@ -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<u8> = Some(3).map(|x| x);
let _: Result<i8, f32> = 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<u8> = None.map(|x: u8| x - 1);
let _: Result<i8, f32> = Err(2.3).map(|x: i8| {
return x + 3;
});
}

fn not_identity(x: &u16) -> u16 {
*x
}
37 changes: 37 additions & 0 deletions tests/ui/map_identity.stderr
Original file line number Diff line number Diff line change
@@ -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<u8> = 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<i8, f32> = Ok(-3).map(|x| {
| ____________________________________^
LL | | return x;
LL | | });
| |______^ help: remove the call to `map`

error: aborting due to 5 previous errors

0 comments on commit 40ee620

Please sign in to comment.