From 90f345df94ffa92760ce338208a034d0a3785a2d Mon Sep 17 00:00:00 2001 From: Marcus Klaas Date: Mon, 4 Sep 2017 20:16:34 -0400 Subject: [PATCH] Add lint to detect manual slice copies --- clippy_lints/src/loops.rs | 346 +++++++++++++++++++++++++++++++++++--- tests/ui/for_loop.stderr | 79 +++++---- 2 files changed, 368 insertions(+), 57 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 2f87fe0f3963a..6cf2b1d9fedc3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -1,3 +1,4 @@ +use itertools::Itertools; use reexport::*; use rustc::hir::*; use rustc::hir::def::Def; @@ -15,10 +16,29 @@ use syntax::ast; use utils::sugg; use utils::{get_enclosing_block, get_parent_expr, higher, in_external_macro, is_integer_literal, is_refutable, - last_path_segment, match_trait_method, match_type, multispan_sugg, snippet, span_help_and_lint, span_lint, - span_lint_and_sugg, span_lint_and_then}; + last_path_segment, match_trait_method, match_type, multispan_sugg, snippet, snippet_opt, + span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then}; use utils::paths; +/// **What it does:** Checks for for loops that manually copy items between +/// slices that could be optimized by having a memcpy. +/// +/// **Why is this bad?** It is not as fast as a memcpy. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// for i in 0..src.len() { +/// dst[i + 64] = src[i]; +/// } +/// ``` +declare_lint! { + pub MANUAL_MEMCPY, + Warn, + "manually copying items between slices" +} + /// **What it does:** Checks for looping over the range of `0..len` of some /// collection just to get the values by index. /// @@ -314,6 +334,7 @@ pub struct Pass; impl LintPass for Pass { fn get_lints(&self) -> LintArray { lint_array!( + MANUAL_MEMCPY, NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, EXPLICIT_INTO_ITER_LOOP, @@ -570,6 +591,249 @@ fn check_for_loop<'a, 'tcx>( check_for_loop_arg(cx, pat, arg, expr); check_for_loop_explicit_counter(cx, arg, body, expr); check_for_loop_over_map_kv(cx, pat, arg, body, expr); + detect_manual_memcpy(cx, pat, arg, body, expr); +} + +fn same_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr, var: DefId) -> bool { + if_let_chain! {[ + let ExprPath(ref qpath) = expr.node, + let QPath::Resolved(None, ref path) = *qpath, + path.segments.len() == 1, + // our variable! + cx.tables.qpath_def(qpath, expr.hir_id).def_id() == var + ], { + return true; + }} + + false +} + +struct Offset { + value: String, + negate: bool, +} + +impl Offset { + fn negative(s: String) -> Self { + Self { + value: s, + negate: true, + } + } + + fn positive(s: String) -> Self { + Self { + value: s, + negate: false, + } + } +} + +struct FixedOffsetVar { + var_name: String, + offset: Offset, +} + +fn is_slice_like<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty) -> bool { + let is_slice = match ty.sty { + ty::TyRef(_, ref subty) => is_slice_like(cx, subty.ty), + ty::TySlice(..) | ty::TyArray(..) => true, + _ => false, + }; + + is_slice || match_type(cx, ty, &paths::VEC) || match_type(cx, ty, &paths::VEC_DEQUE) +} + +fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr, var: DefId) -> Option { + fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr, var: DefId) -> Option { + match e.node { + 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, + } + } + + if let ExprIndex(ref seqexpr, ref idx) = expr.node { + let ty = cx.tables.expr_ty(seqexpr); + if !is_slice_like(cx, ty) { + return None; + } + + 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 + }; + + 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 + }, + _ => None, + }; + + offset.map(|o| { + FixedOffsetVar { + var_name: snippet_opt(cx, seqexpr.span).unwrap_or_else(|| "???".into()), + offset: o, + } + }) + } else { + None + } +} + +fn get_indexed_assignments<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + body: &Expr, + var: DefId, +) -> Vec<(FixedOffsetVar, FixedOffsetVar)> { + fn get_assignment<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + e: &Expr, + var: DefId, + ) -> 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)) { + (Some(offset_left), Some(offset_right)) => Some((offset_left, offset_right)), + _ => None, + } + } else { + None + } + } + + if let Expr_::ExprBlock(ref b) = body.node { + let Block { + ref stmts, + ref expr, + .. + } = **b; + + stmts + .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)), + }) + .chain( + expr.as_ref() + .into_iter() + .map(|e| Some(get_assignment(cx, &*e, var))), + ) + .filter_map(|op| op) + .collect::>>() + .unwrap_or_else(|| vec![]) + } else { + get_assignment(cx, body, var).into_iter().collect() + } +} + +/// Check for for loops that sequentially copy items from one slice-like +/// object to another. +fn detect_manual_memcpy<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + pat: &'tcx Pat, + arg: &'tcx Expr, + body: &'tcx Expr, + expr: &'tcx Expr, +) { + if let Some(higher::Range { + start: Some(start), + ref end, + limits, + }) = higher::range(arg) + { + // the var must be a single name + if let PatKind::Binding(_, def_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), + (x, false, y, false) => format!("({} + {})", x, y), + (x, false, y, true) => format!("({} - {})", x, y), + (x, true, y, false) => format!("({} - {})", y, x), + (x, true, y, true) => format!("-({} + {})", x, y), + } + }; + + let print_limit = |end: &Option<&Expr>, offset: Offset, var_name: &str| if let Some(end) = *end { + if_let_chain! {[ + let ExprMethodCall(ref method, _, ref len_args) = end.node, + method.name == "len", + len_args.len() == 1, + let Some(arg) = len_args.get(0), + snippet(cx, arg.span, "??") == var_name, + ], { + return if offset.negate { + format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value) + } else { + "".to_owned() + }; + }} + + let end_str = match limits { + ast::RangeLimits::Closed => { + let end = sugg::Sugg::hir(cx, end, ""); + format!("{}", end + sugg::ONE) + }, + ast::RangeLimits::HalfOpen => format!("{}", snippet(cx, end.span, "..")), + }; + + print_sum(&Offset::positive(end_str), &offset) + } else { + "..".into() + }; + + // The only statements in the for loops can be indexed assignments from + // indexed retrievals. + let manual_copies = get_indexed_assignments(cx, body, def_id); + + let big_sugg = manual_copies + .into_iter() + .map(|(dst_var, src_var)| { + let start_str = Offset::positive(snippet_opt(cx, start.span).unwrap_or_else(|| "".into())); + let dst_offset = print_sum(&start_str, &dst_var.offset); + let dst_limit = print_limit(end, dst_var.offset, &dst_var.var_name); + let src_offset = print_sum(&start_str, &src_var.offset); + let src_limit = print_limit(end, src_var.offset, &src_var.var_name); + let dst = if dst_offset == "" && dst_limit == "" { + dst_var.var_name + } else { + format!("{}[{}..{}]", dst_var.var_name, dst_offset, dst_limit) + }; + + format!("{}.clone_from_slice(&{}[{}..{}])", dst, src_var.var_name, src_offset, src_limit) + }) + .join("\n "); + + if !big_sugg.is_empty() { + span_lint_and_sugg( + cx, + MANUAL_MEMCPY, + expr.span, + "it looks like you're manually copying between slices", + "try replacing the loop by", + big_sugg, + ); + } + } + } } /// Check for looping over a range and then indexing a sequence with it. @@ -1024,9 +1288,29 @@ impl<'tcx> Visitor<'tcx> for UsedVisitor { fn visit_expr(&mut self, expr: &'tcx Expr) { if match_var(expr, self.var) { self.used = true; - return; + } else { + walk_expr(self, expr); + } + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} + +struct DefIdUsedVisitor<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + def_id: DefId, + used: bool, +} + +impl<'a, 'tcx: 'a> Visitor<'tcx> for DefIdUsedVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx Expr) { + if same_var(self.cx, expr, self.def_id) { + self.used = true; + } else { + walk_expr(self, expr); } - walk_expr(self, expr); } fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { @@ -1054,40 +1338,46 @@ impl<'a, 'tcx> Visitor<'tcx> for VarVisitor<'a, 'tcx> { 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.hir_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.hir_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 hir_id = self.cx.tcx.hir.node_to_hir_id(node_id); - - 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)); - return; // no need to walk further - } - Def::Static(..) | Def::Const(..) => { - self.indexed.insert(seqvar.segments[0].name, None); - return; // no need to walk further + let index_used = same_var(self.cx, idx, self.var) || { + let mut used_visitor = DefIdUsedVisitor { + cx: self.cx, + def_id: self.var, + used: false, + }; + walk_expr(&mut used_visitor, idx); + used_visitor.used + }; + + if index_used { + let def = self.cx.tables.qpath_def(seqpath, seqexpr.hir_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 hir_id = self.cx.tcx.hir.node_to_hir_id(node_id); + + 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)); + return; // no need to walk further *on the variable* + } + Def::Static(..) | Def::Const(..) => { + self.indexed.insert(seqvar.segments[0].name, None); + return; // no need to walk further *on the variable* + } + _ => (), } - _ => (), } }} if_let_chain! {[ - // directly indexing a variable + // directly using a variable let ExprPath(ref qpath) = expr.node, let QPath::Resolved(None, ref path) = *qpath, path.segments.len() == 1, diff --git a/tests/ui/for_loop.stderr b/tests/ui/for_loop.stderr index 090caf1779a5e..721b2833dec26 100644 --- a/tests/ui/for_loop.stderr +++ b/tests/ui/for_loop.stderr @@ -505,57 +505,78 @@ help: use the corresponding method 409 | for k in rm.keys() { | ^ -error: the loop variable `i` is used to index `src` +error: it looks like you're manually copying between slices + --> $DIR/for_loop.rs:462:5 + | +462 | / for i in 0..src.len() { +463 | | dst[i] = src[i]; +464 | | } + | |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` + | + = note: `-D manual-memcpy` implied by `-D warnings` + +error: it looks like you're manually copying between slices --> $DIR/for_loop.rs:467:5 | 467 | / for i in 0..src.len() { 468 | | dst[i + 10] = src[i]; 469 | | } - | |_____^ - | -help: consider using an iterator - | -467 | for (i, ) in src.iter().enumerate() { - | ^^^^^^^^^^^ + | |_____^ help: try replacing the loop by: `dst[10..(src.len() + 10)].clone_from_slice(&src[..])` -error: the loop variable `i` is used to index `dst` +error: it looks like you're manually copying between slices --> $DIR/for_loop.rs:472:5 | 472 | / for i in 0..src.len() { 473 | | dst[i] = src[i + 10]; 474 | | } - | |_____^ - | -help: consider using an iterator - | -472 | for (i, ) in dst.iter().enumerate().take(src.len()) { - | ^^^^^^^^^^^ + | |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[10..])` -error: the loop variable `i` is used to index `dst` +error: it looks like you're manually copying between slices --> $DIR/for_loop.rs:477:5 | 477 | / for i in 11..src.len() { 478 | | dst[i] = src[i - 10]; 479 | | } - | |_____^ - | -help: consider using an iterator + | |_____^ help: try replacing the loop by: `dst[11..src.len()].clone_from_slice(&src[(11 - 10)..(src.len() - 10)])` + +error: it looks like you're manually copying between slices + --> $DIR/for_loop.rs:482:5 | -477 | for (i, ) in dst.iter().enumerate().take(src.len()).skip(11) { - | ^^^^^^^^^^^ +482 | / for i in 0..dst.len() { +483 | | dst[i] = src[i]; +484 | | } + | |_____^ help: try replacing the loop by: `dst.clone_from_slice(&src[..dst.len()])` -error: the loop variable `i` is used to index `src` - --> $DIR/for_loop.rs:512:5 +error: it looks like you're manually copying between slices + --> $DIR/for_loop.rs:495:5 | -512 | / for i in 0..10 { -513 | | dst[i + i] = src[i]; -514 | | } +495 | / for i in 10..256 { +496 | | dst[i] = src[i - 5]; +497 | | dst2[i + 500] = src[i] +498 | | } | |_____^ | -help: consider using an iterator +help: try replacing the loop by | -512 | for (i, ) in src.iter().enumerate().take(10) { - | ^^^^^^^^^^^ +495 | dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]) +496 | dst2[(10 + 500)..(256 + 500)].clone_from_slice(&src[10..256]) + | + +error: it looks like you're manually copying between slices + --> $DIR/for_loop.rs:507:5 + | +507 | / for i in 10..LOOP_OFFSET { +508 | | dst[i + LOOP_OFFSET] = src[i - some_var]; +509 | | } + | |_____^ help: try replacing the loop by: `dst[(10 + LOOP_OFFSET)..(LOOP_OFFSET + LOOP_OFFSET)].clone_from_slice(&src[(10 - some_var)..(LOOP_OFFSET - some_var)])` + +error: it looks like you're manually copying between slices + --> $DIR/for_loop.rs:520:5 + | +520 | / for i in 0..src_vec.len() { +521 | | dst_vec[i] = src_vec[i]; +522 | | } + | |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].clone_from_slice(&src_vec[..])` -error: aborting due to 54 previous errors +error: aborting due to 58 previous errors