diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 80aa7892a26..babf3d3cc16 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -952,16 +952,17 @@ fn check_for_loop_range<'a, 'tcx>( let mut visitor = VarVisitor { cx: cx, var: canonical_id, - indexed: HashMap::new(), + indexed_mut: HashSet::new(), + indexed_indirectly: HashMap::new(), indexed_directly: HashMap::new(), referenced: HashSet::new(), nonindex: false, + prefer_mutable: false, }; walk_expr(&mut visitor, body); // linting condition: we only indexed one variable, and indexed it directly - // (`indexed_directly` is subset of `indexed`) - if visitor.indexed.len() == 1 && visitor.indexed_directly.len() == 1 { + if visitor.indexed_indirectly.is_empty() && visitor.indexed_directly.len() == 1 { let (indexed, indexed_extent) = visitor .indexed_directly .into_iter() @@ -1009,6 +1010,12 @@ fn check_for_loop_range<'a, 'tcx>( "".to_owned() }; + let (ref_mut, method) = if visitor.indexed_mut.contains(&indexed) { + ("mut ", "iter_mut") + } else { + ("", "iter") + }; + if visitor.nonindex { span_lint_and_then( cx, @@ -1021,16 +1028,16 @@ fn check_for_loop_range<'a, 'tcx>( "consider using an iterator".to_string(), vec![ (pat.span, format!("({}, )", ident.node)), - (arg.span, format!("{}.iter().enumerate(){}{}", indexed, take, skip)), + (arg.span, format!("{}.{}().enumerate(){}{}", indexed, method, take, skip)), ], ); }, ); } else { let repl = if starts_at_zero && take.is_empty() { - format!("&{}", indexed) + format!("&{}{}", ref_mut, indexed) } else { - format!("{}.iter(){}{}", indexed, take, skip) + format!("{}.{}(){}{}", indexed, method, take, skip) }; span_lint_and_then( @@ -1537,8 +1544,10 @@ struct VarVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, /// var name to look for as index var: ast::NodeId, - /// indexed variables, the extend is `None` for global - indexed: HashMap>, + /// indexed variables that are used mutably + indexed_mut: HashSet, + /// indirectly indexed variables (`v[(i + 4) % N]`), the extend is `None` for global + indexed_indirectly: HashMap>, /// subset of `indexed` of vars that are indexed directly: `v[i]` /// this will not contain cases like `v[calc_index(i)]` or `v[(i + 4) % N]` indexed_directly: HashMap>, @@ -1548,20 +1557,21 @@ struct VarVisitor<'a, 'tcx: 'a> { /// has the loop variable been used in expressions other than the index of /// an index op? nonindex: bool, + /// Whether we are inside the `$` in `&mut $` or `$ = foo` or `$.bar`, where bar + /// takes `&mut self` + prefer_mutable: bool, } -impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { - fn visit_expr(&mut self, expr: &'tcx Expr) { +impl<'a, 'tcx> VarVisitor<'a, 'tcx> { + fn check(&mut self, idx: &'tcx Expr, seqexpr: &'tcx Expr, expr: &'tcx Expr) -> bool { if_chain! { - // an index op - if let ExprIndex(ref seqexpr, ref idx) = expr.node; // the indexed container is referenced by a name if let ExprPath(ref seqpath) = seqexpr.node; if let QPath::Resolved(None, ref seqvar) = *seqpath; if seqvar.segments.len() == 1; then { let index_used_directly = same_var(self.cx, idx, self.var); - let index_used = index_used_directly || { + let indexed_indirectly = { let mut used_visitor = LocalUsedVisitor { cx: self.cx, local: self.var, @@ -1571,7 +1581,10 @@ fn visit_expr(&mut self, expr: &'tcx Expr) { used_visitor.used }; - if index_used { + if indexed_indirectly || index_used_directly { + if self.prefer_mutable { + self.indexed_mut.insert(seqvar.segments[0].name); + } let def = self.cx.tables.qpath_def(seqpath, seqexpr.hir_id); match def { Def::Local(node_id) | Def::Upvar(node_id, ..) => { @@ -1580,24 +1593,48 @@ fn visit_expr(&mut self, expr: &'tcx Expr) { let parent_id = self.cx.tcx.hir.get_parent(expr.id); let parent_def_id = self.cx.tcx.hir.local_def_id(parent_id); let extent = self.cx.tcx.region_scope_tree(parent_def_id).var_scope(hir_id.local_id); - self.indexed.insert(seqvar.segments[0].name, Some(extent)); + if indexed_indirectly { + self.indexed_indirectly.insert(seqvar.segments[0].name, Some(extent)); + } if index_used_directly { self.indexed_directly.insert(seqvar.segments[0].name, Some(extent)); } - return; // no need to walk further *on the variable* + return false; // no need to walk further *on the variable* } Def::Static(..) | Def::Const(..) => { - self.indexed.insert(seqvar.segments[0].name, None); + if indexed_indirectly { + self.indexed_indirectly.insert(seqvar.segments[0].name, None); + } if index_used_directly { self.indexed_directly.insert(seqvar.segments[0].name, None); } - return; // no need to walk further *on the variable* + return false; // no need to walk further *on the variable* } _ => (), } } } } + true + } +} + +impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr) { + if_chain! { + // a range index op + if let ExprMethodCall(ref meth, _, ref args) = expr.node; + if meth.name == "index" || meth.name == "index_mut"; + if !self.check(&args[1], &args[0], expr); + then { return } + } + + if_chain! { + // an index op + if let ExprIndex(ref seqexpr, ref idx) = expr.node; + if !self.check(idx, seqexpr, expr); + then { return } + } if_chain! { // directly using a variable @@ -1615,8 +1652,47 @@ fn visit_expr(&mut self, expr: &'tcx Expr) { } } } - - walk_expr(self, expr); + let old = self.prefer_mutable; + match expr.node { + ExprAssignOp(_, ref lhs, ref rhs) | + ExprAssign(ref lhs, ref rhs) => { + self.prefer_mutable = true; + self.visit_expr(lhs); + self.prefer_mutable = false; + self.visit_expr(rhs); + }, + ExprAddrOf(mutbl, ref expr) => { + if mutbl == MutMutable { + self.prefer_mutable = true; + } + self.visit_expr(expr); + }, + ExprCall(ref f, ref args) => { + for (ty, expr) in self.cx.tables.expr_ty(f).fn_sig(self.cx.tcx).inputs().skip_binder().iter().zip(args) { + self.prefer_mutable = false; + if let ty::TyRef(_, mutbl) = ty.sty { + if mutbl.mutbl == MutMutable { + self.prefer_mutable = true; + } + } + self.visit_expr(expr); + } + }, + ExprMethodCall(_, _, ref args) => { + let def_id = self.cx.tables.type_dependent_defs()[expr.hir_id].def_id(); + for (ty, expr) in self.cx.tcx.fn_sig(def_id).inputs().skip_binder().iter().zip(args) { + self.prefer_mutable = false; + if let ty::TyRef(_, mutbl) = ty.sty { + if mutbl.mutbl == MutMutable { + self.prefer_mutable = true; + } + } + self.visit_expr(expr); + } + }, + _ => walk_expr(self, expr), + } + self.prefer_mutable = old; } fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { NestedVisitorMap::None diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs index b960e3990c1..30613f98f2b 100644 --- a/tests/ui/needless_range_loop.rs +++ b/tests/ui/needless_range_loop.rs @@ -24,4 +24,32 @@ fn main() { for i in 3..10 { println!("{}", ns[calc_idx(i) % 4]); } + + let mut ms = vec![1, 2, 3, 4, 5, 6]; + for i in 0..ms.len() { + ms[i] *= 2; + } + assert_eq!(ms, vec![2, 4, 6, 8, 10, 12]); + + let mut ms = vec![1, 2, 3, 4, 5, 6]; + for i in 0..ms.len() { + let x = &mut ms[i]; + *x *= 2; + } + assert_eq!(ms, vec![2, 4, 6, 8, 10, 12]); + + let g = vec![1, 2, 3, 4, 5, 6]; + let glen = g.len(); + for i in 0..glen { + let x: u32 = g[i+1..].iter().sum(); + println!("{}", g[i] + x); + } + assert_eq!(g, vec![20, 18, 15, 11, 6, 0]); + + let mut g = vec![1, 2, 3, 4, 5, 6]; + let glen = g.len(); + for i in 0..glen { + g[i] = g[i+1..].iter().sum(); + } + assert_eq!(g, vec![20, 18, 15, 11, 6, 0]); } diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index e54c0e7d011..94ee5f613fc 100644 --- a/tests/ui/needless_range_loop.stderr +++ b/tests/ui/needless_range_loop.stderr @@ -12,3 +12,30 @@ help: consider using an iterator 8 | for in ns.iter().take(10).skip(3) { | +error: the loop variable `i` is only used to index `ms`. + --> $DIR/needless_range_loop.rs:29:5 + | +29 | / for i in 0..ms.len() { +30 | | ms[i] *= 2; +31 | | } + | |_____^ + | +help: consider using an iterator + | +29 | for in &mut ms { + | + +error: the loop variable `i` is only used to index `ms`. + --> $DIR/needless_range_loop.rs:35:5 + | +35 | / for i in 0..ms.len() { +36 | | let x = &mut ms[i]; +37 | | *x *= 2; +38 | | } + | |_____^ + | +help: consider using an iterator + | +35 | for in &mut ms { + | +