Skip to content

Commit 434c5d5

Browse files
JarchoJarcho
committed
Review comments
- Error message formatting - Check for the identity function
1 parent c35137d commit 434c5d5

File tree

4 files changed

+64
-18
lines changed

4 files changed

+64
-18
lines changed

‎clippy_lints/src/option_manual_map.rs

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
use crate::utils::{is_type_diagnostic_item, match_def_path, paths, snippet_with_applicability, span_lint_and_sugg};
1+
use super::map_identity::MAP_IDENTITY;
2+
use crate::utils::{
3+
is_allowed, is_type_diagnostic_item, match_def_path, match_path, paths, snippet_with_applicability,
4+
span_lint_and_sugg,
5+
};
26
use core::fmt;
37
use if_chain::if_chain;
48
use rustc_errors::Applicability;
@@ -54,28 +58,49 @@ impl LateLintPass<'_> for OptionManualMap {
5458
then {
5559
let mut app = Applicability::MachineApplicable;
5660

61+
let scrutinee = snippet_with_applicability(cx, scrutinee.span, "_", &mut app);
5762
let body = if_chain! {
5863
if let SomeBinding::Name(some_name) = some_binding;
5964
if let ExprKind::Call(func, [arg]) = some_expr.kind;
6065
if let ExprKind::Path(QPath::Resolved(_, arg_path)) = arg.kind;
6166
if let [arg_name] = arg_path.segments;
6267
if arg_name.ident.name == some_name.name;
6368
then {
64-
snippet_with_applicability(cx, func.span, "_", &mut app).into_owned()
69+
match func.kind {
70+
ExprKind::Path(QPath::Resolved(_, func_path))
71+
if match_path(func_path, &paths::STD_CONVERT_IDENTITY)
72+
&& !is_allowed(cx, MAP_IDENTITY, expr.hir_id) =>
73+
{
74+
span_lint_and_sugg(
75+
cx,
76+
MAP_IDENTITY,
77+
expr.span,
78+
"unnecessary map of the identity function",
79+
"remove the `match` expression",
80+
scrutinee.into(),
81+
app,
82+
);
83+
return;
84+
}
85+
_ => snippet_with_applicability(cx, func.span, "_", &mut app).into_owned(),
86+
}
6587
} else {
66-
format!("|{}| {}", some_binding, snippet_with_applicability(cx, some_expr.span, "..", &mut app))
88+
format!(
89+
"|{}| {}",
90+
some_binding,
91+
snippet_with_applicability(cx, some_expr.span, "..", &mut app),
92+
)
6793
}
6894
};
6995

70-
let scrutinee = snippet_with_applicability(cx, scrutinee.span, "_", &mut app);
7196
span_lint_and_sugg(
7297
cx,
7398
OPTION_MANUAL_MAP,
7499
expr.span,
75100
"manual implementation of `Option::map`",
76-
"use map instead",
101+
"use `map` instead",
77102
format!("{}.map({})", scrutinee, body),
78-
app
103+
app,
79104
);
80105
}
81106
}

‎tests/ui/option_manual_map.fixed

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// run-rustfix
22

33
#![warn(clippy::option_manual_map)]
4-
#![allow(clippy::map_identity)]
4+
#![allow(clippy::no_effect)]
55

66
fn main() {
77
Some(0).map(|_| 2);
@@ -13,6 +13,9 @@ fn main() {
1313
Some(0).map(|x| !x);
1414

1515
#[rustfmt::skip]
16+
let _ = Some(0);
17+
18+
#[allow(clippy::map_identity)]
1619
Some(0).map(std::convert::identity);
1720

1821
match Some(0) {

‎tests/ui/option_manual_map.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// run-rustfix
22

33
#![warn(clippy::option_manual_map)]
4-
#![allow(clippy::map_identity)]
4+
#![allow(clippy::no_effect)]
55

66
fn main() {
77
match Some(0) {
@@ -26,11 +26,17 @@ fn main() {
2626
};
2727

2828
#[rustfmt::skip]
29-
match Some(0) {
29+
let _ = match Some(0) {
3030
Some(x) => { Some(std::convert::identity(x)) }
3131
None => { None }
3232
};
3333

34+
#[allow(clippy::map_identity)]
35+
match Some(0) {
36+
Some(x) => Some(std::convert::identity(x)),
37+
None => None,
38+
};
39+
3440
match Some(0) {
3541
Some(x) if false => Some(x + 1),
3642
_ => None,

‎tests/ui/option_manual_map.stderr

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ LL | / match Some(0) {
55
LL | | Some(_) => Some(2),
66
LL | | None::<u32> => None,
77
LL | | };
8-
| |_____^ help: use map instead: `Some(0).map(|_| 2)`
8+
| |_____^ help: use `map` instead: `Some(0).map(|_| 2)`
99
|
1010
= note: `-D clippy::option-manual-map` implied by `-D warnings`
1111

@@ -16,7 +16,7 @@ LL | / match Some(0) {
1616
LL | | Some(x) => Some(x + 1),
1717
LL | | _ => None,
1818
LL | | };
19-
| |_____^ help: use map instead: `Some(0).map(|x| x + 1)`
19+
| |_____^ help: use `map` instead: `Some(0).map(|x| x + 1)`
2020

2121
error: manual implementation of `Option::map`
2222
--> $DIR/option_manual_map.rs:17:5
@@ -25,7 +25,7 @@ LL | / match Some("") {
2525
LL | | Some(x) => Some(x.is_empty()),
2626
LL | | None => None,
2727
LL | | };
28-
| |_____^ help: use map instead: `Some("").map(|x| x.is_empty())`
28+
| |_____^ help: use `map` instead: `Some("").map(|x| x.is_empty())`
2929

3030
error: manual implementation of `Option::map`
3131
--> $DIR/option_manual_map.rs:22:5
@@ -35,16 +35,28 @@ LL | | Some(!x)
3535
LL | | } else {
3636
LL | | None
3737
LL | | };
38-
| |_____^ help: use map instead: `Some(0).map(|x| !x)`
38+
| |_____^ help: use `map` instead: `Some(0).map(|x| !x)`
3939

40-
error: manual implementation of `Option::map`
41-
--> $DIR/option_manual_map.rs:29:5
40+
error: unnecessary map of the identity function
41+
--> $DIR/option_manual_map.rs:29:13
4242
|
43-
LL | / match Some(0) {
43+
LL | let _ = match Some(0) {
44+
| _____________^
4445
LL | | Some(x) => { Some(std::convert::identity(x)) }
4546
LL | | None => { None }
4647
LL | | };
47-
| |_____^ help: use map instead: `Some(0).map(std::convert::identity)`
48+
| |_____^ help: remove the `match` expression: `Some(0)`
49+
|
50+
= note: `-D clippy::map-identity` implied by `-D warnings`
51+
52+
error: manual implementation of `Option::map`
53+
--> $DIR/option_manual_map.rs:35:5
54+
|
55+
LL | / match Some(0) {
56+
LL | | Some(x) => Some(std::convert::identity(x)),
57+
LL | | None => None,
58+
LL | | };
59+
| |_____^ help: use `map` instead: `Some(0).map(std::convert::identity)`
4860

49-
error: aborting due to 5 previous errors
61+
error: aborting due to 6 previous errors
5062

0 commit comments

Comments
 (0)