+use itertools::Itertools;
use reexport::*;
use rustc::hir::*;
use rustc::hir::def::Def;
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.
///
impl LintPass for Pass {
fn get_lints(&self) -> LintArray {
lint_array!(
+ MANUAL_MEMCPY,
NEEDLESS_RANGE_LOOP,
EXPLICIT_ITER_LOOP,
EXPLICIT_INTO_ITER_LOOP,
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<FixedOffsetVar> {
+ fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr, var: DefId) -> Option<String> {
+ 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::<Option<Vec<_>>>()
+ .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, "<src>.len()"), offset.value)
+ } else {
+ "".to_owned()
+ };
+ }}
+
+ let end_str = match limits {
+ ast::RangeLimits::Closed => {
+ let end = sugg::Sugg::hir(cx, end, "<count>");
+ 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.
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> {
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,
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, <item>) 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, <item>) 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, <item>) 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, <item>) 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
projects / rust.git / commitdiff