Skip to content

rename unchecked_duration_subtraction to unchecked_time_subtraction and check for Duration - Duration #13800

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

rename unchecked_duration_subtraction to unchecked_time_subtraction and check for Duration - Duration #13800

wants to merge 3 commits into from

Conversation

sharksforarms
Copy link

@sharksforarms sharksforarms commented Dec 9, 2024 •
edited
Loading

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
sharksforarms
sharksforarms commented Dec 9, 2024
View reviewed changes
@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
Author

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

dswij
dswij reviewed Dec 17, 2024
View reviewed changes
@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.

Sign in to view
@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
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
samueltardieu
samueltardieu reviewed Feb 26, 2025
View reviewed changes
Copy link
Contributor

@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.

samueltardieu
samueltardieu reviewed Feb 26, 2025
View reviewed changes
@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
Copy link
Collaborator

rustbot commented Mar 31, 2025

☔ The latest upstream changes (possibly d28d234) made this pull request unmergeable. Please resolve the merge conflicts.

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch 2 times, most recently from cfbe1be to d0874f8 Compare June 28, 2025 23:10
sharksforarms
sharksforarms commented Jun 28, 2025
View reviewed changes
tests/ui/unchecked_time_subtraction_unfixable.stderr
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
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?

@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from d0874f8 to d141105 Compare June 28, 2025 23:23
@sharksforarms sharksforarms force-pushed the sharksforarms/duration-sub branch from d141105 to 2960444 Compare June 28, 2025 23:47
@sharksforarms
Copy link
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
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samueltardieu samueltardieu samueltardieu left review comments

@dswij dswij dswij left review comments

@blyxyas blyxyas blyxyas left review comments

Assignees

@dswij dswij

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duration arithmetic
6 participants
@sharksforarms @rustbot @alex-semenyuk @samueltardieu @dswij @blyxyas