.last() to .next_back() requires a mutable receiver (#14140)

In the case where `iter` is a `DoubleEndedIterator`, replacing a call to
`iter.last()` (which consumes `iter`) by `iter.next_back()` (which
requires a mutable reference to `iter`) cannot be done when `iter` is a
non-mutable binding which is not a mutable reference. When possible, a
local immutable binding is made into a mutable one.

Also, the applicability is switched to `MaybeIncorrect` and a note is
added to the output when the element types have a significant drop,
because the drop order will potentially be modified because
`.next_back()` does not consume the iterator nor the elements before the
last one.

Fix #14139

changelog: [`double_ended_iterator_last`]: do not trigger on
non-reference immutable receiver, and warn about possible drop order
change

Author: Centri3

clippy_lints/src/methods/double_ended_iterator_last.rs

@@ -1,8 +1,8 @@
-use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::is_trait_method;
+use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::ty::implements_trait;
+use clippy_utils::{is_mutable, is_trait_method, path_to_local};
 use rustc_errors::Applicability;
-use rustc_hir::Expr;
+use rustc_hir::{Expr, Node, PatKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty::Instance;
 use rustc_span::{Span, sym};
@@ -28,14 +28,47 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp
         // if the resolved method is the same as the provided definition
         && fn_def.def_id() == last_def.def_id
     {
-        span_lint_and_sugg(
+        let mut sugg = vec![(call_span, String::from("next_back()"))];
+        let mut dont_apply = false;
+        // if `self_expr` is a reference, it is mutable because it is used for `.last()`
+        if !(is_mutable(cx, self_expr) || self_type.is_ref()) {
+            if let Some(hir_id) = path_to_local(self_expr)
+                && 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.
+                dont_apply = true;
+            }
+        }
+        span_lint_and_then(
             cx,
             DOUBLE_ENDED_ITERATOR_LAST,
-            call_span,
+            expr.span,
             "called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator",
-            "try",
-            "next_back()".to_string(),
-            Applicability::MachineApplicable,
+            |diag| {
+                let expr_ty = cx.typeck_results().expr_ty(expr);
+                let droppable_elements = expr_ty.has_significant_drop(cx.tcx, cx.typing_env());
+                diag.multipart_suggestion(
+                    "try",
+                    sugg,
+                    if dont_apply {
+                        Applicability::Unspecified
+                    } else if droppable_elements {
+                        Applicability::MaybeIncorrect
+                    } else {
+                        Applicability::MachineApplicable
+                    },
+                );
+                if droppable_elements {
+                    diag.note("this change will alter drop order which may be undesirable");
+                }
+                if dont_apply {
+                    diag.span_note(self_expr.span, "this must be made mutable to use `.next_back()`");
+                }
+            },
         );
     }
 }

clippy_lints/src/unnecessary_struct_initialization.rs

@@ -1,8 +1,8 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet;
 use clippy_utils::ty::is_copy;
-use clippy_utils::{get_parent_expr, path_to_local};
-use rustc_hir::{BindingMode, Expr, ExprField, ExprKind, Node, PatKind, Path, QPath, StructTailExpr, UnOp};
+use clippy_utils::{get_parent_expr, is_mutable, path_to_local};
+use rustc_hir::{Expr, ExprField, ExprKind, Path, QPath, StructTailExpr, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
 
@@ -157,16 +157,6 @@ fn same_path_in_all_fields<'tcx>(
     }
 }
 
-fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    if let Some(hir_id) = path_to_local(expr)
-        && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
-    {
-        matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
-    } else {
-        true
-    }
-}
-
 fn check_references(cx: &LateContext<'_>, expr_a: &Expr<'_>, expr_b: &Expr<'_>) -> bool {
     if let Some(parent) = get_parent_expr(cx, expr_a)
         && let parent_ty = cx.typeck_results().expr_ty_adjusted(parent)

clippy_utils/src/lib.rs

@@ -3679,3 +3679,24 @@ pub fn expr_requires_coercion<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -
         _ => false,
     }
 }
+
+/// Returns `true` if `expr` designates a mutable static, a mutable local binding, or an expression
+/// that can be owned.
+pub fn is_mutable(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let Some(hir_id) = path_to_local(expr)
+        && let Node::Pat(pat) = cx.tcx.hir_node(hir_id)
+    {
+        matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
+    } else if let ExprKind::Path(p) = &expr.kind
+        && let Some(mutability) = cx
+            .qpath_res(p, expr.hir_id)
+            .opt_def_id()
+            .and_then(|id| cx.tcx.static_mutability(id))
+    {
+        mutability == Mutability::Mut
+    } else if let ExprKind::Field(parent, _) = expr.kind {
+        is_mutable(cx, parent)
+    } else {
+        true
+    }
+}

tests/ui/double_ended_iterator_last.fixed

@@ -2,8 +2,7 @@
 
 // Typical case
 pub fn last_arg(s: &str) -> Option<&str> {
-    s.split(' ').next_back()
-    //~^ double_ended_iterator_last
+    s.split(' ').next_back() //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
 }
 
 fn main() {
@@ -20,8 +19,7 @@ fn main() {
             Some(())
         }
     }
-    let _ = DeIterator.next_back();
-    //~^ double_ended_iterator_last
+    let _ = DeIterator.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
     // Should not apply to other methods of Iterator
     let _ = DeIterator.count();
 
@@ -53,3 +51,42 @@ fn main() {
     }
     let _ = CustomLast.last();
 }
+
+fn issue_14139() {
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let subindex = &mut subindex;
+    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let subindex = &mut subindex;
+    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let (mut subindex, _) = (index.by_ref().take(3), 42);
+    let _ = subindex.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+}
+
+fn drop_order() {
+    struct S(&'static str);
+    impl std::ops::Drop for S {
+        fn drop(&mut self) {
+            println!("Dropping {}", self.0);
+        }
+    }
+
+    let v = vec![S("one"), S("two"), S("three")];
+    let mut v = v.into_iter();
+    println!("Last element is {}", v.next_back().unwrap().0);
+    //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    println!("Done");
+}

tests/ui/double_ended_iterator_last.rs

@@ -2,8 +2,7 @@
 
 // Typical case
 pub fn last_arg(s: &str) -> Option<&str> {
-    s.split(' ').last()
-    //~^ double_ended_iterator_last
+    s.split(' ').last() //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
 }
 
 fn main() {
@@ -20,8 +19,7 @@ fn main() {
             Some(())
         }
     }
-    let _ = DeIterator.last();
-    //~^ double_ended_iterator_last
+    let _ = DeIterator.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
     // Should not apply to other methods of Iterator
     let _ = DeIterator.count();
 
@@ -53,3 +51,42 @@ fn main() {
     }
     let _ = CustomLast.last();
 }
+
+fn issue_14139() {
+    let mut index = [true, true, false, false, false, true].iter();
+    let subindex = index.by_ref().take(3);
+    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let subindex = &mut subindex;
+    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let mut subindex = index.by_ref().take(3);
+    let subindex = &mut subindex;
+    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+
+    let mut index = [true, true, false, false, false, true].iter();
+    let (subindex, _) = (index.by_ref().take(3), 42);
+    let _ = subindex.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+}
+
+fn drop_order() {
+    struct S(&'static str);
+    impl std::ops::Drop for S {
+        fn drop(&mut self) {
+            println!("Dropping {}", self.0);
+        }
+    }
+
+    let v = vec![S("one"), S("two"), S("three")];
+    let v = v.into_iter();
+    println!("Last element is {}", v.last().unwrap().0);
+    //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+    println!("Done");
+}

tests/ui/double_ended_iterator_last.stderr

@@ -1,17 +1,82 @@
 error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:5:18
+  --> tests/ui/double_ended_iterator_last.rs:5:5
    |
 LL |     s.split(' ').last()
-   |                  ^^^^^^ help: try: `next_back()`
+   |     ^^^^^^^^^^^^^------
+   |                  |
+   |                  help: try: `next_back()`
    |
    = note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
 
 error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
-  --> tests/ui/double_ended_iterator_last.rs:23:24
+  --> tests/ui/double_ended_iterator_last.rs:22:13
    |
 LL |     let _ = DeIterator.last();
-   |                        ^^^^^^ help: try: `next_back()`
+   |             ^^^^^^^^^^^------
+   |                        |
+   |                        help: try: `next_back()`
 
-error: aborting due to 2 previous errors
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last.rs:58:13
+   |
+LL |     let _ = subindex.last();
+   |             ^^^^^^^^^^^^^^^
+   |
+help: try
+   |
+LL ~     let mut subindex = index.by_ref().take(3);
+LL ~     let _ = subindex.next_back();
+   |
+
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last.rs:62:13
+   |
+LL |     let _ = subindex.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:67:13
+   |
+LL |     let _ = subindex.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:72:13
+   |
+LL |     let _ = subindex.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:76:13
+   |
+LL |     let _ = subindex.last();
+   |             ^^^^^^^^^^^^^^^
+   |
+help: try
+   |
+LL ~     let (mut subindex, _) = (index.by_ref().take(3), 42);
+LL ~     let _ = subindex.next_back();
+   |
+
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last.rs:89:36
+   |
+LL |     println!("Last element is {}", v.last().unwrap().0);
+   |                                    ^^^^^^^^
+   |
+   = note: this change will alter drop order which may be undesirable
+help: try
+   |
+LL ~     let mut v = v.into_iter();
+LL ~     println!("Last element is {}", v.next_back().unwrap().0);
+   |
+
+error: aborting due to 8 previous errors
 

tests/ui/double_ended_iterator_last_unfixable.rs

@@ -0,0 +1,23 @@
+//@no-rustfix
+#![warn(clippy::double_ended_iterator_last)]
+
+fn main() {
+    let mut index = [true, true, false, false, false, true].iter();
+    let subindex = (index.by_ref().take(3), 42);
+    let _ = subindex.0.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator`
+}
+
+fn drop_order() {
+    struct S(&'static str);
+    impl std::ops::Drop for S {
+        fn drop(&mut self) {
+            println!("Dropping {}", self.0);
+        }
+    }
+
+    let v = vec![S("one"), S("two"), S("three")];
+    let v = (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_unfixable.stderr

@@ -0,0 +1,33 @@
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last_unfixable.rs:7:13
+   |
+LL |     let _ = subindex.0.last();
+   |             ^^^^^^^^^^^------
+   |                        |
+   |                        help: try: `next_back()`
+   |
+note: this must be made mutable to use `.next_back()`
+  --> tests/ui/double_ended_iterator_last_unfixable.rs:7:13
+   |
+LL |     let _ = subindex.0.last();
+   |             ^^^^^^^^^^
+   = note: `-D clippy::double-ended-iterator-last` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::double_ended_iterator_last)]`
+
+error: called `Iterator::last` on a `DoubleEndedIterator`; this will needlessly iterate the entire iterator
+  --> tests/ui/double_ended_iterator_last_unfixable.rs:20:36
+   |
+LL |     println!("Last element is {}", v.0.last().unwrap().0);
+   |                                    ^^^^------
+   |                                        |
+   |                                        help: try: `next_back()`
+   |
+   = note: this change will alter drop order which may be undesirable
+note: this must be made mutable to use `.next_back()`
+  --> tests/ui/double_ended_iterator_last_unfixable.rs:20:36
+   |
+LL |     println!("Last element is {}", v.0.last().unwrap().0);
+   |                                    ^^^
+
+error: aborting due to 2 previous errors
+