Skip to content

WIP: map_identity: suggest making the variable mutable when necessary #15268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 48 additions & 18 deletions clippy_lints/src/methods/map_identity.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{is_expr_untyped_identity_function, is_trait_method, path_to_local};
use rustc_ast::BindingMode;
use clippy_utils::{is_expr_untyped_identity_function, is_mutable, is_trait_method, path_to_local};
use rustc_errors::Applicability;
use rustc_hir::{self as hir, Node, PatKind};
use rustc_hir::{self as hir, ExprKind, Node, PatKind};
use rustc_lint::LateContext;
use rustc_span::{Span, Symbol, sym};

Expand All @@ -23,26 +22,57 @@ pub(super) fn check(
|| is_type_diagnostic_item(cx, caller_ty, sym::Result)
|| is_type_diagnostic_item(cx, caller_ty, sym::Option))
&& is_expr_untyped_identity_function(cx, map_arg)
&& let Some(sugg_span) = expr.span.trim_start(caller.span)
&& let Some(call_span) = expr.span.trim_start(caller.span)
{
// If the result of `.map(identity)` is used as a mutable reference,
// the caller must not be an immutable binding.
if cx.typeck_results().expr_ty_adjusted(expr).is_mutable_ptr()
&& let Some(hir_id) = path_to_local(caller)
&& let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
&& !matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
{
return;
let mut sugg = vec![(call_span, String::new())];
let mut apply = true;
if !is_mutable(cx, caller) {
if let Some(hir_id) = path_to_local(caller)
Copy link
Member

@y21 y21 Aug 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it'd be worth moving this function to clippy_utils (as path_to_local_with_projections), and using that one instead of path_to_local. That would make the test case in _unfixable fixable right?

The lint where this is from is also doing the same kind of thing as this PR is, so it seems like this is not too uncommon of an issue that it could justify adding a new util function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea!

In fact, I was already able to find one more place that could make use of this function:

/// Checks if an expression is a path to a local variable (with optional projections), e.g.
/// `x.field[0].field2` would return true.
fn is_local_with_projections(expr: &Expr<'_>) -> bool {
match expr.kind {
ExprKind::Path(_) => path_to_local(expr).is_some(),
ExprKind::Field(expr, _) | ExprKind::Index(expr, ..) => is_local_with_projections(expr),
_ => false,
}
}

could be just path_to_local_with_projections(expr).is_some().

So maybe it would make sense to open a separate PR for creating this function and finding all the places it could be used in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and opened a PR at #15396, let me know what you think of it

&& let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
&& let PatKind::Binding(_, _, ident, _) = pat.kind
{
sugg.push((ident.span.shrink_to_lo(), String::from("mut ")));
} else {
// If we can't make the binding mutable, make the suggestion `Unspecified` to prevent it from being
// automatically applied, and add a complementary help message.
apply = false;
}
}

span_lint_and_sugg(
let method_requiring_mut = if let Node::Expr(expr) = cx.tcx.parent_hir_node(expr.hir_id)
&& let ExprKind::MethodCall(method, ..) = expr.kind
{
Some(method.ident)
} else {
None
};

span_lint_and_then(
cx,
MAP_IDENTITY,
sugg_span,
call_span,
"unnecessary map of the identity function",
format!("remove the call to `{name}`"),
String::new(),
Applicability::MachineApplicable,
|diag| {
diag.multipart_suggestion(
format!("remove the call to `{name}`"),
sugg,
if apply {
Applicability::MachineApplicable
} else {
Applicability::Unspecified
},
);
if !apply {
if let Some(method_requiring_mut) = method_requiring_mut {
diag.span_note(
caller.span,
format!("this must be made mutable to use `{method_requiring_mut}`"),
);
} else {
diag.span_note(caller.span, "this must be made mutable".to_string());
}
}
},
);
}
}
9 changes: 6 additions & 3 deletions tests/ui/map_identity.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,12 @@ fn issue11764() {
}

fn issue13904() {
// don't lint: `it.next()` would not be legal as `it` is immutable
let it = [1, 2, 3].into_iter();
let _ = it.map(|x| x).next();
// lint, but there's a catch:
// when we remove the `.map()`, `it.next()` would require `it` to be mutable
// therefore, include that in the suggestion as well
let mut it = [1, 2, 3].into_iter();
let _ = it.next();
//~^ map_identity

// lint
#[allow(unused_mut)]
Expand Down
5 changes: 4 additions & 1 deletion tests/ui/map_identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,12 @@ fn issue11764() {
}

fn issue13904() {
// don't lint: `it.next()` would not be legal as `it` is immutable
// lint, but there's a catch:
// when we remove the `.map()`, `it.next()` would require `it` to be mutable
// therefore, include that in the suggestion as well
let it = [1, 2, 3].into_iter();
let _ = it.map(|x| x).next();
//~^ map_identity

// lint
#[allow(unused_mut)]
Expand Down
32 changes: 22 additions & 10 deletions tests/ui/map_identity.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -76,58 +76,70 @@ LL | let _ = x.iter().copied().map(|(x, y)| (x, y));
| ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`

error: unnecessary map of the identity function
--> tests/ui/map_identity.rs:88:15
--> tests/ui/map_identity.rs:85:15
|
LL | let _ = it.map(|x| x).next();
| ^^^^^^^^^^^
|
help: remove the call to `map`
|
LL ~ let mut it = [1, 2, 3].into_iter();
LL ~ let _ = it.next();
|

error: unnecessary map of the identity function
--> tests/ui/map_identity.rs:91:15
|
LL | let _ = it.map(|x| x).next();
| ^^^^^^^^^^^ help: remove the call to `map`

error: unnecessary map of the identity function
--> tests/ui/map_identity.rs:93:19
--> tests/ui/map_identity.rs:96:19
|
LL | let _ = { it }.map(|x| x).next();
| ^^^^^^^^^^^ help: remove the call to `map`

error: unnecessary map of the identity function
--> tests/ui/map_identity.rs:105:30
--> tests/ui/map_identity.rs:108:30
|
LL | let _ = x.iter().copied().map(|[x, y]| [x, y]);
| ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`

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

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

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

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

error: unnecessary map of the identity function
--> tests/ui/map_identity.rs:157:26
--> tests/ui/map_identity.rs:160:26
|
LL | let _ = x.into_iter().map(|Foo2(foo, bar)| Foo2(foo, bar));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`

error: unnecessary map of the identity function
--> tests/ui/map_identity.rs:161:26
--> tests/ui/map_identity.rs:164:26
|
LL | let _ = x.into_iter().map(|Foo2(foo, bar)| foo::Foo2(foo, bar));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`

error: aborting due to 20 previous errors
error: aborting due to 21 previous errors

9 changes: 9 additions & 0 deletions tests/ui/map_identity_unfixable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//@no-rustfix
#![warn(clippy::map_identity)]

fn main() {
let mut index = [true, true, false, false, false, true].iter();
let subindex = (index.by_ref().take(3), 42);
let _ = subindex.0.map(|n| n).next();
//~^ map_identity
}
16 changes: 16 additions & 0 deletions tests/ui/map_identity_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error: unnecessary map of the identity function
--> tests/ui/map_identity_unfixable.rs:7:23
|
LL | let _ = subindex.0.map(|n| n).next();
| ^^^^^^^^^^^ help: remove the call to `map`
|
note: this must be made mutable to use `next`
--> tests/ui/map_identity_unfixable.rs:7:13
|
LL | let _ = subindex.0.map(|n| n).next();
| ^^^^^^^^^^
= note: `-D clippy::map-identity` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::map_identity)]`

error: aborting due to 1 previous error