From 86a10f01d10c28c26b280b913d57d3e8199ffa86 Mon Sep 17 00:00:00 2001 From: yanglsh Date: Fri, 28 Mar 2025 13:25:46 +0800 Subject: [PATCH 1/3] fix: `double_ended_iterator_last` FP when iter has side effects --- .../src/methods/double_ended_iterator_last.rs | 4 ++- clippy_utils/src/ty/mod.rs | 31 +++++++++++++++++++ tests/ui/double_ended_iterator_last.fixed | 11 +++++++ tests/ui/double_ended_iterator_last.rs | 11 +++++++ 4 files changed, 56 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index b5adc69e9a79..88e357ec45d1 100644 --- a/clippy_lints/src/methods/double_ended_iterator_last.rs +++ b/clippy_lints/src/methods/double_ended_iterator_last.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::ty::implements_trait; +use clippy_utils::ty::{implements_trait, is_iter_with_side_effects}; use clippy_utils::{is_mutable, is_trait_method, path_to_local}; use rustc_errors::Applicability; use rustc_hir::{Expr, Node, PatKind}; @@ -27,6 +27,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &'_ Expr<'_>, self_expr: &'_ Exp && let Some(last_def) = cx.tcx.provided_trait_methods(item).find(|m| m.name().as_str() == "last") // if the resolved method is the same as the provided definition && fn_def.def_id() == last_def.def_id + && let self_ty = cx.typeck_results().expr_ty(self_expr) + && !is_iter_with_side_effects(cx, self_ty) { let mut sugg = vec![(call_span, String::from("next_back()"))]; let mut dont_apply = false; diff --git a/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index 14ccd35afdd6..5b48ffa44267 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -1376,3 +1376,34 @@ pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option None, } } + +/// Check if `ty` is an `Iterator` and has side effects when iterated over. Currently, this only +/// checks if the `ty` contains mutable captures, and thus may be imcomplete. +pub fn is_iter_with_side_effects<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool { + let Some(iter_trait) = cx.tcx.lang_items().iterator_trait() else { + return false; + }; + + is_iter_with_side_effects_impl(cx, iter_ty, iter_trait) +} + +fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>, iter_trait: DefId) -> bool { + if implements_trait(cx, iter_ty, iter_trait, &[]) + && let ty::Adt(_, args) = iter_ty.kind() + { + return args.types().any(|arg_ty| { + if let ty::Closure(_, closure_args) = arg_ty.kind() + && let Some(captures) = closure_args.types().next_back() + { + captures + .tuple_fields() + .iter() + .any(|capture_ty| matches!(capture_ty.ref_mutability(), Some(Mutability::Mut))) + } else { + is_iter_with_side_effects_impl(cx, arg_ty, iter_trait) + } + }); + } + + false +} diff --git a/tests/ui/double_ended_iterator_last.fixed b/tests/ui/double_ended_iterator_last.fixed index 17d0d71a8854..07a3ab4cf88f 100644 --- a/tests/ui/double_ended_iterator_last.fixed +++ b/tests/ui/double_ended_iterator_last.fixed @@ -90,3 +90,14 @@ fn drop_order() { //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator` println!("Done"); } + +fn issue_14444() { + let mut squares = vec![]; + let last_square = [1, 2, 3] + .into_iter() + .map(|x| { + squares.push(x * x); + Some(x * x) + }) + .last(); +} diff --git a/tests/ui/double_ended_iterator_last.rs b/tests/ui/double_ended_iterator_last.rs index 41bc669b1719..385462e5c2b8 100644 --- a/tests/ui/double_ended_iterator_last.rs +++ b/tests/ui/double_ended_iterator_last.rs @@ -90,3 +90,14 @@ fn drop_order() { //~^ ERROR: called `Iterator::last` on a `DoubleEndedIterator` println!("Done"); } + +fn issue_14444() { + let mut squares = vec![]; + let last_square = [1, 2, 3] + .into_iter() + .map(|x| { + squares.push(x * x); + Some(x * x) + }) + .last(); +} From c6d76bb69d5ed9f12284007338d3b7d04bcf4385 Mon Sep 17 00:00:00 2001 From: yanglsh Date: Fri, 28 Mar 2025 14:08:20 +0800 Subject: [PATCH 2/3] fix: `needless_collect` does not consider side effects --- clippy_lints/src/methods/needless_collect.rs | 10 ++++++- tests/ui/needless_collect.fixed | 29 ++++++++++++++++++++ tests/ui/needless_collect.rs | 29 ++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs index 84d47f29fe8e..eec96e383e7f 100644 --- a/clippy_lints/src/methods/needless_collect.rs +++ b/clippy_lints/src/methods/needless_collect.rs @@ -4,7 +4,9 @@ use super::NEEDLESS_COLLECT; use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::sugg::Sugg; -use clippy_utils::ty::{get_type_diagnostic_name, make_normalized_projection, make_projection}; +use clippy_utils::ty::{ + get_type_diagnostic_name, is_iter_with_side_effects, make_normalized_projection, make_projection, +}; use clippy_utils::{ CaptureKind, can_move_expr_to_closure, fn_def_id, get_enclosing_block, higher, is_trait_method, path_to_local, path_to_local_id, @@ -23,6 +25,7 @@ use rustc_span::{Span, sym}; const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed"; +#[expect(clippy::too_many_lines)] pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, name_span: Span, @@ -30,6 +33,11 @@ pub(super) fn check<'tcx>( iter_expr: &'tcx Expr<'tcx>, call_span: Span, ) { + let iter_ty = cx.typeck_results().expr_ty(iter_expr); + if is_iter_with_side_effects(cx, iter_ty) { + return; // don't lint if the iterator has side effects + } + match cx.tcx.parent_hir_node(collect_expr.hir_id) { Node::Expr(parent) => { check_collect_into_intoiterator(cx, parent, collect_expr, call_span, iter_expr); diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index 6551fa56b42c..e41ac72b2328 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -126,3 +126,32 @@ fn main() { fn foo(_: impl IntoIterator) {} fn bar>(_: Vec, _: I) {} fn baz>(_: I, _: (), _: impl IntoIterator) {} + +mod issue9191 { + use std::collections::HashSet; + + fn foo(xs: Vec, mut ys: HashSet) { + if xs.iter().map(|x| ys.remove(x)).collect::>().contains(&true) { + todo!() + } + } +} + +pub fn issue8055(v: impl IntoIterator) -> Result, usize> { + let mut zeros = 0; + + let res: Vec<_> = v + .into_iter() + .filter(|i| { + if *i == 0 { + zeros += 1 + }; + *i != 0 + }) + .collect(); + + if zeros != 0 { + return Err(zeros); + } + Ok(res.into_iter()) +} diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 973c41c68754..821a6d8017e6 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -126,3 +126,32 @@ fn main() { fn foo(_: impl IntoIterator) {} fn bar>(_: Vec, _: I) {} fn baz>(_: I, _: (), _: impl IntoIterator) {} + +mod issue9191 { + use std::collections::HashSet; + + fn foo(xs: Vec, mut ys: HashSet) { + if xs.iter().map(|x| ys.remove(x)).collect::>().contains(&true) { + todo!() + } + } +} + +pub fn issue8055(v: impl IntoIterator) -> Result, usize> { + let mut zeros = 0; + + let res: Vec<_> = v + .into_iter() + .filter(|i| { + if *i == 0 { + zeros += 1 + }; + *i != 0 + }) + .collect(); + + if zeros != 0 { + return Err(zeros); + } + Ok(res.into_iter()) +} From a50e043d3226822768922e67c64c5376edbf7dfe Mon Sep 17 00:00:00 2001 From: yanglsh Date: Tue, 1 Apr 2025 20:14:59 +0800 Subject: [PATCH 3/3] Expand mutable capture check for `is_iter_with_side_effects()` --- .../src/methods/double_ended_iterator_last.rs | 7 +- clippy_lints/src/methods/needless_collect.rs | 4 +- clippy_utils/src/ty/mod.rs | 67 ++++++++++++------- tests/ui/crashes/ice-11230.fixed | 4 +- tests/ui/crashes/ice-11230.rs | 2 +- tests/ui/crashes/ice-11230.stderr | 11 +-- tests/ui/double_ended_iterator_last.fixed | 21 ++++-- tests/ui/double_ended_iterator_last.rs | 17 +++-- tests/ui/double_ended_iterator_last.stderr | 52 +------------- .../double_ended_iterator_last_unfixable.rs | 5 +- ...ouble_ended_iterator_last_unfixable.stderr | 24 ++----- tests/ui/needless_collect.fixed | 57 +++++++++++++++- tests/ui/needless_collect.rs | 57 +++++++++++++++- 13 files changed, 201 insertions(+), 127 deletions(-) diff --git a/clippy_lints/src/methods/double_ended_iterator_last.rs b/clippy_lints/src/methods/double_ended_iterator_last.rs index 88e357ec45d1..e666f31217cc 100644 --- a/clippy_lints/src/methods/double_ended_iterator_last.rs +++ b/clippy_lints/src/methods/double_ended_iterator_last.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::ty::{implements_trait, is_iter_with_side_effects}; +use clippy_utils::ty::{has_non_owning_mutable_access, implements_trait}; use clippy_utils::{is_mutable, is_trait_method, path_to_local}; use rustc_errors::Applicability; use rustc_hir::{Expr, Node, PatKind}; @@ -28,11 +28,14 @@ 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 && let self_ty = cx.typeck_results().expr_ty(self_expr) - && !is_iter_with_side_effects(cx, self_ty) + && !has_non_owning_mutable_access(cx, self_ty) { 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()` + // 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) && let Node::Pat(pat) = cx.tcx.hir_node(hir_id) diff --git a/clippy_lints/src/methods/needless_collect.rs b/clippy_lints/src/methods/needless_collect.rs index eec96e383e7f..6efaba525e3e 100644 --- a/clippy_lints/src/methods/needless_collect.rs +++ b/clippy_lints/src/methods/needless_collect.rs @@ -5,7 +5,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{ - get_type_diagnostic_name, is_iter_with_side_effects, make_normalized_projection, make_projection, + get_type_diagnostic_name, has_non_owning_mutable_access, make_normalized_projection, make_projection, }; use clippy_utils::{ CaptureKind, can_move_expr_to_closure, fn_def_id, get_enclosing_block, higher, is_trait_method, path_to_local, @@ -34,7 +34,7 @@ pub(super) fn check<'tcx>( call_span: Span, ) { let iter_ty = cx.typeck_results().expr_ty(iter_expr); - if is_iter_with_side_effects(cx, iter_ty) { + if has_non_owning_mutable_access(cx, iter_ty) { return; // don't lint if the iterator has side effects } diff --git a/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index 5b48ffa44267..8db9cd593b33 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -1377,33 +1377,48 @@ pub fn option_arg_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool { - let Some(iter_trait) = cx.tcx.lang_items().iterator_trait() else { - return false; - }; - - is_iter_with_side_effects_impl(cx, iter_ty, iter_trait) -} +/// Check if a Ty<'_> of `Iterator` contains any mutable access to non-owning types by checking if +/// it contains fields of mutable references or pointers, or references/pointers to non-`Freeze` +/// types, or `PhantomData` types containing any of the previous. This can be used to check whether +/// skipping iterating over an iterator will change its behavior. +pub fn has_non_owning_mutable_access<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>) -> bool { + fn normalize_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> { + cx.tcx.try_normalize_erasing_regions(cx.typing_env(), ty).unwrap_or(ty) + } -fn is_iter_with_side_effects_impl<'tcx>(cx: &LateContext<'tcx>, iter_ty: Ty<'tcx>, iter_trait: DefId) -> bool { - if implements_trait(cx, iter_ty, iter_trait, &[]) - && let ty::Adt(_, args) = iter_ty.kind() - { - return args.types().any(|arg_ty| { - if let ty::Closure(_, closure_args) = arg_ty.kind() - && let Some(captures) = closure_args.types().next_back() - { - captures - .tuple_fields() - .iter() - .any(|capture_ty| matches!(capture_ty.ref_mutability(), Some(Mutability::Mut))) - } else { - is_iter_with_side_effects_impl(cx, arg_ty, iter_trait) - } - }); + /// Check if `ty` contains mutable references or equivalent, which includes: + /// - A mutable reference/pointer. + /// - A reference/pointer to a non-`Freeze` type. + /// - A `PhantomData` type containing any of the previous. + fn has_non_owning_mutable_access_inner<'tcx>( + cx: &LateContext<'tcx>, + phantoms: &mut FxHashSet>, + ty: Ty<'tcx>, + ) -> bool { + match ty.kind() { + ty::Adt(adt_def, args) if adt_def.is_phantom_data() => { + phantoms.insert(ty) + && args + .types() + .any(|arg_ty| has_non_owning_mutable_access_inner(cx, phantoms, arg_ty)) + }, + ty::Adt(adt_def, args) => adt_def.all_fields().any(|field| { + has_non_owning_mutable_access_inner(cx, phantoms, normalize_ty(cx, field.ty(cx.tcx, args))) + }), + ty::Array(elem_ty, _) | ty::Slice(elem_ty) => has_non_owning_mutable_access_inner(cx, phantoms, *elem_ty), + ty::RawPtr(pointee_ty, mutability) | ty::Ref(_, pointee_ty, mutability) => { + mutability.is_mut() || !pointee_ty.is_freeze(cx.tcx, cx.typing_env()) + }, + ty::Closure(_, closure_args) => { + matches!(closure_args.types().next_back(), Some(captures) if has_non_owning_mutable_access_inner(cx, phantoms, captures)) + }, + ty::Tuple(tuple_args) => tuple_args + .iter() + .any(|arg_ty| has_non_owning_mutable_access_inner(cx, phantoms, arg_ty)), + _ => false, + } } - false + let mut phantoms = FxHashSet::default(); + has_non_owning_mutable_access_inner(cx, &mut phantoms, iter_ty) } diff --git a/tests/ui/crashes/ice-11230.fixed b/tests/ui/crashes/ice-11230.fixed index 181e1ebbe5a3..c49a419f0d4b 100644 --- a/tests/ui/crashes/ice-11230.fixed +++ b/tests/ui/crashes/ice-11230.fixed @@ -12,7 +12,7 @@ fn main() { // needless_collect trait Helper<'a>: Iterator {} +// Should not be linted because we have no idea whether the iterator has side effects fn x(w: &mut dyn for<'a> Helper<'a>) { - w.next().is_none(); - //~^ needless_collect + w.collect::>().is_empty(); } diff --git a/tests/ui/crashes/ice-11230.rs b/tests/ui/crashes/ice-11230.rs index fb05dc781bc0..f66b7e961c88 100644 --- a/tests/ui/crashes/ice-11230.rs +++ b/tests/ui/crashes/ice-11230.rs @@ -12,7 +12,7 @@ fn main() { // needless_collect trait Helper<'a>: Iterator {} +// Should not be linted because we have no idea whether the iterator has side effects fn x(w: &mut dyn for<'a> Helper<'a>) { w.collect::>().is_empty(); - //~^ needless_collect } diff --git a/tests/ui/crashes/ice-11230.stderr b/tests/ui/crashes/ice-11230.stderr index b4a3f67081ae..91d59121ac4e 100644 --- a/tests/ui/crashes/ice-11230.stderr +++ b/tests/ui/crashes/ice-11230.stderr @@ -7,14 +7,5 @@ LL | for v in A.iter() {} = note: `-D clippy::explicit-iter-loop` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::explicit_iter_loop)]` -error: avoid using `collect()` when not needed - --> tests/ui/crashes/ice-11230.rs:16:7 - | -LL | w.collect::>().is_empty(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `next().is_none()` - | - = note: `-D clippy::needless-collect` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::needless_collect)]` - -error: aborting due to 2 previous errors +error: aborting due to 1 previous error diff --git a/tests/ui/double_ended_iterator_last.fixed b/tests/ui/double_ended_iterator_last.fixed index 07a3ab4cf88f..2ce0c04c3017 100644 --- a/tests/ui/double_ended_iterator_last.fixed +++ b/tests/ui/double_ended_iterator_last.fixed @@ -52,28 +52,35 @@ fn main() { let _ = CustomLast.last(); } +// Should not be linted because applying the lint would move the original iterator. This can only be +// linted if the iterator is used thereafter. 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 subindex = index.by_ref().take(3); + let _ = subindex.last(); + let _ = index.next(); 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 _ = subindex.last(); + let _ = index.next(); 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 _ = subindex.last(); + let _ = index.next(); 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 _ = subindex.last(); + let _ = index.next(); 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` + let (subindex, _) = (index.by_ref().take(3), 42); + let _ = subindex.last(); + let _ = index.next(); } fn drop_order() { diff --git a/tests/ui/double_ended_iterator_last.rs b/tests/ui/double_ended_iterator_last.rs index 385462e5c2b8..a4eb9b3337b9 100644 --- a/tests/ui/double_ended_iterator_last.rs +++ b/tests/ui/double_ended_iterator_last.rs @@ -52,28 +52,35 @@ fn main() { let _ = CustomLast.last(); } +// Should not be linted because applying the lint would move the original iterator. This can only be +// linted if the iterator is used thereafter. 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 _ = subindex.last(); + let _ = index.next(); 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 _ = subindex.last(); + let _ = index.next(); 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 _ = subindex.last(); + let _ = index.next(); 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 _ = 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.last(); //~ ERROR: called `Iterator::last` on a `DoubleEndedIterator` + let _ = subindex.last(); + let _ = index.next(); } fn drop_order() { diff --git a/tests/ui/double_ended_iterator_last.stderr b/tests/ui/double_ended_iterator_last.stderr index 1702a24d7a05..fe8cf2dcb259 100644 --- a/tests/ui/double_ended_iterator_last.stderr +++ b/tests/ui/double_ended_iterator_last.stderr @@ -18,55 +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: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 + --> tests/ui/double_ended_iterator_last.rs:96:36 | LL | println!("Last element is {}", v.last().unwrap().0); | ^^^^^^^^ @@ -78,5 +30,5 @@ LL ~ let mut v = v.into_iter(); LL ~ println!("Last element is {}", v.next_back().unwrap().0); | -error: aborting due to 8 previous errors +error: aborting due to 3 previous errors diff --git a/tests/ui/double_ended_iterator_last_unfixable.rs b/tests/ui/double_ended_iterator_last_unfixable.rs index 3f125c7f20c1..7c5de8832d69 100644 --- a/tests/ui/double_ended_iterator_last_unfixable.rs +++ b/tests/ui/double_ended_iterator_last_unfixable.rs @@ -1,10 +1,13 @@ //@no-rustfix #![warn(clippy::double_ended_iterator_last)] +// Should not be linted because applying the lint would move the original iterator. This can only be +// linted if the iterator is used thereafter. 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` + let _ = subindex.0.last(); + let _ = index.next(); } fn drop_order() { diff --git a/tests/ui/double_ended_iterator_last_unfixable.stderr b/tests/ui/double_ended_iterator_last_unfixable.stderr index f4be757d00d2..845afc11f042 100644 --- a/tests/ui/double_ended_iterator_last_unfixable.stderr +++ b/tests/ui/double_ended_iterator_last_unfixable.stderr @@ -1,21 +1,5 @@ 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 + --> tests/ui/double_ended_iterator_last_unfixable.rs:23:36 | LL | println!("Last element is {}", v.0.last().unwrap().0); | ^^^^------ @@ -24,10 +8,12 @@ LL | println!("Last element is {}", v.0.last().unwrap().0); | = 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 + --> tests/ui/double_ended_iterator_last_unfixable.rs:23:36 | LL | println!("Last element is {}", v.0.last().unwrap().0); | ^^^ + = 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 2 previous errors +error: aborting due to 1 previous error diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index e41ac72b2328..b09efe9888f5 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -128,13 +128,45 @@ fn bar>(_: Vec, _: I) {} fn baz>(_: I, _: (), _: impl IntoIterator) {} mod issue9191 { + use std::cell::Cell; use std::collections::HashSet; + use std::hash::Hash; + use std::marker::PhantomData; + use std::ops::Deref; - fn foo(xs: Vec, mut ys: HashSet) { + fn captures_ref_mut(xs: Vec, mut ys: HashSet) { if xs.iter().map(|x| ys.remove(x)).collect::>().contains(&true) { todo!() } } + + #[derive(Debug, Clone)] + struct MyRef<'a>(PhantomData<&'a mut Cell>>, *mut Cell>); + + impl MyRef<'_> { + fn new(target: &mut Cell>) -> Self { + MyRef(PhantomData, target) + } + + fn get(&mut self) -> &mut Cell> { + unsafe { &mut *self.1 } + } + } + + fn captures_phantom(xs: Vec, mut ys: Cell>) { + let mut ys_ref = MyRef::new(&mut ys); + if xs + .iter() + .map({ + let mut ys_ref = ys_ref.clone(); + move |x| ys_ref.get().get_mut().remove(x) + }) + .collect::>() + .contains(&true) + { + todo!() + } + } } pub fn issue8055(v: impl IntoIterator) -> Result, usize> { @@ -155,3 +187,26 @@ pub fn issue8055(v: impl IntoIterator) -> Result { + inner: T, + marker: core::marker::PhantomData, + } + + impl Iterator for Foo { + type Item = T::Item; + fn next(&mut self) -> Option { + self.inner.next() + } + } + + fn foo() { + Foo { + inner: [].iter(), + marker: core::marker::PhantomData, + } + .collect::>() + .len(); + } +} diff --git a/tests/ui/needless_collect.rs b/tests/ui/needless_collect.rs index 821a6d8017e6..da4182966bb1 100644 --- a/tests/ui/needless_collect.rs +++ b/tests/ui/needless_collect.rs @@ -128,13 +128,45 @@ fn bar>(_: Vec, _: I) {} fn baz>(_: I, _: (), _: impl IntoIterator) {} mod issue9191 { + use std::cell::Cell; use std::collections::HashSet; + use std::hash::Hash; + use std::marker::PhantomData; + use std::ops::Deref; - fn foo(xs: Vec, mut ys: HashSet) { + fn captures_ref_mut(xs: Vec, mut ys: HashSet) { if xs.iter().map(|x| ys.remove(x)).collect::>().contains(&true) { todo!() } } + + #[derive(Debug, Clone)] + struct MyRef<'a>(PhantomData<&'a mut Cell>>, *mut Cell>); + + impl MyRef<'_> { + fn new(target: &mut Cell>) -> Self { + MyRef(PhantomData, target) + } + + fn get(&mut self) -> &mut Cell> { + unsafe { &mut *self.1 } + } + } + + fn captures_phantom(xs: Vec, mut ys: Cell>) { + let mut ys_ref = MyRef::new(&mut ys); + if xs + .iter() + .map({ + let mut ys_ref = ys_ref.clone(); + move |x| ys_ref.get().get_mut().remove(x) + }) + .collect::>() + .contains(&true) + { + todo!() + } + } } pub fn issue8055(v: impl IntoIterator) -> Result, usize> { @@ -155,3 +187,26 @@ pub fn issue8055(v: impl IntoIterator) -> Result { + inner: T, + marker: core::marker::PhantomData, + } + + impl Iterator for Foo { + type Item = T::Item; + fn next(&mut self) -> Option { + self.inner.next() + } + } + + fn foo() { + Foo { + inner: [].iter(), + marker: core::marker::PhantomData, + } + .collect::>() + .len(); + } +}