size_of_in_element_count: Disable lint on division by byte-size

[MarijnS95]

Fixes #6511

It is fairly common to divide some length in bytes by the byte-size of a single element before creating a from_raw_parts slice or similar operation. This lint would erroneously disallow such expressions.

Just in case, instead of simply disabling this lint in the RHS of a division, keep track of the inversion and enable it again on recursive division.

---

changelog: Do not trigger size_of_in_element_count when dividing by element size

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[MarijnS95]

@nico-abram Is there by chance a test you'd be fine to remove? I guess it should be fine to test all affected functions just once to make sure they trigger, then test all the expression variants on just one function (ie. no need to test size_of and size_of_val, or a certain multiplication pattern multiple times).

[nico-abram]

I think most of the tests left are testing different functions/methods. Maybe you can combine some of the element count expression tests with tests for specific functions?

Like removing unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) }; and putting that expression in one of the other calls

If possible IMO keeping each case as cleanly separated as possible is best but I'm not a maintainer. Whoever reviews this can probably give you better feedback

[MarijnS95]

@nico-abram Correct, in hindsight the top half tests various functions (ie. none has been accidentally skipped from the linting code), the bottom half tests all expressions. There was only one overlapping thing with copy() which has been removed to get back below 200 lines.

I guess the only recommended solution beyond that is to split the test out in two parts (one for function inclusion, the other for variation in the expressions), but that doesn't seem necessary in this MR at least (unless one wishes to re-add the test for copy()).

[ThibsG]

On another side, do you think it's relevant to also fix #6590 here?

[MarijnS95]

@ThibsG That looks like exactly the mistake this lint is protecting against. The function described there takes a single u8 as input, that interpreted as slice should have a constant length of 1. I guess it only makes more sense to use the constant 1 or the output from .len(), not size_of::<u8> nor multiplied by that.

Otherwise, matching types left and right is a completely different issue (and iirc why this lint doesn't trigger when the type on the left and right is different).

[flip1995]

Changes LGTM

There was only one overlapping thing with copy() which has been removed to get back below 200 lines.

This restriction in Clippy is there to keep tests to a maintainable size. Please don't remove tests because of this. Your suggestion of splitting up the tests sounds reasonable to me. Please do this in a separate commit. You can create a directory for tests for this lint or just append the test file with _{function,expression} or similar.

[MarijnS95]

@flip1995 Thanks for confirming, the tests are now split up in two files distinguishing function "completeness" and expression variants, obviously in separate commits 😉

If I understand the docs correctly, I should now (be able to) do this:

@rustbot label -S-waiting-on-author +S-waiting-on-review

Right?

[flip1995]

@bors r+

Thanks! Great job!

[bors]

📌 Commit d4bf59b has been approved by flip1995

[bors]

⌛ Testing commit d4bf59b with merge fb0d7f1...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing fb0d7f1 to master...