Update the existing arithmetic lint

[h3nill]

re: #6209

Updates the lint to not the error message if RHS of binary operation / of % is a literal/constant that is not 0 or -1, as suggested here

changelog: Expand [integer_arithmetic] to work with RHS literals and constants

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @phansch (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.

[mlegner]

Disclaimer: I'm not a maintainer of the clippy repo, I just came across your PR as I was also looking at the related issue.

I see a few problems with this PR:

• As far as I understand the float-arithmetic lint, this is supposed to check for all floating-point arithmetic. In contrast, the integer-arithmetic lint only considers those that can overflow or panic. That means, that you should probably still trigger the float-arithmetic lint on all divisions, even when the denominator is a non-zero literal.
• You are now only triggering the integer-arithmetic lint for the literals 0 and -1 but not for other denominators that might still cause overflow or panic.
• Tests for the integer-arithmetic lint with a denominator of 0 or -1 (for division and remainder) would be nice.

[h3nill]

First of all, Thank you for the review.

As far as I understand the float-arithmetic lint, this is supposed to check for all floating-point arithmetic. In contrast, the integer-arithmetic lint only considers those that can overflow or panic. That means, that you should probably still trigger the float-arithmetic lint on all divisions, even when the denominator is a non-zero literal.

Oh right.

You are now only triggering the integer-arithmetic lint for the literals 0 and -1 but not for other denominators that might still cause overflow or panic.

I made those changes referring to this comment, I was also not able to think of any other literals that might cause overflow or panic. Do you know of any?
EDIT: Oh I see, currently it does not trigger if the value is not literal.

Tests for the integer-arithmetic lint with a denominator of 0 or -1 (for division and remainder) would be nice.

Sure.

[h3nill]

Made the requested changes.

[mlegner]

Thanks for the changes! 👍

Looking at the errors produced by the tests, I noticed that a division by literal 0 (and remainder with a divisor of 0) is already caught by rustc, see for example here.

I'm not sure what the policy is regarding redundancy between rustc and rust-clippy, but it seems to me that it's not too helpful to produce errors multiple times for the same code. Probably @phansch knows what to do here.

[h3nill]

(Refactored to reduce duplication.)

[phansch]

Thanks for the PR and the reviews alike both of you 💙

not too helpful to produce errors multiple times for the same code

Definitely not - I think it would be enough for us to skip linting on division/modulo by 0 here and let rustc handle that part? Apart from that, the rest looks good to me 👍

(Ideally there would be a generic way for Rust/Clippy to only emit one lint per span, but that's not something for this PR and needs some groundwork in rustc, see Zulip here)

[h3nill]

Definitely not - I think it would be enough for us to skip linting on division/modulo by 0 here and let rustc handle that part? Apart from that, the rest looks good to me +1

Yeah, I also think having the same error twice is not ideal. I will remove that part.

[phansch]

@bors r+ thanks!

[bors]

📌 Commit fa0a78b has been approved by phansch

[bors]

⌛ Testing commit fa0a78b with merge 0be6544...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 0be6544 to master...