Skip to content

Commit 40ee620

Browse files
theo-lwtheo-lw
committed
Added a lint for .map(|x| x)
1 parent 7427065 commit 40ee620

File tree

10 files changed

+228
-2
lines changed

10 files changed

+228
-2
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1508,6 +1508,7 @@ Released 2018-09-13
15081508
[`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
15091509
[`map_entry`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_entry
15101510
[`map_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_flatten
1511+
[`map_identity`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_identity
15111512
[`map_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or
15121513
[`match_as_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_as_ref
15131514
[`match_bool`]: https://rust-lang.github.io/rust-clippy/master/index.html#match_bool

‎clippy_lints/src/lib.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ mod main_recursion;
252252
mod manual_async_fn;
253253
mod manual_non_exhaustive;
254254
mod map_clone;
255+
mod map_identity;
255256
mod map_unit_fn;
256257
mod match_on_vec_items;
257258
mod matches;
@@ -631,6 +632,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
631632
&manual_async_fn::MANUAL_ASYNC_FN,
632633
&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
633634
&map_clone::MAP_CLONE,
635+
&map_identity::MAP_IDENTITY,
634636
&map_unit_fn::OPTION_MAP_UNIT_FN,
635637
&map_unit_fn::RESULT_MAP_UNIT_FN,
636638
&match_on_vec_items::MATCH_ON_VEC_ITEMS,
@@ -1080,6 +1082,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
10801082
});
10811083
store.register_early_pass(|| box unnested_or_patterns::UnnestedOrPatterns);
10821084
store.register_late_pass(|| box macro_use::MacroUseImports::default());
1085+
store.register_late_pass(|| box map_identity::MapIdentity);
10831086

10841087
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
10851088
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1295,6 +1298,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
12951298
LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
12961299
LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
12971300
LintId::of(&map_clone::MAP_CLONE),
1301+
LintId::of(&map_identity::MAP_IDENTITY),
12981302
LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
12991303
LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),
13001304
LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),
@@ -1573,6 +1577,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
15731577
LintId::of(&loops::EXPLICIT_COUNTER_LOOP),
15741578
LintId::of(&loops::MUT_RANGE_BOUND),
15751579
LintId::of(&loops::WHILE_LET_LOOP),
1580+
LintId::of(&map_identity::MAP_IDENTITY),
15761581
LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
15771582
LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),
15781583
LintId::of(&matches::MATCH_AS_REF),

‎clippy_lints/src/map_identity.rs

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
use crate::utils::{
2+
is_adjusted, is_type_diagnostic_item, match_path, match_trait_method, match_var, paths, remove_blocks,
3+
span_lint_and_sugg,
4+
};
5+
use if_chain::if_chain;
6+
use rustc_errors::Applicability;
7+
use rustc_hir::{Body, Expr, ExprKind, Pat, PatKind, QPath, StmtKind};
8+
use rustc_lint::{LateContext, LateLintPass};
9+
use rustc_session::{declare_lint_pass, declare_tool_lint};
10+
11+
declare_clippy_lint! {
12+
/// **What it does:** Checks for instances of `map(f)` where `f` is the identity function.
13+
///
14+
/// **Why is this bad?** It can be written more concisely without the call to `map`.
15+
///
16+
/// **Known problems:** None.
17+
///
18+
/// **Example:**
19+
///
20+
/// ```rust
21+
/// let x = [1, 2, 3];
22+
/// let y: Vec<_> = x.iter().map(|x| x).map(|x| 2*x).collect();
23+
/// ```
24+
/// Use instead:
25+
/// ```rust
26+
/// let x = [1, 2, 3];
27+
/// let y: Vec<_> = x.iter().map(|x| 2*x).collect();
28+
/// ```
29+
pub MAP_IDENTITY,
30+
complexity,
31+
"using iterator.map(|x| x)"
32+
}
33+
34+
declare_lint_pass!(MapIdentity => [MAP_IDENTITY]);
35+
36+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MapIdentity {
37+
fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
38+
if expr.span.from_expansion() {
39+
return;
40+
}
41+
42+
if_chain! {
43+
if let Some([caller, func]) = get_map_argument(cx, expr);
44+
if is_expr_identity_function(cx, func);
45+
then {
46+
span_lint_and_sugg(
47+
cx,
48+
MAP_IDENTITY,
49+
expr.span.trim_start(caller.span).unwrap(),
50+
"unnecessary map of the identity function",
51+
"remove the call to `map`",
52+
String::new(),
53+
Applicability::MachineApplicable
54+
)
55+
}
56+
}
57+
}
58+
}
59+
60+
/// Returns the arguments passed into map() if the expression is a method call to
61+
/// map(). Otherwise, returns None.
62+
fn get_map_argument<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'a>) -> Option<&'a [Expr<'a>]> {
63+
if_chain! {
64+
if let ExprKind::MethodCall(ref method, _, ref args) = expr.kind;
65+
if args.len() == 2 && method.ident.as_str() == "map";
66+
let caller_ty = cx.tables.expr_ty(&args[0]);
67+
if match_trait_method(cx, expr, &paths::ITERATOR)
68+
|| is_type_diagnostic_item(cx, caller_ty, sym!(result_type))
69+
|| is_type_diagnostic_item(cx, caller_ty, sym!(option_type));
70+
then {
71+
Some(args)
72+
} else {
73+
None
74+
}
75+
}
76+
}
77+
78+
/// Checks if an expression represents the identity function
79+
/// Only examines closures and `std::convert::identity`
80+
fn is_expr_identity_function(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
81+
match expr.kind {
82+
ExprKind::Closure(_, _, body_id, _, _) => is_body_identity_function(cx, cx.tcx.hir().body(body_id)),
83+
ExprKind::Path(QPath::Resolved(_, ref path)) => match_path(path, &paths::STD_CONVERT_IDENTITY),
84+
_ => false,
85+
}
86+
}
87+
88+
/// Checks if a function's body represents the identity function
89+
/// Looks for bodies of the form `|x| x`, `|x| return x`, `|x| { return x }` or `|x| {
90+
/// return x; }`
91+
fn is_body_identity_function(cx: &LateContext<'_, '_>, func: &Body<'_>) -> bool {
92+
let params = func.params;
93+
let body = remove_blocks(&func.value);
94+
95+
// if there's less/more than one parameter, then it is not the identity function
96+
if params.len() != 1 {
97+
return false;
98+
}
99+
100+
match body.kind {
101+
ExprKind::Path(QPath::Resolved(None, _)) => match_expr_param(cx, body, params[0].pat),
102+
ExprKind::Ret(Some(ref ret_val)) => match_expr_param(cx, ret_val, params[0].pat),
103+
ExprKind::Block(ref block, _) => {
104+
if_chain! {
105+
if block.stmts.len() == 1;
106+
if let StmtKind::Semi(ref expr) | StmtKind::Expr(ref expr) = block.stmts[0].kind;
107+
if let ExprKind::Ret(Some(ref ret_val)) = expr.kind;
108+
then {
109+
match_expr_param(cx, ret_val, params[0].pat)
110+
} else {
111+
false
112+
}
113+
}
114+
},
115+
_ => false,
116+
}
117+
}
118+
119+
/// Returns true iff an expression returns the same thing as a parameter's pattern
120+
fn match_expr_param(cx: &LateContext<'_, '_>, expr: &Expr<'_>, pat: &Pat<'_>) -> bool {
121+
if let PatKind::Binding(_, _, ident, _) = pat.kind {
122+
match_var(expr, ident.name) && !(cx.tables.hir_owner == Some(expr.hir_id.owner) && is_adjusted(cx, expr))
123+
} else {
124+
false
125+
}
126+
}

‎src/lintlist/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,6 +1144,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
11441144
deprecation: None,
11451145
module: "methods",
11461146
},
1147+
Lint {
1148+
name: "map_identity",
1149+
group: "complexity",
1150+
desc: "using iterator.map(|x| x)",
1151+
deprecation: None,
1152+
module: "map_identity",
1153+
},
11471154
Lint {
11481155
name: "map_unwrap_or",
11491156
group: "pedantic",

‎tests/ui/map_flatten.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#![warn(clippy::all, clippy::pedantic)]
44
#![allow(clippy::missing_docs_in_private_items)]
5+
#![allow(clippy::map_identity)]
56

67
fn main() {
78
let _: Vec<_> = vec![5_i8; 6].into_iter().flat_map(|x| 0..x).collect();

‎tests/ui/map_flatten.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#![warn(clippy::all, clippy::pedantic)]
44
#![allow(clippy::missing_docs_in_private_items)]
5+
#![allow(clippy::map_identity)]
56

67
fn main() {
78
let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();

‎tests/ui/map_flatten.stderr

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
error: called `map(..).flatten()` on an `Iterator`. This is more succinctly expressed by calling `.flat_map(..)`
2-
--> $DIR/map_flatten.rs:7:21
2+
--> $DIR/map_flatten.rs:8:21
33
|
44
LL | let _: Vec<_> = vec![5_i8; 6].into_iter().map(|x| 0..x).flatten().collect();
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `flat_map` instead: `vec![5_i8; 6].into_iter().flat_map(|x| 0..x)`
66
|
77
= note: `-D clippy::map-flatten` implied by `-D warnings`
88

99
error: called `map(..).flatten()` on an `Option`. This is more succinctly expressed by calling `.and_then(..)`
10-
--> $DIR/map_flatten.rs:8:24
10+
--> $DIR/map_flatten.rs:9:24
1111
|
1212
LL | let _: Option<_> = (Some(Some(1))).map(|x| x).flatten();
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try using `and_then` instead: `(Some(Some(1))).and_then(|x| x)`

‎tests/ui/map_identity.fixed

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// run-rustfix
2+
#![warn(clippy::map_identity)]
3+
#![allow(clippy::needless_return)]
4+
5+
fn main() {
6+
let x: [u16; 3] = [1, 2, 3];
7+
// should lint
8+
let _: Vec<_> = x.iter().map(not_identity).collect();
9+
let _: Vec<_> = x.iter().collect();
10+
let _: Option<u8> = Some(3);
11+
let _: Result<i8, f32> = Ok(-3);
12+
// should not lint
13+
let _: Vec<_> = x.iter().map(|x| 2 * x).collect();
14+
let _: Vec<_> = x.iter().map(not_identity).map(|x| return x - 4).collect();
15+
let _: Option<u8> = None.map(|x: u8| x - 1);
16+
let _: Result<i8, f32> = Err(2.3).map(|x: i8| {
17+
return x + 3;
18+
});
19+
}
20+
21+
fn not_identity(x: &u16) -> u16 {
22+
*x
23+
}

‎tests/ui/map_identity.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// run-rustfix
2+
#![warn(clippy::map_identity)]
3+
#![allow(clippy::needless_return)]
4+
5+
fn main() {
6+
let x: [u16; 3] = [1, 2, 3];
7+
// should lint
8+
let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
9+
let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
10+
let _: Option<u8> = Some(3).map(|x| x);
11+
let _: Result<i8, f32> = Ok(-3).map(|x| {
12+
return x;
13+
});
14+
// should not lint
15+
let _: Vec<_> = x.iter().map(|x| 2 * x).collect();
16+
let _: Vec<_> = x.iter().map(not_identity).map(|x| return x - 4).collect();
17+
let _: Option<u8> = None.map(|x: u8| x - 1);
18+
let _: Result<i8, f32> = Err(2.3).map(|x: i8| {
19+
return x + 3;
20+
});
21+
}
22+
23+
fn not_identity(x: &u16) -> u16 {
24+
*x
25+
}

‎tests/ui/map_identity.stderr

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
error: unnecessary map of the identity function
2+
--> $DIR/map_identity.rs:8:47
3+
|
4+
LL | let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
5+
| ^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
6+
|
7+
= note: `-D clippy::map-identity` implied by `-D warnings`
8+
9+
error: unnecessary map of the identity function
10+
--> $DIR/map_identity.rs:9:57
11+
|
12+
LL | let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
13+
| ^^^^^^^^^^^ help: remove the call to `map`
14+
15+
error: unnecessary map of the identity function
16+
--> $DIR/map_identity.rs:9:29
17+
|
18+
LL | let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
20+
21+
error: unnecessary map of the identity function
22+
--> $DIR/map_identity.rs:10:32
23+
|
24+
LL | let _: Option<u8> = Some(3).map(|x| x);
25+
| ^^^^^^^^^^^ help: remove the call to `map`
26+
27+
error: unnecessary map of the identity function
28+
--> $DIR/map_identity.rs:11:36
29+
|
30+
LL | let _: Result<i8, f32> = Ok(-3).map(|x| {
31+
| ____________________________________^
32+
LL | | return x;
33+
LL | | });
34+
| |______^ help: remove the call to `map`
35+
36+
error: aborting due to 5 previous errors
37+

0 commit comments

Comments
 (0)