Add lint size_of_in_element_count #6394

nico-abram · 2020-11-28T01:53:32Z

Fixes #6381
changelog: Add lint to check for using size_of:: or size_of_val:: in the count parameter to ptr::copy or ptr::copy_nonoverlapping, which take a count of Ts (And not a count of bytes)

Followed lint naming conventions
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

Running cargo test locally fails with this error:

running 1 test
test fmt ... FAILED

failures:

---- fmt stdout ----
status: exit code: 1
stdout:
stderr: error: unable to unlink old fallback exe
error: caused by: Access is denied. (os error 5)

thread 'fmt' panicked at 'Formatting check failed. Run `cargo dev fmt` to update formatting.', tests\fmt.rs:32:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    fmt

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

But I did run cargo dev fmt

rust-highfive · 2020-11-28T01:53:35Z

r? @ebroto

(rust-highfive has picked a reviewer for you, use r? to override)

flip1995 · 2020-11-28T15:49:56Z

@nico-abram Do you have a nightly toolchain with rustfmt installed? You can check this by running cargo +nightly fmt --version, which should give you an output similar to rustfmt 1.4.22-nightly (97d0301 2020-10-04)

nico-abram · 2020-11-28T18:13:47Z

Thanks @flip1995 , you're completely right (rustfmt 1.4.27-nightly (580d826e 2020-11-16)). Trying with latest stable now

flip1995 · 2020-11-28T18:19:23Z

@nico-abram You have to use the nightly version. So it should work. cargo dev fmt should give you an error.

nico-abram · 2020-11-28T18:24:46Z

Mhm. Here's the output from both commands:

D:\dev\rust-clippy>cargo dev fmt
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
     Running `clippy_dev/target\debug\clippy_dev.exe fmt`

D:\dev\rust-clippy>cargo test --test fmt
    Finished test [unoptimized + debuginfo] target(s) in 0.15s
     Running target\debug\deps\fmt-8bbfa8d523755791.exe

running 1 test
test fmt ... FAILED

failures:

---- fmt stdout ----
status: exit code: 1
stdout:
stderr: error: unable to unlink old fallback exe
error: caused by: Access is denied. (os error 5)

thread 'fmt' panicked at 'Formatting check failed. Run `cargo dev fmt` to update formatting.', tests\fmt.rs:32:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    fmt

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test fmt'

I'm not getting errors from cargo dev fmt

clippy_lints/src/unsafe_sizeof_count_copies.rs

thomcc · 2020-11-29T03:24:32Z

Would it be hard to make this handle copy_{from,to}{,_nonoverlapping}?

nico-abram · 2020-11-29T05:12:53Z

@thomcc it should handle those now

thomcc · 2020-11-29T05:13:34Z

:D Wonderful!

ebroto

Thanks for this!

Besides the nits, do you think it would make sense to also lint in these cases?

Functions from std::ptr
- slice_from_raw_parts
- slice_from_raw_parts_mut
- swap_nonoverlapping
- write_bytes
Methods from pointer primitive
- write_bytes

tests/ui/unsafe_sizeof_count_copies.stderr

clippy_lints/src/unsafe_sizeof_count_copies.rs

nico-abram · 2020-11-29T16:48:01Z

@ebroto Sounds good to me! Will try to fix the change requests and add those today

Looking for "count:" in the ptr primitive docs I also came across these: wrapping_sub, wrapping_add, add, sub, offset, wrapping_offset
But I think maybe those should be a separate lint

ebroto

Thanks for addressing the changes so quickly!

Looking for "count:" in the ptr primitive docs I also came across these: wrapping_sub, wrapping_add, add, sub, offset, wrapping_offset

Yeah I saw them and initially also thought that they were not related to this lint, but after re-reading the documentation it seems to me the same problem may appear if someone uses size_of/size_of_val, right?

I think it may make sense to lint also on these, and maybe change the name of the lint to not make reference to copy or unsafe, because slice_from_raw_parts/slice_from_raw_parts_mut are not unsafe. We should also avoid referring to these terms in the error message.

tests/ui/unsafe_sizeof_count_copies.rs

clippy_lints/src/unsafe_sizeof_count_copies.rs

nico-abram · 2020-11-30T22:57:31Z

Yeah I saw them and initially also thought that they were not related to this lint, but after re-reading the documentation it seems to me the same problem may appear if someone uses size_of/size_of_val, right?

AIUI, yes, because they also work in units of T

I think it may make sense to lint also on these, and maybe change the name of the lint to not make reference to copy or unsafe, because slice_from_raw_parts/slice_from_raw_parts_mut are not unsafe. We should also avoid referring to these terms in the error message.

Seems good to me. I think I was thinking of the slice counterparts of those two when I assumed they were unsafe. Do you think the lint should also happen on those 2? (Their docs say The len argument is the number of elements, not the number of bytes.)

Does size_of_in_count_of_t sound like a good lint name?

ebroto · 2020-11-30T23:07:26Z

Do you think the lint should also happen on those 2?

Good catch! I think the same logic applies.

Does size_of_in_count_of_t sound like a good lint name?

I think it respects the naming convention, but I would be more inclined for size_of_in_element_count (is that proper english? 😄). It's just the lowercase t that I find a bit confusing. What do you think?

nico-abram · 2020-11-30T23:11:58Z

size_of_in_element_count sounds good to me 👍 , I agree the lowercase t looks weird

nico-abram · 2020-12-01T01:21:56Z

I'm not sure why CI is failing. It seems to be an ICE test. Could rustc's error output have changed? (Assuming CI is using nightly)

rustbot · 2020-12-01T19:20:39Z

Error: The feature major_change is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please let @rust-lang/release know if you're having trouble with this bot.

nico-abram · 2020-12-02T13:53:05Z

I think the current CI failure is (as mentioned here) indeed upstream rustc-repo-clippy changes that are waiting to be synced

bors · 2020-12-03T09:50:18Z

☔ The latest upstream changes (presumably #6415) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ebroto

LGTM, just needs a rebase

ebroto · 2020-12-03T22:18:11Z

clippy_lints/src/size_of_in_element_count.rs

-                    UNSAFE_SIZEOF_COUNT_COPIES,
-                    expr.span,
+                    SIZE_OF_IN_ELEMENT_COUNT,
+                    count_expr.span,


Good catch!

…alls

Specifically: - find std::ptr::write_bytes - find std::ptr::swap_nonoverlapping - find std::ptr::slice_from_raw_parts - find std::ptr::slice_from_raw_parts_mut - pointer_primitive::write_bytes

Also fix review comments: - Use const arrays and iterate them for the method/function names - merge 2 if_chain's into one using a rest pattern - remove unnecessary unsafe block in test And make the lint only point to the count expression instead of the entire function call

Specifically ptr::{sub, wrapping_sub, add, wrapping_add, offset, wrapping_offset} and slice::{from_raw_parts, from_raw_parts_mut} The lint now also looks for size_of calls through casts (Since offset takes an isize)

ebroto · 2020-12-04T00:42:56Z

@bors r+
Thanks!

bors · 2020-12-04T00:42:57Z

📌 Commit c1a5329 has been approved by ebroto

bors · 2020-12-04T00:43:05Z

⌛ Testing commit c1a5329 with merge 7f22b1c...

bors · 2020-12-04T01:04:45Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 7f22b1c to master...

rust-highfive assigned ebroto Nov 28, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 28, 2020

flip1995 reviewed Nov 28, 2020

View reviewed changes

clippy_lints/src/unsafe_sizeof_count_copies.rs Outdated Show resolved Hide resolved

nico-abram force-pushed the unsafe_sizeof_count_copies branch 4 times, most recently from 430a292 to a06f548 Compare November 29, 2020 02:59

nico-abram force-pushed the unsafe_sizeof_count_copies branch from ea8b52b to 336e41d Compare November 29, 2020 04:59

ebroto mentioned this pull request Nov 29, 2020

Add invalid null pointer usage lint. #6192

Closed

ebroto suggested changes Nov 29, 2020

View reviewed changes

ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 29, 2020

ebroto mentioned this pull request Nov 29, 2020

6399 Lint: is_ascii_digit instead of is_digit #6400

Closed

nico-abram requested a review from ebroto November 29, 2020 17:46

ebroto suggested changes Nov 30, 2020

View reviewed changes

tests/ui/unsafe_sizeof_count_copies.rs Outdated Show resolved Hide resolved

clippy_lints/src/unsafe_sizeof_count_copies.rs Outdated Show resolved Hide resolved

clippy_lints/src/unsafe_sizeof_count_copies.rs Outdated Show resolved Hide resolved

nico-abram force-pushed the unsafe_sizeof_count_copies branch 3 times, most recently from efc90e3 to faad9a1 Compare December 1, 2020 01:05

nico-abram changed the title ~~Add lint unsafe_sizeof_count_copies~~ Add lint size_of_in_element_count Dec 1, 2020

ebroto approved these changes Dec 3, 2020

View reviewed changes

nico-abram added 5 commits December 3, 2020 20:54

Add lint unsafe_sizeof_count_copies

b611306

Make the unsafe_sizeof_count_copies lint find copy_{from,to} method c…

0f954ba

…alls

Make the unsafe_sizeof_count_copies lint work with more functions

1b80990

Specifically: - find std::ptr::write_bytes - find std::ptr::swap_nonoverlapping - find std::ptr::slice_from_raw_parts - find std::ptr::slice_from_raw_parts_mut - pointer_primitive::write_bytes

Remove unnecessary unsafe_size_count_copies tests

63a3c44

nico-abram force-pushed the unsafe_sizeof_count_copies branch from faad9a1 to 8322e30 Compare December 3, 2020 23:59

Add more functions to size_of_in_element_count

c1a5329

Specifically ptr::{sub, wrapping_sub, add, wrapping_add, offset, wrapping_offset} and slice::{from_raw_parts, from_raw_parts_mut} The lint now also looks for size_of calls through casts (Since offset takes an isize)

nico-abram force-pushed the unsafe_sizeof_count_copies branch from 8322e30 to c1a5329 Compare December 4, 2020 00:14

bors merged commit 7f22b1c into rust-lang:master Dec 4, 2020

MarijnS95 mentioned this pull request Dec 28, 2020

size_of_in_element_count false-positive when dividing byte-size by element size #6511

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint size_of_in_element_count #6394

Add lint size_of_in_element_count #6394

nico-abram commented Nov 28, 2020

rust-highfive commented Nov 28, 2020

flip1995 commented Nov 28, 2020

nico-abram commented Nov 28, 2020

flip1995 commented Nov 28, 2020

nico-abram commented Nov 28, 2020 •

edited

Loading

thomcc commented Nov 29, 2020

nico-abram commented Nov 29, 2020

thomcc commented Nov 29, 2020

ebroto left a comment

nico-abram commented Nov 29, 2020

ebroto left a comment

nico-abram commented Nov 30, 2020

ebroto commented Nov 30, 2020

nico-abram commented Nov 30, 2020

nico-abram commented Dec 1, 2020

rustbot commented Dec 1, 2020

nico-abram commented Dec 2, 2020 •

edited

Loading

bors commented Dec 3, 2020

ebroto left a comment

ebroto Dec 3, 2020

ebroto commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 4, 2020

Add lint size_of_in_element_count #6394

Add lint size_of_in_element_count #6394

Conversation

nico-abram commented Nov 28, 2020

rust-highfive commented Nov 28, 2020

flip1995 commented Nov 28, 2020

nico-abram commented Nov 28, 2020

flip1995 commented Nov 28, 2020

nico-abram commented Nov 28, 2020 • edited Loading

thomcc commented Nov 29, 2020

nico-abram commented Nov 29, 2020

thomcc commented Nov 29, 2020

ebroto left a comment

Choose a reason for hiding this comment

nico-abram commented Nov 29, 2020

ebroto left a comment

Choose a reason for hiding this comment

nico-abram commented Nov 30, 2020

ebroto commented Nov 30, 2020

nico-abram commented Nov 30, 2020

nico-abram commented Dec 1, 2020

rustbot commented Dec 1, 2020

nico-abram commented Dec 2, 2020 • edited Loading

bors commented Dec 3, 2020

ebroto left a comment

Choose a reason for hiding this comment

ebroto Dec 3, 2020

Choose a reason for hiding this comment

ebroto commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 4, 2020

bors commented Dec 4, 2020

nico-abram commented Nov 28, 2020 •

edited

Loading

nico-abram commented Dec 2, 2020 •

edited

Loading