[`manual_div_ceil`]: init #12987

belyakov-am · 2024-06-23T17:57:56Z

Suggest using div_ceil() instead of manual implementation for basic cases.

Partially fixes #12894

changelog: new lint: [manual_div_ceil]

rustbot · 2024-06-23T17:58:00Z

rustbot has assigned @blyxyas.
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

belyakov-am · 2024-06-23T17:58:34Z

See #12894 for more details

J-ZhengLi · 2024-06-24T17:35:02Z

clippy_lints/src/manual_div_ceil.rs

+    /// ```
+    #[clippy::version = "1.81.0"]
+    pub MANUAL_DIV_CEIL,
+    complexity,


Some drive-by thoughts 😄 :

It seems like div_ceil for signed integers are still unstable, so if this was put in complexity group, we might ended up suggesting something that the users cannot fix. (not to mention that it have an applicability of MachineApplicable)
As for unsigned integers, this function was stabilized for 1.73.0 and onward, so a msrv configuration would be very nice~

Thanks for your input!

I'm struggling to find a proper way to separate lint for stable and unstable (i.e. unsigned and signed integers)
Can you please point me in a direction that would be helpful for this?

BTW, when a binary operand is cast expr, the suggestion might be broken, something like:

let z: u32 = 7; let _ = (z as i32 + (y - 1)) / y;

this suggests z as i32.div_ceil(y), you'll need a pair of parentheses to wrap the caller (see maybe_par).

I'm struggling to find a proper way to separate lint for stable and unstable (i.e. unsigned and signed integers) Can you please point me in a direction that would be helpful for this?

ah yes, it appears that you already have a function to check if it's Int or Uint right?

Then you should be able to separate it into two scenarios, one is where the type is ty::Int, in this case, don't offer a fixable suggestion yet... unless the user already have that required feature enabled, meaning cx.tcx.features().declared_features.contains(&Symbol::inter("int_roundings")) is true.

Thanks a lot!

This declared_features hashset is very insightful, I'd never find it on my own

blyxyas · 2024-07-01T18:41:17Z

Not related, but maybe we should do some benchmarks on (a + (b - 1)) / b vs div_ceil, it seems to be 1 instruction faster

blyxyas

Very good for a first iteration on the lint! Some minor improvements, this is really close to the finish line!

clippy_lints/src/manual_div_ceil.rs

blyxyas · 2024-07-02T18:31:22Z

clippy_lints/src/manual_div_ceil.rs

+        if let ExprKind::Binary(div_op, div_lhs, div_rhs) = expr.kind
+            && check_int_ty_and_feature(cx, div_lhs)
+            && check_int_ty_and_feature(cx, div_rhs)
+            && div_op.node == BinOpKind::Div


This check should be before the double check_int_ty_and_feature, and you could also put the if let ExprKind::Binary(add_op, add_lhs, add_rhs) = div_lhs.kind here, because it's repeated three times without any reason (more than changing variable name)

clippy_lints/src/manual_div_ceil.rs

blyxyas · 2024-07-02T18:38:43Z

tests/ui/manual_div_ceil.rs

+    let z = 11_u32;
+
+    // Lint.
+    let _ = (x + (y - 1)) / y;


You can use this notation //~ ERROR to indicate an error 🌈

bors · 2024-07-04T05:53:01Z

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

blyxyas

LGTM, thanks! ❤️

Sorry for the delay, I've been busy and turns out I didn't actually start the Final Comment Period with the team (which I could've sweared I did, but anyways 😅 ), in a few days I'll merge this PR.

Thanks for your contribution! And sorry again for keeping you waiting

In-between it gets accepted for merge by the team, could you squash the commits into one?

flip1995 · 2024-07-19T12:35:06Z

Looking at lintcheck, all new lint suggestions are valid, so no FPs there. But the suggestions generation for macros is broken:

warning: manually reimplementing `div_ceil`
   --> target/lintcheck/sources/rand_core-0.6.0/src/impls.rs:59:26
    |
59  |         let chunk_size = (chunk_size_u8 + SIZE - 1) / SIZE;
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `.div_ceil()`: `chunk_size_u8.div_ceil(fill_via_chunks!(src, dest, u64))`
...
132 |     fill_via_chunks!(src, dest, u64)
    |     -------------------------------- in this macro invocation
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_div_ceil
    = note: this warning originates in the macro `fill_via_chunks` (in Nightly builds, run with -Z macro-backtrace for more info)

bors · 2024-08-28T08:08:55Z

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

blyxyas · 2024-08-28T22:41:50Z

I've done some updating. But I'm not being able to replicate the macro bug that Philipp is talking about, is there any more information? @belyakov-am Can you replicate it?

If we're not able to replicate it, we can safely merge this.

bors · 2024-09-03T17:43:25Z

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

belyakov-am · 2024-09-05T09:11:32Z

Thanks! And sorry for disappearing for so long
I wasn't able to reproduce this one

flip1995 · 2024-09-05T14:00:18Z

I also doesn't show up in the latest lintcheck run anymore. So I guess my concern is resolved.

blyxyas

LGTM, thanks! ❤️ (I just synced and rebased to get this rerolling, just lib.rs issues)

blyxyas · 2024-09-05T23:15:23Z

Just looked at lintcheck, everything looks correct!

@bors r+

bors · 2024-09-05T23:15:25Z

📌 Commit 9415e6e has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-09-05T23:16:32Z

⌛ Testing commit 9415e6e with merge 9e9042a...

bors · 2024-09-05T23:23:54Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 9e9042a to master...

rustbot assigned blyxyas Jun 23, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 23, 2024

belyakov-am force-pushed the lint/manual_div_ceil branch from a90e8e6 to 0f32a26 Compare June 23, 2024 18:02

Alexendoo mentioned this pull request Jun 23, 2024

Add lints against more manual integer ops where direct methods exist #12894

Open

10 tasks

J-ZhengLi reviewed Jun 24, 2024

View reviewed changes

blyxyas requested changes Jul 2, 2024

View reviewed changes

belyakov-am force-pushed the lint/manual_div_ceil branch from 876732d to b9eb1d2 Compare July 6, 2024 08:10

blyxyas approved these changes Jul 18, 2024

View reviewed changes

belyakov-am force-pushed the lint/manual_div_ceil branch from b9eb1d2 to ea83c3c Compare July 27, 2024 10:33

blyxyas force-pushed the lint/manual_div_ceil branch from ea83c3c to cb897d0 Compare August 28, 2024 15:55

Add manual_div_ceil

9415e6e

blyxyas force-pushed the lint/manual_div_ceil branch from 390f038 to 9415e6e Compare September 5, 2024 22:55

blyxyas approved these changes Sep 5, 2024

View reviewed changes

bors merged commit 9e9042a into rust-lang:master Sep 5, 2024
11 checks passed

This was referenced Sep 5, 2024

Added new non_zero_suggestions lint #13167

Merged

Unused trait imports (formerly anonymous trait import) #13322

Merged

This was referenced Sep 5, 2024

Add new lint manual_is_power_of_two #13327

Merged

Add [manual_ilog2] lint #13331

Open

Add manual_ignore_cast_cmp lint #13334

Merged

belyakov-am deleted the lint/manual_div_ceil branch September 7, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`manual_div_ceil`]: init #12987

[`manual_div_ceil`]: init #12987

belyakov-am commented Jun 23, 2024 •

edited

Loading

rustbot commented Jun 23, 2024

belyakov-am commented Jun 23, 2024

J-ZhengLi Jun 24, 2024 •

edited

Loading

belyakov-am Jun 26, 2024

J-ZhengLi Jun 27, 2024 •

edited

Loading

J-ZhengLi Jun 27, 2024 •

edited

Loading

belyakov-am Jun 30, 2024

blyxyas commented Jul 1, 2024 •

edited

Loading

blyxyas left a comment

blyxyas Jul 2, 2024

blyxyas Jul 2, 2024

bors commented Jul 4, 2024

blyxyas left a comment •

edited

Loading

flip1995 commented Jul 19, 2024

bors commented Aug 28, 2024

blyxyas commented Aug 28, 2024

bors commented Sep 3, 2024

belyakov-am commented Sep 5, 2024

flip1995 commented Sep 5, 2024

blyxyas left a comment

blyxyas commented Sep 5, 2024

bors commented Sep 5, 2024

bors commented Sep 5, 2024

bors commented Sep 5, 2024

[manual_div_ceil]: init #12987

[manual_div_ceil]: init #12987

Conversation

belyakov-am commented Jun 23, 2024 • edited Loading

rustbot commented Jun 23, 2024

belyakov-am commented Jun 23, 2024

J-ZhengLi Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

belyakov-am Jun 26, 2024

Choose a reason for hiding this comment

J-ZhengLi Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

J-ZhengLi Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

belyakov-am Jun 30, 2024

Choose a reason for hiding this comment

blyxyas commented Jul 1, 2024 • edited Loading

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas Jul 2, 2024

Choose a reason for hiding this comment

blyxyas Jul 2, 2024

Choose a reason for hiding this comment

bors commented Jul 4, 2024

blyxyas left a comment • edited Loading

Choose a reason for hiding this comment

flip1995 commented Jul 19, 2024

bors commented Aug 28, 2024

blyxyas commented Aug 28, 2024

bors commented Sep 3, 2024

belyakov-am commented Sep 5, 2024

flip1995 commented Sep 5, 2024

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas commented Sep 5, 2024

bors commented Sep 5, 2024

bors commented Sep 5, 2024

bors commented Sep 5, 2024

[`manual_div_ceil`]: init #12987

[`manual_div_ceil`]: init #12987

belyakov-am commented Jun 23, 2024 •

edited

Loading

J-ZhengLi Jun 24, 2024 •

edited

Loading

J-ZhengLi Jun 27, 2024 •

edited

Loading

J-ZhengLi Jun 27, 2024 •

edited

Loading

blyxyas commented Jul 1, 2024 •

edited

Loading

blyxyas left a comment •

edited

Loading