Skip to content

Commit 60e68d6

Browse files
borsbors
committed
Auto merge of rust-lang#8226 - Jarcho:manual_memcpy_8160, r=flip1995
`manual_memcpy` fix fixes rust-lang#8160 Ideally this would work with `VecDeque`, but the current interface is unsuitable for it. At a minimum something like `range_as_slices` would be needed. changelog: Don't lint `manual_memcpy` on `VecDeque` changelog: Suggest `copy_from_slice` for `manual_memcpy` when applicable
2 parents 5479024 + 062db10 commit 60e68d6

File tree

4 files changed

+58
-40
lines changed

4 files changed

+58
-40
lines changed

‎clippy_lints/src/loops/manual_memcpy.rs‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use super::{IncrementVisitor, InitializeVisitor, MANUAL_MEMCPY};
22
use clippy_utils::diagnostics::span_lint_and_sugg;
33
use clippy_utils::source::snippet;
44
use clippy_utils::sugg::Sugg;
5-
use clippy_utils::ty::is_type_diagnostic_item;
5+
use clippy_utils::ty::is_copy;
66
use clippy_utils::{get_enclosing_block, higher, path_to_local, sugg};
77
use if_chain::if_chain;
88
use rustc_ast::ast;
@@ -62,23 +62,23 @@ pub(super) fn check<'tcx>(
6262
if_chain! {
6363
if let ExprKind::Index(base_left, idx_left) = lhs.kind;
6464
if let ExprKind::Index(base_right, idx_right) = rhs.kind;
65-
if is_slice_like(cx, cx.typeck_results().expr_ty(base_left));
66-
if is_slice_like(cx, cx.typeck_results().expr_ty(base_right));
65+
if let Some(ty) = get_slice_like_element_ty(cx, cx.typeck_results().expr_ty(base_left));
66+
if get_slice_like_element_ty(cx, cx.typeck_results().expr_ty(base_right)).is_some();
6767
if let Some((start_left, offset_left)) = get_details_from_idx(cx, idx_left, &starts);
6868
if let Some((start_right, offset_right)) = get_details_from_idx(cx, idx_right, &starts);
6969

7070
// Source and destination must be different
7171
if path_to_local(base_left) != path_to_local(base_right);
7272
then {
73-
Some((IndexExpr { base: base_left, idx: start_left, idx_offset: offset_left },
73+
Some((ty, IndexExpr { base: base_left, idx: start_left, idx_offset: offset_left },
7474
IndexExpr { base: base_right, idx: start_right, idx_offset: offset_right }))
7575
} else {
7676
None
7777
}
7878
}
7979
})
8080
})
81-
.map(|o| o.map(|(dst, src)| build_manual_memcpy_suggestion(cx, start, end, limits, &dst, &src)))
81+
.map(|o| o.map(|(ty, dst, src)| build_manual_memcpy_suggestion(cx, start, end, limits, ty, &dst, &src)))
8282
.collect::<Option<Vec<_>>>()
8383
.filter(|v| !v.is_empty())
8484
.map(|v| v.join("\n "));
@@ -105,6 +105,7 @@ fn build_manual_memcpy_suggestion<'tcx>(
105105
start: &Expr<'_>,
106106
end: &Expr<'_>,
107107
limits: ast::RangeLimits,
108+
elem_ty: Ty<'tcx>,
108109
dst: &IndexExpr<'_>,
109110
src: &IndexExpr<'_>,
110111
) -> String {
@@ -187,9 +188,16 @@ fn build_manual_memcpy_suggestion<'tcx>(
187188
.into()
188189
};
189190

191+
let method_str = if is_copy(cx, elem_ty) {
192+
"copy_from_slice"
193+
} else {
194+
"clone_from_slice"
195+
};
196+
190197
format!(
191-
"{}.clone_from_slice(&{}[{}..{}]);",
198+
"{}.{}(&{}[{}..{}]);",
192199
dst,
200+
method_str,
193201
src_base_str,
194202
src_offset.maybe_par(),
195203
src_limit.maybe_par()
@@ -324,14 +332,13 @@ struct Start<'hir> {
324332
kind: StartKind<'hir>,
325333
}
326334

327-
fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'_>) -> bool {
328-
let is_slice = match ty.kind() {
329-
ty::Ref(_, subty, _) => is_slice_like(cx, subty),
330-
ty::Slice(..) | ty::Array(..) => true,
331-
_ => false,
332-
};
333-
334-
is_slice || is_type_diagnostic_item(cx, ty, sym::Vec) || is_type_diagnostic_item(cx, ty, sym::VecDeque)
335+
fn get_slice_like_element_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
336+
match ty.kind() {
337+
ty::Adt(adt, subs) if cx.tcx.is_diagnostic_item(sym::Vec, adt.did) => Some(subs.type_at(0)),
338+
ty::Ref(_, subty, _) => get_slice_like_element_ty(cx, subty),
339+
ty::Slice(ty) | ty::Array(ty, _) => Some(ty),
340+
_ => None,
341+
}
335342
}
336343

337344
fn fetch_cloned_expr<'tcx>(expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {

‎tests/ui/manual_memcpy/with_loop_counters.stderr‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ LL | / for i in 3..src.len() {
55
LL | | dst[i] = src[count];
66
LL | | count += 1;
77
LL | | }
8-
| |_____^ help: try replacing the loop by: `dst[3..src.len()].clone_from_slice(&src[..(src.len() - 3)]);`
8+
| |_____^ help: try replacing the loop by: `dst[3..src.len()].copy_from_slice(&src[..(src.len() - 3)]);`
99
|
1010
= note: `-D clippy::manual-memcpy` implied by `-D warnings`
1111

@@ -16,7 +16,7 @@ LL | / for i in 3..src.len() {
1616
LL | | dst[count] = src[i];
1717
LL | | count += 1;
1818
LL | | }
19-
| |_____^ help: try replacing the loop by: `dst[..(src.len() - 3)].clone_from_slice(&src[3..]);`
19+
| |_____^ help: try replacing the loop by: `dst[..(src.len() - 3)].copy_from_slice(&src[3..]);`
2020

2121
error: it looks like you're manually copying between slices
2222
--> $DIR/with_loop_counters.rs:17:5
@@ -25,7 +25,7 @@ LL | / for i in 0..src.len() {
2525
LL | | dst[count] = src[i];
2626
LL | | count += 1;
2727
LL | | }
28-
| |_____^ help: try replacing the loop by: `dst[3..(src.len() + 3)].clone_from_slice(&src[..]);`
28+
| |_____^ help: try replacing the loop by: `dst[3..(src.len() + 3)].copy_from_slice(&src[..]);`
2929

3030
error: it looks like you're manually copying between slices
3131
--> $DIR/with_loop_counters.rs:23:5
@@ -34,7 +34,7 @@ LL | / for i in 0..src.len() {
3434
LL | | dst[i] = src[count];
3535
LL | | count += 1;
3636
LL | | }
37-
| |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[3..(src.len() + 3)]);`
37+
| |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[3..(src.len() + 3)]);`
3838

3939
error: it looks like you're manually copying between slices
4040
--> $DIR/with_loop_counters.rs:29:5
@@ -43,7 +43,7 @@ LL | / for i in 3..(3 + src.len()) {
4343
LL | | dst[i] = src[count];
4444
LL | | count += 1;
4545
LL | | }
46-
| |_____^ help: try replacing the loop by: `dst[3..(3 + src.len())].clone_from_slice(&src[..(3 + src.len() - 3)]);`
46+
| |_____^ help: try replacing the loop by: `dst[3..(3 + src.len())].copy_from_slice(&src[..(3 + src.len() - 3)]);`
4747

4848
error: it looks like you're manually copying between slices
4949
--> $DIR/with_loop_counters.rs:35:5
@@ -52,7 +52,7 @@ LL | / for i in 5..src.len() {
5252
LL | | dst[i] = src[count - 2];
5353
LL | | count += 1;
5454
LL | | }
55-
| |_____^ help: try replacing the loop by: `dst[5..src.len()].clone_from_slice(&src[(3 - 2)..((src.len() - 2) + 3 - 5)]);`
55+
| |_____^ help: try replacing the loop by: `dst[5..src.len()].copy_from_slice(&src[(3 - 2)..((src.len() - 2) + 3 - 5)]);`
5656

5757
error: it looks like you're manually copying between slices
5858
--> $DIR/with_loop_counters.rs:41:5
@@ -61,7 +61,7 @@ LL | / for i in 0..dst.len() {
6161
LL | | dst[i] = src[count];
6262
LL | | count += 1;
6363
LL | | }
64-
| |_____^ help: try replacing the loop by: `dst.clone_from_slice(&src[2..(dst.len() + 2)]);`
64+
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[2..(dst.len() + 2)]);`
6565

6666
error: it looks like you're manually copying between slices
6767
--> $DIR/with_loop_counters.rs:47:5
@@ -70,7 +70,7 @@ LL | / for i in 3..10 {
7070
LL | | dst[i] = src[count];
7171
LL | | count += 1;
7272
LL | | }
73-
| |_____^ help: try replacing the loop by: `dst[3..10].clone_from_slice(&src[5..(10 + 5 - 3)]);`
73+
| |_____^ help: try replacing the loop by: `dst[3..10].copy_from_slice(&src[5..(10 + 5 - 3)]);`
7474

7575
error: it looks like you're manually copying between slices
7676
--> $DIR/with_loop_counters.rs:54:5
@@ -85,8 +85,8 @@ LL | | }
8585
|
8686
help: try replacing the loop by
8787
|
88-
LL ~ dst[3..(src.len() + 3)].clone_from_slice(&src[..]);
89-
LL + dst2[30..(src.len() + 30)].clone_from_slice(&src[..]);
88+
LL ~ dst[3..(src.len() + 3)].copy_from_slice(&src[..]);
89+
LL + dst2[30..(src.len() + 30)].copy_from_slice(&src[..]);
9090
|
9191

9292
error: it looks like you're manually copying between slices
@@ -96,7 +96,7 @@ LL | / for i in 0..1 << 1 {
9696
LL | | dst[count] = src[i + 2];
9797
LL | | count += 1;
9898
LL | | }
99-
| |_____^ help: try replacing the loop by: `dst[(0 << 1)..((1 << 1) + (0 << 1))].clone_from_slice(&src[2..((1 << 1) + 2)]);`
99+
| |_____^ help: try replacing the loop by: `dst[(0 << 1)..((1 << 1) + (0 << 1))].copy_from_slice(&src[2..((1 << 1) + 2)]);`
100100

101101
error: it looks like you're manually copying between slices
102102
--> $DIR/with_loop_counters.rs:71:5
@@ -105,7 +105,7 @@ LL | / for i in 3..src.len() {
105105
LL | | dst[i] = src[count];
106106
LL | | count += 1
107107
LL | | }
108-
| |_____^ help: try replacing the loop by: `dst[3..src.len()].clone_from_slice(&src[..(src.len() - 3)]);`
108+
| |_____^ help: try replacing the loop by: `dst[3..src.len()].copy_from_slice(&src[..(src.len() - 3)]);`
109109

110110
error: aborting due to 11 previous errors
111111

‎tests/ui/manual_memcpy/without_loop_counters.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,17 @@ pub fn manual_copy(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) {
113113
for i in 0.. {
114114
dst[i] = src[i];
115115
}
116+
117+
// VecDeque - ideally this would work, but would require something like `range_as_slices`
118+
let mut dst = std::collections::VecDeque::from_iter([0; 5]);
119+
let src = std::collections::VecDeque::from_iter([0, 1, 2, 3, 4]);
120+
for i in 0..dst.len() {
121+
dst[i] = src[i];
122+
}
123+
let src = vec![0, 1, 2, 3, 4];
124+
for i in 0..dst.len() {
125+
dst[i] = src[i];
126+
}
116127
}
117128

118129
#[warn(clippy::needless_range_loop, clippy::manual_memcpy)]

‎tests/ui/manual_memcpy/without_loop_counters.stderr‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ error: it looks like you're manually copying between slices
44
LL | / for i in 0..src.len() {
55
LL | | dst[i] = src[i];
66
LL | | }
7-
| |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[..]);`
7+
| |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[..]);`
88
|
99
= note: `-D clippy::manual-memcpy` implied by `-D warnings`
1010

@@ -14,31 +14,31 @@ error: it looks like you're manually copying between slices
1414
LL | / for i in 0..src.len() {
1515
LL | | dst[i + 10] = src[i];
1616
LL | | }
17-
| |_____^ help: try replacing the loop by: `dst[10..(src.len() + 10)].clone_from_slice(&src[..]);`
17+
| |_____^ help: try replacing the loop by: `dst[10..(src.len() + 10)].copy_from_slice(&src[..]);`
1818

1919
error: it looks like you're manually copying between slices
2020
--> $DIR/without_loop_counters.rs:17:5
2121
|
2222
LL | / for i in 0..src.len() {
2323
LL | | dst[i] = src[i + 10];
2424
LL | | }
25-
| |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[10..(src.len() + 10)]);`
25+
| |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[10..(src.len() + 10)]);`
2626

2727
error: it looks like you're manually copying between slices
2828
--> $DIR/without_loop_counters.rs:22:5
2929
|
3030
LL | / for i in 11..src.len() {
3131
LL | | dst[i] = src[i - 10];
3232
LL | | }
33-
| |_____^ help: try replacing the loop by: `dst[11..src.len()].clone_from_slice(&src[(11 - 10)..(src.len() - 10)]);`
33+
| |_____^ help: try replacing the loop by: `dst[11..src.len()].copy_from_slice(&src[(11 - 10)..(src.len() - 10)]);`
3434

3535
error: it looks like you're manually copying between slices
3636
--> $DIR/without_loop_counters.rs:27:5
3737
|
3838
LL | / for i in 0..dst.len() {
3939
LL | | dst[i] = src[i];
4040
LL | | }
41-
| |_____^ help: try replacing the loop by: `dst.clone_from_slice(&src[..dst.len()]);`
41+
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[..dst.len()]);`
4242

4343
error: it looks like you're manually copying between slices
4444
--> $DIR/without_loop_counters.rs:40:5
@@ -51,8 +51,8 @@ LL | | }
5151
|
5252
help: try replacing the loop by
5353
|
54-
LL ~ dst[10..256].clone_from_slice(&src[(10 - 5)..(256 - 5)]);
55-
LL + dst2[(10 + 500)..(256 + 500)].clone_from_slice(&src[10..256]);
54+
LL ~ dst[10..256].copy_from_slice(&src[(10 - 5)..(256 - 5)]);
55+
LL + dst2[(10 + 500)..(256 + 500)].copy_from_slice(&src[10..256]);
5656
|
5757

5858
error: it looks like you're manually copying between slices
@@ -61,50 +61,50 @@ error: it looks like you're manually copying between slices
6161
LL | / for i in 10..LOOP_OFFSET {
6262
LL | | dst[i + LOOP_OFFSET] = src[i - some_var];
6363
LL | | }
64-
| |_____^ 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)]);`
64+
| |_____^ 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)]);`
6565

6666
error: it looks like you're manually copying between slices
6767
--> $DIR/without_loop_counters.rs:65:5
6868
|
6969
LL | / for i in 0..src_vec.len() {
7070
LL | | dst_vec[i] = src_vec[i];
7171
LL | | }
72-
| |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].clone_from_slice(&src_vec[..]);`
72+
| |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].copy_from_slice(&src_vec[..]);`
7373

7474
error: it looks like you're manually copying between slices
7575
--> $DIR/without_loop_counters.rs:94:5
7676
|
7777
LL | / for i in from..from + src.len() {
7878
LL | | dst[i] = src[i - from];
7979
LL | | }
80-
| |_____^ help: try replacing the loop by: `dst[from..(from + src.len())].clone_from_slice(&src[..(from + src.len() - from)]);`
80+
| |_____^ help: try replacing the loop by: `dst[from..(from + src.len())].copy_from_slice(&src[..(from + src.len() - from)]);`
8181

8282
error: it looks like you're manually copying between slices
8383
--> $DIR/without_loop_counters.rs:98:5
8484
|
8585
LL | / for i in from..from + 3 {
8686
LL | | dst[i] = src[i - from];
8787
LL | | }
88-
| |_____^ help: try replacing the loop by: `dst[from..(from + 3)].clone_from_slice(&src[..(from + 3 - from)]);`
88+
| |_____^ help: try replacing the loop by: `dst[from..(from + 3)].copy_from_slice(&src[..(from + 3 - from)]);`
8989

9090
error: it looks like you're manually copying between slices
9191
--> $DIR/without_loop_counters.rs:103:5
9292
|
9393
LL | / for i in 0..5 {
9494
LL | | dst[i - 0] = src[i];
9595
LL | | }
96-
| |_____^ help: try replacing the loop by: `dst[..5].clone_from_slice(&src[..5]);`
96+
| |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src[..5]);`
9797

9898
error: it looks like you're manually copying between slices
9999
--> $DIR/without_loop_counters.rs:108:5
100100
|
101101
LL | / for i in 0..0 {
102102
LL | | dst[i] = src[i];
103103
LL | | }
104-
| |_____^ help: try replacing the loop by: `dst[..0].clone_from_slice(&src[..0]);`
104+
| |_____^ help: try replacing the loop by: `dst[..0].copy_from_slice(&src[..0]);`
105105

106106
error: it looks like you're manually copying between slices
107-
--> $DIR/without_loop_counters.rs:120:5
107+
--> $DIR/without_loop_counters.rs:131:5
108108
|
109109
LL | / for i in 0..src.len() {
110110
LL | | dst[i] = src[i].clone();

0 commit comments

Comments
 (0)