`map_identity`: suggest making the variable mutable when necessary

4 files changed, 148 insertions, 50 deletions

diff --git a/clippy_lints/src/methods/map_identity.rs b/clippy_lints/src/methods/map_identity.rs
index 98def66ca14..a98cfff8bfb 100644
--- a/clippy_lints/src/methods/map_identity.rs
+++ b/clippy_lints/src/methods/map_identity.rs
@@ -1,14 +1,16 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
-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::diagnostics::{span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::{is_copy, is_type_diagnostic_item};
+use clippy_utils::{is_expr_untyped_identity_function, is_mutable, is_trait_method, path_to_local_with_projections};
 use rustc_errors::Applicability;
-use rustc_hir::{self as hir, Node, PatKind};
-use rustc_lint::LateContext;
+use rustc_hir::{self as hir, ExprKind, Node, PatKind};
+use rustc_lint::{LateContext, LintContext};
 use rustc_span::{Span, Symbol, sym};
 
 use super::MAP_IDENTITY;
 
+const MSG: &str = "unnecessary map of the identity function";
+
 pub(super) fn check(
     cx: &LateContext<'_>,
     expr: &hir::Expr<'_>,
@@ -23,26 +25,70 @@ 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 main_sugg = (call_span, String::new());
+        let mut app = if is_copy(cx, caller_ty) {
+            // there is technically a behavioral change here for `Copy` iterators, where
+            // `iter.map(|x| x).next()` would mutate a temporary copy of the iterator and
+            // changing it to `iter.next()` mutates iter directly
+            Applicability::Unspecified
+        } else {
+            Applicability::MachineApplicable
+        };
+
+        let needs_to_be_mutable = cx.typeck_results().expr_ty_adjusted(expr).is_mutable_ptr();
+        if needs_to_be_mutable && !is_mutable(cx, caller) {
+            if let Some(hir_id) = path_to_local_with_projections(caller)
+                && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
+                && let PatKind::Binding(_, _, ident, _) = pat.kind
+            {
+                // We can reach the binding -- suggest making it mutable
+                let suggs = vec![main_sugg, (ident.span.shrink_to_lo(), String::from("mut "))];
+
+                let ident = snippet_with_applicability(cx.sess(), ident.span, "_", &mut app);
 
-        span_lint_and_sugg(
-            cx,
-            MAP_IDENTITY,
-            sugg_span,
-            "unnecessary map of the identity function",
-            format!("remove the call to `{name}`"),
-            String::new(),
-            Applicability::MachineApplicable,
-        );
+                span_lint_and_then(cx, MAP_IDENTITY, call_span, MSG, |diag| {
+                    diag.multipart_suggestion(
+                        format!("remove the call to `{name}`, and make `{ident}` mutable"),
+                        suggs,
+                        app,
+                    );
+                });
+            } else {
+                // If we can't make the binding mutable, prevent the suggestion from being automatically applied,
+                // and add a complementary help message.
+                app = Applicability::Unspecified;
+
+                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, call_span, MSG, |diag| {
+                    diag.span_suggestion(main_sugg.0, format!("remove the call to `{name}`"), main_sugg.1, app);
+
+                    let note = if let Some(method_requiring_mut) = method_requiring_mut {
+                        format!("this must be made mutable to use `{method_requiring_mut}`")
+                    } else {
+                        "this must be made mutable".to_string()
+                    };
+                    diag.span_note(caller.span, note);
+                });
+            }
+        } else {
+            span_lint_and_sugg(
+                cx,
+                MAP_IDENTITY,
+                main_sugg.0,
+                MSG,
+                format!("remove the call to `{name}`"),
+                main_sugg.1,
+                app,
+            );
+        }
     }
 }
diff --git a/tests/ui/map_identity.fixed b/tests/ui/map_identity.fixed
index 6c971ba6338..a10aae8cc12 100644
--- a/tests/ui/map_identity.fixed
+++ b/tests/ui/map_identity.fixed
@@ -1,3 +1,4 @@
+//@require-annotations-for-level: ERROR
 #![warn(clippy::map_identity)]
 #![allow(clippy::needless_return, clippy::disallowed_names)]
 
@@ -72,20 +73,33 @@ 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
+    //~| HELP: remove the call to `map`, and make `it` mutable
+
+    // lint
+    let mut index = [1, 2, 3].into_iter();
+    let mut subindex = (index.by_ref().take(3), 42);
+    let _ = subindex.0.next();
+    //~^ map_identity
+    //~| HELP: remove the call to `map`, and make `subindex` mutable
 
     // lint
     #[allow(unused_mut)]
     let mut it = [1, 2, 3].into_iter();
     let _ = it.next();
     //~^ map_identity
+    //~| HELP: remove the call to `map`
 
     // lint
     let it = [1, 2, 3].into_iter();
     let _ = { it }.next();
     //~^ map_identity
+    //~| HELP: remove the call to `map`
 }
 
 // same as `issue11764`, but for arrays
diff --git a/tests/ui/map_identity.rs b/tests/ui/map_identity.rs
index 59dcfcda3b6..fc6e018f924 100644
--- a/tests/ui/map_identity.rs
+++ b/tests/ui/map_identity.rs
@@ -1,3 +1,4 @@
+//@require-annotations-for-level: ERROR
 #![warn(clippy::map_identity)]
 #![allow(clippy::needless_return, clippy::disallowed_names)]
 
@@ -78,20 +79,33 @@ 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
+    //~| HELP: remove the call to `map`, and make `it` mutable
+
+    // lint
+    let mut index = [1, 2, 3].into_iter();
+    let subindex = (index.by_ref().take(3), 42);
+    let _ = subindex.0.map(|n| n).next();
+    //~^ map_identity
+    //~| HELP: remove the call to `map`, and make `subindex` mutable
 
     // lint
     #[allow(unused_mut)]
     let mut it = [1, 2, 3].into_iter();
     let _ = it.map(|x| x).next();
     //~^ map_identity
+    //~| HELP: remove the call to `map`
 
     // lint
     let it = [1, 2, 3].into_iter();
     let _ = { it }.map(|x| x).next();
     //~^ map_identity
+    //~| HELP: remove the call to `map`
 }
 
 // same as `issue11764`, but for arrays
diff --git a/tests/ui/map_identity.stderr b/tests/ui/map_identity.stderr
index a50c0d6b87b..8d19d62cdc6 100644
--- a/tests/ui/map_identity.stderr
+++ b/tests/ui/map_identity.stderr
@@ -1,5 +1,5 @@
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:7:47
+  --> tests/ui/map_identity.rs:8:47
    |
 LL |     let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
    |                                               ^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
@@ -8,25 +8,25 @@ LL |     let _: Vec<_> = x.iter().map(not_identity).map(|x| return x).collect();
    = help: to override `-D warnings` add `#[allow(clippy::map_identity)]`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:9:57
+  --> tests/ui/map_identity.rs:10: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
-  --> tests/ui/map_identity.rs:9:29
+  --> tests/ui/map_identity.rs:10: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
-  --> tests/ui/map_identity.rs:12:32
+  --> tests/ui/map_identity.rs:13:32
    |
 LL |     let _: Option<u8> = Some(3).map(|x| x);
    |                                ^^^^^^^^^^^ help: remove the call to `map`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:14:36
+  --> tests/ui/map_identity.rs:15:36
    |
 LL |       let _: Result<i8, f32> = Ok(-3).map(|x| {
    |  ____________________________________^
@@ -36,19 +36,19 @@ LL | |     });
    | |______^ help: remove the call to `map`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:25:36
+  --> tests/ui/map_identity.rs:26:36
    |
 LL |     let _: Result<u32, u32> = Ok(1).map_err(|a| a);
    |                                    ^^^^^^^^^^^^^^^ help: remove the call to `map_err`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:36:22
+  --> tests/ui/map_identity.rs:37:22
    |
 LL |     let _ = x.clone().map(|(x, y)| (x, y));
    |                      ^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:38:22
+  --> tests/ui/map_identity.rs:39:22
    |
 LL |       let _ = x.clone().map(|(x, y)| {
    |  ______________________^
@@ -58,76 +58,100 @@ LL | |     });
    | |______^ help: remove the call to `map`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:42:22
+  --> tests/ui/map_identity.rs:43:22
    |
 LL |     let _ = x.clone().map(|(x, y)| return (x, y));
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:46:22
+  --> tests/ui/map_identity.rs:47:22
    |
 LL |     let _ = y.clone().map(|(x, y, (z, (w,)))| (x, y, (z, (w,))));
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the call to `map`
 
 error: unnecessary map of the identity function
-  --> tests/ui/map_identity.rs:76:30
+  --> tests/ui/map_identity.rs:77: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:88:15
+  --> tests/ui/map_identity.rs:86:15
+   |
+LL |     let _ = it.map(|x| x).next();
+   |               ^^^^^^^^^^^
+   |
+help: remove the call to `map`, and make `it` mutable
+   |
+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:93:23
+   |
+LL |     let _ = subindex.0.map(|n| n).next();
+   |                       ^^^^^^^^^^^
+   |
+help: remove the call to `map`, and make `subindex` mutable
+   |
+LL ~     let mut subindex = (index.by_ref().take(3), 42);
+LL ~     let _ = subindex.0.next();
+   |
+
+error: unnecessary map of the identity function
+  --> tests/ui/map_identity.rs:100: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:106: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:119: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:145: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:149: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:157: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:161: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:171: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:175: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 22 previous errors