From 14f34454b04e6fd578ccbeaed99d8d3ad3ba1bc9 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 5 Jan 2022 12:15:10 -0500 Subject: [PATCH 1/3] Don't lint `DequeVec` in `manual_memcpy` --- clippy_lints/src/loops/manual_memcpy.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs index c62fa5e998bd4..0bff868641cc4 100644 --- a/clippy_lints/src/loops/manual_memcpy.rs +++ b/clippy_lints/src/loops/manual_memcpy.rs @@ -2,7 +2,6 @@ use super::{IncrementVisitor, InitializeVisitor, MANUAL_MEMCPY}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; use clippy_utils::sugg::Sugg; -use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::{get_enclosing_block, higher, path_to_local, sugg}; use if_chain::if_chain; use rustc_ast::ast; @@ -325,13 +324,12 @@ struct Start<'hir> { } fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'_>) -> bool { - let is_slice = match ty.kind() { + match ty.kind() { + ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(sym::Vec, adt.did), ty::Ref(_, subty, _) => is_slice_like(cx, subty), ty::Slice(..) | ty::Array(..) => true, _ => false, - }; - - is_slice || is_type_diagnostic_item(cx, ty, sym::Vec) || is_type_diagnostic_item(cx, ty, sym::VecDeque) + } } fn fetch_cloned_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> { From 3925def9cf0daace7e725840d6044ad9d831c40d Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 5 Jan 2022 12:35:35 -0500 Subject: [PATCH 2/3] Suggest `copy_from_slice` for `manual_memcpy` when possible --- clippy_lints/src/loops/manual_memcpy.rs | 29 ++++++++++++------- .../manual_memcpy/with_loop_counters.stderr | 24 +++++++-------- .../without_loop_counters.stderr | 26 ++++++++--------- 3 files changed, 44 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/loops/manual_memcpy.rs b/clippy_lints/src/loops/manual_memcpy.rs index 0bff868641cc4..17302f43c324c 100644 --- a/clippy_lints/src/loops/manual_memcpy.rs +++ b/clippy_lints/src/loops/manual_memcpy.rs @@ -2,6 +2,7 @@ use super::{IncrementVisitor, InitializeVisitor, MANUAL_MEMCPY}; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet; use clippy_utils::sugg::Sugg; +use clippy_utils::ty::is_copy; use clippy_utils::{get_enclosing_block, higher, path_to_local, sugg}; use if_chain::if_chain; use rustc_ast::ast; @@ -61,15 +62,15 @@ pub(super) fn check<'tcx>( if_chain! { if let ExprKind::Index(base_left, idx_left) = lhs.kind; if let ExprKind::Index(base_right, idx_right) = rhs.kind; - if is_slice_like(cx, cx.typeck_results().expr_ty(base_left)); - if is_slice_like(cx, cx.typeck_results().expr_ty(base_right)); + if let Some(ty) = get_slice_like_element_ty(cx, cx.typeck_results().expr_ty(base_left)); + if get_slice_like_element_ty(cx, cx.typeck_results().expr_ty(base_right)).is_some(); if let Some((start_left, offset_left)) = get_details_from_idx(cx, idx_left, &starts); if let Some((start_right, offset_right)) = get_details_from_idx(cx, idx_right, &starts); // Source and destination must be different if path_to_local(base_left) != path_to_local(base_right); then { - Some((IndexExpr { base: base_left, idx: start_left, idx_offset: offset_left }, + Some((ty, IndexExpr { base: base_left, idx: start_left, idx_offset: offset_left }, IndexExpr { base: base_right, idx: start_right, idx_offset: offset_right })) } else { None @@ -77,7 +78,7 @@ pub(super) fn check<'tcx>( } }) }) - .map(|o| o.map(|(dst, src)| build_manual_memcpy_suggestion(cx, start, end, limits, &dst, &src))) + .map(|o| o.map(|(ty, dst, src)| build_manual_memcpy_suggestion(cx, start, end, limits, ty, &dst, &src))) .collect::>>() .filter(|v| !v.is_empty()) .map(|v| v.join("\n ")); @@ -104,6 +105,7 @@ fn build_manual_memcpy_suggestion<'tcx>( start: &Expr<'_>, end: &Expr<'_>, limits: ast::RangeLimits, + elem_ty: Ty<'tcx>, dst: &IndexExpr<'_>, src: &IndexExpr<'_>, ) -> String { @@ -186,9 +188,16 @@ fn build_manual_memcpy_suggestion<'tcx>( .into() }; + let method_str = if is_copy(cx, elem_ty) { + "copy_from_slice" + } else { + "clone_from_slice" + }; + format!( - "{}.clone_from_slice(&{}[{}..{}]);", + "{}.{}(&{}[{}..{}]);", dst, + method_str, src_base_str, src_offset.maybe_par(), src_limit.maybe_par() @@ -323,12 +332,12 @@ struct Start<'hir> { kind: StartKind<'hir>, } -fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'_>) -> bool { +fn get_slice_like_element_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { match ty.kind() { - ty::Adt(adt, _) => cx.tcx.is_diagnostic_item(sym::Vec, adt.did), - ty::Ref(_, subty, _) => is_slice_like(cx, subty), - ty::Slice(..) | ty::Array(..) => true, - _ => false, + ty::Adt(adt, subs) if cx.tcx.is_diagnostic_item(sym::Vec, adt.did) => Some(subs.type_at(0)), + ty::Ref(_, subty, _) => get_slice_like_element_ty(cx, subty), + ty::Slice(ty) | ty::Array(ty, _) => Some(ty), + _ => None, } } diff --git a/tests/ui/manual_memcpy/with_loop_counters.stderr b/tests/ui/manual_memcpy/with_loop_counters.stderr index 2e3ebadd7b5d2..79d40c0bcb1d9 100644 --- a/tests/ui/manual_memcpy/with_loop_counters.stderr +++ b/tests/ui/manual_memcpy/with_loop_counters.stderr @@ -5,7 +5,7 @@ LL | / for i in 3..src.len() { LL | | dst[i] = src[count]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[3..src.len()].clone_from_slice(&src[..(src.len() - 3)]);` + | |_____^ help: try replacing the loop by: `dst[3..src.len()].copy_from_slice(&src[..(src.len() - 3)]);` | = note: `-D clippy::manual-memcpy` implied by `-D warnings` @@ -16,7 +16,7 @@ LL | / for i in 3..src.len() { LL | | dst[count] = src[i]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[..(src.len() - 3)].clone_from_slice(&src[3..]);` + | |_____^ help: try replacing the loop by: `dst[..(src.len() - 3)].copy_from_slice(&src[3..]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:17:5 @@ -25,7 +25,7 @@ LL | / for i in 0..src.len() { LL | | dst[count] = src[i]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[3..(src.len() + 3)].clone_from_slice(&src[..]);` + | |_____^ help: try replacing the loop by: `dst[3..(src.len() + 3)].copy_from_slice(&src[..]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:23:5 @@ -34,7 +34,7 @@ LL | / for i in 0..src.len() { LL | | dst[i] = src[count]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[3..(src.len() + 3)]);` + | |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[3..(src.len() + 3)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:29:5 @@ -43,7 +43,7 @@ LL | / for i in 3..(3 + src.len()) { LL | | dst[i] = src[count]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[3..(3 + src.len())].clone_from_slice(&src[..(3 + src.len() - 3)]);` + | |_____^ help: try replacing the loop by: `dst[3..(3 + src.len())].copy_from_slice(&src[..(3 + src.len() - 3)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:35:5 @@ -52,7 +52,7 @@ LL | / for i in 5..src.len() { LL | | dst[i] = src[count - 2]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[5..src.len()].clone_from_slice(&src[(3 - 2)..((src.len() - 2) + 3 - 5)]);` + | |_____^ help: try replacing the loop by: `dst[5..src.len()].copy_from_slice(&src[(3 - 2)..((src.len() - 2) + 3 - 5)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:41:5 @@ -61,7 +61,7 @@ LL | / for i in 0..dst.len() { LL | | dst[i] = src[count]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst.clone_from_slice(&src[2..(dst.len() + 2)]);` + | |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[2..(dst.len() + 2)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:47:5 @@ -70,7 +70,7 @@ LL | / for i in 3..10 { LL | | dst[i] = src[count]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[3..10].clone_from_slice(&src[5..(10 + 5 - 3)]);` + | |_____^ help: try replacing the loop by: `dst[3..10].copy_from_slice(&src[5..(10 + 5 - 3)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:54:5 @@ -85,8 +85,8 @@ LL | | } | help: try replacing the loop by | -LL ~ dst[3..(src.len() + 3)].clone_from_slice(&src[..]); -LL + dst2[30..(src.len() + 30)].clone_from_slice(&src[..]); +LL ~ dst[3..(src.len() + 3)].copy_from_slice(&src[..]); +LL + dst2[30..(src.len() + 30)].copy_from_slice(&src[..]); | error: it looks like you're manually copying between slices @@ -96,7 +96,7 @@ LL | / for i in 0..1 << 1 { LL | | dst[count] = src[i + 2]; LL | | count += 1; LL | | } - | |_____^ help: try replacing the loop by: `dst[(0 << 1)..((1 << 1) + (0 << 1))].clone_from_slice(&src[2..((1 << 1) + 2)]);` + | |_____^ help: try replacing the loop by: `dst[(0 << 1)..((1 << 1) + (0 << 1))].copy_from_slice(&src[2..((1 << 1) + 2)]);` error: it looks like you're manually copying between slices --> $DIR/with_loop_counters.rs:71:5 @@ -105,7 +105,7 @@ LL | / for i in 3..src.len() { LL | | dst[i] = src[count]; LL | | count += 1 LL | | } - | |_____^ help: try replacing the loop by: `dst[3..src.len()].clone_from_slice(&src[..(src.len() - 3)]);` + | |_____^ help: try replacing the loop by: `dst[3..src.len()].copy_from_slice(&src[..(src.len() - 3)]);` error: aborting due to 11 previous errors diff --git a/tests/ui/manual_memcpy/without_loop_counters.stderr b/tests/ui/manual_memcpy/without_loop_counters.stderr index 8ff0137a8120e..654dee32a1e84 100644 --- a/tests/ui/manual_memcpy/without_loop_counters.stderr +++ b/tests/ui/manual_memcpy/without_loop_counters.stderr @@ -4,7 +4,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..src.len() { LL | | dst[i] = src[i]; LL | | } - | |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..]);` + | |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[..]);` | = note: `-D clippy::manual-memcpy` implied by `-D warnings` @@ -14,7 +14,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..src.len() { LL | | dst[i + 10] = src[i]; LL | | } - | |_____^ help: try replacing the loop by: `dst[10..(src.len() + 10)].clone_from_slice(&src[..]);` + | |_____^ help: try replacing the loop by: `dst[10..(src.len() + 10)].copy_from_slice(&src[..]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:17:5 @@ -22,7 +22,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..src.len() { LL | | dst[i] = src[i + 10]; LL | | } - | |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[10..(src.len() + 10)]);` + | |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[10..(src.len() + 10)]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:22:5 @@ -30,7 +30,7 @@ error: it looks like you're manually copying between slices LL | / for i in 11..src.len() { LL | | dst[i] = src[i - 10]; LL | | } - | |_____^ help: try replacing the loop by: `dst[11..src.len()].clone_from_slice(&src[(11 - 10)..(src.len() - 10)]);` + | |_____^ help: try replacing the loop by: `dst[11..src.len()].copy_from_slice(&src[(11 - 10)..(src.len() - 10)]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:27:5 @@ -38,7 +38,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..dst.len() { LL | | dst[i] = src[i]; LL | | } - | |_____^ help: try replacing the loop by: `dst.clone_from_slice(&src[..dst.len()]);` + | |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[..dst.len()]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:40:5 @@ -51,8 +51,8 @@ LL | | } | help: try replacing the loop by | -LL ~ dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]); -LL + dst2[(10 + 500)..(256 + 500)].clone_from_slice(&src[10..256]); +LL ~ dst[10..256].copy_from_slice(&src[(10 - 5)..(256 - 5)]); +LL + dst2[(10 + 500)..(256 + 500)].copy_from_slice(&src[10..256]); | error: it looks like you're manually copying between slices @@ -61,7 +61,7 @@ error: it looks like you're manually copying between slices LL | / for i in 10..LOOP_OFFSET { LL | | dst[i + LOOP_OFFSET] = src[i - some_var]; LL | | } - | |_____^ 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)]);` + | |_____^ help: try replacing the loop by: `dst[(10 + LOOP_OFFSET)..(LOOP_OFFSET + LOOP_OFFSET)].copy_from_slice(&src[(10 - some_var)..(LOOP_OFFSET - some_var)]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:65:5 @@ -69,7 +69,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..src_vec.len() { LL | | dst_vec[i] = src_vec[i]; LL | | } - | |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].clone_from_slice(&src_vec[..]);` + | |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].copy_from_slice(&src_vec[..]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:94:5 @@ -77,7 +77,7 @@ error: it looks like you're manually copying between slices LL | / for i in from..from + src.len() { LL | | dst[i] = src[i - from]; LL | | } - | |_____^ help: try replacing the loop by: `dst[from..(from + src.len())].clone_from_slice(&src[..(from + src.len() - from)]);` + | |_____^ help: try replacing the loop by: `dst[from..(from + src.len())].copy_from_slice(&src[..(from + src.len() - from)]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:98:5 @@ -85,7 +85,7 @@ error: it looks like you're manually copying between slices LL | / for i in from..from + 3 { LL | | dst[i] = src[i - from]; LL | | } - | |_____^ help: try replacing the loop by: `dst[from..(from + 3)].clone_from_slice(&src[..(from + 3 - from)]);` + | |_____^ help: try replacing the loop by: `dst[from..(from + 3)].copy_from_slice(&src[..(from + 3 - from)]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:103:5 @@ -93,7 +93,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..5 { LL | | dst[i - 0] = src[i]; LL | | } - | |_____^ help: try replacing the loop by: `dst[..5].clone_from_slice(&src[..5]);` + | |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src[..5]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:108:5 @@ -101,7 +101,7 @@ error: it looks like you're manually copying between slices LL | / for i in 0..0 { LL | | dst[i] = src[i]; LL | | } - | |_____^ help: try replacing the loop by: `dst[..0].clone_from_slice(&src[..0]);` + | |_____^ help: try replacing the loop by: `dst[..0].copy_from_slice(&src[..0]);` error: it looks like you're manually copying between slices --> $DIR/without_loop_counters.rs:120:5 From 062db10c12d5a1f75dbda667dd955095cfe16057 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 12 Jan 2022 12:33:47 -0500 Subject: [PATCH 3/3] Add `manual_memcpy_test` for `VecDeque` --- tests/ui/manual_memcpy/without_loop_counters.rs | 11 +++++++++++ tests/ui/manual_memcpy/without_loop_counters.stderr | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/ui/manual_memcpy/without_loop_counters.rs b/tests/ui/manual_memcpy/without_loop_counters.rs index 0083f94798fe4..ea0535d076b81 100644 --- a/tests/ui/manual_memcpy/without_loop_counters.rs +++ b/tests/ui/manual_memcpy/without_loop_counters.rs @@ -113,6 +113,17 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) { for i in 0.. { dst[i] = src[i]; } + + // VecDeque - ideally this would work, but would require something like `range_as_slices` + let mut dst = std::collections::VecDeque::from_iter([0; 5]); + let src = std::collections::VecDeque::from_iter([0, 1, 2, 3, 4]); + for i in 0..dst.len() { + dst[i] = src[i]; + } + let src = vec![0, 1, 2, 3, 4]; + for i in 0..dst.len() { + dst[i] = src[i]; + } } #[warn(clippy::needless_range_loop, clippy::manual_memcpy)] diff --git a/tests/ui/manual_memcpy/without_loop_counters.stderr b/tests/ui/manual_memcpy/without_loop_counters.stderr index 654dee32a1e84..c163ae061dfc9 100644 --- a/tests/ui/manual_memcpy/without_loop_counters.stderr +++ b/tests/ui/manual_memcpy/without_loop_counters.stderr @@ -104,7 +104,7 @@ LL | | } | |_____^ help: try replacing the loop by: `dst[..0].copy_from_slice(&src[..0]);` error: it looks like you're manually copying between slices - --> $DIR/without_loop_counters.rs:120:5 + --> $DIR/without_loop_counters.rs:131:5 | LL | / for i in 0..src.len() { LL | | dst[i] = src[i].clone();