diff --git a/clippy_lints/src/loops/while_let_on_iterator.rs b/clippy_lints/src/loops/while_let_on_iterator.rs index 2545f81f1afa..c063e9263ba0 100644 --- a/clippy_lints/src/loops/while_let_on_iterator.rs +++ b/clippy_lints/src/loops/while_let_on_iterator.rs @@ -12,6 +12,7 @@ use rustc_hir::intravisit::{Visitor, walk_expr}; use rustc_hir::{Closure, Expr, ExprKind, HirId, LetStmt, Mutability, UnOp}; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter::OnlyBodies; +use rustc_middle::ty; use rustc_middle::ty::adjustment::Adjust; use rustc_span::Symbol; use rustc_span::symbol::sym; @@ -43,26 +44,26 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { }; // If the iterator is a field or the iterator is accessed after the loop is complete it needs to be - // borrowed mutably. TODO: If the struct can be partially moved from and the struct isn't used + // passed by reference. TODO: If the struct can be partially moved from and the struct isn't used // afterwards a mutable borrow of a field isn't necessary. - let by_ref = if cx.typeck_results().expr_ty(iter_expr).ref_mutability() == Some(Mutability::Mut) + let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability); + let iterator_by_ref = if cx.typeck_results().expr_ty(iter_expr).ref_mutability() == Some(Mutability::Mut) || !iter_expr_struct.can_move || !iter_expr_struct.fields.is_empty() || needs_mutable_borrow(cx, &iter_expr_struct, expr) { - ".by_ref()" + make_iterator_snippet(cx, iter_expr, iterator) } else { - "" + iterator.to_string() }; - let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability); span_lint_and_sugg( cx, WHILE_LET_ON_ITERATOR, expr.span.with_hi(let_expr.span.hi()), "this loop could be written as a `for` loop", "try", - format!("{loop_label}for {loop_var} in {iterator}{by_ref}"), + format!("{loop_label}for {loop_var} in {iterator_by_ref}"), applicability, ); } @@ -355,3 +356,19 @@ fn needs_mutable_borrow(cx: &LateContext<'_>, iter_expr: &IterExpr, loop_expr: & .is_break() } } + +/// Constructs the transformed iterator expression for the suggestion. +/// Returns `iterator.by_ref()` unless the iterator type is a reference to an unsized type, +/// in which case it returns `&mut *iterator`. +fn make_iterator_snippet<'tcx>(cx: &LateContext<'tcx>, iter_expr: &Expr<'tcx>, iterator: impl Into) -> String { + let iterator = iterator.into(); + let ty = cx.typeck_results().expr_ty(iter_expr); + + if let ty::Ref(_, inner_ty, _) = ty.kind() + && !inner_ty.is_sized(cx.tcx, cx.typing_env()) + { + return format!("&mut *{iterator}"); + } + + format!("{iterator}.by_ref()") +} diff --git a/tests/ui/while_let_on_iterator.fixed b/tests/ui/while_let_on_iterator.fixed index f9ccefab5898..4e03e954108e 100644 --- a/tests/ui/while_let_on_iterator.fixed +++ b/tests/ui/while_let_on_iterator.fixed @@ -492,6 +492,37 @@ fn issue13123() { } } +fn issue16089() { + trait CertainTrait: Iterator { + fn iter_over_self(&mut self) { + let mut a = 0; + for r in &mut *self { + //~^ while_let_on_iterator + a = r; + } + self.use_after_iter() + } + + fn use_after_iter(&mut self) {} + } +} + +fn issue16089_sized_trait_not_reborrowed() { + trait CertainTrait: Iterator + Sized { + fn iter_over_self(&mut self) { + let mut a = 0; + // Check that the suggestion is just "self", since the trait is sized. + for r in self.by_ref() { + //~^ while_let_on_iterator + a = r; + } + self.use_after_iter() + } + + fn use_after_iter(&mut self) {} + } +} + fn main() { let mut it = 0..20; for _ in it { diff --git a/tests/ui/while_let_on_iterator.rs b/tests/ui/while_let_on_iterator.rs index f957f2e5a523..cc65fda6d18f 100644 --- a/tests/ui/while_let_on_iterator.rs +++ b/tests/ui/while_let_on_iterator.rs @@ -492,6 +492,37 @@ fn issue13123() { } } +fn issue16089() { + trait CertainTrait: Iterator { + fn iter_over_self(&mut self) { + let mut a = 0; + while let Some(r) = self.next() { + //~^ while_let_on_iterator + a = r; + } + self.use_after_iter() + } + + fn use_after_iter(&mut self) {} + } +} + +fn issue16089_sized_trait_not_reborrowed() { + trait CertainTrait: Iterator + Sized { + fn iter_over_self(&mut self) { + let mut a = 0; + // Check that the suggestion is just "self", since the trait is sized. + while let Some(r) = self.next() { + //~^ while_let_on_iterator + a = r; + } + self.use_after_iter() + } + + fn use_after_iter(&mut self) {} + } +} + fn main() { let mut it = 0..20; while let Some(..) = it.next() { diff --git a/tests/ui/while_let_on_iterator.stderr b/tests/ui/while_let_on_iterator.stderr index 50f20227b90f..21ebc22f699d 100644 --- a/tests/ui/while_let_on_iterator.stderr +++ b/tests/ui/while_let_on_iterator.stderr @@ -164,10 +164,22 @@ LL | 'label: while let Some(n) = it.next() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `'label: for n in it` error: this loop could be written as a `for` loop - --> tests/ui/while_let_on_iterator.rs:497:5 + --> tests/ui/while_let_on_iterator.rs:499:13 + | +LL | while let Some(r) = self.next() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for r in &mut *self` + +error: this loop could be written as a `for` loop + --> tests/ui/while_let_on_iterator.rs:515:13 + | +LL | while let Some(r) = self.next() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for r in self.by_ref()` + +error: this loop could be written as a `for` loop + --> tests/ui/while_let_on_iterator.rs:528:5 | LL | while let Some(..) = it.next() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it` -error: aborting due to 28 previous errors +error: aborting due to 30 previous errors