Add new lint [`manual_checked_sub`] #14236

benacq · 2025-02-16T22:53:09Z

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

Partially fixes #12894

changelog: new lint: [manual_checked_sub]

rustbot · 2025-02-16T22:53:14Z

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

samueltardieu · 2025-02-17T09:48:14Z

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 · 2025-02-17T16:57:08Z

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

pitaj

Couple nits

clippy_lints/src/dereference.rs

clippy_lints/src/manual_checked_sub.rs

samueltardieu

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;
}

clippy_lints/src/manual_checked_sub.rs

tests/ui/manual_checked_sub.fixed

clippy_lints/src/manual_checked_sub.rs

clippy_utils/src/msrvs.rs

clippy_lints/src/manual_checked_sub.rs

clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs

samueltardieu · 2025-03-05T18:14:12Z

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 · 2025-03-05T20:21:38Z

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 · 2025-03-27T06:01:54Z

r? @samueltardieu

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

samueltardieu · 2025-03-27T07:00:19Z

@rustbot author

…stdoc example

…nerator function

This reverts commit ae4d493.

Jarcho · 2025-04-24T10:26:48Z

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

benacq · 2025-04-24T12:16:38Z

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?

samueltardieu

After you've handled the review comments, could you please rebase/squash your code into two commits?

one commit with the new lint
one commit with the fixes due to this lint in Clippy source code itself

Once the review is satisfactory, I will open a FCP thread on Zulip where we will also discuss the lint name/granularity as mentioned in #14236 (comment).

samueltardieu · 2025-05-06T08:37:34Z

clippy_lints/.DS_Store

This file should be removed

samueltardieu · 2025-05-06T08:37:46Z

clippy_lints/src/.DS_Store

This file should be removed

samueltardieu · 2025-05-06T08:40:13Z

clippy_lints/src/manual_checked_sub.rs

+
+impl<'tcx> LateLintPass<'tcx> for ManualCheckedSub {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if !self.msrv.meets(cx, msrvs::CHECKED_SUB) {


MSRV checking is more expensive than checking for the presence of an if expression or the fact that there is an unsigned subtraction, so it should rather be moved at the end of the subsequent if let check.

samueltardieu · 2025-05-06T08:41:08Z

clippy_lints/src/manual_checked_sub.rs

+            if (!matches!(lhs.kind, ExprKind::Lit(_)) && is_mutable(cx, lhs))
+                || (!matches!(rhs.kind, ExprKind::Lit(_)) && is_mutable(cx, rhs))
+            {
+                return;
+            }
+
+            // Skip if either lhs or rhs is a macro call
+            if lhs.span.from_expansion() || rhs.span.from_expansion() {
+                return;
+            }
+
+            if let BinOpKind::Ge | BinOpKind::Gt | BinOpKind::Le | BinOpKind::Lt = op.node {


Those checks can probably be merged into the earlier if let test.

samueltardieu · 2025-05-06T08:45:21Z

clippy_lints/src/manual_checked_sub.rs

+}
+
+impl SubExprVisitor<'_, '_> {
+    fn emit_lint(&mut self) {


Suggested change

fn emit_lint(&mut self) {

fn emit_lint(&self) {

samueltardieu · 2025-05-06T08:52:35Z

clippy_lints/src/manual_checked_sub.rs

+fn is_unsigned_int<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
+    let expr_type = cx.typeck_results().expr_ty(expr).peel_refs();
+    if matches!(expr_type.kind(), ty::Uint(_)) {
+        return true;
+    }
+
+    false
+}


You can use the shorter

Suggested change

fn is_unsigned_int<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {

let expr_type = cx.typeck_results().expr_ty(expr).peel_refs();

if matches!(expr_type.kind(), ty::Uint(_)) {

return true;

}

false

}

fn is_unsigned_int(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {

matches!(cx.typeck_results().expr_ty(expr).peel_refs().kind(), ty::Uint(_))

}

Also, please add tests with references since you seem to need to peel them.

samueltardieu · 2025-05-06T08:58:29Z

tests/ui/implicit_saturating_sub.rs

+    unused_assignments,
+    unused_mut,
+    clippy::assign_op_pattern,
+    clippy::manual_checked_sub


I'm not sure modifying this test file is needed, as implicit_saturating_sub requires the modified variable to be mutable, which you filter out.

samueltardieu · 2025-05-06T09:00:54Z

tests/ui/manual_abs_diff.rs

@@ -1,5 +1,5 @@
+#![allow(clippy::manual_checked_sub)]


Why is this needed? You should not detect situations like

if a <= b { b - a } else { a - b }

in your lint, or at least you should make sure that it only triggers if manual_abs_diff doesn't.

samueltardieu · 2025-05-06T09:01:17Z

tests/ui/manual_abs_diff.rs

 #![warn(clippy::manual_abs_diff)]
-


Gratuitous line removal

samueltardieu · 2025-05-06T09:01:45Z

tests/ui/manual_checked_sub.rs

@@ -0,0 +1,150 @@
+#![allow(clippy::collapsible_if)]
+// #![allow(clippy::manual_abs_diff)]


This commented out line should be removed

Jarcho · 2025-05-07T07:36:30Z

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 · 2025-05-13T05:41:59Z

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

rustbot assigned dswij Feb 16, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2025

This comment has been minimized.

Sign in to view

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 16, 2025

benacq force-pushed the manual_checked_sub branch from 90bb498 to 98b1666 Compare February 16, 2025 22:59

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 16, 2025

benacq changed the title ~~Add new lint [Manual checked sub]~~ Add new lint [manual_checked_sub] Feb 16, 2025

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 1, 2025

This comment has been minimized.

Sign in to view

benacq force-pushed the manual_checked_sub branch from 9141494 to 414b39d Compare March 1, 2025 23:20

This comment has been minimized.

Sign in to view

benacq force-pushed the manual_checked_sub branch from 414b39d to 5ef4f44 Compare March 1, 2025 23:24

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 1, 2025

pitaj suggested changes Mar 5, 2025

View reviewed changes

clippy_lints/src/dereference.rs Show resolved Hide resolved

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

samueltardieu reviewed Mar 5, 2025

View reviewed changes

benacq mentioned this pull request Mar 9, 2025

extend clippy_utils::path_to_local() to handle Cast expressions #14378

Closed

rustbot assigned samueltardieu and unassigned dswij Mar 27, 2025

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 27, 2025

benacq added 19 commits April 24, 2025 08:53

Lint test case false positives corrected

bdf9ac8

Lint test case false positives corrected

2e85d1a

Lint test case false positives corrected

e0b131a

Lint test case false positives corrected

797075a

Lint test case false positives corrected

2669fc9

Lint test case false positives corrected

06383da

Lint test case false positives corrected

6b034a7

In progress

0e66fd9

manual_checked_sub implementation review feedback fixes

99be19d

manual_checked_sub implementation review feedback fixes

e3b0262

manual_checked_sub implementation review feedback fixes

cc58db7

manual_checked_sub implementation review feedback fixes - Improved ru…

9d21893

…stdoc example

manual_checked_sub implementation review feedback fixes - Improved ru…

46d448f

…stdoc example

manual_checked_sub implementation review feedback fixes - Variable ge…

31aca94

…nerator function

manual_checked_sub implementation review feedback fixes - Variable ge…

510d847

…nerator function

manual_checked_sub implementation review feedback fixes - Variable ge…

8d8dece

…nerator function

manual_checked_sub implementation review feedback fixes - Variable ge…

3f42120

…nerator function

Macro call guard and lint suggestion refactor

3cb4e86

Revert "Macro guard implementation and lint suggestion refector"

b1f3340

This reverts commit ae4d493.

benacq force-pushed the manual_checked_sub branch from fbaf8ad to b1f3340 Compare April 24, 2025 09:00

rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Apr 24, 2025

Macro guard implementation and lint suggestion refactor

e61986c

Macro guard implementation and lint suggestion refactor

090496e

samueltardieu requested changes May 6, 2025

View reviewed changes

rustbot added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label May 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new lint [`manual_checked_sub`] #14236

Add new lint [`manual_checked_sub`] #14236

benacq commented Feb 16, 2025

rustbot commented Feb 16, 2025

This comment has been minimized.

samueltardieu commented Feb 17, 2025

benacq commented Feb 17, 2025

This comment has been minimized.

This comment has been minimized.

pitaj left a comment

samueltardieu left a comment

samueltardieu commented Mar 5, 2025

benacq commented Mar 5, 2025

dswij commented Mar 27, 2025

samueltardieu commented Mar 27, 2025

Jarcho commented Apr 24, 2025

benacq commented Apr 24, 2025

samueltardieu left a comment

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

samueltardieu May 6, 2025

Jarcho commented May 7, 2025

rustbot commented May 13, 2025

		@@ -0,0 +1,150 @@
		#![allow(clippy::collapsible_if)]
		// #![allow(clippy::manual_abs_diff)]

Add new lint [manual_checked_sub] #14236

Are you sure you want to change the base?

Add new lint [manual_checked_sub] #14236

Conversation

benacq commented Feb 16, 2025

rustbot commented Feb 16, 2025

This comment has been minimized.

samueltardieu commented Feb 17, 2025

benacq commented Feb 17, 2025

This comment has been minimized.

This comment has been minimized.

pitaj left a comment

Choose a reason for hiding this comment

samueltardieu left a comment

Choose a reason for hiding this comment

samueltardieu commented Mar 5, 2025

benacq commented Mar 5, 2025

dswij commented Mar 27, 2025

samueltardieu commented Mar 27, 2025

Jarcho commented Apr 24, 2025

benacq commented Apr 24, 2025

samueltardieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarcho commented May 7, 2025

rustbot commented May 13, 2025

Add new lint [`manual_checked_sub`] #14236

Add new lint [`manual_checked_sub`] #14236