From 45f7a60d313f75709690bfcb1cc4232d0f44ed3f Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Sun, 2 Feb 2025 17:16:36 +0100 Subject: [PATCH] `.last()` to `.next_back()` requires a mutable receiver 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 not a mutable binding or a mutable reference. When `iter` is a local binding, it can be made mutable by fixing its definition site. --- .../src/methods/double_ended_iterator_last.rs | 42 ++++++++++--- tests/ui/double_ended_iterator_last.fixed | 30 +++++++-- tests/ui/double_ended_iterator_last.rs | 30 +++++++-- tests/ui/double_ended_iterator_last.stderr | 62 +++++++++++++++++-- .../double_ended_iterator_last_unfixable.rs | 8 +++ ...ouble_ended_iterator_last_unfixable.stderr | 18 ++++++ 6 files changed, 169 insertions(+), 21 deletions(-) create mode 100644 tests/ui/double_ended_iterator_last_unfixable.rs create mode 100644 tests/ui/double_ended_iterator_last_unfixable.stderr diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index 208172980c9f..19797cc32f59 100644 --- a/clippy_lints/src/methods/double_ended_iterator_last.rs +++ b/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,40 @@ 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| { + diag.multipart_suggestion( + "try", + sugg, + if dont_apply { + Applicability::Unspecified + } else { + Applicability::MachineApplicable + }, + ); + if dont_apply { + diag.span_note(self_expr.span, "this must be made mutable to use `.next_back()`"); + } + }, ); } } diff --git a/tests/ui/double_ended_iterator_last.fixed b/tests/ui/double_ended_iterator_last.fixed index 08f3d4907153..09eba48ea89a 100644 --- a/tests/ui/double_ended_iterator_last.fixed +++ b/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,27 @@ 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` +} diff --git a/tests/ui/double_ended_iterator_last.rs b/tests/ui/double_ended_iterator_last.rs index 2c2f311805ef..2bde5e3ecb31 100644 --- a/tests/ui/double_ended_iterator_last.rs +++ b/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,27 @@ 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` +} diff --git a/tests/ui/double_ended_iterator_last.stderr b/tests/ui/double_ended_iterator_last.stderr index ea1962176a61..6c900b078262 100644 --- a/tests/ui/double_ended_iterator_last.stderr +++ b/tests/ui/double_ended_iterator_last.stderr @@ -1,17 +1,69 @@ 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: aborting due to 7 previous errors diff --git a/tests/ui/double_ended_iterator_last_unfixable.rs b/tests/ui/double_ended_iterator_last_unfixable.rs new file mode 100644 index 000000000000..edc2a05649dd --- /dev/null +++ b/tests/ui/double_ended_iterator_last_unfixable.rs @@ -0,0 +1,8 @@ +//@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` +} diff --git a/tests/ui/double_ended_iterator_last_unfixable.stderr b/tests/ui/double_ended_iterator_last_unfixable.stderr new file mode 100644 index 000000000000..9dd3f9386481 --- /dev/null +++ b/tests/ui/double_ended_iterator_last_unfixable.stderr @@ -0,0 +1,18 @@ +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: aborting due to 1 previous error +