Skip to content

Commit 61ce0c5

Browse files
ada4aada4a
committed
map_identity: suggest making the variable mutable when necessary
1 parent cc0df52 commit 61ce0c5

File tree

4 files changed

+148
-50
lines changed

4 files changed

+148
-50
lines changed

‎clippy_lints/src/methods/map_identity.rs‎

Lines changed: 71 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,16 @@
1-
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::ty::is_type_diagnostic_item;
3-
use clippy_utils::{is_expr_untyped_identity_function, is_trait_method, path_to_local};
4-
use rustc_ast::BindingMode;
1+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
2+
use clippy_utils::source::snippet_with_applicability;
3+
use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
4+
use clippy_utils::{is_expr_untyped_identity_function, is_mutable, is_trait_method, path_to_local_with_projections};
55
use rustc_errors::Applicability;
6-
use rustc_hir::{self as hir, Node, PatKind};
7-
use rustc_lint::LateContext;
6+
use rustc_hir::{self as hir, ExprKind, Node, PatKind};
7+
use rustc_lint::{LateContext, LintContext};
88
use rustc_span::{Span, Symbol, sym};
99

1010
use super::MAP_IDENTITY;
1111

12+
const MSG: &str = "unnecessary map of the identity function";
13+
1214
pub(super) fn check(
1315
cx: &LateContext<'_>,
1416
expr: &hir::Expr<'_>,
@@ -23,26 +25,70 @@ pub(super) fn check(
2325
|| is_type_diagnostic_item(cx, caller_ty, sym::Result)
2426
|| is_type_diagnostic_item(cx, caller_ty, sym::Option))
2527
&& is_expr_untyped_identity_function(cx, map_arg)
26-
&& let Some(sugg_span) = expr.span.trim_start(caller.span)
28+
&& let Some(call_span) = expr.span.trim_start(caller.span)
2729
{
28-
// If the result of `.map(identity)` is used as a mutable reference,
29-
// the caller must not be an immutable binding.
30-
if cx.typeck_results().expr_ty_adjusted(expr).is_mutable_ptr()
31-
&& let Some(hir_id) = path_to_local(caller)
32-
&& let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
33-
&& !matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
34-
{
35-
return;
36-
}
30+
let main_sugg = (call_span, String::new());
31+
let mut app = if is_copy(cx, caller_ty) {
32+
// there is technically a behavioral change here for `Copy` iterators, where
33+
// `iter.map(|x| x).next()` would mutate a temporary copy of the iterator and
34+
// changing it to `iter.next()` mutates iter directly
35+
Applicability::Unspecified
36+
} else {
37+
Applicability::MachineApplicable
38+
};
39+
40+
let needs_to_be_mutable = cx.typeck_results().expr_ty_adjusted(expr).is_mutable_ptr();
41+
if needs_to_be_mutable && !is_mutable(cx, caller) {
42+
if let Some(hir_id) = path_to_local_with_projections(caller)
43+
&& let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
44+
&& let PatKind::Binding(_, _, ident, _) = pat.kind
45+
{
46+
// We can reach the binding -- suggest making it mutable
47+
let suggs = vec![main_sugg, (ident.span.shrink_to_lo(), String::from("mut "))];
48+
49+
let ident = snippet_with_applicability(cx.sess(), ident.span, "_", &mut app);
3750

38-
span_lint_and_sugg(
39-
cx,
40-
MAP_IDENTITY,
41-
sugg_span,
42-
"unnecessary map of the identity function",
43-
format!("remove the call to `{name}`"),
44-
String::new(),
45-
Applicability::MachineApplicable,
46-
);
51+
span_lint_and_then(cx, MAP_IDENTITY, call_span, MSG, |diag| {
52+
diag.multipart_suggestion(
53+
format!("remove the call to `{name}`, and make `{ident}` mutable"),
54+
suggs,
55+
app,
56+
);
57+
});
58+
} else {
59+
// If we can't make the binding mutable, prevent the suggestion from being automatically applied,
60+
// and add a complementary help message.
61+
app = Applicability::Unspecified;
62+
63+
let method_requiring_mut = if let Node::Expr(expr) = cx.tcx.parent_hir_node(expr.hir_id)
64+
&& let ExprKind::MethodCall(method, ..) = expr.kind
65+
{
66+
Some(method.ident)
67+
} else {
68+
None
69+
};
70+
71+
span_lint_and_then(cx, MAP_IDENTITY, call_span, MSG, |diag| {
72+
diag.span_suggestion(main_sugg.0, format!("remove the call to `{name}`"), main_sugg.1, app);
73+
74+
let note = if let Some(method_requiring_mut) = method_requiring_mut {
75+
format!("this must be made mutable to use `{method_requiring_mut}`")
76+
} else {
77+
"this must be made mutable".to_string()
78+
};
79+
diag.span_note(caller.span, note);
80+
});
81+
}
82+
} else {
83+
span_lint_and_sugg(
84+
cx,
85+
MAP_IDENTITY,
86+
main_sugg.0,
87+
MSG,
88+
format!("remove the call to `{name}`"),
89+
main_sugg.1,
90+
app,
91+
);
92+
}
4793
}
4894
}

‎tests/ui/map_identity.fixed‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
//@require-annotations-for-level: ERROR
12
#![warn(clippy::map_identity)]
23
#![allow(clippy::needless_return, clippy::disallowed_names)]
34

@@ -72,20 +73,33 @@ fn issue11764() {
7273
}
7374

7475
fn issue13904() {
75-
// don't lint: `it.next()` would not be legal as `it` is immutable
76-
let it = [1, 2, 3].into_iter();
77-
let _ = it.map(|x| x).next();
76+
// lint, but there's a catch:
77+
// when we remove the `.map()`, `it.next()` would require `it` to be mutable
78+
// therefore, include that in the suggestion as well
79+
let mut it = [1, 2, 3].into_iter();
80+
let _ = it.next();
81+
//~^ map_identity
82+
//~| HELP: remove the call to `map`, and make `it` mutable
83+
84+
// lint
85+
let mut index = [1, 2, 3].into_iter();
86+
let mut subindex = (index.by_ref().take(3), 42);
87+
let _ = subindex.0.next();
88+
//~^ map_identity
89+
//~| HELP: remove the call to `map`, and make `subindex` mutable
7890

7991
// lint
8092
#[allow(unused_mut)]
8193
let mut it = [1, 2, 3].into_iter();
8294
let _ = it.next();
8395
//~^ map_identity
96+
//~| HELP: remove the call to `map`
8497

8598
// lint
8699
let it = [1, 2, 3].into_iter();
87100
let _ = { it }.next();
88101
//~^ map_identity
102+
//~| HELP: remove the call to `map`
89103
}
90104

91105
// same as `issue11764`, but for arrays

‎tests/ui/map_identity.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
//@require-annotations-for-level: ERROR
12
#![warn(clippy::map_identity)]
23
#![allow(clippy::needless_return, clippy::disallowed_names)]
34

@@ -78,20 +79,33 @@ fn issue11764() {
7879
}
7980

8081
fn issue13904() {
81-
// don't lint: `it.next()` would not be legal as `it` is immutable
82+
// lint, but there's a catch:
83+
// when we remove the `.map()`, `it.next()` would require `it` to be mutable
84+
// therefore, include that in the suggestion as well
8285
let it = [1, 2, 3].into_iter();
8386
let _ = it.map(|x| x).next();
87+
//~^ map_identity
88+
//~| HELP: remove the call to `map`, and make `it` mutable
89+
90+
// lint
91+
let mut index = [1, 2, 3].into_iter();
92+
let subindex = (index.by_ref().take(3), 42);
93+
let _ = subindex.0.map(|n| n).next();
94+
//~^ map_identity
95+
//~| HELP: remove the call to `map`, and make `subindex` mutable
8496

8597
// lint
8698
#[allow(unused_mut)]
8799
let mut it = [1, 2, 3].into_iter();
88100
let _ = it.map(|x| x).next();
89101
//~^ map_identity
102+
//~| HELP: remove the call to `map`
90103

91104
// lint
92105
let it = [1, 2, 3].into_iter();
93106
let _ = { it }.map(|x| x).next();
94107
//~^ map_identity
108+
//~| HELP: remove the call to `map`
95109
}
96110

97111
// same as `issue11764`, but for arrays

‎tests/ui/map_identity.stderr‎

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: unnecessary map of the identity function
2-
--> tests/ui/map_identity.rs:7:47
2+
--> tests/ui/map_identity.rs:8:47
33
|
44
LL | let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
55
| ^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
@@ -8,25 +8,25 @@ LL | let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
88
= help: to override `-D warnings` add `#[allow(clippy::map_identity)]`
99

1010
error: unnecessary map of the identity function
11-
--> tests/ui/map_identity.rs:9:57
11+
--> tests/ui/map_identity.rs:10:57
1212
|
1313
LL | let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
1414
| ^^^^^^^^^^^ help: remove the call to `map`
1515

1616
error: unnecessary map of the identity function
17-
--> tests/ui/map_identity.rs:9:29
17+
--> tests/ui/map_identity.rs:10:29
1818
|
1919
LL | let _: Vec<_> = x.iter().map(std::convert::identity).map(|y| y).collect();
2020
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
2121

2222
error: unnecessary map of the identity function
23-
--> tests/ui/map_identity.rs:12:32
23+
--> tests/ui/map_identity.rs:13:32
2424
|
2525
LL | let _: Option<u8> = Some(3).map(|x| x);
2626
| ^^^^^^^^^^^ help: remove the call to `map`
2727

2828
error: unnecessary map of the identity function
29-
--> tests/ui/map_identity.rs:14:36
29+
--> tests/ui/map_identity.rs:15:36
3030
|
3131
LL | let _: Result<i8, f32> = Ok(-3).map(|x| {
3232
| ____________________________________^
@@ -36,19 +36,19 @@ LL | | });
3636
| |______^ help: remove the call to `map`
3737

3838
error: unnecessary map of the identity function
39-
--> tests/ui/map_identity.rs:25:36
39+
--> tests/ui/map_identity.rs:26:36
4040
|
4141
LL | let _: Result<u32, u32> = Ok(1).map_err(|a| a);
4242
| ^^^^^^^^^^^^^^^ help: remove the call to `map_err`
4343

4444
error: unnecessary map of the identity function
45-
--> tests/ui/map_identity.rs:36:22
45+
--> tests/ui/map_identity.rs:37:22
4646
|
4747
LL | let _ = x.clone().map(|(x, y)| (x, y));
4848
| ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
4949

5050
error: unnecessary map of the identity function
51-
--> tests/ui/map_identity.rs:38:22
51+
--> tests/ui/map_identity.rs:39:22
5252
|
5353
LL | let _ = x.clone().map(|(x, y)| {
5454
| ______________________^
@@ -58,76 +58,100 @@ LL | | });
5858
| |______^ help: remove the call to `map`
5959

6060
error: unnecessary map of the identity function
61-
--> tests/ui/map_identity.rs:42:22
61+
--> tests/ui/map_identity.rs:43:22
6262
|
6363
LL | let _ = x.clone().map(|(x, y)| return (x, y));
6464
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
6565

6666
error: unnecessary map of the identity function
67-
--> tests/ui/map_identity.rs:46:22
67+
--> tests/ui/map_identity.rs:47:22
6868
|
6969
LL | let _ = y.clone().map(|(x, y, (z, (w,)))| (x, y, (z, (w,))));
7070
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
7171

7272
error: unnecessary map of the identity function
73-
--> tests/ui/map_identity.rs:76:30
73+
--> tests/ui/map_identity.rs:77:30
7474
|
7575
LL | let _ = x.iter().copied().map(|(x, y)| (x, y));
7676
| ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
7777

7878
error: unnecessary map of the identity function
79-
--> tests/ui/map_identity.rs:88:15
79+
--> tests/ui/map_identity.rs:86:15
80+
|
81+
LL | let _ = it.map(|x| x).next();
82+
| ^^^^^^^^^^^
83+
|
84+
help: remove the call to `map`, and make `it` mutable
85+
|
86+
LL ~ let mut it = [1, 2, 3].into_iter();
87+
LL ~ let _ = it.next();
88+
|
89+
90+
error: unnecessary map of the identity function
91+
--> tests/ui/map_identity.rs:93:23
92+
|
93+
LL | let _ = subindex.0.map(|n| n).next();
94+
| ^^^^^^^^^^^
95+
|
96+
help: remove the call to `map`, and make `subindex` mutable
97+
|
98+
LL ~ let mut subindex = (index.by_ref().take(3), 42);
99+
LL ~ let _ = subindex.0.next();
100+
|
101+
102+
error: unnecessary map of the identity function
103+
--> tests/ui/map_identity.rs:100:15
80104
|
81105
LL | let _ = it.map(|x| x).next();
82106
| ^^^^^^^^^^^ help: remove the call to `map`
83107

84108
error: unnecessary map of the identity function
85-
--> tests/ui/map_identity.rs:93:19
109+
--> tests/ui/map_identity.rs:106:19
86110
|
87111
LL | let _ = { it }.map(|x| x).next();
88112
| ^^^^^^^^^^^ help: remove the call to `map`
89113

90114
error: unnecessary map of the identity function
91-
--> tests/ui/map_identity.rs:105:30
115+
--> tests/ui/map_identity.rs:119:30
92116
|
93117
LL | let _ = x.iter().copied().map(|[x, y]| [x, y]);
94118
| ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
95119

96120
error: unnecessary map of the identity function
97-
--> tests/ui/map_identity.rs:131:26
121+
--> tests/ui/map_identity.rs:145:26
98122
|
99123
LL | let _ = x.into_iter().map(|Foo { foo, bar }| Foo { foo, bar });
100124
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
101125

102126
error: unnecessary map of the identity function
103-
--> tests/ui/map_identity.rs:135:26
127+
--> tests/ui/map_identity.rs:149:26
104128
|
105129
LL | let _ = x.into_iter().map(|Foo { foo, bar }| foo::Foo { foo, bar });
106130
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
107131

108132
error: unnecessary map of the identity function
109-
--> tests/ui/map_identity.rs:143:26
133+
--> tests/ui/map_identity.rs:157:26
110134
|
111135
LL | let _ = x.into_iter().map(|Foo { foo, bar }| Foo { foo: foo, bar: bar });
112136
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
113137

114138
error: unnecessary map of the identity function
115-
--> tests/ui/map_identity.rs:147:26
139+
--> tests/ui/map_identity.rs:161:26
116140
|
117141
LL | let _ = x.into_iter().map(|Foo { foo, bar }| Foo { bar, foo });
118142
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
119143

120144
error: unnecessary map of the identity function
121-
--> tests/ui/map_identity.rs:157:26
145+
--> tests/ui/map_identity.rs:171:26
122146
|
123147
LL | let _ = x.into_iter().map(|Foo2(foo, bar)| Foo2(foo, bar));
124148
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
125149

126150
error: unnecessary map of the identity function
127-
--> tests/ui/map_identity.rs:161:26
151+
--> tests/ui/map_identity.rs:175:26
128152
|
129153
LL | let _ = x.into_iter().map(|Foo2(foo, bar)| foo::Foo2(foo, bar));
130154
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
131155

132-
error: aborting due to 20 previous errors
156+
error: aborting due to 22 previous errors
133157

0 commit comments

Comments
 (0)