Skip to content

Commit 70a4d95

Browse files
authored
Fixes #16109: For non-sized types, use reborrow in suggestions to remove while-let loops (#16100)
Fixes #16019 by reborrowing, instead of using by_ref, when the iterator expression is not `Sized`. ``` changelog: [`while_let_on_iterator`]: fixes broken suggestion by using reborrow instead of `by_ref` for references to traits that are not `Sized` ```
2 parents 66ebabe + bbadb72 commit 70a4d95

File tree

4 files changed

+265
-8
lines changed

4 files changed

+265
-8
lines changed

clippy_lints/src/loops/while_let_on_iterator.rs

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,26 +43,26 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
4343
};
4444

4545
// If the iterator is a field or the iterator is accessed after the loop is complete it needs to be
46-
// borrowed mutably. TODO: If the struct can be partially moved from and the struct isn't used
46+
// passed by reference. TODO: If the struct can be partially moved from and the struct isn't used
4747
// afterwards a mutable borrow of a field isn't necessary.
48-
let by_ref = if cx.typeck_results().expr_ty(iter_expr).ref_mutability() == Some(Mutability::Mut)
48+
let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability);
49+
let iterator_by_ref = if cx.typeck_results().expr_ty(iter_expr).ref_mutability() == Some(Mutability::Mut)
4950
|| !iter_expr_struct.can_move
5051
|| !iter_expr_struct.fields.is_empty()
5152
|| needs_mutable_borrow(cx, &iter_expr_struct, expr)
5253
{
53-
".by_ref()"
54+
make_iterator_snippet(cx, iter_expr, &iterator)
5455
} else {
55-
""
56+
iterator.into_owned()
5657
};
5758

58-
let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability);
5959
span_lint_and_sugg(
6060
cx,
6161
WHILE_LET_ON_ITERATOR,
6262
expr.span.with_hi(let_expr.span.hi()),
6363
"this loop could be written as a `for` loop",
6464
"try",
65-
format!("{loop_label}for {loop_var} in {iterator}{by_ref}"),
65+
format!("{loop_label}for {loop_var} in {iterator_by_ref}"),
6666
applicability,
6767
);
6868
}
@@ -355,3 +355,22 @@ fn needs_mutable_borrow(cx: &LateContext<'_>, iter_expr: &IterExpr, loop_expr: &
355355
.is_break()
356356
}
357357
}
358+
359+
/// Constructs the transformed iterator expression for the suggestion.
360+
/// Returns `iterator.by_ref()` unless the last deref adjustment targets an unsized type,
361+
/// in which case it applies all derefs (e.g., `&mut **iterator` or `&mut ***iterator`).
362+
fn make_iterator_snippet<'tcx>(cx: &LateContext<'tcx>, iter_expr: &Expr<'tcx>, iterator: &str) -> String {
363+
if let Some((n, adjust)) = cx
364+
.typeck_results()
365+
.expr_adjustments(iter_expr)
366+
.iter()
367+
.take_while(|x| matches!(x.kind, Adjust::Deref(_)))
368+
.enumerate()
369+
.last()
370+
&& !adjust.target.is_sized(cx.tcx, cx.typing_env())
371+
{
372+
format!("&mut {:*<n$}{iterator}", '*', n = n + 1)
373+
} else {
374+
format!("{iterator}.by_ref()")
375+
}
376+
}

tests/ui/while_let_on_iterator.fixed

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,110 @@ fn issue13123() {
492492
}
493493
}
494494

495+
fn issue16089() {
496+
trait CertainTrait: Iterator<Item = u8> {
497+
fn iter_over_self(&mut self) {
498+
let mut a = 0;
499+
for r in &mut *self {
500+
//~^ while_let_on_iterator
501+
a = r;
502+
}
503+
self.use_after_iter()
504+
}
505+
506+
fn use_after_iter(&mut self) {}
507+
}
508+
}
509+
510+
fn issue16089_sized_trait_not_reborrowed() {
511+
trait CertainTrait: Iterator<Item = u8> + Sized {
512+
fn iter_over_self(&mut self) {
513+
let mut a = 0;
514+
// Check that the suggestion is just "self", since the trait is sized.
515+
for r in self.by_ref() {
516+
//~^ while_let_on_iterator
517+
a = r;
518+
}
519+
self.use_after_iter()
520+
}
521+
522+
fn use_after_iter(&mut self) {}
523+
}
524+
}
525+
526+
fn issue16089_nested_derefs() {
527+
struct S<T>(T);
528+
impl<T> core::ops::Deref for S<T> {
529+
type Target = T;
530+
fn deref(&self) -> &Self::Target {
531+
&self.0
532+
}
533+
}
534+
impl<T> core::ops::DerefMut for S<T> {
535+
fn deref_mut(&mut self) -> &mut Self::Target {
536+
&mut self.0
537+
}
538+
}
539+
540+
fn f(mut x: S<S<&mut dyn Iterator<Item = u32>>>) {
541+
for _ in &mut ***x {}
542+
//~^ while_let_on_iterator
543+
}
544+
}
545+
546+
fn issue16089_nested_derefs_last_not_sized() {
547+
struct WithSize<T>(T);
548+
impl<T> core::ops::Deref for WithSize<T> {
549+
type Target = T;
550+
fn deref(&self) -> &Self::Target {
551+
&self.0
552+
}
553+
}
554+
impl<T> core::ops::DerefMut for WithSize<T> {
555+
fn deref_mut(&mut self) -> &mut Self::Target {
556+
&mut self.0
557+
}
558+
}
559+
// The suggestion must use `&mut **x`. Using `x.by_ref()` doesn't work in this
560+
// case, since the last type adjustment for `x` in the expression `x.next()` is
561+
// to dereference a `?Sized` trait.
562+
fn f(mut x: WithSize<&mut dyn Iterator<Item = u32>>) {
563+
for _ in &mut **x {}
564+
//~^ while_let_on_iterator
565+
}
566+
}
567+
568+
fn issue16089_nested_derefs_last_sized() {
569+
struct NoSize<T: ?Sized>(T);
570+
impl<T: ?Sized> core::ops::Deref for NoSize<T> {
571+
type Target = T;
572+
fn deref(&self) -> &Self::Target {
573+
&self.0
574+
}
575+
}
576+
impl<T: ?Sized> core::ops::DerefMut for NoSize<T> {
577+
fn deref_mut(&mut self) -> &mut Self::Target {
578+
&mut self.0
579+
}
580+
}
581+
582+
struct SizedIter {}
583+
584+
impl Iterator for SizedIter {
585+
type Item = u32;
586+
fn next(&mut self) -> Option<u32> {
587+
Some(0)
588+
}
589+
}
590+
591+
// We want the suggestion to be `x.by_ref()`. It works in this case since the last type
592+
// adjustment for `x` in the expression `x.next()` is to dereference a Sized type.
593+
fn f(mut x: NoSize<NoSize<SizedIter>>) {
594+
for _ in x.by_ref() {}
595+
//~^ while_let_on_iterator
596+
}
597+
}
598+
495599
fn main() {
496600
let mut it = 0..20;
497601
for _ in it {

tests/ui/while_let_on_iterator.rs

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,110 @@ fn issue13123() {
492492
}
493493
}
494494

495+
fn issue16089() {
496+
trait CertainTrait: Iterator<Item = u8> {
497+
fn iter_over_self(&mut self) {
498+
let mut a = 0;
499+
while let Some(r) = self.next() {
500+
//~^ while_let_on_iterator
501+
a = r;
502+
}
503+
self.use_after_iter()
504+
}
505+
506+
fn use_after_iter(&mut self) {}
507+
}
508+
}
509+
510+
fn issue16089_sized_trait_not_reborrowed() {
511+
trait CertainTrait: Iterator<Item = u8> + Sized {
512+
fn iter_over_self(&mut self) {
513+
let mut a = 0;
514+
// Check that the suggestion is just "self", since the trait is sized.
515+
while let Some(r) = self.next() {
516+
//~^ while_let_on_iterator
517+
a = r;
518+
}
519+
self.use_after_iter()
520+
}
521+
522+
fn use_after_iter(&mut self) {}
523+
}
524+
}
525+
526+
fn issue16089_nested_derefs() {
527+
struct S<T>(T);
528+
impl<T> core::ops::Deref for S<T> {
529+
type Target = T;
530+
fn deref(&self) -> &Self::Target {
531+
&self.0
532+
}
533+
}
534+
impl<T> core::ops::DerefMut for S<T> {
535+
fn deref_mut(&mut self) -> &mut Self::Target {
536+
&mut self.0
537+
}
538+
}
539+
540+
fn f(mut x: S<S<&mut dyn Iterator<Item = u32>>>) {
541+
while let Some(_) = x.next() {}
542+
//~^ while_let_on_iterator
543+
}
544+
}
545+
546+
fn issue16089_nested_derefs_last_not_sized() {
547+
struct WithSize<T>(T);
548+
impl<T> core::ops::Deref for WithSize<T> {
549+
type Target = T;
550+
fn deref(&self) -> &Self::Target {
551+
&self.0
552+
}
553+
}
554+
impl<T> core::ops::DerefMut for WithSize<T> {
555+
fn deref_mut(&mut self) -> &mut Self::Target {
556+
&mut self.0
557+
}
558+
}
559+
// The suggestion must use `&mut **x`. Using `x.by_ref()` doesn't work in this
560+
// case, since the last type adjustment for `x` in the expression `x.next()` is
561+
// to dereference a `?Sized` trait.
562+
fn f(mut x: WithSize<&mut dyn Iterator<Item = u32>>) {
563+
while let Some(_) = x.next() {}
564+
//~^ while_let_on_iterator
565+
}
566+
}
567+
568+
fn issue16089_nested_derefs_last_sized() {
569+
struct NoSize<T: ?Sized>(T);
570+
impl<T: ?Sized> core::ops::Deref for NoSize<T> {
571+
type Target = T;
572+
fn deref(&self) -> &Self::Target {
573+
&self.0
574+
}
575+
}
576+
impl<T: ?Sized> core::ops::DerefMut for NoSize<T> {
577+
fn deref_mut(&mut self) -> &mut Self::Target {
578+
&mut self.0
579+
}
580+
}
581+
582+
struct SizedIter {}
583+
584+
impl Iterator for SizedIter {
585+
type Item = u32;
586+
fn next(&mut self) -> Option<u32> {
587+
Some(0)
588+
}
589+
}
590+
591+
// We want the suggestion to be `x.by_ref()`. It works in this case since the last type
592+
// adjustment for `x` in the expression `x.next()` is to dereference a Sized type.
593+
fn f(mut x: NoSize<NoSize<SizedIter>>) {
594+
while let Some(_) = x.next() {}
595+
//~^ while_let_on_iterator
596+
}
597+
}
598+
495599
fn main() {
496600
let mut it = 0..20;
497601
while let Some(..) = it.next() {

tests/ui/while_let_on_iterator.stderr

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,10 +164,40 @@ LL | 'label: while let Some(n) = it.next() {
164164
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `'label: for n in it`
165165

166166
error: this loop could be written as a `for` loop
167-
--> tests/ui/while_let_on_iterator.rs:497:5
167+
--> tests/ui/while_let_on_iterator.rs:499:13
168+
|
169+
LL | while let Some(r) = self.next() {
170+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for r in &mut *self`
171+
172+
error: this loop could be written as a `for` loop
173+
--> tests/ui/while_let_on_iterator.rs:515:13
174+
|
175+
LL | while let Some(r) = self.next() {
176+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for r in self.by_ref()`
177+
178+
error: this loop could be written as a `for` loop
179+
--> tests/ui/while_let_on_iterator.rs:541:9
180+
|
181+
LL | while let Some(_) = x.next() {}
182+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in &mut ***x`
183+
184+
error: this loop could be written as a `for` loop
185+
--> tests/ui/while_let_on_iterator.rs:563:9
186+
|
187+
LL | while let Some(_) = x.next() {}
188+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in &mut **x`
189+
190+
error: this loop could be written as a `for` loop
191+
--> tests/ui/while_let_on_iterator.rs:594:9
192+
|
193+
LL | while let Some(_) = x.next() {}
194+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in x.by_ref()`
195+
196+
error: this loop could be written as a `for` loop
197+
--> tests/ui/while_let_on_iterator.rs:601:5
168198
|
169199
LL | while let Some(..) = it.next() {
170200
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it`
171201

172-
error: aborting due to 28 previous errors
202+
error: aborting due to 33 previous errors
173203

0 commit comments

Comments
 (0)