Skip to content

new restriction lint: integer_division_remainder_used #12451

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Mar 17, 2024

Conversation

Jacherr
Copy link
Contributor

@Jacherr Jacherr commented Mar 10, 2024

Fixes #12391

Introduces a restriction lint which disallows the use of / and % operators on any Int or Uint types (i.e., any that use the default Div or Rem trait implementations). Custom implementations of these traits are ignored.


changelog: Add new restriction lint [integer_division_remainder_used]

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2024

r? @Manishearth

rustbot has assigned @Manishearth.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 10, 2024
@Jacherr
Copy link
Contributor Author

Jacherr commented Mar 10, 2024

@tarcieri The example I included is extremely basic and not necessarily relevant to the lint itself; however, I'm no cryptography expert and found the example you provided in the original issue to be a bit long and specific (and also uses someone else's code).

If you can think of a better (ideally short) example than 10 / 2 -> 10 >> 1 I'd be more than willing to include it! Cheers

@bors
Copy link
Contributor

bors commented Mar 10, 2024

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

declare_clippy_lint! {
/// ### What it does
/// Checks for the usage of division (/) and remainder (%) operations
/// when performed on any integer types using the default Div and Rem trait implementaions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// when performed on any integer types using the default Div and Rem trait implementaions.
/// when performed on any integer types using the default Div and Rem trait implementations.

@Manishearth
Copy link
Member

r? rust-lang/clippy

still a bit busy, redirecting reviews

@rustbot rustbot assigned llogiq and unassigned Manishearth Mar 11, 2024
/// let my_div = 10 >> 1;
/// ```
#[clippy::version = "1.78.0"]
pub DIVISION_REMAINDER_USED,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: INTEGER_DIVISION_REMAINDER_USED

@tarcieri
Copy link

@Jacherr the specific example doesn't really matter as long as it covers usages of division / remainder. I just picked a real-world one to illustrate what the problem looked like in the wild and how it was fixed.

@llogiq llogiq changed the title new restriction lint: division_remainder_used new restriction lint: integer_division_remainder_used Mar 17, 2024
@llogiq
Copy link
Contributor

llogiq commented Mar 17, 2024

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2024

📌 Commit edcf10e has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 17, 2024

⌛ Testing commit edcf10e with merge e9a50f2...

@bors
Copy link
Contributor

bors commented Mar 17, 2024

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

@tarcieri
Copy link

A bit late, but we noticed there should potentially be an exception to this for operations which are exclusively on constants and/or which are statically known to always be in a const context.

It's fine as-is, but operations which are only on constants would be considered a false positive for cryptographic purposes.

I can open a separate issue for that if it's helpful.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint suggestion: disallow division/remainder operations
7 participants