Auto merge of rust-lang#6966 - Jarcho:while_let_on_iterator_fp, r=xFr…

…ednet

`while_let_on_iterator` Improvements

fixes: rust-lang#6491
fixes: rust-lang#6231
fixes: rust-lang#5844
fixes: rust-lang#1924
fixes: rust-lang#1033

The check for whether a field can be borrowed should probably be moved to utils at some point, but it would require some cleanup work and knowing what parts can actually be shared.

changelog: Suggest `&mut iter` when the iterator is used after the loop.
changelog: Suggest `&mut iter` when the iterator is a field in a struct.
changelog: Don't lint when the iterator is a field in a struct, and the struct is used in the loop.
changelog: Lint when the loop is nested in another loop, but suggest `&mut iter` unless the iterator is from a local declared inside the loop.

clippy_lints/src/doc.rs

@@ -383,7 +383,7 @@ pub fn strip_doc_comment_decoration(doc: &str, comment_kind: CommentKind, span:
     let mut no_stars = String::with_capacity(doc.len());
     for line in doc.lines() {
         let mut chars = line.chars();
-        while let Some(c) = chars.next() {
+        for c in &mut chars {
             if c.is_whitespace() {
                 no_stars.push(c);
             } else {

clippy_utils/src/lib.rs

@@ -855,6 +855,24 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
     })
 }
 
+/// Gets the loop enclosing the given expression, if any.
+pub fn get_enclosing_loop(tcx: TyCtxt<'tcx>, expr: &Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
+    let map = tcx.hir();
+    for (_, node) in map.parent_iter(expr.hir_id) {
+        match node {
+            Node::Expr(
+                e @ Expr {
+                    kind: ExprKind::Loop(..),
+                    ..
+                },
+            ) => return Some(e),
+            Node::Expr(_) | Node::Stmt(_) | Node::Block(_) | Node::Local(_) | Node::Arm(_) => (),
+            _ => break,
+        }
+    }
+    None
+}
+
 /// Gets the parent node if it's an impl block.
 pub fn get_parent_as_impl(tcx: TyCtxt<'_>, id: HirId) -> Option<&Impl<'_>> {
     let map = tcx.hir();

clippy_utils/src/sugg.rs

@@ -289,7 +289,7 @@ fn has_enclosing_paren(sugg: impl AsRef<str>) -> bool {
     let mut chars = sugg.as_ref().chars();
     if let Some('(') = chars.next() {
         let mut depth = 1;
-        while let Some(c) = chars.next() {
+        for c in &mut chars {
             if c == '(' {
                 depth += 1;
             } else if c == ')' {

clippy_utils/src/visitors.rs

@@ -1,7 +1,7 @@
 use crate::path_to_local_id;
 use rustc_hir as hir;
 use rustc_hir::intravisit::{self, walk_expr, ErasedMap, NestedVisitorMap, Visitor};
-use rustc_hir::{Arm, Block, Body, Destination, Expr, ExprKind, HirId, Stmt};
+use rustc_hir::{def::Res, Arm, Block, Body, BodyId, Destination, Expr, ExprKind, HirId, Stmt};
 use rustc_lint::LateContext;
 use rustc_middle::hir::map::Map;
 
@@ -218,6 +218,7 @@ impl<'tcx> Visitable<'tcx> for &'tcx Arm<'tcx> {
     }
 }
 
+/// Calls the given function for each break expression.
 pub fn visit_break_exprs<'tcx>(
     node: impl Visitable<'tcx>,
     f: impl FnMut(&'tcx Expr<'tcx>, Destination, Option<&'tcx Expr<'tcx>>),
@@ -239,3 +240,36 @@ pub fn visit_break_exprs<'tcx>(
 
     node.visit(&mut V(f));
 }
+
+/// Checks if the given resolved path is used in the given body.
+pub fn is_res_used(cx: &LateContext<'_>, res: Res, body: BodyId) -> bool {
+    struct V<'a, 'tcx> {
+        cx: &'a LateContext<'tcx>,
+        res: Res,
+        found: bool,
+    }
+    impl Visitor<'tcx> for V<'_, 'tcx> {
+        type Map = Map<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if self.found {
+                return;
+            }
+
+            if let ExprKind::Path(p) = &e.kind {
+                if self.cx.qpath_res(p, e.hir_id) == self.res {
+                    self.found = true;
+                }
+            } else {
+                walk_expr(self, e)
+            }
+        }
+    }
+
+    let mut v = V { cx, res, found: false };
+    v.visit_expr(&cx.tcx.hir().body(body).value);
+    v.found
+}

tests/ui/while_let_on_iterator.fixed

@@ -1,7 +1,7 @@
 // run-rustfix
 
 #![warn(clippy::while_let_on_iterator)]
-#![allow(clippy::never_loop, unreachable_code, unused_mut)]
+#![allow(clippy::never_loop, unreachable_code, unused_mut, dead_code)]
 
 fn base() {
     let mut iter = 1..20;
@@ -38,13 +38,6 @@ fn base() {
         println!("next: {:?}", iter.next());
     }
 
-    // or this
-    let mut iter = 1u32..20;
-    while let Some(_) = iter.next() {
-        break;
-    }
-    println!("Remaining iter {:?}", iter);
-
     // or this
     let mut iter = 1u32..20;
     while let Some(_) = iter.next() {
@@ -135,18 +128,6 @@ fn refutable2() {
 
 fn nested_loops() {
     let a = [42, 1337];
-    let mut y = a.iter();
-    loop {
-        // x is reused, so don't lint here
-        while let Some(_) = y.next() {}
-    }
-
-    let mut y = a.iter();
-    for _ in 0..2 {
-        while let Some(_) = y.next() {
-            // y is reused, don't lint
-        }
-    }
 
     loop {
         let mut y = a.iter();
@@ -167,10 +148,8 @@ fn issue1121() {
 }
 
 fn issue2965() {
-    // This should not cause an ICE and suggest:
-    //
-    // for _ in values.iter() {}
-    //
+    // This should not cause an ICE
+
     use std::collections::HashSet;
     let mut values = HashSet::new();
     values.insert(1);
@@ -205,13 +184,145 @@ fn issue1654() {
     }
 }
 
+fn issue6491() {
+    // Used in outer loop, needs &mut
+    let mut it = 1..40;
+    while let Some(n) = it.next() {
+        for m in &mut it {
+            if m % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+        println!("n still is {}", n);
+    }
+
+    // This is fine, inner loop uses a new iterator.
+    let mut it = 1..40;
+    for n in it {
+        let mut it = 1..40;
+        for m in it {
+            if m % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+
+        // Weird binding shouldn't change anything.
+        let (mut it, _) = (1..40, 0);
+        for m in it {
+            if m % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+
+        // Used after the loop, needs &mut.
+        let mut it = 1..40;
+        for m in &mut it {
+            if m % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+        println!("next item {}", it.next().unwrap());
+
+        println!("n still is {}", n);
+    }
+}
+
+fn issue6231() {
+    // Closure in the outer loop, needs &mut
+    let mut it = 1..40;
+    let mut opt = Some(0);
+    while let Some(n) = opt.take().or_else(|| it.next()) {
+        for m in &mut it {
+            if n % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+        println!("n still is {}", n);
+    }
+}
+
+fn issue1924() {
+    struct S<T>(T);
+    impl<T: Iterator<Item = u32>> S<T> {
+        fn f(&mut self) -> Option<u32> {
+            // Used as a field.
+            for i in &mut self.0 {
+                if !(3..=7).contains(&i) {
+                    return Some(i);
+                }
+            }
+            None
+        }
+
+        fn f2(&mut self) -> Option<u32> {
+            // Don't lint, self borrowed inside the loop
+            while let Some(i) = self.0.next() {
+                if i == 1 {
+                    return self.f();
+                }
+            }
+            None
+        }
+    }
+    impl<T: Iterator<Item = u32>> S<(S<T>, Option<u32>)> {
+        fn f3(&mut self) -> Option<u32> {
+            // Don't lint, self borrowed inside the loop
+            while let Some(i) = self.0.0.0.next() {
+                if i == 1 {
+                    return self.0.0.f();
+                }
+            }
+            while let Some(i) = self.0.0.0.next() {
+                if i == 1 {
+                    return self.f3();
+                }
+            }
+            // This one is fine, a different field is borrowed
+            for i in &mut self.0.0.0 {
+                if i == 1 {
+                    return self.0.1.take();
+                } else {
+                    self.0.1 = Some(i);
+                }
+            }
+            None
+        }
+    }
+
+    struct S2<T>(T, u32);
+    impl<T: Iterator<Item = u32>> Iterator for S2<T> {
+        type Item = u32;
+        fn next(&mut self) -> Option<u32> {
+            self.0.next()
+        }
+    }
+
+    // Don't lint, field of the iterator is accessed in the loop
+    let mut it = S2(1..40, 0);
+    while let Some(n) = it.next() {
+        if n == it.1 {
+            break;
+        }
+    }
+
+    // Needs &mut, field of the iterator is accessed after the loop
+    let mut it = S2(1..40, 0);
+    for n in &mut it {
+        if n == 0 {
+            break;
+        }
+    }
+    println!("iterator field {}", it.1);
+}
+
 fn main() {
-    base();
-    refutable();
-    refutable2();
-    nested_loops();
-    issue1121();
-    issue2965();
-    issue3670();
-    issue1654();
+    let mut it = 0..20;
+    for _ in it {
+        println!("test");
+    }
 }