diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index 208172980c9f..19a74809566f 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()"))]; + // 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 { + // Do not lint if we can't make the binding mutable. + return; + } + } + 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 droppable_elements { + Applicability::MaybeIncorrect + } else { + Applicability::MachineApplicable + }, + ); + if droppable_elements { + diag.note("this might cause unretrieved elements to be dropped after the retrieved one"); + } + }, ); } } diff --git a/clippy_lints/src/unnecessary_struct_initialization.rs b/clippy_lints/src/unnecessary_struct_initialization.rs index 1df229c330eb..5792b6b3178d 100644 --- a/clippy_lints/src/unnecessary_struct_initialization.rs +++ b/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) diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 80d52a26e223..719566e1aaa4 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -3595,3 +3595,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 + } +} diff --git a/tests/ui/double_ended_iterator_last.fixed b/tests/ui/double_ended_iterator_last.fixed index 06c48e337537..ace3d7df312b 100644 --- a/tests/ui/double_ended_iterator_last.fixed +++ b/tests/ui/double_ended_iterator_last.fixed @@ -2,7 +2,7 @@ // Typical case pub fn last_arg(s: &str) -> Option<&str> { - s.split(' ').next_back() + s.split(' ').next_back() //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator` } fn main() { @@ -19,7 +19,7 @@ fn main() { Some(()) } } - let _ = DeIterator.next_back(); + let _ = DeIterator.next_back(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator` // Should not apply to other methods of Iterator let _ = DeIterator.count(); @@ -51,3 +51,46 @@ 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 subindex = (index.by_ref().take(3), 42); + let _ = subindex.0.last(); // Do not lint, cannot make mutable easily + + 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"); +} diff --git a/tests/ui/double_ended_iterator_last.rs b/tests/ui/double_ended_iterator_last.rs index 9c13b496d117..d9eb2b67d641 100644 --- a/tests/ui/double_ended_iterator_last.rs +++ b/tests/ui/double_ended_iterator_last.rs @@ -2,7 +2,7 @@ // Typical case pub fn last_arg(s: &str) -> Option<&str> { - s.split(' ').last() + s.split(' ').last() //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator` } fn main() { @@ -19,7 +19,7 @@ fn main() { Some(()) } } - let _ = DeIterator.last(); + let _ = DeIterator.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator` // Should not apply to other methods of Iterator let _ = DeIterator.count(); @@ -51,3 +51,46 @@ 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.0.last(); // Do not lint, cannot make mutable easily + + 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"); +} diff --git a/tests/ui/double_ended_iterator_last.stderr b/tests/ui/double_ended_iterator_last.stderr index b795c18a736e..94f5c890ca0f 100644 --- a/tests/ui/double_ended_iterator_last.stderr +++ b/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:22: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:80: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:93:36 + | +LL | println!("Last element is {}", v.last().unwrap().0); + | ^^^^^^^^ + | + = note: this might cause unretrieved elements to be dropped after the retrieved one +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