Auto merge of #6966 - Jarcho:while_let_on_iterator_fp, r=xFrednet

`while_let_on_iterator` Improvements

fixes: #6491
fixes: #6231
fixes: #5844
fixes: #1924
fixes: #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.

8 files changed, 725 insertions, 206 deletions

diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs
index fb53b55ebd6..e67ec4e06c5 100644
--- a/clippy_lints/src/doc.rs
+++ b/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 {
diff --git a/clippy_lints/src/loops/while_let_on_iterator.rs b/clippy_lints/src/loops/while_let_on_iterator.rs
index 82715d9bafa..63560047578 100644
--- a/clippy_lints/src/loops/while_let_on_iterator.rs
+++ b/clippy_lints/src/loops/while_let_on_iterator.rs
@@ -1,170 +1,347 @@
-use super::utils::{LoopNestVisitor, Nesting};
 use super::WHILE_LET_ON_ITERATOR;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_applicability;
-use clippy_utils::ty::implements_trait;
-use clippy_utils::usage::mutated_variables;
-use clippy_utils::{
-    get_enclosing_block, is_refutable, is_trait_method, last_path_segment, path_to_local, path_to_local_id,
-};
+use clippy_utils::{get_enclosing_loop, is_refutable, is_trait_method, match_def_path, paths, visitors::is_res_used};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::intravisit::{walk_block, walk_expr, NestedVisitorMap, Visitor};
-use rustc_hir::{Expr, ExprKind, HirId, MatchSource, Node, PatKind};
+use rustc_hir::intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor};
+use rustc_hir::{def::Res, Expr, ExprKind, HirId, Local, MatchSource, Node, PatKind, QPath, UnOp};
 use rustc_lint::LateContext;
-use rustc_middle::hir::map::Map;
-use rustc_span::symbol::sym;
+use rustc_span::{symbol::sym, Span, Symbol};
 
 pub(super) fn check(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-    if let ExprKind::Match(match_expr, arms, MatchSource::WhileLetDesugar) = expr.kind {
-        let pat = &arms[0].pat.kind;
-        if let (&PatKind::TupleStruct(ref qpath, pat_args, _), &ExprKind::MethodCall(method_path, _, method_args, _)) =
-            (pat, &match_expr.kind)
-        {
-            let iter_expr = &method_args[0];
-
-            // Don't lint when the iterator is recreated on every iteration
-            if_chain! {
-                if let ExprKind::MethodCall(..) | ExprKind::Call(..) = iter_expr.kind;
-                if let Some(iter_def_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
-                if implements_trait(cx, cx.typeck_results().expr_ty(iter_expr), iter_def_id, &[]);
-                then {
-                    return;
-                }
-            }
+    let (scrutinee_expr, iter_expr, some_pat, loop_expr) = if_chain! {
+        if let ExprKind::Match(scrutinee_expr, [arm, _], MatchSource::WhileLetDesugar) = expr.kind;
+        // check for `Some(..)` pattern
+        if let PatKind::TupleStruct(QPath::Resolved(None, pat_path), some_pat, _) = arm.pat.kind;
+        if let Res::Def(_, pat_did) = pat_path.res;
+        if match_def_path(cx, pat_did, &paths::OPTION_SOME);
+        // check for call to `Iterator::next`
+        if let ExprKind::MethodCall(method_name, _, [iter_expr], _) = scrutinee_expr.kind;
+        if method_name.ident.name == sym::next;
+        if is_trait_method(cx, scrutinee_expr, sym::Iterator);
+        if let Some(iter_expr) = try_parse_iter_expr(cx, iter_expr);
+        // get the loop containing the match expression
+        if let Some((_, Node::Expr(loop_expr))) = cx.tcx.hir().parent_iter(expr.hir_id).nth(1);
+        if !uses_iter(cx, &iter_expr, arm.body);
+        then {
+            (scrutinee_expr, iter_expr, some_pat, loop_expr)
+        } else {
+            return;
+        }
+    };
 
-            let lhs_constructor = last_path_segment(qpath);
-            if method_path.ident.name == sym::next
-                && is_trait_method(cx, match_expr, sym::Iterator)
-                && lhs_constructor.ident.name == sym::Some
-                && (pat_args.is_empty()
-                    || !is_refutable(cx, pat_args[0])
-                        && !is_used_inside(cx, iter_expr, arms[0].body)
-                        && !is_iterator_used_after_while_let(cx, iter_expr)
-                        && !is_nested(cx, expr, &method_args[0]))
-            {
-                let mut applicability = Applicability::MachineApplicable;
-                let iterator = snippet_with_applicability(cx, method_args[0].span, "_", &mut applicability);
-                let loop_var = if pat_args.is_empty() {
-                    "_".to_string()
-                } else {
-                    snippet_with_applicability(cx, pat_args[0].span, "_", &mut applicability).into_owned()
-                };
-                span_lint_and_sugg(
-                    cx,
-                    WHILE_LET_ON_ITERATOR,
-                    expr.span.with_hi(match_expr.span.hi()),
-                    "this loop could be written as a `for` loop",
-                    "try",
-                    format!("for {} in {}", loop_var, iterator),
-                    applicability,
-                );
-            }
+    let mut applicability = Applicability::MachineApplicable;
+    let loop_var = if let Some(some_pat) = some_pat.first() {
+        if is_refutable(cx, some_pat) {
+            // Refutable patterns don't work with for loops.
+            return;
         }
-    }
-}
+        snippet_with_applicability(cx, some_pat.span, "..", &mut applicability)
+    } else {
+        "_".into()
+    };
 
-fn is_used_inside<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, container: &'tcx Expr<'_>) -> bool {
-    let def_id = match path_to_local(expr) {
-        Some(id) => id,
-        None => return false,
+    // 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
+    // afterwards a mutable borrow of a field isn't necessary.
+    let ref_mut = if !iter_expr.fields.is_empty() || needs_mutable_borrow(cx, &iter_expr, loop_expr) {
+        "&mut "
+    } else {
+        ""
     };
-    if let Some(used_mutably) = mutated_variables(container, cx) {
-        if used_mutably.contains(&def_id) {
-            return true;
+
+    let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability);
+    span_lint_and_sugg(
+        cx,
+        WHILE_LET_ON_ITERATOR,
+        expr.span.with_hi(scrutinee_expr.span.hi()),
+        "this loop could be written as a `for` loop",
+        "try",
+        format!("for {} in {}{}", loop_var, ref_mut, iterator),
+        applicability,
+    );
+}
+
+#[derive(Debug)]
+struct IterExpr {
+    /// The span of the whole expression, not just the path and fields stored here.
+    span: Span,
+    /// The fields used, in order of child to parent.
+    fields: Vec<Symbol>,
+    /// The path being used.
+    path: Res,
+}
+/// Parses any expression to find out which field of which variable is used. Will return `None` if
+/// the expression might have side effects.
+fn try_parse_iter_expr(cx: &LateContext<'_>, mut e: &Expr<'_>) -> Option<IterExpr> {
+    let span = e.span;
+    let mut fields = Vec::new();
+    loop {
+        match e.kind {
+            ExprKind::Path(ref path) => {
+                break Some(IterExpr {
+                    span,
+                    fields,
+                    path: cx.qpath_res(path, e.hir_id),
+                });
+            },
+            ExprKind::Field(base, name) => {
+                fields.push(name.name);
+                e = base;
+            },
+            // Dereferencing a pointer has no side effects and doesn't affect which field is being used.
+            ExprKind::Unary(UnOp::Deref, base) if cx.typeck_results().expr_ty(base).is_ref() => e = base,
+
+            // Shouldn't have side effects, but there's no way to trace which field is used. So forget which fields have
+            // already been seen.
+            ExprKind::Index(base, idx) if !idx.can_have_side_effects() => {
+                fields.clear();
+                e = base;
+            },
+            ExprKind::Unary(UnOp::Deref, base) => {
+                fields.clear();
+                e = base;
+            },
+
+            // No effect and doesn't affect which field is being used.
+            ExprKind::DropTemps(base) | ExprKind::AddrOf(_, _, base) | ExprKind::Type(base, _) => e = base,
+            _ => break None,
         }
     }
-    false
 }
 
-fn is_iterator_used_after_while_let<'tcx>(cx: &LateContext<'tcx>, iter_expr: &'tcx Expr<'_>) -> bool {
-    let def_id = match path_to_local(iter_expr) {
-        Some(id) => id,
-        None => return false,
-    };
-    let mut visitor = VarUsedAfterLoopVisitor {
-        def_id,
-        iter_expr_id: iter_expr.hir_id,
-        past_while_let: false,
-        var_used_after_while_let: false,
-    };
-    if let Some(enclosing_block) = get_enclosing_block(cx, def_id) {
-        walk_block(&mut visitor, enclosing_block);
+fn is_expr_same_field(cx: &LateContext<'_>, mut e: &Expr<'_>, mut fields: &[Symbol], path_res: Res) -> bool {
+    loop {
+        match (&e.kind, fields) {
+            (&ExprKind::Field(base, name), [head_field, tail_fields @ ..]) if name.name == *head_field => {
+                e = base;
+                fields = tail_fields;
+            },
+            (ExprKind::Path(path), []) => {
+                break cx.qpath_res(path, e.hir_id) == path_res;
+            },
+            (&(ExprKind::DropTemps(base) | ExprKind::AddrOf(_, _, base) | ExprKind::Type(base, _)), _) => e = base,
+            _ => break false,
+        }
     }
-    visitor.var_used_after_while_let
 }
 
-fn is_nested(cx: &LateContext<'_>, match_expr: &Expr<'_>, iter_expr: &Expr<'_>) -> bool {
-    if_chain! {
-        if let Some(loop_block) = get_enclosing_block(cx, match_expr.hir_id);
-        let parent_node = cx.tcx.hir().get_parent_node(loop_block.hir_id);
-        if let Some(Node::Expr(loop_expr)) = cx.tcx.hir().find(parent_node);
-        then {
-            return is_loop_nested(cx, loop_expr, iter_expr)
-        }
+/// Checks if the given expression is the same field as, is a child of, or is the parent of the
+/// given field. Used to check if the expression can be used while the given field is borrowed
+/// mutably. e.g. if checking for `x.y`, then `x.y`, `x.y.z`, and `x` will all return true, but
+/// `x.z`, and `y` will return false.
+fn is_expr_same_child_or_parent_field(cx: &LateContext<'_>, expr: &Expr<'_>, fields: &[Symbol], path_res: Res) -> bool {
+    match expr.kind {
+        ExprKind::Field(base, name) => {
+            if let Some((head_field, tail_fields)) = fields.split_first() {
+                if name.name == *head_field && is_expr_same_field(cx, base, fields, path_res) {
+                    return true;
+                }
+                // Check if the expression is a parent field
+                let mut fields_iter = tail_fields.iter();
+                while let Some(field) = fields_iter.next() {
+                    if *field == name.name && is_expr_same_field(cx, base, fields_iter.as_slice(), path_res) {
+                        return true;
+                    }
+                }
+            }
+
+            // Check if the expression is a child field.
+            let mut e = base;
+            loop {
+                match e.kind {
+                    ExprKind::Field(..) if is_expr_same_field(cx, e, fields, path_res) => break true,
+                    ExprKind::Field(base, _) | ExprKind::DropTemps(base) | ExprKind::Type(base, _) => e = base,
+                    ExprKind::Path(ref path) if fields.is_empty() => {
+                        break cx.qpath_res(path, e.hir_id) == path_res;
+                    },
+                    _ => break false,
+                }
+            }
+        },
+        // If the path matches, this is either an exact match, or the expression is a parent of the field.
+        ExprKind::Path(ref path) => cx.qpath_res(path, expr.hir_id) == path_res,
+        ExprKind::DropTemps(base) | ExprKind::Type(base, _) | ExprKind::AddrOf(_, _, base) => {
+            is_expr_same_child_or_parent_field(cx, base, fields, path_res)
+        },
+        _ => false,
     }
-    false
 }
 
-fn is_loop_nested(cx: &LateContext<'_>, loop_expr: &Expr<'_>, iter_expr: &Expr<'_>) -> bool {
-    let mut id = loop_expr.hir_id;
-    let iter_id = if let Some(id) = path_to_local(iter_expr) {
-        id
-    } else {
-        return true;
+/// Strips off all field and path expressions. This will return true if a field or path has been
+/// skipped. Used to skip them after failing to check for equality.
+fn skip_fields_and_path(expr: &'tcx Expr<'_>) -> (Option<&'tcx Expr<'tcx>>, bool) {
+    let mut e = expr;
+    let e = loop {
+        match e.kind {
+            ExprKind::Field(base, _) | ExprKind::DropTemps(base) | ExprKind::Type(base, _) => e = base,
+            ExprKind::Path(_) => return (None, true),
+            _ => break e,
+        }
     };
-    loop {
-        let parent = cx.tcx.hir().get_parent_node(id);
-        if parent == id {
-            return false;
+    (Some(e), e.hir_id != expr.hir_id)
+}
+
+/// Checks if the given expression uses the iterator.
+fn uses_iter(cx: &LateContext<'tcx>, iter_expr: &IterExpr, container: &'tcx Expr<'_>) -> bool {
+    struct V<'a, 'b, 'tcx> {
+        cx: &'a LateContext<'tcx>,
+        iter_expr: &'b IterExpr,
+        uses_iter: bool,
+    }
+    impl Visitor<'tcx> for V<'_, '_, 'tcx> {
+        type Map = ErasedMap<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::None
         }
-        match cx.tcx.hir().find(parent) {
-            Some(Node::Expr(expr)) => {
-                if let ExprKind::Loop(..) = expr.kind {
-                    return true;
-                };
-            },
-            Some(Node::Block(block)) => {
-                let mut block_visitor = LoopNestVisitor {
-                    hir_id: id,
-                    iterator: iter_id,
-                    nesting: Nesting::Unknown,
-                };
-                walk_block(&mut block_visitor, block);
-                if block_visitor.nesting == Nesting::RuledOut {
-                    return false;
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if self.uses_iter {
+                // return
+            } else if is_expr_same_child_or_parent_field(self.cx, e, &self.iter_expr.fields, self.iter_expr.path) {
+                self.uses_iter = true;
+            } else if let (e, true) = skip_fields_and_path(e) {
+                if let Some(e) = e {
+                    self.visit_expr(e);
                 }
-            },
-            Some(Node::Stmt(_)) => (),
-            _ => {
-                return false;
-            },
+            } else if let ExprKind::Closure(_, _, id, _, _) = e.kind {
+                if is_res_used(self.cx, self.iter_expr.path, id) {
+                    self.uses_iter = true;
+                }
+            } else {
+                walk_expr(self, e);
+            }
         }
-        id = parent;
     }
-}
 
-struct VarUsedAfterLoopVisitor {
-    def_id: HirId,
-    iter_expr_id: HirId,
-    past_while_let: bool,
-    var_used_after_while_let: bool,
+    let mut v = V {
+        cx,
+        iter_expr,
+        uses_iter: false,
+    };
+    v.visit_expr(container);
+    v.uses_iter
 }
 
-impl<'tcx> Visitor<'tcx> for VarUsedAfterLoopVisitor {
-    type Map = Map<'tcx>;
+#[allow(clippy::too_many_lines)]
+fn needs_mutable_borrow(cx: &LateContext<'tcx>, iter_expr: &IterExpr, loop_expr: &'tcx Expr<'_>) -> bool {
+    struct AfterLoopVisitor<'a, 'b, 'tcx> {
+        cx: &'a LateContext<'tcx>,
+        iter_expr: &'b IterExpr,
+        loop_id: HirId,
+        after_loop: bool,
+        used_iter: bool,
+    }
+    impl Visitor<'tcx> for AfterLoopVisitor<'_, '_, 'tcx> {
+        type Map = ErasedMap<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::None
+        }
 
-    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
-        if self.past_while_let {
-            if path_to_local_id(expr, self.def_id) {
-                self.var_used_after_while_let = true;
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if self.used_iter {
+                return;
+            }
+            if self.after_loop {
+                if is_expr_same_child_or_parent_field(self.cx, e, &self.iter_expr.fields, self.iter_expr.path) {
+                    self.used_iter = true;
+                } else if let (e, true) = skip_fields_and_path(e) {
+                    if let Some(e) = e {
+                        self.visit_expr(e);
+                    }
+                } else if let ExprKind::Closure(_, _, id, _, _) = e.kind {
+                    self.used_iter = is_res_used(self.cx, self.iter_expr.path, id);
+                } else {
+                    walk_expr(self, e);
+                }
+            } else if self.loop_id == e.hir_id {
+                self.after_loop = true;
+            } else {
+                walk_expr(self, e);
+            }
+        }
+    }
+
+    struct NestedLoopVisitor<'a, 'b, 'tcx> {
+        cx: &'a LateContext<'tcx>,
+        iter_expr: &'b IterExpr,
+        local_id: HirId,
+        loop_id: HirId,
+        after_loop: bool,
+        found_local: bool,
+        used_after: bool,
+    }
+    impl Visitor<'tcx> for NestedLoopVisitor<'a, 'b, 'tcx> {
+        type Map = ErasedMap<'tcx>;
+        fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+            NestedVisitorMap::None
+        }
+
+        fn visit_local(&mut self, l: &'tcx Local<'_>) {
+            if !self.after_loop {
+                l.pat.each_binding_or_first(&mut |_, id, _, _| {
+                    if id == self.local_id {
+                        self.found_local = true;
+                    }
+                });
+            }
+            if let Some(e) = l.init {
+                self.visit_expr(e);
+            }
+        }
+
+        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
+            if self.used_after {
+                return;
+            }
+            if self.after_loop {
+                if is_expr_same_child_or_parent_field(self.cx, e, &self.iter_expr.fields, self.iter_expr.path) {
+                    self.used_after = true;
+                } else if let (e, true) = skip_fields_and_path(e) {
+                    if let Some(e) = e {
+                        self.visit_expr(e);
+                    }
+                } else if let ExprKind::Closure(_, _, id, _, _) = e.kind {
+                    self.used_after = is_res_used(self.cx, self.iter_expr.path, id);
+                } else {
+                    walk_expr(self, e);
+                }
+            } else if e.hir_id == self.loop_id {
+                self.after_loop = true;
+            } else {
+                walk_expr(self, e);
             }
-        } else if self.iter_expr_id == expr.hir_id {
-            self.past_while_let = true;
         }
-        walk_expr(self, expr);
     }
-    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
-        NestedVisitorMap::None
+
+    if let Some(e) = get_enclosing_loop(cx.tcx, loop_expr) {
+        // The iterator expression will be used on the next iteration unless it is declared within the outer
+        // loop.
+        let local_id = match iter_expr.path {
+            Res::Local(id) => id,
+            _ => return true,
+        };
+        let mut v = NestedLoopVisitor {
+            cx,
+            iter_expr,
+            local_id,
+            loop_id: loop_expr.hir_id,
+            after_loop: false,
+            found_local: false,
+            used_after: false,
+        };
+        v.visit_expr(e);
+        v.used_after || !v.found_local
+    } else {
+        let mut v = AfterLoopVisitor {
+            cx,
+            iter_expr,
+            loop_id: loop_expr.hir_id,
+            after_loop: false,
+            used_iter: false,
+        };
+        v.visit_expr(&cx.tcx.hir().body(cx.enclosing_body.unwrap()).value);
+        v.used_iter
     }
 }
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index 7ca9d3a860d..371fe23bedc 100644
--- a/clippy_utils/src/lib.rs
+++ b/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();
diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs
index 0633a19391f..0c950661757 100644
--- a/clippy_utils/src/sugg.rs
+++ b/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 == ')' {
diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs
index d431bdf34ee..ecdc666b5f6 100644
--- a/clippy_utils/src/visitors.rs
+++ b/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
+}
diff --git a/tests/ui/while_let_on_iterator.fixed b/tests/ui/while_let_on_iterator.fixed
index 749393db124..c3e2cf0c4a4 100644
--- a/tests/ui/while_let_on_iterator.fixed
+++ b/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;
@@ -41,13 +41,6 @@ fn base() {
     // 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() {
         iter = 1..20;
     }
 }
@@ -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");
+    }
 }
diff --git a/tests/ui/while_let_on_iterator.rs b/tests/ui/while_let_on_iterator.rs
index 30e3b82a7cc..1717006a449 100644
--- a/tests/ui/while_let_on_iterator.rs
+++ b/tests/ui/while_let_on_iterator.rs
@@ -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;
@@ -41,13 +41,6 @@ fn base() {
     // 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() {
         iter = 1..20;
     }
 }
@@ -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() {
+        while let Some(m) = it.next() {
+            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;
+    while let Some(n) = it.next() {
+        let mut it = 1..40;
+        while let Some(m) = it.next() {
+            if m % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+
+        // Weird binding shouldn't change anything.
+        let (mut it, _) = (1..40, 0);
+        while let Some(m) = it.next() {
+            if m % 10 == 0 {
+                break;
+            }
+            println!("doing something with m: {}", m);
+        }
+
+        // Used after the loop, needs &mut.
+        let mut it = 1..40;
+        while let Some(m) = it.next() {
+            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()) {
+        while let Some(m) = it.next() {
+            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.
+            while let Some(i) = self.0.next() {
+                if i < 3 || i > 7 {
+                    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
+            while let Some(i) = self.0.0.0.next() {
+                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);
+    while let Some(n) = it.next() {
+        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;
+    while let Some(..) = it.next() {
+        println!("test");
+    }
 }
diff --git a/tests/ui/while_let_on_iterator.stderr b/tests/ui/while_let_on_iterator.stderr
index 6554977c798..eff559bef7e 100644
--- a/tests/ui/while_let_on_iterator.stderr
+++ b/tests/ui/while_let_on_iterator.stderr
@@ -19,28 +19,96 @@ LL |     while let Some(_) = iter.next() {}
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in iter`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:101:9
+  --> $DIR/while_let_on_iterator.rs:94:9
    |
 LL |         while let Some([..]) = it.next() {}
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for [..] in it`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:108:9
+  --> $DIR/while_let_on_iterator.rs:101:9
    |
 LL |         while let Some([_x]) = it.next() {}
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for [_x] in it`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:121:9
+  --> $DIR/while_let_on_iterator.rs:114:9
    |
 LL |         while let Some(x @ [_]) = it.next() {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for x @ [_] in it`
 
 error: this loop could be written as a `for` loop
-  --> $DIR/while_let_on_iterator.rs:153:9
+  --> $DIR/while_let_on_iterator.rs:134:9
    |
 LL |         while let Some(_) = y.next() {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in y`
 
-error: aborting due to 7 previous errors
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:191:9
+   |
+LL |         while let Some(m) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for m in &mut it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:202:5
+   |
+LL |     while let Some(n) = it.next() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for n in it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:204:9
+   |
+LL |         while let Some(m) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for m in it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:213:9
+   |
+LL |         while let Some(m) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for m in it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:222:9
+   |
+LL |         while let Some(m) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for m in &mut it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:239:9
+   |
+LL |         while let Some(m) = it.next() {
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for m in &mut it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:254:13
+   |
+LL |             while let Some(i) = self.0.next() {
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for i in &mut self.0`
+
+error: manual `!RangeInclusive::contains` implementation
+  --> $DIR/while_let_on_iterator.rs:255:20
+   |
+LL |                 if i < 3 || i > 7 {
+   |                    ^^^^^^^^^^^^^^ help: use: `!(3..=7).contains(&i)`
+   |
+   = note: `-D clippy::manual-range-contains` implied by `-D warnings`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:286:13
+   |
+LL |             while let Some(i) = self.0.0.0.next() {
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for i in &mut self.0.0.0`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:315:5
+   |
+LL |     while let Some(n) = it.next() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for n in &mut it`
+
+error: this loop could be written as a `for` loop
+  --> $DIR/while_let_on_iterator.rs:325:5
+   |
+LL |     while let Some(..) = it.next() {
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `for _ in it`
+
+error: aborting due to 18 previous errors