From 652df0fb79666dc976bed3a08d0db7f454014951 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Nov 2017 14:41:54 +0100 Subject: [PATCH 1/2] Differentiate between mutable iteration and immutable iteration in `needless_range_loop` --- clippy_lints/src/loops.rs | 65 ++++++++++++++++++++++++++--- tests/ui/needless_range_loop.rs | 13 ++++++ tests/ui/needless_range_loop.stderr | 27 ++++++++++++ 3 files changed, 100 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 80aa7892a267..ecaa64c8724c 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -952,10 +952,12 @@ fn check_for_loop_range<'a, 'tcx>( let mut visitor = VarVisitor { cx: cx, var: canonical_id, + indexed_mut: HashSet::new(), indexed: HashMap::new(), indexed_directly: HashMap::new(), referenced: HashSet::new(), nonindex: false, + prefer_mutable: false, }; walk_expr(&mut visitor, body); @@ -1009,6 +1011,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 +1029,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,6 +1545,8 @@ struct VarVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, /// var name to look for as index var: ast::NodeId, + /// indexed variables that are used mutably + indexed_mut: HashSet, /// indexed variables, the extend is `None` for global indexed: HashMap>, /// subset of `indexed` of vars that are indexed directly: `v[i]` @@ -1548,6 +1558,9 @@ 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> { @@ -1572,6 +1585,9 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { }; if index_used { + 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, ..) => { @@ -1615,8 +1631,47 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { } } } - - 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 b960e3990c11..6f1e5f53ce20 100644 --- a/tests/ui/needless_range_loop.rs +++ b/tests/ui/needless_range_loop.rs @@ -24,4 +24,17 @@ 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]); } diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index e2c3e18e8219..9b6be856b857 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 { + | ^^^^^^ + From 1b323b9f355c0b3cdf0729f57880240ade00d56f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 7 Nov 2017 15:32:52 +0100 Subject: [PATCH 2/2] Don't lint mixed slice indexing and usize indexing in `needless_range_loop` --- clippy_lints/src/loops.rs | 51 +++++++++++++++++++++++---------- tests/ui/needless_range_loop.rs | 15 ++++++++++ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index ecaa64c8724c..babf3d3cc16f 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -953,7 +953,7 @@ fn check_for_loop_range<'a, 'tcx>( cx: cx, var: canonical_id, indexed_mut: HashSet::new(), - indexed: HashMap::new(), + indexed_indirectly: HashMap::new(), indexed_directly: HashMap::new(), referenced: HashSet::new(), nonindex: false, @@ -962,8 +962,7 @@ fn check_for_loop_range<'a, 'tcx>( 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() @@ -1547,8 +1546,8 @@ struct VarVisitor<'a, 'tcx: 'a> { var: ast::NodeId, /// indexed variables that are used mutably indexed_mut: HashSet, - /// indexed variables, the extend is `None` for global - indexed: HashMap>, + /// 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>, @@ -1563,18 +1562,16 @@ struct VarVisitor<'a, 'tcx: 'a> { 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, @@ -1584,7 +1581,7 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { used_visitor.used }; - if index_used { + if indexed_indirectly || index_used_directly { if self.prefer_mutable { self.indexed_mut.insert(seqvar.segments[0].name); } @@ -1596,24 +1593,48 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { 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 diff --git a/tests/ui/needless_range_loop.rs b/tests/ui/needless_range_loop.rs index 6f1e5f53ce20..30613f98f2bc 100644 --- a/tests/ui/needless_range_loop.rs +++ b/tests/ui/needless_range_loop.rs @@ -37,4 +37,19 @@ fn main() { *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]); }