Add new lint [`manual_checked_sub`]

[benacq]

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

Partially fixes https://github.com/rust-lang/rust-clippy/issues/12894

changelog: new lint: [manual_checked_sub]

[rustbot]

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]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

• 90bb4983f12ee74e34f2b2f8e59ad9f6254de60f

[samueltardieu]

a.checked_sub(b) will return an Option. The test cases seem to "work" because the results are always ignored.

The following code will fail if the proposed fix is applied:

    if a >= b {
        let _ = (a-b)+1;
    }

[benacq]

a.checked_sub(b) will return an Option. The test cases seem to "work" because the results are always ignored.

The following code will fail if the proposed fix is applied:

    if a >= b {
        let _ = (a-b)+1;
    }

Thanks for the feedback, i think get what you mean. that will evaluate to

    a.checked_sub(b) + 1

which will try to do: Option<T> + 1, that will fail. i will rectify and update the PR

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

• 9141494e353477013d6d287d577b1e7b91e26e59

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

• 7bee23dcb05dfb7eb5f8135cf950717c726e77e2

[samueltardieu]

You should have a look at the output of lintcheck. Most of the suggestions are wrong because of .checked_sub(0), but once this is fixed, we should examine whether they are beneficial or not.

[benacq]

Thanks for the comprehensive feedback, I will resolve them all and submit an update.

You should have a look at the output of lintcheck. Most of the suggestions are wrong because of .checked_sub(0), but once this is fixed, we should examine whether they are beneficial or not.

[dswij]

r? @samueltardieu

Hope you don't mind since you reviewed this and are now a part of the clippy team :).

[samueltardieu]

@rustbot author

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

• d6c0130afbf733c3b4911febc6bf517793791fe7

[benacq]

I don't know what to think about the result name. It seems generic, and moreover it may exist. For example, the suggestion for this code will fail to compile:

let mut result = 0;
if a >= b {
    result = a - b;
}

because it will suggest

if let Some(result) = a.checked_sub(b) {
    result = result;
}

@samueltardieu I think this is the only feedback from your initial review that i have not fixed yet, i want to write a name generator function that checks the scope and append a counter to the name if it already exist but i am have a little challenge with checking the scope, could you please recommend some pointers on how i can go about this? or any existing implementation of this? thank you

[samueltardieu]

i want to write a name generator function that checks the scope and append a counter to the name if it already exist but i am have a little challenge with checking the scope, could you please recommend some pointers on how i can go about this? or any existing implementation of this? thank you

You could use a Visitor to check what variable names are used within the if block. But the real challenge will be to choose a meaningful name IMO.

[benacq]

i want to write a name generator function that checks the scope and append a counter to the name if it already exist but i am have a little challenge with checking the scope, could you please recommend some pointers on how i can go about this? or any existing implementation of this? thank you

You could use a Visitor to check what variable names are used within the if block. But the real challenge will be to choose a meaningful name IMO.

@samueltardieu i have update the pr with the implementation, what are your thoughts on the variable names used?

for if a >= b case, the variable name is difference and for if a > 0 case, the variable name is decremented

[samueltardieu]

This will be discussed during the FCP, but I am not a big fan of either names. I think that naming is hard, in general, and having a program generate a name which is equivalent to what a human would have chosen is difficult.

I think the lint is valuable in that it detects and warns about situation where .checked_sub() should be used. I am not so sure about the suggestion though, because of this naming problem. Maybe just linting would be greater.

[rustbot]

:umbrella: The latest upstream changes (possibly 8eed35023f26bc45ab86d5b8f080c4025faa58cf) made this pull request unmergeable. Please resolve the merge conflicts.

[benacq]

This will be discussed during the FCP, but I am not a big fan of either names. I think that naming is hard, in general, and having a program generate a name which is equivalent to what a human would have chosen is difficult.

I think the lint is valuable in that it detects and warns about situation where .checked_sub() should be used. I am not so sure about the suggestion though, because of this naming problem. Maybe just linting would be greater.

I agree with the naming challenge, i will go ahead and refactor to just emit the lint without a suggestion, if we later come up with a better naming, i can come back and make the fix.

i will work on the macro test case as well and update the PR. Thanks for the feedback

[rustbot]

:warning: Warning :warning:

• The following commits have merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

  • 33312260743e35c3ca3bef5aff5db2a4f4db9b8a

  You can start a rebase with the following commands:

  $ # rebase
  $ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
  $ git push --force-with-lease
  

[Jarcho]

There is #11789. It's still held up on other changes right now, but you should at least use the same lint name.

[benacq]

There is #11789. It's still held up on other changes right now, but you should at least use the same lint name.

Thanks for pointing this out, first time seeing this PR. If we generically catch all checked_* patterns under manual_checked_op lint, what happens when one wants to opt-out of a specific lint? Or doesn't that matter in this case?

Also I am not quite sure about changing the lint name, as that would significantly expand the scope of this PR that has undergone some initial reviews and iterations. I was considering picking up another checked_* lint after this gets approved.

@samueltardieu thoughts?

[Jarcho]

Thanks for pointing this out, first time seeing this PR. If we generically catch all checked_* patterns under manual_checked_op lint, what happens when one wants to opt-out of a specific lint? Or doesn't that matter in this case?

Also I am not quite sure about changing the lint name, as that would significantly expand the scope of this PR that has undergone some initial reviews and iterations. I was considering picking up another checked_* lint after this gets approved.

Having multiple lints is a downside on it's own and this case doesn't really gain anything by splitting them up. There are a large number of checked operations for integer types, all of which are clearer to use the checked_* method rather than implement them inline.

For changing the name you don't have to implement all of them in this PR, just leave a note in the docs for which operations are currently handled.

[rustbot]

:umbrella: The latest upstream changes (possibly 7bac114c8645d40291502c0d8028ffd16c7e8c01) made this pull request unmergeable. Please resolve the merge conflicts.

[Jarcho]

Ping @benacq from triage. Do you plan to return to working on this?

[Jarcho]

Closing since there's been no response. @benacq if you would like to return to working on this feel free to reopen or create a new PR.