Skip to content

Conversation

@sharksforarms
Copy link
Contributor

@sharksforarms sharksforarms commented Dec 9, 2024

fixes #13734

This PR renames unchecked_duration_subtraction lint to unchecked_time_subtraction and extends it to include Duration - Duration operations. Previously, it was only Instant - Duration. Duration - Duration is a common operation which may panic in the same way.

Note: This is my first clippy PR, feedback is appreciated.

changelog: [unchecked_time_subtraction]: renamed from unchecked_duration_subtraction, extend lint to include subtraction of a Duration with a Duration

@rustbot
Copy link
Collaborator

rustbot commented Dec 9, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 9, 2024
@alex-semenyuk
Copy link
Member

@sharksforarms
Could you please take a look tests, seems some are failed

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from 722aad6 to 59a03d7 Compare December 10, 2024 20:13
@sharksforarms
Copy link
Contributor Author

@alex-semenyuk ah! thanks. forgot to rerun uibless. I rebased.

@dswij dswij added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 30, 2024
@rustbot rustbot added the has-merge-commits PR has merge commits, merge with caution. label Feb 8, 2025
@rustbot

This comment has been minimized.

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from 5e59cee to 500c35e Compare February 8, 2025 18:55
@rustbot rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Feb 8, 2025
@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch 3 times, most recently from b615b2d to 5a8f778 Compare February 8, 2025 19:03
@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch 2 times, most recently from 4396486 to 12a7f21 Compare February 25, 2025 14:35
@sharksforarms sharksforarms changed the title extend unchecked_duration_subtraction to check for Duration - Duration rename unchecked_duration_subtraction to unchecked_time_subtraction and check for Duration - Duration Feb 25, 2025
@sharksforarms
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 25, 2025
Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Did the lint get renamed using cargo dev rename_lint? I don't see the expected changes in clippy_lints/src/deprecated_lints.rs and tests/ui/rename.rs.

Also, duration - duration - duration still crashes in tests. Something should be done about it.

@dswij dswij added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 27, 2025
@rustbot

This comment has been minimized.

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch 2 times, most recently from cfbe1be to d0874f8 Compare June 28, 2025 23:10
Comment on lines +1 to +15
error: unchecked subtraction between 'Duration' values
--> tests/ui/unchecked_time_subtraction_unfixable.rs:12:13
|
LL | let _ = dur1 - dur2 - dur3;
| ^^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::unchecked-time-subtraction` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unchecked_time_subtraction)]`

error: unchecked subtraction between 'Duration' values
--> tests/ui/unchecked_time_subtraction_unfixable.rs:12:13
|
LL | let _ = dur1 - dur2 - dur3;
| ^^^^^^^^^^^ help: try: `dur1.checked_sub(dur2).unwrap()`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this an "unfixable", not sure if this is the right approach. It's nice to not have a suggestion for the dur - dur - dur, but have some sort of suggestion come after for the nested? is there a precedent for handling this sort of thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to not give a suggestion, but ideally the whole thing would get a single suggestion.

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch 2 times, most recently from d141105 to 2960444 Compare June 28, 2025 23:47
@sharksforarms
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 28, 2025
@sharksforarms
Copy link
Contributor Author

hi reviewers, it's been a while since I touched this. I decided to restart, given the tip on cargo dev rename_lint. I've attempted to integrate your feedback and additional test cases. I have rebased/force-pushed, each commit should be reviewable, the first two being the rename / file move and the third being the code change itself.

@Jarcho
Copy link
Contributor

Jarcho commented Sep 17, 2025

r? clippy

I think @dswij has been busy.

@rustbot rustbot assigned Jarcho and unassigned dswij Sep 17, 2025
@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from 2960444 to ed4e369 Compare September 17, 2025 12:17
@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@github-actions
Copy link

github-actions bot commented Sep 17, 2025

Lintcheck changes for f296de4

Lint Added Removed Changed
clippy::unchecked_duration_subtraction 0 3 0
clippy::unchecked_time_subtraction 7 0 0

This comment will be updated if you push new changes

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from ed4e369 to 9a6d3e1 Compare September 17, 2025 13:04
@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from 9a6d3e1 to f296de4 Compare September 17, 2025 13:30
Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

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

@Jarcho Jarcho added this pull request to the merge queue Sep 27, 2025
Merged via the queue into rust-lang:master with commit 5e02a4e Sep 27, 2025
11 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duration arithmetic

7 participants