Extend `implicit_saturating_sub` lint #12476

GuillaumeGomez · 2024-03-13T15:37:07Z

It can serve as base if we want to add equivalent checks for other arithmetic operations.

Also one important note: when writing this lint, I realized that I could check for wrong conditions performed beforehand on subtraction and added another part in the lint. Considering they both rely on the same checks, I kept both in the same place. Not sure if it makes sense though...

changelog: Extend implicit_saturating_sub lint

rustbot · 2024-03-13T15:37:13Z

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

Manishearth · 2024-03-13T15:57:37Z

r? clippy

still busy

bors · 2024-03-17T08:26:49Z

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

GuillaumeGomez · 2024-03-17T10:14:58Z

Fixed merge conflict.

r? @y21

y21

Neat lint idea, but I'm not too sure what we want to do with the other implicit_saturating_{sub,add}/manual_saturating_arithmetic lints since they all seem really similar in their name and what they're intending to catch.
With this, we'd have four different lints that all look for manual (saturating) arithmetic

clippy_lints/src/manual_arithmetic_check.rs

tests/ui/manual_arithmetic_check.rs

clippy_lints/src/manual_arithmetic_check.rs

GuillaumeGomez · 2024-03-19T16:02:41Z

@y21: Instead of creating a new lint, I extended implicit_saturating_sub instead.

y21 · 2024-03-27T09:55:50Z

I haven't forgotten about this, will try to look at it again soon :)

xFrednet · 2024-06-20T20:09:01Z

Hey, this is a ping from triage. @y21 can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

y21

Sorry, I completely forgot about this 😅 thanks for reminding me

clippy_lints/src/implicit_saturating_sub.rs

tests/ui/manual_arithmetic_check.rs

clippy_lints/src/implicit_saturating_sub.rs

tests/ui/manual_arithmetic_check-2.stderr

GuillaumeGomez · 2024-06-28T13:56:10Z

Changes I pushed:

Added test with other statement in if/else to ensure it doesn't lint.
Added test with floats to ensure it doesn't lint.
Added test for new checks for <= and >=.
Updated code to limit the use of peel_blocks.
Added #[allow(...)] to ignore non-related lints.

With this I think I covered all cases. Thanks a lot @y21!

GuillaumeGomez · 2024-07-17T18:42:38Z

Fixed the test I added. Sorry for the delay.

clippy_lints/src/implicit_saturating_sub.rs

tests/ui/manual_arithmetic_check-2.stderr

clippy_lints/src/implicit_saturating_sub.rs

GuillaumeGomez · 2024-07-19T13:04:27Z

Updated!

GuillaumeGomez · 2024-07-22T09:45:05Z

Added MSRV check in const context.

GuillaumeGomez · 2024-07-26T09:38:17Z

Fixed dogfood!

y21

Some small nits for the last change. I'll start the FCP in the meantime since this shouldn't really be a blocker for that I don't think

clippy_lints/src/implicit_saturating_sub.rs

GuillaumeGomez · 2024-08-02T09:25:24Z

Applied suggestions.

…e`, it will not break clippy

GuillaumeGomez · 2024-08-29T18:56:17Z

Rebased on master and fixed dogfood.

y21

Looks all good to me now! Thanks for bearing with me for so long 😄

The FCP for the new lint has been open for a while but didn't get any comments (meaning no concerns), so I think we can just consider it complete now.

Mind squashing some of the commits? Specifically commits like dogfood etc. You can r=me with that

GuillaumeGomez · 2024-08-31T14:23:36Z

Done. Time for r+. :D

@bors r=y21

bors · 2024-08-31T14:23:39Z

@GuillaumeGomez: 🔑 Insufficient privileges: Not in reviewers

GuillaumeGomez · 2024-08-31T14:24:43Z

Ah apparently not. ^^'

Jarcho · 2024-08-31T16:02:36Z

@bors r=y21

bors · 2024-08-31T16:02:38Z

📌 Commit 2832faf has been approved by y21

It is now in the queue for this repository.

bors · 2024-08-31T16:03:45Z

⌛ Testing commit 2832faf with merge ac914d3...

bors · 2024-08-31T16:11:19Z

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

rustbot assigned Manishearth Mar 13, 2024

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

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 0ce4af5 to 0e63323 Compare March 13, 2024 15:40

rustbot assigned flip1995 and unassigned Manishearth Mar 13, 2024

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 0e63323 to 8a86926 Compare March 17, 2024 10:14

rustbot assigned y21 and unassigned flip1995 Mar 17, 2024

y21 reviewed Mar 17, 2024

View reviewed changes

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 8a86926 to 1d26be1 Compare March 19, 2024 16:00

GuillaumeGomez changed the title ~~Add new manual_arithmetic_check lint~~ Extend implicit_saturating_sub lint Mar 19, 2024

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 1d26be1 to b88a7a9 Compare March 19, 2024 16:02

y21 reviewed Jun 22, 2024

View reviewed changes

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch 3 times, most recently from 6acaf81 to 91ebe40 Compare June 28, 2024 13:55

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 91ebe40 to cd95c58 Compare July 17, 2024 18:34

y21 reviewed Jul 18, 2024

View reviewed changes

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

tests/ui/manual_arithmetic_check-2.stderr Show resolved Hide resolved

clippy_lints/src/implicit_saturating_sub.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from cd95c58 to 8aa4e51 Compare July 19, 2024 12:58

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from f18f779 to a16510a Compare July 26, 2024 09:26

y21 reviewed Aug 1, 2024

View reviewed changes

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

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

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from a16510a to c25ec0e Compare August 2, 2024 09:25

y21 added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 22, 2024

GuillaumeGomez added 4 commits August 29, 2024 20:20

Extend implicit_saturation_sub lint

74a2344

Add ui regression tests for implicit_saturation_sub lint extension

2622a87

Add ui test to ensure that if 0 is returned from both if and `els…

27c6343

…e`, it will not break clippy

Create new inverted_saturating_sub lint

d20fc38

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from c25ec0e to 9c6dd14 Compare August 29, 2024 18:31

y21 approved these changes Aug 30, 2024

View reviewed changes

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 9c6dd14 to 754255a Compare August 31, 2024 14:01

GuillaumeGomez added 2 commits August 31, 2024 16:24

Add MSRV check for saturating_sub lints in const contexts

e845366

Move MSRV check later in implicit_saturating_sub

2832faf

GuillaumeGomez force-pushed the add-manual_arithmetic_check branch from 754255a to 2832faf Compare August 31, 2024 14:24

bors merged commit ac914d3 into rust-lang:master Aug 31, 2024
11 checks passed

GuillaumeGomez deleted the add-manual_arithmetic_check branch August 31, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `implicit_saturating_sub` lint #12476

Extend `implicit_saturating_sub` lint #12476

GuillaumeGomez commented Mar 13, 2024 •

edited

Loading

rustbot commented Mar 13, 2024

Manishearth commented Mar 13, 2024

bors commented Mar 17, 2024

GuillaumeGomez commented Mar 17, 2024

y21 left a comment

GuillaumeGomez commented Mar 19, 2024

y21 commented Mar 27, 2024 •

edited

Loading

xFrednet commented Jun 20, 2024

y21 left a comment

GuillaumeGomez commented Jun 28, 2024

GuillaumeGomez commented Jul 17, 2024

GuillaumeGomez commented Jul 19, 2024

GuillaumeGomez commented Jul 22, 2024

GuillaumeGomez commented Jul 26, 2024

y21 left a comment

GuillaumeGomez commented Aug 2, 2024

GuillaumeGomez commented Aug 29, 2024

y21 left a comment •

edited

Loading

GuillaumeGomez commented Aug 31, 2024

bors commented Aug 31, 2024

GuillaumeGomez commented Aug 31, 2024

Jarcho commented Aug 31, 2024

bors commented Aug 31, 2024

bors commented Aug 31, 2024

bors commented Aug 31, 2024

Extend implicit_saturating_sub lint #12476

Extend implicit_saturating_sub lint #12476

Conversation

GuillaumeGomez commented Mar 13, 2024 • edited Loading

rustbot commented Mar 13, 2024

Manishearth commented Mar 13, 2024

bors commented Mar 17, 2024

GuillaumeGomez commented Mar 17, 2024

y21 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Mar 19, 2024

y21 commented Mar 27, 2024 • edited Loading

xFrednet commented Jun 20, 2024

y21 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 28, 2024

GuillaumeGomez commented Jul 17, 2024

GuillaumeGomez commented Jul 19, 2024

GuillaumeGomez commented Jul 22, 2024

GuillaumeGomez commented Jul 26, 2024

y21 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 2, 2024

GuillaumeGomez commented Aug 29, 2024

y21 left a comment • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 31, 2024

bors commented Aug 31, 2024

GuillaumeGomez commented Aug 31, 2024

Jarcho commented Aug 31, 2024

bors commented Aug 31, 2024

bors commented Aug 31, 2024

bors commented Aug 31, 2024

Extend `implicit_saturating_sub` lint #12476

Extend `implicit_saturating_sub` lint #12476

GuillaumeGomez commented Mar 13, 2024 •

edited

Loading

y21 commented Mar 27, 2024 •

edited

Loading

y21 left a comment •

edited

Loading