WIP: map_identity: suggest making the variable mutable when necessary

clippy_lints/src/methods/double_ended_iterator_last.rs

@@ -1,6 +1,6 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::ty::{has_non_owning_mutable_access, implements_trait};
-use clippy_utils::{is_mutable, is_trait_method, path_to_local, sym};
+use clippy_utils::{is_mutable, is_trait_method, path_to_local_with_projections, sym};
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, Node, PatKind};
 use rustc_lint::LateContext;
@@ -37,7 +37,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp
         // TODO: Change this to lint only when the referred iterator is not used later. If it is used later,
         // changing to `next_back()` may change its behavior.
         if !(is_mutable(cx, self_expr) || self_type.is_ref()) {
-            if let Some(hir_id) = path_to_local(self_expr)
+            if let Some(hir_id) = path_to_local_with_projections(self_expr)
                 && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
                 && let PatKind::Binding(_, _, ident, _) = pat.kind
             {

clippy_lints/src/methods/filter_next.rs

@@ -1,4 +1,5 @@
 use clippy_utils::diagnostics::{span_lint, span_lint_and_then};
+use clippy_utils::path_to_local_with_projections;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::implements_trait;
 use rustc_ast::{BindingMode, Mutability};
@@ -9,21 +10,6 @@ use rustc_span::sym;
 
 use super::FILTER_NEXT;
 
-fn path_to_local(expr: &hir::Expr<'_>) -> Option<hir::HirId> {
-    match expr.kind {
-        hir::ExprKind::Field(f, _) => path_to_local(f),
-        hir::ExprKind::Index(recv, _, _) => path_to_local(recv),
-        hir::ExprKind::Path(hir::QPath::Resolved(
-            _,
-            hir::Path {
-                res: rustc_hir::def::Res::Local(local),
-                ..
-            },
-        )) => Some(*local),
-        _ => None,
-    }
-}
-
 /// lint use of `filter().next()` for `Iterators`
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
@@ -44,7 +30,7 @@ pub(super) fn check<'tcx>(
             let iter_snippet = snippet(cx, recv.span, "..");
             // add note if not multi-line
             span_lint_and_then(cx, FILTER_NEXT, expr.span, msg, |diag| {
-                let (applicability, pat) = if let Some(id) = path_to_local(recv)
+                let (applicability, pat) = if let Some(id) = path_to_local_with_projections(recv)
                     && let hir::Node::Pat(pat) = cx.tcx.hir_node(id)
                     && let hir::PatKind::Binding(BindingMode(_, Mutability::Not), _, ident, _) = pat.kind
                 {

clippy_lints/src/methods/map_identity.rs

@@ -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_with_projections};
 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};
 
@@ -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_with_projections(caller)
+                && 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());
+                    }
+                }
+            },
         );
     }
 }

clippy_lints/src/transmute/eager_transmute.rs

@@ -1,5 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_then;
-use clippy_utils::{eq_expr_value, path_to_local, sym};
+use clippy_utils::{eq_expr_value, path_to_local_with_projections, sym};
 use rustc_abi::WrappingRange;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind, Node};
@@ -63,11 +63,7 @@ fn binops_with_local(cx: &LateContext<'_>, local_expr: &Expr<'_>, expr: &Expr<'_
 /// 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,
-    }
+    path_to_local_with_projections(expr).is_some()
 }
 
 pub(super) fn check<'tcx>(

clippy_utils/src/lib.rs

@@ -460,6 +460,23 @@ pub fn path_to_local_id(expr: &Expr<'_>, id: HirId) -> bool {
     path_to_local(expr) == Some(id)
 }
 
+/// If the expression is a path to a local (with optional projections),
+/// returns the canonical `HirId` of the local.
+///
+/// For example, `x.field[0].field2` would return the `HirId` of `x`.
+pub fn path_to_local_with_projections(expr: &Expr<'_>) -> Option<HirId> {
+    match expr.kind {
+        ExprKind::Field(recv, _) | ExprKind::Index(recv, _, _) => path_to_local_with_projections(recv),
+        ExprKind::Path(QPath::Resolved(
+            _,
+            Path {
+                res: Res::Local(local), ..
+            },
+        )) => Some(*local),
+        _ => None,
+    }
+}
+
 pub trait MaybePath<'hir> {
     fn hir_id(&self) -> HirId;
     fn qpath_opt(&self) -> Option<&QPath<'hir>>;

tests/ui/double_ended_iterator_last.fixed

@@ -81,6 +81,11 @@ fn issue_14139() {
     let (subindex, _) = (index.by_ref().take(3), 42);
     let _ = subindex.last();
     let _ = index.next();
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let subindex = (index.by_ref().take(3), 42);
+    let _ = subindex.0.last();
+    let _ = index.next();
 }
 
 fn drop_order() {
@@ -108,6 +113,12 @@ fn drop_order() {
     let mut v = DropDeIterator(v.into_iter());
     println!("Last element is {}", v.next_back().unwrap().0);
     //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let v = vec![S("four"), S("five"), S("six")];
+    let mut v = (DropDeIterator(v.into_iter()), 42);
+    println!("Last element is {}", v.0.next_back().unwrap().0);
+    //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
     println!("Done");
 }
 

tests/ui/double_ended_iterator_last.rs

@@ -81,6 +81,11 @@ fn issue_14139() {
     let (subindex, _) = (index.by_ref().take(3), 42);
     let _ = subindex.last();
     let _ = index.next();
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let subindex = (index.by_ref().take(3), 42);
+    let _ = subindex.0.last();
+    let _ = index.next();
 }
 
 fn drop_order() {
@@ -108,6 +113,12 @@ fn drop_order() {
     let v = DropDeIterator(v.into_iter());
     println!("Last element is {}", v.last().unwrap().0);
     //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let v = vec![S("four"), S("five"), S("six")];
+    let v = (DropDeIterator(v.into_iter()), 42);
+    println!("Last element is {}", v.0.last().unwrap().0);
+    //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
     println!("Done");
 }
 

tests/ui/double_ended_iterator_last.stderr

@@ -18,7 +18,7 @@ LL |     let _ = DeIterator.last();
    |                        help: try: `next_back()`
 
 error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:109:36
+  --> tests/ui/double_ended_iterator_last.rs:114:36
    |
 LL |     println!("Last element is {}", v.last().unwrap().0);
    |                                    ^^^^^^^^
@@ -30,5 +30,18 @@ LL ~     let mut v = DropDeIterator(v.into_iter());
 LL ~     println!("Last element is {}", v.next_back().unwrap().0);
    |
 
-error: aborting due to 3 previous errors
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last.rs:119:36
+   |
+LL |     println!("Last element is {}", v.0.last().unwrap().0);
+   |                                    ^^^^^^^^^^
+   |
+   = note: this change will alter drop order which may be undesirable
+help: try
+   |
+LL ~     let mut v = (DropDeIterator(v.into_iter()), 42);
+LL ~     println!("Last element is {}", v.0.next_back().unwrap().0);
+   |
+
+error: aborting due to 4 previous errors
 

tests/ui/map_identity.fixed

@@ -72,9 +72,18 @@ 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
+    let mut index = [1, 2, 3].into_iter();
+    let mut subindex = (index.by_ref().take(3), 42);
+    let _ = subindex.0.next();
+    //~^ map_identity
 
     // lint
     #[allow(unused_mut)]

tests/ui/map_identity.rs

@@ -78,9 +78,18 @@ 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
+    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
 
     // lint
     #[allow(unused_mut)]