From daca50a515f28218142d9945ca714f7420b4fc75 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 24 Mar 2021 09:32:29 -0400 Subject: [PATCH] Improvements to `while_let_on_iterator` * Suggest `&mut iter` when the iterator is used after the loop. * Suggest `&mut iter` when the iterator is a field in a struct. * Don't lint when the iterator is a field in a struct, and the struct is used in the loop. * 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 | 2 +- .../src/loops/while_let_on_iterator.rs | 475 ++++++++++++------ clippy_utils/src/lib.rs | 18 + clippy_utils/src/sugg.rs | 2 +- clippy_utils/src/visitors.rs | 36 +- tests/ui/while_let_on_iterator.fixed | 164 ++++-- tests/ui/while_let_on_iterator.rs | 164 ++++-- tests/ui/while_let_on_iterator.stderr | 72 ++- 8 files changed, 721 insertions(+), 212 deletions(-) diff --git a/clippy_lints/src/doc.rs b/clippy_lints/src/doc.rs index fb53b55eb..e67ec4e06 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 82715d9ba..75e92f08d 100644 --- a/clippy_lints/src/loops/while_let_on_iterator.rs +++ b/clippy_lints/src/loops/while_let_on_iterator.rs @@ -1,170 +1,353 @@ -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 if_chain::if_chain; +use clippy_utils::{get_enclosing_loop, is_refutable, is_trait_method, match_def_path, paths, visitors::is_res_used}; 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]; + if let ExprKind::Match(scrutinee_expr, [arm, _], MatchSource::WhileLetDesugar) = expr.kind { + let some_pat = match arm.pat.kind { + PatKind::TupleStruct(QPath::Resolved(None, path), sub_pats, _) => match path.res { + Res::Def(_, id) if match_def_path(cx, id, &paths::OPTION_SOME) => sub_pats.first(), + _ => return, + }, + _ => return, + }; - // 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 { + let iter_expr = match scrutinee_expr.kind { + ExprKind::MethodCall(name, _, [iter_expr], _) + if name.ident.name == sym::next && is_trait_method(cx, scrutinee_expr, sym::Iterator) => + { + if let Some(iter_expr) = try_parse_iter_expr(cx, iter_expr) { + iter_expr + } else { return; } } + _ => 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, - ); - } + // Needed to find an outer loop, if there are any. + let loop_expr = if let Some((_, Node::Expr(e))) = cx.tcx.hir().parent_iter(expr.hir_id).nth(1) { + e + } else { + return; + }; + + // Refutable patterns don't work with for loops. + // The iterator also can't be accessed withing the loop. + if some_pat.map_or(true, |p| is_refutable(cx, p)) || uses_iter(cx, &iter_expr, arm.body) { + return; } + + // 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 { + "" + }; + let mut applicability = Applicability::MachineApplicable; + let iterator = snippet_with_applicability(cx, iter_expr.span, "_", &mut applicability); + let loop_var = some_pat.map_or_else( + || "_".into(), + |pat| snippet_with_applicability(cx, pat.span, "_", &mut applicability).into_owned(), + ); + 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, + ); } } -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 let Some(used_mutably) = mutated_variables(container, cx) { - if used_mutably.contains(&def_id) { - return true; - } - } - false +#[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, + /// The path being used. + path: Res, } - -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); - } - 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) - } - } - 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; - }; +/// 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 { + let span = e.span; + let mut fields = Vec::new(); loop { - let parent = cx.tcx.hir().get_parent_node(id); - if parent == id { - return false; + 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, } - match cx.tcx.hir().find(parent) { - Some(Node::Expr(expr)) => { - if let ExprKind::Loop(..) = expr.kind { + } +} + +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, + } + } +} + +/// Checks if the given expression is the same field as, is a child of, of the parent of the given +/// field. Used to check if the expression can be used while the given field is borrowed. +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; - }; - }, - 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; } - }, - Some(Node::Stmt(_)) => (), - _ => { - return false; - }, - } - id = parent; - } -} - -struct VarUsedAfterLoopVisitor { - def_id: HirId, - iter_expr_id: HirId, - past_while_let: bool, - var_used_after_while_let: bool, -} - -impl<'tcx> Visitor<'tcx> for VarUsedAfterLoopVisitor { - type Map = Map<'tcx>; - - 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; + // 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; + } + } } - } 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 { - NestedVisitorMap::None + + // 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, of 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, + } +} + +/// Strips off all field and path expressions. 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, + } + }; + (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 { + NestedVisitorMap::None + } + + 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); + } + } 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); + } + } + } + + let mut v = V { + cx, + iter_expr, + uses_iter: false, + }; + v.visit_expr(container); + v.uses_iter +} + +#[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 { + NestedVisitorMap::None + } + + 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 { + 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); + } + } + } + + 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 7ca9d3a86..371fe23be 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 0633a1939..0c9506617 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -289,7 +289,7 @@ fn has_enclosing_paren(sugg: impl AsRef) -> 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 d431bdf34..ffd150760 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 the 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 { + 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 749393db1..0562db48a 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; @@ -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(); @@ -205,13 +186,138 @@ fn issue1654() { } } -fn main() { - base(); - refutable(); - refutable2(); - nested_loops(); - issue1121(); - issue2965(); - issue3670(); - 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); + impl> S { + fn f(&mut self) -> Option { + // Used as a field. + for i in &mut self.0 { + if !(3..=7).contains(&i) { + return Some(i); + } + } + None + } + + fn f2(&mut self) -> Option { + // Don't lint, self borrowed inside the loop + while let Some(i) = self.0.next() { + if i == 1 { + return self.f(); + } + } + None + } + } + impl> S<(S, Option)> { + fn f3(&mut self) -> Option { + // 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(); + } + } + None + } + } + + struct S2(T, u32); + impl> Iterator for S2 { + type Item = u32; + fn next(&mut self) -> Option { + 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() {} diff --git a/tests/ui/while_let_on_iterator.rs b/tests/ui/while_let_on_iterator.rs index 30e3b82a7..a7c217dc7 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; @@ -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(); @@ -205,13 +186,138 @@ fn issue1654() { } } -fn main() { - base(); - refutable(); - refutable2(); - nested_loops(); - issue1121(); - issue2965(); - issue3670(); - 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); + impl> S { + fn f(&mut self) -> Option { + // 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 { + // Don't lint, self borrowed inside the loop + while let Some(i) = self.0.next() { + if i == 1 { + return self.f(); + } + } + None + } + } + impl> S<(S, Option)> { + fn f3(&mut self) -> Option { + // 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(); + } + } + None + } + } + + struct S2(T, u32); + impl> Iterator for S2 { + type Item = u32; + fn next(&mut self) -> Option { + 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() {} diff --git a/tests/ui/while_let_on_iterator.stderr b/tests/ui/while_let_on_iterator.stderr index 6554977c7..0feca2574 100644 --- a/tests/ui/while_let_on_iterator.stderr +++ b/tests/ui/while_let_on_iterator.stderr @@ -19,28 +19,90 @@ 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:193: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:204: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:206: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:215: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:224: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:241: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:256: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:257: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:288: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: aborting due to 17 previous errors