diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 14088c15689..5b04a232257 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -10,7 +10,7 @@ use rustc::middle::region::CodeExtent; use rustc::ty::{self, Ty}; use rustc::ty::subst::Subst; use rustc_const_eval::ConstContext; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use syntax::ast; use utils::sugg; @@ -579,6 +579,7 @@ fn check_for_loop_range<'a, 'tcx>( cx: cx, var: def_id, indexed: HashMap::new(), + referenced: HashSet::new(), nonindex: false, }; walk_expr(&mut visitor, body); @@ -588,7 +589,7 @@ fn check_for_loop_range<'a, 'tcx>( let (indexed, indexed_extent) = visitor.indexed .into_iter() .next() - .unwrap_or_else(|| unreachable!() /* len == 1 */); + .expect("already checked that we have exactly 1 element"); // ensure that the indexed variable was declared before the loop, see #601 if let Some(indexed_extent) = indexed_extent { @@ -601,6 +602,11 @@ fn check_for_loop_range<'a, 'tcx>( } } + // don't lint if the container that is indexed into is also used without indexing + if visitor.referenced.contains(&indexed) { + return; + } + let starts_at_zero = is_integer_literal(start, 0); let skip = if starts_at_zero { @@ -952,50 +958,70 @@ impl<'tcx> Visitor<'tcx> for UsedVisitor { } struct VarVisitor<'a, 'tcx: 'a> { - cx: &'a LateContext<'a, 'tcx>, // context reference - var: DefId, // var name to look for as index - indexed: HashMap>, // indexed variables, the extent is None for global - nonindex: bool, // has the var been used otherwise? + /// context reference + cx: &'a LateContext<'a, 'tcx>, + /// var name to look for as index + var: DefId, + /// indexed variables, the extend is `None` for global + indexed: HashMap>, + /// Any names that are used outside an index operation. + /// Used to detect things like `&mut vec` used together with `vec[i]` + referenced: HashSet, + /// has the loop variable been used in expressions other than the index of an index op? + nonindex: bool, } impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { fn visit_expr(&mut self, expr: &'tcx Expr) { - if let ExprPath(ref qpath) = expr.node { - if let QPath::Resolved(None, ref path) = *qpath { - if path.segments.len() == 1 && self.cx.tables.qpath_def(qpath, expr.id).def_id() == self.var { - // we are referencing our variable! now check if it's as an index - if_let_chain! {[ - let Some(parexpr) = get_parent_expr(self.cx, expr), - let ExprIndex(ref seqexpr, _) = parexpr.node, - let ExprPath(ref seqpath) = seqexpr.node, - let QPath::Resolved(None, ref seqvar) = *seqpath, - seqvar.segments.len() == 1 - ], { - let def = self.cx.tables.qpath_def(seqpath, seqexpr.id); - match def { - Def::Local(..) | Def::Upvar(..) => { - let def_id = def.def_id(); - let node_id = self.cx.tcx.hir.as_local_node_id(def_id).expect("local/upvar are local nodes"); + if_let_chain! {[ + // an index op + let ExprIndex(ref seqexpr, ref idx) = expr.node, + // directly indexing a variable + let ExprPath(ref qpath) = idx.node, + let QPath::Resolved(None, ref path) = *qpath, + path.segments.len() == 1, + // our variable! + self.cx.tables.qpath_def(qpath, expr.id).def_id() == self.var, + // the indexed container is referenced by a name + let ExprPath(ref seqpath) = seqexpr.node, + let QPath::Resolved(None, ref seqvar) = *seqpath, + seqvar.segments.len() == 1, + ], { + let def = self.cx.tables.qpath_def(seqpath, seqexpr.id); + match def { + Def::Local(..) | Def::Upvar(..) => { + let def_id = def.def_id(); + let node_id = self.cx.tcx.hir.as_local_node_id(def_id).expect("local/upvar are local nodes"); - 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_maps(parent_def_id).var_scope(node_id); - self.indexed.insert(seqvar.segments[0].name, Some(extent)); - return; // no need to walk further - } - Def::Static(..) | Def::Const(..) => { - self.indexed.insert(seqvar.segments[0].name, None); - return; // no need to walk further - } - _ => (), - } - }} - // we are not indexing anything, record that - self.nonindex = true; - return; + 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_maps(parent_def_id).var_scope(node_id); + self.indexed.insert(seqvar.segments[0].name, Some(extent)); + return; // no need to walk further } + Def::Static(..) | Def::Const(..) => { + self.indexed.insert(seqvar.segments[0].name, None); + return; // no need to walk further + } + _ => (), } - } + }} + + if_let_chain! {[ + // directly indexing a variable + let ExprPath(ref qpath) = expr.node, + let QPath::Resolved(None, ref path) = *qpath, + path.segments.len() == 1, + ], { + if self.cx.tables.qpath_def(qpath, expr.id).def_id() == self.var { + // we are not indexing anything, record that + self.nonindex = true; + } else { + // not the correct variable, but still a variable + self.referenced.insert(path.segments[0].name); + } + }} + walk_expr(self, expr); } fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { diff --git a/clippy_tests/examples/for_loop.rs b/clippy_tests/examples/for_loop.rs index 2f47b1b8022..1a4b9765063 100644 --- a/clippy_tests/examples/for_loop.rs +++ b/clippy_tests/examples/for_loop.rs @@ -249,8 +249,8 @@ fn main() { for _v in u.iter() { } // no error let mut out = vec![]; - vec.iter().map(|x| out.push(x)).collect::>(); - let _y = vec.iter().map(|x| out.push(x)).collect::>(); // this is fine + vec.iter().cloned().map(|x| out.push(x)).collect::>(); + let _y = vec.iter().cloned().map(|x| out.push(x)).collect::>(); // this is fine // Loop with explicit counter variable let mut _index = 0; @@ -346,6 +346,18 @@ fn main() { } test_for_kv_map(); + + fn f(_: &T, _: &T) -> bool { unimplemented!() } + fn g(_: &mut [T], _: usize, _: usize) { unimplemented!() } + for i in 1..vec.len() { + if f(&vec[i - 1], &vec[i]) { + g(&mut vec, i - 1, i); + } + } + + for mid in 1..vec.len() { + let (_, _) = vec.split_at(mid); + } } #[allow(used_underscore_binding)] @@ -358,3 +370,17 @@ fn test_for_kv_map() { let _k = k; } } + +#[allow(dead_code)] +fn partition(v: &mut [T]) -> usize { + let pivot = v.len() - 1; + let mut i = 0; + for j in 0..pivot { + if v[j] <= v[pivot] { + v.swap(i, j); + i += 1; + } + } + v.swap(i, pivot); + i +} diff --git a/clippy_tests/examples/for_loop.stderr b/clippy_tests/examples/for_loop.stderr index e65486dd40c..06765f35779 100644 --- a/clippy_tests/examples/for_loop.stderr +++ b/clippy_tests/examples/for_loop.stderr @@ -419,8 +419,8 @@ error: you are iterating over `Iterator::next()` which is an Option; this will c error: you are collect()ing an iterator and throwing away the result. Consider using an explicit for loop to exhaust the iterator --> for_loop.rs:252:5 | -252 | vec.iter().map(|x| out.push(x)).collect::>(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +252 | vec.iter().cloned().map(|x| out.push(x)).collect::>(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `-D unused-collect` implied by `-D warnings`