Skip to content

Commit dbc0285

Browse files
borsbors
committed
Auto merge of #5727 - rail-rain:manual_memcpy_with_counter, r=flip1995
Expands `manual_memcpy` to lint ones with loop counters Closes #1670 This PR expands `manual_memcpy` to lint ones with loop counters as described in #1670 (comment) Although the current code is working, I have a couple of questions and concerns. ~~Firstly, I manually implemented `Clone` for `Sugg` because `AssocOp` lacks `Clone`. As `AssocOp` only holds an enum, which is `Copy`, as a value, it seems `AssocOp` can be `Clone`; but, I was not sure where to ask it. Should I make a PR to `rustc`?~~ The [PR]( rust-lang/rust#73629) was made. Secondly, manual copying with loop counters are likely to trigger `needless_range_loop` and `explicit_counter_loop` along with `manual_memcpy`; in fact, I explicitly allowed them in the tests. Is there any way to disable these two lints when a code triggers `manual_memcpy`? And, another thing I'd like to note is that `Sugg` adds unnecessary parentheses when expressions with parentheses passed to its `hir` function, as seen here: ``` error: it looks like you're manually copying between slices --> $DIR/manual_memcpy.rs:145:14 | LL | for i in 3..(3 + src.len()) { | ^^^^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[3..((3 + src.len()))].clone_from_slice(&src[..((3 + src.len()) - 3)]) ``` However, using the `hir` function is needed to prevent the suggestion causing errors when users use bitwise operations; and also this have already existed, for example: `verbose_bit_mask`. Thus, I think this is fine. changelog: Expands `manual_memcpy` to lint ones with loop counters
2 parents 2bdadd8 + b541884 commit dbc0285

File tree

8 files changed

+804
-297
lines changed

8 files changed

+804
-297
lines changed

‎clippy_lints/src/loops.rs

+437-201
Large diffs are not rendered by default.

‎clippy_lints/src/utils/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -708,7 +708,7 @@ fn reindent_multiline_inner(s: &str, ignore_first: bool, indent: Option<usize>,
708708
}
709709

710710
/// Gets the parent expression, if any –- this is useful to constrain a lint.
711-
pub fn get_parent_expr<'c>(cx: &'c LateContext<'_>, e: &Expr<'_>) -> Option<&'c Expr<'c>> {
711+
pub fn get_parent_expr<'tcx>(cx: &LateContext<'tcx>, e: &Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
712712
let map = &cx.tcx.hir();
713713
let hir_id = e.hir_id;
714714
let parent_id = map.get_parent_node(hir_id);

‎clippy_lints/src/utils/sugg.rs

+52-7
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@ use rustc_span::{BytePos, Pos};
1313
use std::borrow::Cow;
1414
use std::convert::TryInto;
1515
use std::fmt::Display;
16+
use std::ops::{Add, Neg, Not, Sub};
1617

1718
/// A helper type to build suggestion correctly handling parenthesis.
19+
#[derive(Clone, PartialEq)]
1820
pub enum Sugg<'a> {
1921
/// An expression that never needs parenthesis such as `1337` or `[0; 42]`.
2022
NonParen(Cow<'a, str>),
@@ -25,8 +27,12 @@ pub enum Sugg<'a> {
2527
BinOp(AssocOp, Cow<'a, str>),
2628
}
2729

30+
/// Literal constant `0`, for convenience.
31+
pub const ZERO: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("0"));
2832
/// Literal constant `1`, for convenience.
2933
pub const ONE: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("1"));
34+
/// a constant represents an empty string, for convenience.
35+
pub const EMPTY: Sugg<'static> = Sugg::NonParen(Cow::Borrowed(""));
3036

3137
impl Display for Sugg<'_> {
3238
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
@@ -267,21 +273,60 @@ impl<'a> Sugg<'a> {
267273
}
268274
}
269275

270-
impl<'a, 'b> std::ops::Add<Sugg<'b>> for Sugg<'a> {
276+
// Copied from the rust standart library, and then edited
277+
macro_rules! forward_binop_impls_to_ref {
278+
(impl $imp:ident, $method:ident for $t:ty, type Output = $o:ty) => {
279+
impl $imp<$t> for &$t {
280+
type Output = $o;
281+
282+
fn $method(self, other: $t) -> $o {
283+
$imp::$method(self, &other)
284+
}
285+
}
286+
287+
impl $imp<&$t> for $t {
288+
type Output = $o;
289+
290+
fn $method(self, other: &$t) -> $o {
291+
$imp::$method(&self, other)
292+
}
293+
}
294+
295+
impl $imp for $t {
296+
type Output = $o;
297+
298+
fn $method(self, other: $t) -> $o {
299+
$imp::$method(&self, &other)
300+
}
301+
}
302+
};
303+
}
304+
305+
impl Add for &Sugg<'_> {
271306
type Output = Sugg<'static>;
272-
fn add(self, rhs: Sugg<'b>) -> Sugg<'static> {
273-
make_binop(ast::BinOpKind::Add, &self, &rhs)
307+
fn add(self, rhs: &Sugg<'_>) -> Sugg<'static> {
308+
make_binop(ast::BinOpKind::Add, self, rhs)
274309
}
275310
}
276311

277-
impl<'a, 'b> std::ops::Sub<Sugg<'b>> for Sugg<'a> {
312+
impl Sub for &Sugg<'_> {
313+
type Output = Sugg<'static>;
314+
fn sub(self, rhs: &Sugg<'_>) -> Sugg<'static> {
315+
make_binop(ast::BinOpKind::Sub, self, rhs)
316+
}
317+
}
318+
319+
forward_binop_impls_to_ref!(impl Add, add for Sugg<'_>, type Output = Sugg<'static>);
320+
forward_binop_impls_to_ref!(impl Sub, sub for Sugg<'_>, type Output = Sugg<'static>);
321+
322+
impl Neg for Sugg<'_> {
278323
type Output = Sugg<'static>;
279-
fn sub(self, rhs: Sugg<'b>) -> Sugg<'static> {
280-
make_binop(ast::BinOpKind::Sub, &self, &rhs)
324+
fn neg(self) -> Sugg<'static> {
325+
make_unop("-", self)
281326
}
282327
}
283328

284-
impl<'a> std::ops::Not for Sugg<'a> {
329+
impl Not for Sugg<'_> {
285330
type Output = Sugg<'static>;
286331
fn not(self) -> Sugg<'static> {
287332
make_unop("!", self)

‎tests/ui/manual_memcpy.stderr

-88
This file was deleted.

‎tests/ui/manual_memcpy/with_loop_counters.rs

+88
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
#![warn(clippy::needless_range_loop, clippy::manual_memcpy)]
2+
3+
pub fn manual_copy_with_counters(src: &[i32], dst: &mut [i32], dst2: &mut [i32]) {
4+
let mut count = 0;
5+
for i in 3..src.len() {
6+
dst[i] = src[count];
7+
count += 1;
8+
}
9+
10+
let mut count = 0;
11+
for i in 3..src.len() {
12+
dst[count] = src[i];
13+
count += 1;
14+
}
15+
16+
let mut count = 3;
17+
for i in 0..src.len() {
18+
dst[count] = src[i];
19+
count += 1;
20+
}
21+
22+
let mut count = 3;
23+
for i in 0..src.len() {
24+
dst[i] = src[count];
25+
count += 1;
26+
}
27+
28+
let mut count = 0;
29+
for i in 3..(3 + src.len()) {
30+
dst[i] = src[count];
31+
count += 1;
32+
}
33+
34+
let mut count = 3;
35+
for i in 5..src.len() {
36+
dst[i] = src[count - 2];
37+
count += 1;
38+
}
39+
40+
let mut count = 2;
41+
for i in 0..dst.len() {
42+
dst[i] = src[count];
43+
count += 1;
44+
}
45+
46+
let mut count = 5;
47+
for i in 3..10 {
48+
dst[i] = src[count];
49+
count += 1;
50+
}
51+
52+
let mut count = 3;
53+
let mut count2 = 30;
54+
for i in 0..src.len() {
55+
dst[count] = src[i];
56+
dst2[count2] = src[i];
57+
count += 1;
58+
count2 += 1;
59+
}
60+
61+
// make sure parentheses are added properly to bitwise operators, which have lower precedence than
62+
// arithmetric ones
63+
let mut count = 0 << 1;
64+
for i in 0..1 << 1 {
65+
dst[count] = src[i + 2];
66+
count += 1;
67+
}
68+
69+
// make sure incrementing expressions without semicolons at the end of loops are handled correctly.
70+
let mut count = 0;
71+
for i in 3..src.len() {
72+
dst[i] = src[count];
73+
count += 1
74+
}
75+
76+
// make sure ones where the increment is not at the end of the loop.
77+
// As a possible enhancement, one could adjust the offset in the suggestion according to
78+
// the position. For example, if the increment is at the top of the loop;
79+
// treating the loop counter as if it were initialized 1 greater than the original value.
80+
let mut count = 0;
81+
#[allow(clippy::needless_range_loop)]
82+
for i in 0..src.len() {
83+
count += 1;
84+
dst[i] = src[count];
85+
}
86+
}
87+
88+
fn main() {}

‎tests/ui/manual_memcpy/with_loop_counters.stderr

+111
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
error: it looks like you're manually copying between slices
2+
--> $DIR/with_loop_counters.rs:5:5
3+
|
4+
LL | / for i in 3..src.len() {
5+
LL | | dst[i] = src[count];
6+
LL | | count += 1;
7+
LL | | }
8+
| |_____^ help: try replacing the loop by: `dst[3..src.len()].clone_from_slice(&src[..(src.len() - 3)]);`
9+
|
10+
= note: `-D clippy::manual-memcpy` implied by `-D warnings`
11+
12+
error: it looks like you're manually copying between slices
13+
--> $DIR/with_loop_counters.rs:11:5
14+
|
15+
LL | / for i in 3..src.len() {
16+
LL | | dst[count] = src[i];
17+
LL | | count += 1;
18+
LL | | }
19+
| |_____^ help: try replacing the loop by: `dst[..(src.len() - 3)].clone_from_slice(&src[3..]);`
20+
21+
error: it looks like you're manually copying between slices
22+
--> $DIR/with_loop_counters.rs:17:5
23+
|
24+
LL | / for i in 0..src.len() {
25+
LL | | dst[count] = src[i];
26+
LL | | count += 1;
27+
LL | | }
28+
| |_____^ help: try replacing the loop by: `dst[3..(src.len() + 3)].clone_from_slice(&src[..]);`
29+
30+
error: it looks like you're manually copying between slices
31+
--> $DIR/with_loop_counters.rs:23:5
32+
|
33+
LL | / for i in 0..src.len() {
34+
LL | | dst[i] = src[count];
35+
LL | | count += 1;
36+
LL | | }
37+
| |_____^ help: try replacing the loop by: `dst[..src.len()].clone_from_slice(&src[3..(src.len() + 3)]);`
38+
39+
error: it looks like you're manually copying between slices
40+
--> $DIR/with_loop_counters.rs:29:5
41+
|
42+
LL | / for i in 3..(3 + src.len()) {
43+
LL | | dst[i] = src[count];
44+
LL | | count += 1;
45+
LL | | }
46+
| |_____^ help: try replacing the loop by: `dst[3..((3 + src.len()))].clone_from_slice(&src[..((3 + src.len()) - 3)]);`
47+
48+
error: it looks like you're manually copying between slices
49+
--> $DIR/with_loop_counters.rs:35:5
50+
|
51+
LL | / for i in 5..src.len() {
52+
LL | | dst[i] = src[count - 2];
53+
LL | | count += 1;
54+
LL | | }
55+
| |_____^ help: try replacing the loop by: `dst[5..src.len()].clone_from_slice(&src[(3 - 2)..((src.len() - 2) + 3 - 5)]);`
56+
57+
error: it looks like you're manually copying between slices
58+
--> $DIR/with_loop_counters.rs:41:5
59+
|
60+
LL | / for i in 0..dst.len() {
61+
LL | | dst[i] = src[count];
62+
LL | | count += 1;
63+
LL | | }
64+
| |_____^ help: try replacing the loop by: `dst.clone_from_slice(&src[2..(dst.len() + 2)]);`
65+
66+
error: it looks like you're manually copying between slices
67+
--> $DIR/with_loop_counters.rs:47:5
68+
|
69+
LL | / for i in 3..10 {
70+
LL | | dst[i] = src[count];
71+
LL | | count += 1;
72+
LL | | }
73+
| |_____^ help: try replacing the loop by: `dst[3..10].clone_from_slice(&src[5..(10 + 5 - 3)]);`
74+
75+
error: it looks like you're manually copying between slices
76+
--> $DIR/with_loop_counters.rs:54:5
77+
|
78+
LL | / for i in 0..src.len() {
79+
LL | | dst[count] = src[i];
80+
LL | | dst2[count2] = src[i];
81+
LL | | count += 1;
82+
LL | | count2 += 1;
83+
LL | | }
84+
| |_____^
85+
|
86+
help: try replacing the loop by
87+
|
88+
LL | dst[3..(src.len() + 3)].clone_from_slice(&src[..]);
89+
LL | dst2[30..(src.len() + 30)].clone_from_slice(&src[..]);
90+
|
91+
92+
error: it looks like you're manually copying between slices
93+
--> $DIR/with_loop_counters.rs:64:5
94+
|
95+
LL | / for i in 0..1 << 1 {
96+
LL | | dst[count] = src[i + 2];
97+
LL | | count += 1;
98+
LL | | }
99+
| |_____^ help: try replacing the loop by: `dst[(0 << 1)..((1 << 1) + (0 << 1))].clone_from_slice(&src[2..((1 << 1) + 2)]);`
100+
101+
error: it looks like you're manually copying between slices
102+
--> $DIR/with_loop_counters.rs:71:5
103+
|
104+
LL | / for i in 3..src.len() {
105+
LL | | dst[i] = src[count];
106+
LL | | count += 1
107+
LL | | }
108+
| |_____^ help: try replacing the loop by: `dst[3..src.len()].clone_from_slice(&src[..(src.len() - 3)]);`
109+
110+
error: aborting due to 11 previous errors
111+

‎tests/ui/manual_memcpy.rs renamed to ‎tests/ui/manual_memcpy/without_loop_counters.rs

File renamed without changes.

0 commit comments

Comments
 (0)