diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index f06831556c29..5beb7eab71de 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -361,8 +361,11 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { // check for never_loop match expr.node { - ExprWhile(_, ref block, _) | ExprLoop(ref block, _, _) => if never_loop(block, &expr.id) { - span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops"); + ExprWhile(_, ref block, _) | + ExprLoop(ref block, _, _) => { + if never_loop(block, &expr.id) { + span_lint(cx, NEVER_LOOP, expr.span, "this loop never actually loops"); + } }, _ => (), } @@ -389,7 +392,8 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node { // ensure "if let" compatible match structure match *source { - MatchSource::Normal | MatchSource::IfLetDesugar { .. } => { + MatchSource::Normal | + MatchSource::IfLetDesugar { .. } => { if arms.len() == 2 && arms[0].pats.len() == 1 && arms[0].guard.is_none() && arms[1].pats.len() == 1 && arms[1].guard.is_none() && is_simple_break_expr(&arms[1].body) @@ -424,10 +428,8 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { } if let ExprMatch(ref match_expr, ref arms, MatchSource::WhileLetDesugar) = expr.node { let pat = &arms[0].pats[0].node; - if let ( - &PatKind::TupleStruct(ref qpath, ref pat_args, _), - &ExprMethodCall(ref method_path, _, ref method_args), - ) = (pat, &match_expr.node) + if let (&PatKind::TupleStruct(ref qpath, ref pat_args, _), + &ExprMethodCall(ref method_path, _, ref method_args)) = (pat, &match_expr.node) { let iter_expr = &method_args[0]; let lhs_constructor = last_path_segment(qpath); @@ -474,25 +476,28 @@ fn never_loop(block: &Block, id: &NodeId) -> bool { fn contains_continue_block(block: &Block, dest: &NodeId) -> bool { block.stmts.iter().any(|e| contains_continue_stmt(e, dest)) || - block - .expr - .as_ref() - .map_or(false, |e| contains_continue_expr(e, dest)) + block.expr.as_ref().map_or( + false, + |e| contains_continue_expr(e, dest), + ) } fn contains_continue_stmt(stmt: &Stmt, dest: &NodeId) -> bool { match stmt.node { - StmtSemi(ref e, _) | StmtExpr(ref e, _) => contains_continue_expr(e, dest), + StmtSemi(ref e, _) | + StmtExpr(ref e, _) => contains_continue_expr(e, dest), StmtDecl(ref d, _) => contains_continue_decl(d, dest), } } fn contains_continue_decl(decl: &Decl, dest: &NodeId) -> bool { match decl.node { - DeclLocal(ref local) => local - .init - .as_ref() - .map_or(false, |e| contains_continue_expr(e, dest)), + DeclLocal(ref local) => { + local.init.as_ref().map_or( + false, + |e| contains_continue_expr(e, dest), + ) + }, _ => false, } } @@ -508,9 +513,9 @@ fn contains_continue_expr(expr: &Expr, dest: &NodeId) -> bool { ExprTupField(ref e, _) | ExprAddrOf(_, ref e) | ExprRepeat(ref e, _) => contains_continue_expr(e, dest), - ExprArray(ref es) | ExprMethodCall(_, _, ref es) | ExprTup(ref es) => { - es.iter().any(|e| contains_continue_expr(e, dest)) - }, + ExprArray(ref es) | + ExprMethodCall(_, _, ref es) | + ExprTup(ref es) => es.iter().any(|e| contains_continue_expr(e, dest)), ExprCall(ref e, ref es) => { contains_continue_expr(e, dest) || es.iter().any(|e| contains_continue_expr(e, dest)) }, @@ -518,17 +523,23 @@ fn contains_continue_expr(expr: &Expr, dest: &NodeId) -> bool { ExprAssign(ref e1, ref e2) | ExprAssignOp(_, ref e1, ref e2) | ExprIndex(ref e1, ref e2) => [e1, e2].iter().any(|e| contains_continue_expr(e, dest)), - ExprIf(ref e, ref e2, ref e3) => [e, e2] - .iter() - .chain(e3.as_ref().iter()) - .any(|e| contains_continue_expr(e, dest)), + ExprIf(ref e, ref e2, ref e3) => { + [e, e2].iter().chain(e3.as_ref().iter()).any(|e| { + contains_continue_expr(e, dest) + }) + }, ExprWhile(ref e, ref b, _) => contains_continue_expr(e, dest) || contains_continue_block(b, dest), ExprMatch(ref e, ref arms, _) => { contains_continue_expr(e, dest) || arms.iter().any(|a| contains_continue_expr(&a.body, dest)) }, - ExprBlock(ref block) | ExprLoop(ref block, ..) => contains_continue_block(block, dest), - ExprStruct(_, _, ref base) => base.as_ref() - .map_or(false, |e| contains_continue_expr(e, dest)), + ExprBlock(ref block) | + ExprLoop(ref block, ..) => contains_continue_block(block, dest), + ExprStruct(_, _, ref base) => { + base.as_ref().map_or( + false, + |e| contains_continue_expr(e, dest), + ) + }, ExprAgain(d) => d.target_id.opt_id().map_or(false, |id| id == *dest), _ => false, } @@ -540,7 +551,8 @@ fn loop_exit_block(block: &Block) -> bool { fn loop_exit_stmt(stmt: &Stmt) -> bool { match stmt.node { - StmtSemi(ref e, _) | StmtExpr(ref e, _) => loop_exit_expr(e), + StmtSemi(ref e, _) | + StmtExpr(ref e, _) => loop_exit_expr(e), StmtDecl(ref d, _) => loop_exit_decl(d), } } @@ -562,7 +574,9 @@ fn loop_exit_expr(expr: &Expr) -> bool { ExprTupField(ref e, _) | ExprAddrOf(_, ref e) | ExprRepeat(ref e, _) => loop_exit_expr(e), - ExprArray(ref es) | ExprMethodCall(_, _, ref es) | ExprTup(ref es) => es.iter().any(|e| loop_exit_expr(e)), + ExprArray(ref es) | + ExprMethodCall(_, _, ref es) | + ExprTup(ref es) => es.iter().any(|e| loop_exit_expr(e)), ExprCall(ref e, ref es) => loop_exit_expr(e) || es.iter().any(|e| loop_exit_expr(e)), ExprBinary(_, ref e1, ref e2) | ExprAssign(ref e1, ref e2) | @@ -648,9 +662,11 @@ fn is_slice_like<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty) -> bool { fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr, var: ast::NodeId) -> Option { fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr, var: ast::NodeId) -> Option { match e.node { - ExprLit(ref l) => match l.node { - ast::LitKind::Int(x, _ty) => Some(x.to_string()), - _ => None, + ExprLit(ref l) => { + match l.node { + ast::LitKind::Int(x, _ty) => Some(x.to_string()), + _ => None, + } }, ExprPath(..) if !same_var(cx, e, var) => Some(snippet_opt(cx, e.span).unwrap_or_else(|| "??".into())), _ => None, @@ -664,25 +680,29 @@ fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr, var: ast::Node } let offset = match idx.node { - ExprBinary(op, ref lhs, ref rhs) => match op.node { - BinOp_::BiAdd => { - let offset_opt = if same_var(cx, lhs, var) { - extract_offset(cx, rhs, var) - } else if same_var(cx, rhs, var) { - extract_offset(cx, lhs, var) - } else { - None - }; + ExprBinary(op, ref lhs, ref rhs) => { + match op.node { + BinOp_::BiAdd => { + let offset_opt = if same_var(cx, lhs, var) { + extract_offset(cx, rhs, var) + } else if same_var(cx, rhs, var) { + extract_offset(cx, lhs, var) + } else { + None + }; - offset_opt.map(Offset::positive) - }, - BinOp_::BiSub if same_var(cx, lhs, var) => extract_offset(cx, rhs, var).map(Offset::negative), - _ => None, + offset_opt.map(Offset::positive) + }, + BinOp_::BiSub if same_var(cx, lhs, var) => extract_offset(cx, rhs, var).map(Offset::negative), + _ => None, + } }, - ExprPath(..) => if same_var(cx, idx, var) { - Some(Offset::positive("0".into())) - } else { - None + ExprPath(..) => { + if same_var(cx, idx, var) { + Some(Offset::positive("0".into())) + } else { + None + } }, _ => None, }; @@ -698,6 +718,23 @@ fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr, var: ast::Node } } +fn fetch_cloned_fixed_offset_var<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + expr: &Expr, + var: ast::NodeId, +) -> Option { + if_let_chain! {[ + let ExprMethodCall(ref method, _, ref args) = expr.node, + method.name == "clone", + args.len() == 1, + let Some(arg) = args.get(0), + ], { + return get_fixed_offset_var(cx, arg, var); + }} + + get_fixed_offset_var(cx, expr, var) +} + fn get_indexed_assignments<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, body: &Expr, @@ -709,7 +746,7 @@ fn get_assignment<'a, 'tcx>( var: ast::NodeId, ) -> Option<(FixedOffsetVar, FixedOffsetVar)> { if let Expr_::ExprAssign(ref lhs, ref rhs) = e.node { - match (get_fixed_offset_var(cx, lhs, var), get_fixed_offset_var(cx, rhs, var)) { + match (get_fixed_offset_var(cx, lhs, var), fetch_cloned_fixed_offset_var(cx, rhs, var)) { (Some(offset_left), Some(offset_right)) => Some((offset_left, offset_right)), _ => None, } @@ -729,13 +766,12 @@ fn get_assignment<'a, 'tcx>( .iter() .map(|stmt| match stmt.node { Stmt_::StmtDecl(..) => None, - Stmt_::StmtExpr(ref e, _node_id) | Stmt_::StmtSemi(ref e, _node_id) => Some(get_assignment(cx, e, var)), + Stmt_::StmtExpr(ref e, _node_id) | + Stmt_::StmtSemi(ref e, _node_id) => Some(get_assignment(cx, e, var)), }) - .chain( - expr.as_ref() - .into_iter() - .map(|e| Some(get_assignment(cx, &*e, var))), - ) + .chain(expr.as_ref().into_iter().map(|e| { + Some(get_assignment(cx, &*e, var)) + })) .filter_map(|op| op) .collect::>>() .unwrap_or_else(|| vec![]) @@ -754,18 +790,20 @@ fn detect_manual_memcpy<'a, 'tcx>( expr: &'tcx Expr, ) { if let Some(higher::Range { - start: Some(start), - ref end, - limits, - }) = higher::range(arg) + start: Some(start), + ref end, + limits, + }) = higher::range(arg) { // the var must be a single name if let PatKind::Binding(_, canonical_id, _, _) = pat.node { let print_sum = |arg1: &Offset, arg2: &Offset| -> String { match (&arg1.value[..], arg1.negate, &arg2.value[..], arg2.negate) { ("0", _, "0", _) => "".into(), - ("0", _, x, false) | (x, false, "0", false) => x.into(), - ("0", _, x, true) | (x, false, "0", true) => format!("-{}", x), + ("0", _, x, false) | + (x, false, "0", false) => x.into(), + ("0", _, x, true) | + (x, false, "0", true) => format!("-{}", x), (x, false, y, false) => format!("({} + {})", x, y), (x, false, y, true) => format!("({} - {})", x, y), (x, true, y, false) => format!("({} - {})", y, x), @@ -847,10 +885,10 @@ fn check_for_loop_range<'a, 'tcx>( expr: &'tcx Expr, ) { if let Some(higher::Range { - start: Some(start), - ref end, - limits, - }) = higher::range(arg) + start: Some(start), + ref end, + limits, + }) = higher::range(arg) { // the var must be a single name if let PatKind::Binding(_, canonical_id, ref ident, _) = pat.node { @@ -865,11 +903,9 @@ fn check_for_loop_range<'a, 'tcx>( // linting condition: we only indexed one variable if visitor.indexed.len() == 1 { - let (indexed, indexed_extent) = visitor - .indexed - .into_iter() - .next() - .expect("already checked that we have exactly 1 element"); + let (indexed, indexed_extent) = visitor.indexed.into_iter().next().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 { @@ -973,10 +1009,10 @@ fn is_len_call(expr: &Expr, var: &Name) -> bool { fn check_for_loop_reverse_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arg: &'tcx Expr, expr: &'tcx Expr) { // if this for loop is iterating over a two-sided range... if let Some(higher::Range { - start: Some(start), - end: Some(end), - limits, - }) = higher::range(arg) + start: Some(start), + end: Some(end), + limits, + }) = higher::range(arg) { // ...and both sides are compile-time constant integers... let parent_item = cx.tcx.hir.get_parent(arg.id); @@ -990,7 +1026,8 @@ fn check_for_loop_reverse_range<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arg: &'tcx // who think that this will iterate from the larger value to the // smaller value. let (sup, eq) = match (start_idx, end_idx) { - (&ty::Const{ val: ConstVal::Integral(start_idx), .. }, &ty::Const{ val: ConstVal::Integral(end_idx), .. }) => { + (&ty::Const { val: ConstVal::Integral(start_idx), .. }, + &ty::Const { val: ConstVal::Integral(end_idx), .. }) => { (start_idx > end_idx, start_idx == end_idx) }, _ => (false, false), @@ -1162,14 +1199,14 @@ fn check_for_loop_explicit_counter<'a, 'tcx>( // For each candidate, check the parent block to see if // it's initialized to zero at the start of the loop. let map = &cx.tcx.hir; - let parent_scope = map.get_enclosing_scope(expr.id) - .and_then(|id| map.get_enclosing_scope(id)); + let parent_scope = map.get_enclosing_scope(expr.id).and_then(|id| { + map.get_enclosing_scope(id) + }); if let Some(parent_id) = parent_scope { if let NodeBlock(block) = map.get(parent_id) { - for (id, _) in visitor - .states - .iter() - .filter(|&(_, v)| *v == VarState::IncrOnce) + for (id, _) in visitor.states.iter().filter( + |&(_, v)| *v == VarState::IncrOnce, + ) { let mut visitor2 = InitializeVisitor { cx: cx, @@ -1216,10 +1253,12 @@ fn check_for_loop_over_map_kv<'a, 'tcx>( if pat.len() == 2 { let arg_span = arg.span; let (new_pat_span, kind, ty, mutbl) = match cx.tables.expr_ty(arg).sty { - ty::TyRef(_, ref tam) => match (&pat[0].node, &pat[1].node) { - (key, _) if pat_is_wild(key, body) => (pat[1].span, "value", tam.ty, tam.mutbl), - (_, value) if pat_is_wild(value, body) => (pat[0].span, "key", tam.ty, MutImmutable), - _ => return, + ty::TyRef(_, ref tam) => { + match (&pat[0].node, &pat[1].node) { + (key, _) if pat_is_wild(key, body) => (pat[1].span, "value", tam.ty, tam.mutbl), + (_, value) if pat_is_wild(value, body) => (pat[0].span, "key", tam.ty, MutImmutable), + _ => return, + } }, _ => return, }; @@ -1282,7 +1321,7 @@ fn match_var(expr: &Expr, var: Name) -> bool { struct UsedVisitor { var: ast::Name, // var to look for - used: bool, // has the var been used otherwise? + used: bool, // has the var been used otherwise? } impl<'tcx> Visitor<'tcx> for UsedVisitor { @@ -1299,7 +1338,7 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { } } -struct LocalUsedVisitor <'a, 'tcx: 'a> { +struct LocalUsedVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, local: ast::NodeId, used: bool, @@ -1492,15 +1531,19 @@ fn extract_expr_from_first_stmt(block: &Block) -> Option<&Expr> { fn extract_first_expr(block: &Block) -> Option<&Expr> { match block.expr { Some(ref expr) if block.stmts.is_empty() => Some(expr), - None if !block.stmts.is_empty() => match block.stmts[0].node { - StmtExpr(ref expr, _) | StmtSemi(ref expr, _) => Some(expr), - StmtDecl(..) => None, + None if !block.stmts.is_empty() => { + match block.stmts[0].node { + StmtExpr(ref expr, _) | + StmtSemi(ref expr, _) => Some(expr), + StmtDecl(..) => None, + } }, _ => None, } } -/// Return true if expr contains a single break expr without destination label and +/// Return true if expr contains a single break expr without destination label +/// and /// passed expression. The expression may be within a block. fn is_simple_break_expr(expr: &Expr) -> bool { match expr.node { @@ -1520,7 +1563,7 @@ fn is_simple_break_expr(expr: &Expr) -> bool { // at the start of the loop. #[derive(PartialEq)] enum VarState { - Initial, // Not examined yet + Initial, // Not examined yet IncrOnce, // Incremented exactly once, may be a loop counter Declared, // Declared but not (yet) initialized to zero Warn, @@ -1529,9 +1572,9 @@ enum VarState { /// Scan a for loop for variables that are incremented exactly once. struct IncrementVisitor<'a, 'tcx: 'a> { - cx: &'a LateContext<'a, 'tcx>, // context reference + cx: &'a LateContext<'a, 'tcx>, // context reference states: HashMap, // incremented variables - depth: u32, // depth of conditional expressions + depth: u32, // depth of conditional expressions done: bool, } @@ -1585,7 +1628,7 @@ fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { /// Check whether a variable is initialized to zero at the start of a loop. struct InitializeVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, // context reference - end_expr: &'tcx Expr, // the for loop. Stop scanning here. + end_expr: &'tcx Expr, // the for loop. Stop scanning here. var_id: NodeId, state: VarState, name: Option, @@ -1716,11 +1759,13 @@ fn is_loop_nested(cx: &LateContext, loop_expr: &Expr, iter_expr: &Expr) -> bool return false; } match cx.tcx.hir.find(parent) { - Some(NodeExpr(expr)) => match expr.node { - ExprLoop(..) | ExprWhile(..) => { - return true; - }, - _ => (), + Some(NodeExpr(expr)) => { + match expr.node { + ExprLoop(..) | ExprWhile(..) => { + return true; + }, + _ => (), + } }, Some(NodeBlock(block)) => { let mut block_visitor = LoopNestVisitor { @@ -1744,8 +1789,8 @@ fn is_loop_nested(cx: &LateContext, loop_expr: &Expr, iter_expr: &Expr) -> bool #[derive(PartialEq, Eq)] enum Nesting { - Unknown, // no nesting detected yet - RuledOut, // the iterator is initialized or assigned within scope + Unknown, // no nesting detected yet + RuledOut, // the iterator is initialized or assigned within scope LookFurther, // no nesting detected, no further walk required } @@ -1775,8 +1820,11 @@ fn visit_expr(&mut self, expr: &'tcx Expr) { return; } match expr.node { - ExprAssign(ref path, _) | ExprAssignOp(_, ref path, _) => if match_var(path, self.iterator) { - self.nesting = RuledOut; + ExprAssign(ref path, _) | + ExprAssignOp(_, ref path, _) => { + if match_var(path, self.iterator) { + self.nesting = RuledOut; + } }, _ => walk_expr(self, expr), } diff --git a/tests/ui/for_loop.rs b/tests/ui/for_loop.rs index b4aee6d8ce23..aa8b96322930 100644 --- a/tests/ui/for_loop.rs +++ b/tests/ui/for_loop.rs @@ -541,3 +541,10 @@ fn index(&self, _: usize) -> &i32 { dst_vec[i] = src[i]; } } + +#[warn(needless_range_loop)] +pub fn manual_clone(src: &[String], dst: &mut [String]) { + for i in 0..src.len() { + dst[i] = src[i].clone(); + } +} diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 721b2833dec2..79c6c781a7a5 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -578,5 +578,13 @@ error: it looks like you're manually copying between slices 522 | | } | |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].clone_from_slice(&src_vec[..])` -error: aborting due to 58 previous errors +error: it looks like you're manually copying between slices + --> $DIR/for_loop.rs:547:5 + | +547 | / for i in 0..src.len() { +548 | | dst[i] = src[i].clone(); +549 | | } + | |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` + +error: aborting due to 59 previous errors