From 14f34454b04e6fd578ccbeaed99d8d3ad3ba1bc9 Mon Sep 17 00:00:00 2001
From: Jason Newcomb <jsnewcomb@pm.me>
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 c62fa5e998b..0bff868641c 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 <jsnewcomb@pm.me>
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 0bff868641c..17302f43c32 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::<Option<Vec<_>>>()
.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<Ty<'tcx>> {
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 2e3ebadd7b5..79d40c0bcb1 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 8ff0137a812..654dee32a1e 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 <jsnewcomb@pm.me>
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 0083f94798f..ea0535d076b 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 654dee32a1e..c163ae061df 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();