From ad9ad6f402e3e15706519e59ef111a941d28d5af Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 17:42:57 +1200 Subject: [PATCH 01/14] Don't negate resulted offsets when `offset` is subtraction by 0 --- clippy_lints/src/loops.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index cb44eccae684..f16b98883b80 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -960,8 +960,8 @@ fn detect_manual_memcpy<'a, 'tcx>( 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", _) => x.into(), + ("0", _, x, true) => format!("-{}", x), (x, false, y, false) => format!("({} + {})", x, y), (x, false, y, true) => { if x == y { From 37261a904ce2fbd4137180500c57f75f29945828 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 17:51:01 +1200 Subject: [PATCH 02/14] Print 0 when `end` and `offset` is 0, and also simplify the suggestion --- clippy_lints/src/loops.rs | 15 ++++++++++++--- tests/ui/manual_memcpy.stderr | 4 ++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index f16b98883b80..6b5a8498dc92 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -959,7 +959,7 @@ fn detect_manual_memcpy<'a, 'tcx>( if let PatKind::Binding(_, canonical_id, _, _) = pat.kind { let print_sum = |arg1: &Offset, arg2: &Offset| -> String { match (&arg1.value[..], arg1.negate, &arg2.value[..], arg2.negate) { - ("0", _, "0", _) => "".into(), + ("0", _, "0", _) => "0".into(), ("0", _, x, false) | (x, false, "0", _) => x.into(), ("0", _, x, true) => format!("-{}", x), (x, false, y, false) => format!("({} + {})", x, y), @@ -981,6 +981,15 @@ fn detect_manual_memcpy<'a, 'tcx>( } }; + let print_offset = |start_str: &Offset, inline_offset: &Offset| -> String { + let offset = print_sum(start_str, inline_offset); + if offset.as_str() == "0" { + "".into() + } else { + offset + } + }; + let print_limit = |end: &Option<&Expr<'_>>, offset: Offset, var_name: &str| { if let Some(end) = *end { if_chain! { @@ -1020,9 +1029,9 @@ fn detect_manual_memcpy<'a, 'tcx>( .into_iter() .map(|(dst_var, src_var)| { let start_str = Offset::positive(snippet(cx, start.span, "").to_string()); - let dst_offset = print_sum(&start_str, &dst_var.offset); + let dst_offset = print_offset(&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_offset = print_offset(&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 diff --git a/tests/ui/manual_memcpy.stderr b/tests/ui/manual_memcpy.stderr index 3dbb2155d4de..ec80f6070d62 100644 --- a/tests/ui/manual_memcpy.stderr +++ b/tests/ui/manual_memcpy.stderr @@ -58,13 +58,13 @@ error: it looks like you're manually copying between slices --> $DIR/manual_memcpy.rs:94:14 | LL | for i in from..from + src.len() { - | ^^^^^^^^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + src.len()].clone_from_slice(&src[0..(from + src.len() - from)])` + | ^^^^^^^^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + src.len()].clone_from_slice(&src[..(from + src.len() - from)])` error: it looks like you're manually copying between slices --> $DIR/manual_memcpy.rs:98:14 | LL | for i in from..from + 3 { - | ^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + 3].clone_from_slice(&src[0..(from + 3 - from)])` + | ^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + 3].clone_from_slice(&src[..(from + 3 - from)])` error: it looks like you're manually copying between slices --> $DIR/manual_memcpy.rs:105:14 From 75ad839cd26c1da17fe6ba3aae1153ee96de26c8 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 18:04:37 +1200 Subject: [PATCH 03/14] Do not trigger `manual_memcpy` for `RangeTo` --- clippy_lints/src/loops.rs | 50 ++++++++++++++++------------------- tests/ui/manual_memcpy.rs | 5 ++++ tests/ui/manual_memcpy.stderr | 2 +- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 6b5a8498dc92..ca61c97e3e3f 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -951,7 +951,7 @@ fn detect_manual_memcpy<'a, 'tcx>( ) { if let Some(higher::Range { start: Some(start), - ref end, + end: Some(end), limits, }) = higher::range(cx, arg) { @@ -990,35 +990,31 @@ fn detect_manual_memcpy<'a, 'tcx>( } }; - let print_limit = |end: &Option<&Expr<'_>>, offset: Offset, var_name: &str| { - if let Some(end) = *end { - if_chain! { - if let ExprKind::MethodCall(ref method, _, ref len_args) = end.kind; - if method.ident.name == sym!(len); - if len_args.len() == 1; - if let Some(arg) = len_args.get(0); - if snippet(cx, arg.span, "??") == var_name; - then { - return if offset.negate { - format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value) - } else { - String::new() - }; - } + let print_limit = |end: &Expr<'_>, offset: Offset, var_name: &str| { + if_chain! { + if let ExprKind::MethodCall(ref method, _, ref len_args) = end.kind; + if method.ident.name == sym!(len); + if len_args.len() == 1; + if let Some(arg) = len_args.get(0); + if snippet(cx, arg.span, "??") == var_name; + then { + return if offset.negate { + format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value) + } else { + String::new() + }; } + } - 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, "..")), - }; + 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() - } + print_sum(&Offset::positive(end_str), &offset) }; // The only statements in the for loops can be indexed assignments from diff --git a/tests/ui/manual_memcpy.rs b/tests/ui/manual_memcpy.rs index aa347288875d..1f41838fa169 100644 --- a/tests/ui/manual_memcpy.rs +++ b/tests/ui/manual_memcpy.rs @@ -98,6 +98,11 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) { for i in from..from + 3 { dst[i] = src[i - from]; } + + // `RangeTo` `for` loop - don't trigger lint + for i in 0.. { + dst[i] = src[i]; + } } #[warn(clippy::needless_range_loop, clippy::manual_memcpy)] diff --git a/tests/ui/manual_memcpy.stderr b/tests/ui/manual_memcpy.stderr index ec80f6070d62..95114c46f368 100644 --- a/tests/ui/manual_memcpy.stderr +++ b/tests/ui/manual_memcpy.stderr @@ -67,7 +67,7 @@ LL | for i in from..from + 3 { | ^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + 3].clone_from_slice(&src[..(from + 3 - from)])` error: it looks like you're manually copying between slices - --> $DIR/manual_memcpy.rs:105:14 + --> $DIR/manual_memcpy.rs:110:14 | LL | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` From c94f0f49f8e025aae11534f9f2b4c59c34b1edb8 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 18:22:10 +1200 Subject: [PATCH 04/14] Remove all `ref` keyword --- clippy_lints/src/loops.rs | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index ca61c97e3e3f..502bd42214ee 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -772,8 +772,8 @@ fn check_for_loop<'a, 'tcx>( fn same_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, var: HirId) -> bool { if_chain! { - if let ExprKind::Path(ref qpath) = expr.kind; - if let QPath::Resolved(None, ref path) = *qpath; + if let ExprKind::Path(qpath) = &expr.kind; + if let QPath::Resolved(None, path) = qpath; if path.segments.len() == 1; if let Res::Local(local_id) = qpath_res(cx, qpath, expr.hir_id); // our variable! @@ -821,8 +821,8 @@ 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: HirId) -> Option { fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr<'_>, var: HirId) -> Option { - match e.kind { - ExprKind::Lit(ref l) => match l.node { + match &e.kind { + ExprKind::Lit(l) => match l.node { ast::LitKind::Int(x, _ty) => Some(x.to_string()), _ => None, }, @@ -831,14 +831,14 @@ fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, v } } - if let ExprKind::Index(ref seqexpr, ref idx) = expr.kind { + if let ExprKind::Index(seqexpr, idx) = expr.kind { let ty = cx.tables.expr_ty(seqexpr); if !is_slice_like(cx, ty) { return None; } let offset = match idx.kind { - ExprKind::Binary(op, ref lhs, ref rhs) => match op.node { + ExprKind::Binary(op, lhs, rhs) => match op.node { BinOpKind::Add => { let offset_opt = if same_var(cx, lhs, var) { extract_offset(cx, rhs, var) @@ -878,7 +878,7 @@ fn fetch_cloned_fixed_offset_var<'a, 'tcx>( var: HirId, ) -> Option { if_chain! { - if let ExprKind::MethodCall(ref method, _, ref args) = expr.kind; + if let ExprKind::MethodCall(method, _, args) = expr.kind; if method.ident.name == sym!(clone); if args.len() == 1; if let Some(arg) = args.get(0); @@ -900,7 +900,7 @@ fn get_indexed_assignments<'a, 'tcx>( e: &Expr<'_>, var: HirId, ) -> Option<(FixedOffsetVar, FixedOffsetVar)> { - if let ExprKind::Assign(ref lhs, ref rhs, _) = e.kind { + if let ExprKind::Assign(lhs, rhs, _) = e.kind { match ( get_fixed_offset_var(cx, lhs, var), fetch_cloned_fixed_offset_var(cx, rhs, var), @@ -920,16 +920,14 @@ fn get_indexed_assignments<'a, 'tcx>( } } - if let ExprKind::Block(ref b, _) = body.kind { - let Block { - ref stmts, ref expr, .. - } = **b; + if let ExprKind::Block(b, _) = body.kind { + let Block { stmts, expr, .. } = *b; stmts .iter() .map(|stmt| match stmt.kind { StmtKind::Local(..) | StmtKind::Item(..) => None, - StmtKind::Expr(ref e) | StmtKind::Semi(ref e) => Some(get_assignment(cx, e, var)), + StmtKind::Expr(e) | StmtKind::Semi(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) @@ -992,7 +990,7 @@ fn detect_manual_memcpy<'a, 'tcx>( let print_limit = |end: &Expr<'_>, offset: Offset, var_name: &str| { if_chain! { - if let ExprKind::MethodCall(ref method, _, ref len_args) = end.kind; + if let ExprKind::MethodCall(method, _, len_args) = end.kind; if method.ident.name == sym!(len); if len_args.len() == 1; if let Some(arg) = len_args.get(0); From 7dd0f3459f558c1b557223a042f549b378cacae9 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 18:47:24 +1200 Subject: [PATCH 05/14] Refactor `if` to use `else` and iterator combinators --- clippy_lints/src/loops.rs | 50 +++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 502bd42214ee..3dd3a79b2873 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -779,11 +779,11 @@ fn same_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, var: HirId) - // our variable! if local_id == var; then { - return true; + true + } else { + false } } - - false } struct Offset { @@ -853,13 +853,7 @@ fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, v BinOpKind::Sub if same_var(cx, lhs, var) => extract_offset(cx, rhs, var).map(Offset::negative), _ => None, }, - ExprKind::Path(..) => { - if same_var(cx, idx, var) { - Some(Offset::positive("0".into())) - } else { - None - } - }, + ExprKind::Path(..) if same_var(cx, idx, var) => Some(Offset::positive("0".into())), _ => None, }; @@ -883,11 +877,11 @@ fn fetch_cloned_fixed_offset_var<'a, 'tcx>( if args.len() == 1; if let Some(arg) = args.get(0); then { - return get_fixed_offset_var(cx, arg, var); + get_fixed_offset_var(cx, arg, var) + } else { + get_fixed_offset_var(cx, expr, var) } } - - get_fixed_offset_var(cx, expr, var) } fn get_indexed_assignments<'a, 'tcx>( @@ -925,12 +919,12 @@ fn get_indexed_assignments<'a, 'tcx>( stmts .iter() - .map(|stmt| match stmt.kind { + .filter_map(|stmt| match stmt.kind { StmtKind::Local(..) | StmtKind::Item(..) => None, - StmtKind::Expr(e) | StmtKind::Semi(e) => Some(get_assignment(cx, e, var)), + StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e), }) - .chain(expr.as_ref().into_iter().map(|e| Some(get_assignment(cx, &*e, var)))) - .filter_map(|op| op) + .chain(expr.into_iter()) + .map(|op| get_assignment(cx, op, var)) .collect::>>() .unwrap_or_default() } else { @@ -996,23 +990,23 @@ fn detect_manual_memcpy<'a, 'tcx>( if let Some(arg) = len_args.get(0); if snippet(cx, arg.span, "??") == var_name; then { - return if offset.negate { + if offset.negate { format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value) } else { String::new() + } + } else { + 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) } } - - 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) }; // The only statements in the for loops can be indexed assignments from From 3f1e51b3f4a7bfb42c442caf2cb836ba62e2ba53 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 18:57:36 +1200 Subject: [PATCH 06/14] Rename `negate` to `sign` and make it strong types then make `art1` &str --- clippy_lints/src/loops.rs | 56 +++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 3dd3a79b2873..321d5265d0c2 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -786,20 +786,29 @@ fn same_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, var: HirId) - } } +#[derive(Clone, Copy)] +enum OffsetSign { + Positive, + Negative, +} + struct Offset { value: String, - negate: bool, + sign: OffsetSign, } impl Offset { - fn negative(s: String) -> Self { - Self { value: s, negate: true } + fn negative(value: String) -> Self { + Self { + value, + sign: OffsetSign::Negative, + } } - fn positive(s: String) -> Self { + fn positive(value: String) -> Self { Self { - value: s, - negate: false, + value, + sign: OffsetSign::Positive, } } } @@ -949,31 +958,23 @@ fn detect_manual_memcpy<'a, 'tcx>( { // the var must be a single name if let PatKind::Binding(_, canonical_id, _, _) = pat.kind { - let print_sum = |arg1: &Offset, arg2: &Offset| -> String { - match (&arg1.value[..], arg1.negate, &arg2.value[..], arg2.negate) { - ("0", _, "0", _) => "0".into(), - ("0", _, x, false) | (x, false, "0", _) => x.into(), - ("0", _, x, true) => format!("-{}", x), - (x, false, y, false) => format!("({} + {})", x, y), - (x, false, y, true) => { + let print_sum = |arg1: &str, arg2: &Offset| -> String { + match (arg1, &arg2.value[..], arg2.sign) { + ("0", "0", _) => "0".into(), + ("0", x, OffsetSign::Positive) | (x, "0", _) => x.into(), + ("0", x, OffsetSign::Negative) => format!("-{}", x), + (x, y, OffsetSign::Positive) => format!("({} + {})", x, y), + (x, y, OffsetSign::Negative) => { if x == y { "0".into() } else { format!("({} - {})", x, y) } }, - (x, true, y, false) => { - if x == y { - "0".into() - } else { - format!("({} - {})", y, x) - } - }, - (x, true, y, true) => format!("-({} + {})", x, y), } }; - let print_offset = |start_str: &Offset, inline_offset: &Offset| -> String { + let print_offset = |start_str: &str, inline_offset: &Offset| -> String { let offset = print_sum(start_str, inline_offset); if offset.as_str() == "0" { "".into() @@ -990,10 +991,9 @@ fn detect_manual_memcpy<'a, 'tcx>( if let Some(arg) = len_args.get(0); if snippet(cx, arg.span, "??") == var_name; then { - if offset.negate { - format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value) - } else { - String::new() + match offset.sign { + OffsetSign::Negative => format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value), + OffsetSign::Positive => "".into(), } } else { let end_str = match limits { @@ -1004,7 +1004,7 @@ fn detect_manual_memcpy<'a, 'tcx>( ast::RangeLimits::HalfOpen => format!("{}", snippet(cx, end.span, "..")), }; - print_sum(&Offset::positive(end_str), &offset) + print_sum(&end_str, &offset) } } }; @@ -1016,7 +1016,7 @@ fn detect_manual_memcpy<'a, 'tcx>( let big_sugg = manual_copies .into_iter() .map(|(dst_var, src_var)| { - let start_str = Offset::positive(snippet(cx, start.span, "").to_string()); + let start_str = snippet(cx, start.span, "").to_string(); let dst_offset = print_offset(&start_str, &dst_var.offset); let dst_limit = print_limit(end, dst_var.offset, &dst_var.var_name); let src_offset = print_offset(&start_str, &src_var.offset); From ecb472c052c746d87ce26f6b184f2c5f11537e0c Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 19:02:08 +1200 Subject: [PATCH 07/14] Use `fn` instead of closures where unnecessary --- clippy_lints/src/loops.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 321d5265d0c2..e37c44dc026a 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -958,7 +958,7 @@ fn detect_manual_memcpy<'a, 'tcx>( { // the var must be a single name if let PatKind::Binding(_, canonical_id, _, _) = pat.kind { - let print_sum = |arg1: &str, arg2: &Offset| -> String { + fn print_sum(arg1: &str, arg2: &Offset) -> String { match (arg1, &arg2.value[..], arg2.sign) { ("0", "0", _) => "0".into(), ("0", x, OffsetSign::Positive) | (x, "0", _) => x.into(), @@ -972,16 +972,16 @@ fn detect_manual_memcpy<'a, 'tcx>( } }, } - }; + } - let print_offset = |start_str: &str, inline_offset: &Offset| -> String { + fn print_offset(start_str: &str, inline_offset: &Offset) -> String { let offset = print_sum(start_str, inline_offset); if offset.as_str() == "0" { "".into() } else { offset } - }; + } let print_limit = |end: &Expr<'_>, offset: Offset, var_name: &str| { if_chain! { From aab80eedf3e271ada92a6509727461cc3aa6bb12 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 19:04:56 +1200 Subject: [PATCH 08/14] Extract `get_fixed_offset_var`` from `fetch_cloned_fixed_offset_var` --- clippy_lints/src/loops.rs | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index e37c44dc026a..2dc95f53078e 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -828,6 +828,16 @@ fn is_slice_like<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'_>) -> bool { is_slice || is_type_diagnostic_item(cx, ty, sym!(vec_type)) || is_type_diagnostic_item(cx, ty, sym!(vecdeque_type)) } +fn fetch_cloned_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { + if_chain! { + if let ExprKind::MethodCall(method, _, args) = expr.kind; + if method.ident.name == sym!(clone); + if args.len() == 1; + if let Some(arg) = args.get(0); + then { arg } else { expr } + } +} + fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, var: HirId) -> Option { fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr<'_>, var: HirId) -> Option { match &e.kind { @@ -875,24 +885,6 @@ fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, v } } -fn fetch_cloned_fixed_offset_var<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - expr: &Expr<'_>, - var: HirId, -) -> Option { - if_chain! { - if let ExprKind::MethodCall(method, _, args) = expr.kind; - if method.ident.name == sym!(clone); - if args.len() == 1; - if let Some(arg) = args.get(0); - then { - get_fixed_offset_var(cx, arg, var) - } else { - get_fixed_offset_var(cx, expr, var) - } - } -} - fn get_indexed_assignments<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, body: &Expr<'_>, @@ -906,7 +898,7 @@ fn get_indexed_assignments<'a, 'tcx>( if let ExprKind::Assign(lhs, rhs, _) = e.kind { match ( get_fixed_offset_var(cx, lhs, var), - fetch_cloned_fixed_offset_var(cx, rhs, var), + get_fixed_offset_var(cx, fetch_cloned_expr(rhs), var), ) { (Some(offset_left), Some(offset_right)) => { // Source and destination must be different From 3d121d53af9a73ba11226715cd8132f6981ffee9 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 19:15:51 +1200 Subject: [PATCH 09/14] Extract roles getting indexes from `get_indexed_assignments` --- clippy_lints/src/loops.rs | 106 ++++++++++++++++++++------------------ 1 file changed, 55 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 2dc95f53078e..0753b23e45b3 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -10,7 +10,6 @@ use crate::utils::{ }; use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sext, sugg}; use if_chain::if_chain; -use itertools::Itertools; use rustc_ast::ast; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; @@ -885,52 +884,39 @@ fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, v } } -fn get_indexed_assignments<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - body: &Expr<'_>, - var: HirId, -) -> Vec<(FixedOffsetVar, FixedOffsetVar)> { - fn get_assignment<'a, 'tcx>( - cx: &LateContext<'a, 'tcx>, - e: &Expr<'_>, - var: HirId, - ) -> Option<(FixedOffsetVar, FixedOffsetVar)> { +fn get_assignments<'a, 'tcx>( + body: &'tcx Expr<'tcx>, +) -> impl Iterator, &'tcx Expr<'tcx>)>> { + fn get_assignment<'a, 'tcx>(e: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> { if let ExprKind::Assign(lhs, rhs, _) = e.kind { - match ( - get_fixed_offset_var(cx, lhs, var), - get_fixed_offset_var(cx, fetch_cloned_expr(rhs), var), - ) { - (Some(offset_left), Some(offset_right)) => { - // Source and destination must be different - if offset_left.var_name == offset_right.var_name { - None - } else { - Some((offset_left, offset_right)) - } - }, - _ => None, - } + Some((lhs, rhs)) } else { None } } + // This is one of few ways to return different iterators + // derived from: https://stackoverflow.com/questions/29760668/conditionally-iterate-over-one-of-several-possible-iterators/52064434#52064434 + let mut iter_a = None; + let mut iter_b = None; + if let ExprKind::Block(b, _) = body.kind { let Block { stmts, expr, .. } = *b; - stmts + iter_a = stmts .iter() .filter_map(|stmt| match stmt.kind { StmtKind::Local(..) | StmtKind::Item(..) => None, StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e), }) .chain(expr.into_iter()) - .map(|op| get_assignment(cx, op, var)) - .collect::>>() - .unwrap_or_default() + .map(get_assignment) + .into() } else { - get_assignment(cx, body, var).into_iter().collect() + iter_b = Some(get_assignment(body)) } + + iter_a.into_iter().flatten().chain(iter_b.into_iter()) } /// Checks for for loops that sequentially copy items from one slice-like @@ -1003,30 +989,48 @@ fn detect_manual_memcpy<'a, 'tcx>( // The only statements in the for loops can be indexed assignments from // indexed retrievals. - let manual_copies = get_indexed_assignments(cx, body, canonical_id); - - let big_sugg = manual_copies - .into_iter() - .map(|(dst_var, src_var)| { - let start_str = snippet(cx, start.span, "").to_string(); - let dst_offset = print_offset(&start_str, &dst_var.offset); - let dst_limit = print_limit(end, dst_var.offset, &dst_var.var_name); - let src_offset = print_offset(&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) - }; + let big_sugg = get_assignments(body) + .map(|o| { + o.and_then(|(lhs, rhs)| { + let rhs = fetch_cloned_expr(rhs); + if_chain! { + if let Some(offset_left) = get_fixed_offset_var(cx, lhs, canonical_id); + if let Some(offset_right) = get_fixed_offset_var(cx, rhs, canonical_id); + + // Source and destination must be different + if offset_left.var_name != offset_right.var_name; + then { + Some((offset_left, offset_right)) + } else { + return None + } + } + }) + }) + .map(|o| { + o.map(|(dst_var, src_var)| { + let start_str = snippet(cx, start.span, "").to_string(); + let dst_offset = print_offset(&start_str, &dst_var.offset); + let dst_limit = print_limit(end, dst_var.offset, &dst_var.var_name); + let src_offset = print_offset(&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 - ) + format!( + "{}.clone_from_slice(&{}[{}..{}])", + dst, src_var.var_name, src_offset, src_limit + ) + }) }) - .join("\n "); + .collect::>>() + .filter(|v| !v.is_empty()) + .map(|v| v.join("\n ")); - if !big_sugg.is_empty() { + if let Some(big_sugg) = big_sugg { span_lint_and_sugg( cx, MANUAL_MEMCPY, From 4f2617c059f693ec72e5d31ad31fd85eba019ab1 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 19:26:00 +1200 Subject: [PATCH 10/14] Separate getting offsets and getting index expressions --- clippy_lints/src/loops.rs | 63 +++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 0753b23e45b3..75955997af24 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -837,7 +837,7 @@ fn fetch_cloned_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { } } -fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, var: HirId) -> Option { +fn get_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, idx: &Expr<'_>, var: HirId) -> Option { fn extract_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, e: &Expr<'_>, var: HirId) -> Option { match &e.kind { ExprKind::Lit(l) => match l.node { @@ -849,38 +849,24 @@ fn get_fixed_offset_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, v } } - if let ExprKind::Index(seqexpr, idx) = expr.kind { - let ty = cx.tables.expr_ty(seqexpr); - if !is_slice_like(cx, ty) { - return None; - } - - let offset = match idx.kind { - ExprKind::Binary(op, lhs, rhs) => match op.node { - BinOpKind::Add => { - 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 - }; + match idx.kind { + ExprKind::Binary(op, lhs, rhs) => match op.node { + BinOpKind::Add => { + 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) - }, - BinOpKind::Sub if same_var(cx, lhs, var) => extract_offset(cx, rhs, var).map(Offset::negative), - _ => None, + offset_opt.map(Offset::positive) }, - ExprKind::Path(..) if same_var(cx, idx, var) => Some(Offset::positive("0".into())), + BinOpKind::Sub if same_var(cx, lhs, var) => extract_offset(cx, rhs, var).map(Offset::negative), _ => None, - }; - - offset.map(|o| FixedOffsetVar { - var_name: snippet_opt(cx, seqexpr.span).unwrap_or_else(|| "???".into()), - offset: o, - }) - } else { - None + }, + ExprKind::Path(..) if same_var(cx, idx, var) => Some(Offset::positive("0".into())), + _ => None, } } @@ -994,15 +980,22 @@ fn detect_manual_memcpy<'a, 'tcx>( o.and_then(|(lhs, rhs)| { let rhs = fetch_cloned_expr(rhs); if_chain! { - if let Some(offset_left) = get_fixed_offset_var(cx, lhs, canonical_id); - if let Some(offset_right) = get_fixed_offset_var(cx, rhs, canonical_id); + if let ExprKind::Index(seqexpr_left, idx_left) = lhs.kind; + if let ExprKind::Index(seqexpr_right, idx_right) = rhs.kind; + if is_slice_like(cx, cx.tables.expr_ty(seqexpr_left)) + && is_slice_like(cx, cx.tables.expr_ty(seqexpr_right)); + if let Some(offset_left) = get_offset(cx, &idx_left, canonical_id); + if let Some(offset_right) = get_offset(cx, &idx_right, canonical_id); + let var_name_left = snippet_opt(cx, seqexpr_left.span).unwrap_or_else(|| "???".into()); + let var_name_right = snippet_opt(cx, seqexpr_right.span).unwrap_or_else(|| "???".into()); // Source and destination must be different - if offset_left.var_name != offset_right.var_name; + if var_name_left != var_name_right; then { - Some((offset_left, offset_right)) + Some((FixedOffsetVar { var_name: var_name_left, offset: offset_left }, + FixedOffsetVar { var_name: var_name_right, offset: offset_right })) } else { - return None + None } } }) From 9fc6f37778789de94caa280f41afdf651bf5ae10 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 19:34:41 +1200 Subject: [PATCH 11/14] Delay getting the snippet from slices --- clippy_lints/src/loops.rs | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 75955997af24..8ab355566706 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -812,8 +812,8 @@ impl Offset { } } -struct FixedOffsetVar { - var_name: String, +struct FixedOffsetVar<'hir> { + var: &'hir Expr<'hir>, offset: Offset, } @@ -947,13 +947,13 @@ fn detect_manual_memcpy<'a, 'tcx>( } } - let print_limit = |end: &Expr<'_>, offset: Offset, var_name: &str| { + let print_limit = |end: &Expr<'_>, offset: Offset, var: &Expr<'_>| { if_chain! { if let ExprKind::MethodCall(method, _, len_args) = end.kind; if method.ident.name == sym!(len); if len_args.len() == 1; if let Some(arg) = len_args.get(0); - if snippet(cx, arg.span, "??") == var_name; + if var_def_id(cx, arg) == var_def_id(cx, var); then { match offset.sign { OffsetSign::Negative => format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value), @@ -986,14 +986,12 @@ fn detect_manual_memcpy<'a, 'tcx>( && is_slice_like(cx, cx.tables.expr_ty(seqexpr_right)); if let Some(offset_left) = get_offset(cx, &idx_left, canonical_id); if let Some(offset_right) = get_offset(cx, &idx_right, canonical_id); - let var_name_left = snippet_opt(cx, seqexpr_left.span).unwrap_or_else(|| "???".into()); - let var_name_right = snippet_opt(cx, seqexpr_right.span).unwrap_or_else(|| "???".into()); // Source and destination must be different - if var_name_left != var_name_right; + if var_def_id(cx, seqexpr_left) != var_def_id(cx, seqexpr_right); then { - Some((FixedOffsetVar { var_name: var_name_left, offset: offset_left }, - FixedOffsetVar { var_name: var_name_right, offset: offset_right })) + Some((FixedOffsetVar { var: seqexpr_left, offset: offset_left }, + FixedOffsetVar { var: seqexpr_right, offset: offset_right })) } else { None } @@ -1004,18 +1002,22 @@ fn detect_manual_memcpy<'a, 'tcx>( o.map(|(dst_var, src_var)| { let start_str = snippet(cx, start.span, "").to_string(); let dst_offset = print_offset(&start_str, &dst_var.offset); - let dst_limit = print_limit(end, dst_var.offset, &dst_var.var_name); + let dst_limit = print_limit(end, dst_var.offset, dst_var.var); let src_offset = print_offset(&start_str, &src_var.offset); - let src_limit = print_limit(end, src_var.offset, &src_var.var_name); + let src_limit = print_limit(end, src_var.offset, src_var.var); + + let dst_var_name = snippet_opt(cx, dst_var.var.span).unwrap_or_else(|| "???".into()); + let src_var_name = snippet_opt(cx, src_var.var.span).unwrap_or_else(|| "???".into()); + let dst = if dst_offset == "" && dst_limit == "" { - dst_var.var_name + dst_var_name } else { - format!("{}[{}..{}]", dst_var.var_name, dst_offset, dst_limit) + format!("{}[{}..{}]", dst_var_name, dst_offset, dst_limit) }; format!( "{}.clone_from_slice(&{}[{}..{}])", - dst, src_var.var_name, src_offset, src_limit + dst, src_var_name, src_offset, src_limit ) }) }) From 582614fbbe76fed1b06feb640229b71a1886ffd7 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 19:44:44 +1200 Subject: [PATCH 12/14] Extract building the suggestion of `manual_memcpy` --- clippy_lints/src/loops.rs | 154 ++++++++++++++++++++------------------ 1 file changed, 80 insertions(+), 74 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8ab355566706..7cf3e16bef9b 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -905,6 +905,85 @@ fn get_assignments<'a, 'tcx>( iter_a.into_iter().flatten().chain(iter_b.into_iter()) } +fn build_manual_memcpy_suggestion<'a, 'tcx>( + cx: &LateContext<'a, 'tcx>, + start: &Expr<'_>, + end: &Expr<'_>, + limits: ast::RangeLimits, + dst_var: FixedOffsetVar<'_>, + src_var: FixedOffsetVar<'_>, +) -> String { + fn print_sum(arg1: &str, arg2: &Offset) -> String { + match (arg1, &arg2.value[..], arg2.sign) { + ("0", "0", _) => "0".into(), + ("0", x, OffsetSign::Positive) | (x, "0", _) => x.into(), + ("0", x, OffsetSign::Negative) => format!("-{}", x), + (x, y, OffsetSign::Positive) => format!("({} + {})", x, y), + (x, y, OffsetSign::Negative) => { + if x == y { + "0".into() + } else { + format!("({} - {})", x, y) + } + }, + } + } + + fn print_offset(start_str: &str, inline_offset: &Offset) -> String { + let offset = print_sum(start_str, inline_offset); + if offset.as_str() == "0" { + "".into() + } else { + offset + } + } + + let print_limit = |end: &Expr<'_>, offset: Offset, var: &Expr<'_>| { + if_chain! { + if let ExprKind::MethodCall(method, _, len_args) = end.kind; + if method.ident.name == sym!(len); + if len_args.len() == 1; + if let Some(arg) = len_args.get(0); + if var_def_id(cx, arg) == var_def_id(cx, var); + then { + match offset.sign { + OffsetSign::Negative => format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value), + OffsetSign::Positive => "".into(), + } + } else { + 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(&end_str, &offset) + } + } + }; + + let start_str = snippet(cx, start.span, "").to_string(); + let dst_offset = print_offset(&start_str, &dst_var.offset); + let dst_limit = print_limit(end, dst_var.offset, dst_var.var); + let src_offset = print_offset(&start_str, &src_var.offset); + let src_limit = print_limit(end, src_var.offset, src_var.var); + + let dst_var_name = snippet_opt(cx, dst_var.var.span).unwrap_or_else(|| "???".into()); + let src_var_name = snippet_opt(cx, src_var.var.span).unwrap_or_else(|| "???".into()); + + let dst = if dst_offset == "" && dst_limit == "" { + dst_var_name + } else { + format!("{}[{}..{}]", dst_var_name, dst_offset, dst_limit) + }; + + format!( + "{}.clone_from_slice(&{}[{}..{}])", + dst, src_var_name, src_offset, src_limit + ) +} /// Checks for for loops that sequentially copy items from one slice-like /// object to another. fn detect_manual_memcpy<'a, 'tcx>( @@ -922,57 +1001,6 @@ fn detect_manual_memcpy<'a, 'tcx>( { // the var must be a single name if let PatKind::Binding(_, canonical_id, _, _) = pat.kind { - fn print_sum(arg1: &str, arg2: &Offset) -> String { - match (arg1, &arg2.value[..], arg2.sign) { - ("0", "0", _) => "0".into(), - ("0", x, OffsetSign::Positive) | (x, "0", _) => x.into(), - ("0", x, OffsetSign::Negative) => format!("-{}", x), - (x, y, OffsetSign::Positive) => format!("({} + {})", x, y), - (x, y, OffsetSign::Negative) => { - if x == y { - "0".into() - } else { - format!("({} - {})", x, y) - } - }, - } - } - - fn print_offset(start_str: &str, inline_offset: &Offset) -> String { - let offset = print_sum(start_str, inline_offset); - if offset.as_str() == "0" { - "".into() - } else { - offset - } - } - - let print_limit = |end: &Expr<'_>, offset: Offset, var: &Expr<'_>| { - if_chain! { - if let ExprKind::MethodCall(method, _, len_args) = end.kind; - if method.ident.name == sym!(len); - if len_args.len() == 1; - if let Some(arg) = len_args.get(0); - if var_def_id(cx, arg) == var_def_id(cx, var); - then { - match offset.sign { - OffsetSign::Negative => format!("({} - {})", snippet(cx, end.span, ".len()"), offset.value), - OffsetSign::Positive => "".into(), - } - } else { - 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(&end_str, &offset) - } - } - }; - // The only statements in the for loops can be indexed assignments from // indexed retrievals. let big_sugg = get_assignments(body) @@ -998,29 +1026,7 @@ fn detect_manual_memcpy<'a, 'tcx>( } }) }) - .map(|o| { - o.map(|(dst_var, src_var)| { - let start_str = snippet(cx, start.span, "").to_string(); - let dst_offset = print_offset(&start_str, &dst_var.offset); - let dst_limit = print_limit(end, dst_var.offset, dst_var.var); - let src_offset = print_offset(&start_str, &src_var.offset); - let src_limit = print_limit(end, src_var.offset, src_var.var); - - let dst_var_name = snippet_opt(cx, dst_var.var.span).unwrap_or_else(|| "???".into()); - let src_var_name = snippet_opt(cx, src_var.var.span).unwrap_or_else(|| "???".into()); - - let dst = if dst_offset == "" && dst_limit == "" { - dst_var_name - } else { - format!("{}[{}..{}]", dst_var_name, dst_offset, dst_limit) - }; - - format!( - "{}.clone_from_slice(&{}[{}..{}])", - dst, src_var_name, src_offset, src_limit - ) - }) - }) + .map(|o| o.map(|(dst, src)| build_manual_memcpy_suggestion(cx, start, end, limits, dst, src))) .collect::>>() .filter(|v| !v.is_empty()) .map(|v| v.join("\n ")); From 51585a129892f42eb23b0b37fea0e729f6678994 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Mon, 27 Apr 2020 20:37:21 +1200 Subject: [PATCH 13/14] Removed unused lifetimes and a needless bool --- clippy_lints/src/loops.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 7cf3e16bef9b..5f7f08979436 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -775,10 +775,9 @@ fn same_var<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &Expr<'_>, var: HirId) - if let QPath::Resolved(None, path) = qpath; if path.segments.len() == 1; if let Res::Local(local_id) = qpath_res(cx, qpath, expr.hir_id); - // our variable! - if local_id == var; then { - true + // our variable! + local_id == var } else { false } @@ -870,10 +869,8 @@ fn get_offset<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, idx: &Expr<'_>, var: HirId) } } -fn get_assignments<'a, 'tcx>( - body: &'tcx Expr<'tcx>, -) -> impl Iterator, &'tcx Expr<'tcx>)>> { - fn get_assignment<'a, 'tcx>(e: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> { +fn get_assignments<'tcx>(body: &'tcx Expr<'tcx>) -> impl Iterator, &'tcx Expr<'tcx>)>> { + fn get_assignment<'tcx>(e: &'tcx Expr<'tcx>) -> Option<(&'tcx Expr<'tcx>, &'tcx Expr<'tcx>)> { if let ExprKind::Assign(lhs, rhs, _) = e.kind { Some((lhs, rhs)) } else { From 461f4a34660691675434a318ac4fd61a83444428 Mon Sep 17 00:00:00 2001 From: rail <12975677+rail-rain@users.noreply.github.com> Date: Thu, 30 Apr 2020 17:32:37 +1200 Subject: [PATCH 14/14] Add missing tests --- tests/ui/manual_memcpy.rs | 10 ++++++++++ tests/ui/manual_memcpy.stderr | 16 ++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/ui/manual_memcpy.rs b/tests/ui/manual_memcpy.rs index 1f41838fa169..9c24d6d4db1f 100644 --- a/tests/ui/manual_memcpy.rs +++ b/tests/ui/manual_memcpy.rs @@ -99,6 +99,16 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) { dst[i] = src[i - from]; } + #[allow(clippy::identity_op)] + for i in 0..5 { + dst[i - 0] = src[i]; + } + + #[allow(clippy::reverse_range_loop)] + for i in 0..0 { + dst[i] = src[i]; + } + // `RangeTo` `for` loop - don't trigger lint for i in 0.. { dst[i] = src[i]; diff --git a/tests/ui/manual_memcpy.stderr b/tests/ui/manual_memcpy.stderr index 95114c46f368..bad84a589008 100644 --- a/tests/ui/manual_memcpy.stderr +++ b/tests/ui/manual_memcpy.stderr @@ -67,10 +67,22 @@ LL | for i in from..from + 3 { | ^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[from..from + 3].clone_from_slice(&src[..(from + 3 - from)])` error: it looks like you're manually copying between slices - --> $DIR/manual_memcpy.rs:110:14 + --> $DIR/manual_memcpy.rs:103:14 + | +LL | for i in 0..5 { + | ^^^^ help: try replacing the loop by: `dst[..5].clone_from_slice(&src[..5])` + +error: it looks like you're manually copying between slices + --> $DIR/manual_memcpy.rs:108:14 + | +LL | for i in 0..0 { + | ^^^^ help: try replacing the loop by: `dst[..0].clone_from_slice(&src[..0])` + +error: it looks like you're manually copying between slices + --> $DIR/manual_memcpy.rs:120:14 | LL | for i in 0..src.len() { | ^^^^^^^^^^^^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..])` -error: aborting due to 11 previous errors +error: aborting due to 13 previous errors