mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-14 17:07:17 +00:00
Auto merge of #5694 - wangtheo:issue-5626, r=matthiaskrgr
#5626: lint iterator.map(|x| x) changelog: adds a new lint for iterator.map(|x| x) (see https://github.com/rust-lang/rust-clippy/issues/5626) The code also lints for result.map(|x| x) and option.map(|x| x). Also, I'm not sure if I'm checking for type adjustments correctly and I can't think of an example where .map(|x| x) would apply type adjustments.
This commit is contained in:
commit
583d644934
10 changed files with 228 additions and 2 deletions
|
@ -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
|
||||
|
|
|
@ -229,6 +229,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;
|
||||
|
@ -608,6 +609,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,
|
||||
|
@ -1057,6 +1059,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),
|
||||
|
@ -1273,6 +1276,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),
|
||||
|
@ -1550,6 +1554,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),
|
||||
|
|
126
clippy_lints/src/map_identity.rs
Normal file
126
clippy_lints/src/map_identity.rs
Normal file
|
@ -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
|
||||
}
|
||||
}
|
|
@ -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",
|
||||
|
|
|
@ -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();
|
||||
|
|
|
@ -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();
|
||||
|
|
|
@ -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)`
|
||||
|
|
23
tests/ui/map_identity.fixed
Normal file
23
tests/ui/map_identity.fixed
Normal file
|
@ -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
tests/ui/map_identity.rs
Normal file
25
tests/ui/map_identity.rs
Normal file
|
@ -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
tests/ui/map_identity.stderr
Normal file
37
tests/ui/map_identity.stderr
Normal file
|
@ -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
|
||||
|
Loading…
Reference in a new issue