feat: lint unchecked subtraction of a 'Duration' from an 'Instant'

[nfejzic]

Hello all, I tried to tackle the open issue #9371 and this is what I came up with.

I have a difficulty currently - some tests are failing:

failures:
    [ui] ui/manual_instant_elapsed.rs

The manual_instant_elapsed is failing because of Instant::now() - duration test, this now gets also picked by unchecked_duration_subtraction lint.
What is the correct way to proceed in this case? Simply update the .stderr file for manual_instant_elapsed lint?

changelog: [unchecked_duration_subtraction]: Add lint for unchecked subtraction of a Duration from an Instant.

fixes #9371

[rust-highfive]

r? @dswij

(rust-highfive has picked a reviewer for you, use r? to override)

[llogiq]

I think the best way would be to quell linting if the manual-instant-elapsed lint already picks up the expr. This might mean merging the two lint's passes to avoid duplicating the code.

[Alexendoo]

It currently always panics if the subtraction overflows - https://doc.rust-lang.org/1.64.0/src/std/time.rs.html#420-426

[bors]

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

[nfejzic]

I think the best way would be to quell linting if the manual-instant-elapsed lint already picks up the expr. This might mean merging the two lint's passes to avoid duplicating the code.

As far as I can see, the manual_instant_elapsed lint does not catch expression Instant::now() - duration.

In the test file for manual_instant_elapse there is this snippet:

// don't catch
let duration = prev_instant.elapsed();

Instant::now() - duration;

But manual_instant_elapsed does not produce a lint for the given snippet in stderr output file. However, the new unchecked_duration_subtraction does, and that produces a different stderr file, hence the test failing.

The manual_instant_elapsed lint catches subtraction of two Instants, but not Instant and Duration subtraction. One way to fix the test, would be to #![allow(clippy::unchecked_duration_subtraction)] together with other allowed lints.

Either way, I probably can merge the two lints. If we decide to do that, should we rename the (resulting) lint?

[llogiq]

No, we should keep the two lints, but we may be able to merge the lint passes, because if I understand correctly, one lint is basically a special case of the other or there is at least some overlap.

[nfejzic]

No, we should keep the two lints, but we may be able to merge the lint passes, because if I understand correctly, one lint is basically a special case of the other or there is at least some overlap.

I'm not sure about the overlap. Basically, the unchecked_duration_subtraction detects the duration subtraction in the test file:

Instant::now() - duration;

However, that same line is NOT detected by the manual_instant_elapsed lint.

Do you have an example of merging two lint passes? Would like to see how it works.

[llogiq]

Ah, I see, the manual elapsed lint checks for Instant - Instant, this checks for Instant - Duration.

Merging the LintPass is simply putting the relevant code into one pass. One usually starts by copying the lint definition + check_* code from one file to the other and then refactoring common expressions.

[nfejzic]

Merging the LintPass is simply putting the relevant code into one pass. One usually starts by copying the lint definition + check_* code from one file to the other and then refactoring common expressions.

I assume the file should be also renamed then?

I will start working on that and see how it goes.

Also, I think the manual_instant_elapsed checks only against Instant::now().
What is your opinion on this? Should I make it so that unchecked_duration_subtraction lints only against Instant::now() - duration, or should manual_elapsed_lint also check against any Instant, not only Instant::now() expressions?

[nfejzic]

@llogiq I found some time and managed to merge the manual_instant_elapsed and unchecked_duration_subtraction lint passes into a single pass. Hopefully I've done this right.

[bors]

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

[llogiq]

We don't merge from upstream, we rebasse onto it. I usually do this on the command line (git fetch master && git rebase master, git mergetool if necessary), but most git UI tools also have a workflow for this.

[nfejzic]

@llogiq How does it look now? I gave my best 👀

[llogiq]

Now for some reason CI seems to have excused itself. Don't worry, I'll look into it.

[nfejzic]

I just noticed that clippy_version for MANUAL_INSTANT_ELAPSED was changed some time ago to 1.65.0... I should probably push a commit for that, right?

[llogiq]

Yes, that makes sense.

[nfejzic]

@llogiq I think this is ready for review now.

[llogiq]

Thank you!

@bors r+

[bors]

📌 Commit 0dd6ce0 has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0dd6ce0 with merge 5b0d727...

[bors]

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