Implement basic lint detecting manual slice copy #1937

marcusklaas · 2017-08-07T22:07:28Z

This lint detects manual sequential memory copies between slices.

Should address #1831.

oli-obk

This is an absolutely awesome lint. Could you also add some nearly slice copies to the tests that should not be linted about? Just to make sure we don't have any false positives or introduce any in the future.

oli-obk · 2017-08-08T07:30:24Z

clippy_lints/src/loops.rs

+/// **Why is this bad?** It is not as fast as a memcpy.
+///
+/// **Known problems:** The lint assumes the data structure being indexed is
+/// slice-like and will produce false positives when it is not.


What properties of slice are required? Can we simply check whether one of the following is true

The type impls Deref<Target=[T]>

The type impls Index<usize> and has a len method

oli-obk · 2017-08-08T07:41:44Z

clippy_lints/src/loops.rs

@@ -83,7 +103,8 @@ declare_lint! {
 /// implements `IntoIterator`, so that possibly one value will be iterated,
 /// leading to some hard to find bugs. No one will want to write such code
 /// [except to win an Underhanded Rust
-/// Contest](https://www.reddit.com/r/rust/comments/3hb0wm/underhanded_rust_contest/cu5yuhr).
+/// Contest](https://www.reddit.
+/// com/r/rust/comments/3hb0wm/underhanded_rust_contest/cu5yuhr).


I ran rustfmt on master, so you can just rebase over master in order to separate the rustfmt changes from your actual changes.

Are you sure this affected the clippy_lints directory as well? In my experience, cargo fmt does not touch these by default..

We have a script which runs recursively

oli-obk · 2017-08-09T10:32:29Z

tests/ui/for_loop.stderr

+408 | /     for i in 11..src.len() {
+409 | |         dst[i] = src[i - 10];
+410 | |     }
+    | |_____^ help: try replacing the loop by: `dst[11..].clone_from_slice(&src[(11 + -10)..(src.len() + -10)])`


I think we should not produce output like a + -b, can you try to detect this situation, as it seems to be rather common.

oli-obk · 2017-08-09T10:32:41Z

tests/ui/for_loop.stderr

+427 | |         dst[i] = src[i - 5];
+428 | |         dst2[i + 500] = src[i]
+429 | |     }
+    | |_____^ help: try replacing the loop by: `dst[10..].clone_from_slice(&src[(10 + -5)..(256 + -5)])`


same as above.

marcusklaas · 2017-08-09T22:30:57Z

Updated the PR!

oli-obk · 2017-08-10T07:44:00Z

clippy_lints/src/loops.rs

+    let is_slice = match ty.sty {
+        ty::TyRef(_, ref subty) => is_slice_like(cx, subty.ty),
+        ty::TySlice(..) => true,
+        ty::TyArray(..) => true,


rewrite as

ty::TySlice(..) | ty::TyArray(..) => true,

(this was caught by travis. You can check such things yourself by running cargo test dogfood)

oli-obk · 2017-08-10T07:48:09Z

clippy_lints/src/loops.rs

+                    //     "try replacing the loop by",
+                    // format!("{}.clone_from_slice(&{}[{}..{}])", dst, src_var.var_name,
+                    // src_offset, src_limit),
+                    // );


Please remove the commented out code.

oli-obk · 2017-08-10T07:49:08Z

clippy_lints/src/loops.rs

+                })
+                .join("\n    ");
+
+            if big_sugg.len() > 0 {


Can you create a test that shows this with multiple suggestions?

also, replace it with !big_sugg.is_empty()

There is such a test already :-)

oh... why didn't a test change after your changes then?

It was in there from the start. The test output did change, though.

This should make it play more nicely with tools like rustfix.

marcusklaas · 2017-08-10T18:55:19Z

tests/ui/for_loop.stderr

+    |
+help: try replacing the loop by
+    |
+426 |     dst[10..].clone_from_slice(&src[(10 - 5)..(256 - 5)])


This is the test I was referring to.

marcusklaas · 2017-08-13T14:14:07Z

Thanks for the review @oli-obk!

After a more detailed inspection of the documentation on clone_from_slice, I think the current lint suggestions may cause panics. The destination slice must apparently be exactly as long as the source. Also, it may be preferable to use copy_from_slice when T: Copy, although in my experience clone_from_slice also compiles to a memcpy in release builds.

I'll update this PR shortly.

…ents some small doc improvements

oli-obk · 2017-08-14T08:05:30Z

Are you sure this affected the clippy_lints directory as well? In my experience, cargo fmt does not touch these by default..

I screwed up even more (I forgot to push the changes!). Please rebase over master now. If you want I can do the rebase, just ping me. Sorry about the trouble.

Fixes rust-lang#1940

Fix verbose_bit_mask off by one error

This should make it play more nicely with tools like rustfix.

marcusklaas · 2017-08-16T22:52:22Z

Hey @oli-obk - I'm still getting many style conflicts :-(

What command are you using to do the reformat?

oli-obk · 2017-08-17T08:00:11Z

@marcusklaas you seem to have included my commits in your PR now. I'm not sure how that happened. Try rebasing over the master branch.

What command are you using to do the reformat?

cargo fmt, run in clippy_lints and in the root directory

llogiq · 2017-08-17T08:10:32Z

Note that there may be differences between "stable" rustfmt and rustfmt-nightly.

oli-obk · 2017-08-21T11:54:16Z

Our current code base is formatted with stable rustfmt. We could move to rustfmt-nighty. It's not like anyone building clippy doesn't have a nightly lying around somewhere

marcusklaas · 2017-09-05T00:19:56Z

This PR is a mess. #2021 is a clean new PR.

oli-obk suggested changes Aug 9, 2017

View reviewed changes

Implement basic lint detecting manual slice copy

e9991c7

marcusklaas force-pushed the needless-range-loop-fix branch from 2b18964 to e9991c7 Compare August 9, 2017 20:20

marcusklaas added 2 commits August 9, 2017 16:48

Make subtractions prettier in the manual_memcpy lint suggestions

f553895

Check types in manual_memcpy lint

8cb2016

marcusklaas force-pushed the needless-range-loop-fix branch from d397075 to 3eec23b Compare August 9, 2017 22:57

oli-obk reviewed Aug 10, 2017

View reviewed changes

Ensure MANUAL_MEMCPY lint makes at most one suggestion per loop

748e8c6

This should make it play more nicely with tools like rustfix.

marcusklaas force-pushed the needless-range-loop-fix branch from 3eec23b to 748e8c6 Compare August 10, 2017 18:12

marcusklaas commented Aug 10, 2017

View reviewed changes

oli-obk approved these changes Aug 10, 2017

View reviewed changes

some small doc improvements

61a73bb

oli-obk and others added 7 commits August 13, 2017 21:02

Merge pull request rust-lang#1951 from rust-lang-nursery/doc_improvem…

adae2bb

…ents some small doc improvements

Bump the version

705c6ec

Rustfmt

b25b6b3

Handfix dogfood issues with the rustfmt changes

0e4c49b

Rustup

459cf46

Bump the version

4e6dd55

Update for rustc output changes

d6fc34f

mati865 and others added 6 commits August 14, 2017 16:22

Fix verbose_bit_mask off by one error

0d244d3

Fixes rust-lang#1940

Merge pull request rust-lang#1953 from mati865/master

3ab06c1

Fix verbose_bit_mask off by one error

Rustup

f3ae929

Bump the version

7cdaeae

Implement basic lint detecting manual slice copy

fd43e39

Make subtractions prettier in the manual_memcpy lint suggestions

3da8f7a

marcusklaas added 4 commits August 16, 2017 18:40

Check types in manual_memcpy lint

310d4d7

Ensure MANUAL_MEMCPY lint makes at most one suggestion per loop

ab55c5d

This should make it play more nicely with tools like rustfix.

Reformat with latest rustfmt

13fe916

Merge

7aa83a8

marcusklaas mentioned this pull request Sep 5, 2017

Add lint to detect manual slice copies #2021

Merged

marcusklaas closed this Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basic lint detecting manual slice copy #1937

Implement basic lint detecting manual slice copy #1937

marcusklaas commented Aug 7, 2017

oli-obk left a comment

oli-obk Aug 8, 2017

oli-obk Aug 8, 2017

marcusklaas Aug 9, 2017

oli-obk Aug 10, 2017

oli-obk Aug 9, 2017

oli-obk Aug 9, 2017

marcusklaas commented Aug 9, 2017

oli-obk Aug 10, 2017

oli-obk Aug 10, 2017

oli-obk Aug 10, 2017

oli-obk Aug 10, 2017

marcusklaas Aug 10, 2017

oli-obk Aug 10, 2017

marcusklaas Aug 10, 2017

marcusklaas Aug 10, 2017

marcusklaas commented Aug 13, 2017

oli-obk commented Aug 14, 2017

marcusklaas commented Aug 16, 2017

oli-obk commented Aug 17, 2017

llogiq commented Aug 17, 2017

oli-obk commented Aug 21, 2017

marcusklaas commented Sep 5, 2017

Implement basic lint detecting manual slice copy #1937

Implement basic lint detecting manual slice copy #1937

Conversation

marcusklaas commented Aug 7, 2017

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusklaas commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusklaas commented Aug 13, 2017

oli-obk commented Aug 14, 2017

marcusklaas commented Aug 16, 2017

oli-obk commented Aug 17, 2017

llogiq commented Aug 17, 2017

oli-obk commented Aug 21, 2017

marcusklaas commented Sep 5, 2017