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

samueltardieu · 2025-03-05T17:04:32Z

clippy_lints/src/manual_checked_sub.rs

+    if let ExprKind::Lit(lit) = &expr.kind {
+        if let LitKind::Int(_, LitIntType::Unsuffixed) = &lit.node {
+            return true;
+        }
+    }


When can this part trigger? Wouldn't the expression be typed already? Is there a test which fails if we remove this?

Just realized i didn't include the test case for this in this commit, but this triggers for untyped integer literals that can be inferred as unsigned int.
example expression:

if 10 >= 5 { let _ = 10 - 5; }

Just realized i didn't include the test case for this in this commit, but this triggers for untyped integer literals that can be inferred as unsigned int. example expression:

if 10 >= 5 { let _ = 10 - 5; }

Does it? Because in are_literals_equal() it looks like you require literals to be suffixed in order to return true, so unsuffixed literals should not match.

I think the literal minus literal case can be forgotten about anyway, as I said, it makes no sense to suggest using .checked_sub() when the result can be determined at compilation time.

Yes, that's right, i just added the test case locally and realized it still failed. However that was the intended use of that snippet.
I'm not sure if i get your last statement correctly, are you suggesting we don't trigger .checked_sub lint when we are dealing with literals?

Oh got it, never mind.

i missed this part of the review when i was going through it, just saw it, thanks again for feedback, will send and update soon.

samueltardieu · 2025-03-05T17:05:31Z

clippy_lints/src/manual_checked_sub.rs

+            return;
+        }
+
+        let applicability = Applicability::MachineApplicable;


Why define a variable if its value cannot change? You can use the enum variant directly instead of this variable. In fact, since SubExprVisitor::applicability is not modified during the traversal either, this field can be dropped entirely, and made local to SubExprVisitor::build_suggession().

samueltardieu · 2025-03-05T17:06:45Z

clippy_lints/src/manual_checked_sub.rs

+        let applicability = Applicability::MachineApplicable;
+
+        if let ExprKind::If(drop_temp_expr, body_block, else_block) = expr.kind
+            && let ExprKind::DropTemps(condition_expr) = drop_temp_expr.kind


By using clippy_utils::higher::If::hir(expr), you would not have to deal with DropTemps, especially if at one point the HIR changes, that would be covered by the higher module for every lint.

samueltardieu · 2025-03-05T17:08:08Z

clippy_lints/src/manual_checked_sub.rs

+                SubExprVisitor {
+                    cx,
+                    if_expr: expr,
+                    if_body_block: body_block,
+                    else_block,
+                    applicability,
+                    condition_lhs: lhs,
+                    condition_rhs: rhs,
+                }
+                .visit_expr(body_block);
+            }
+
+            if let BinOpKind::Gt = op.node {
+                if let ExprKind::Lit(lit) = &rhs.kind
+                    && let LitKind::Int(Pu128(0), _) = lit.node
+                {
+                    SubExprVisitor {
+                        cx,
+                        if_expr: expr,
+                        if_body_block: body_block,
+                        else_block,
+                        applicability,
+                        condition_lhs: lhs,
+                        condition_rhs: rhs,
+                    }
+                    .visit_expr(body_block);


Both code fragments seem identical, maybe they should be factored in. Also, is it right that this code will recognize if a >= b, but not if b <= a? Same question with a > 0 vs. a < 0.

samueltardieu · 2025-03-05T17:12:44Z

tests/ui/manual_checked_sub.fixed

+    }
+
+    // Decrementing inside an if condition
+    if let Some(result) = a.checked_sub(0)  {


This looks wrong.

samueltardieu · 2025-03-05T17:28:59Z

clippy_lints/src/manual_checked_sub.rs

+        }
+    }
+    false
+}


A blank line is missing after this line.

samueltardieu · 2025-03-05T17:30:20Z

clippy_lints/src/manual_checked_sub.rs

+    }
+    false
+}
+fn is_referencing_same_variable<'tcx>(expr1: &'tcx Expr<'_>, expr2: &'tcx Expr<'_>) -> bool {


Couldn't this function be made simpler by using clippy_utils::path_to_local()?

Thanks for mentioning this, looks super helpful, however, one limitation i noticed is that, it does not work for casted types. issue here

samueltardieu · 2025-03-05T17:31:08Z

clippy_utils/src/msrvs.rs

@@ -50,7 +50,7 @@ msrv_aliases! {
    1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
    1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS }
    1,50,0 { BOOL_THEN, CLAMP, SLICE_FILL }
-    1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST }
+    1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, MANUAL_CHECKED_SUB }


Suggested change

1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, MANUAL_CHECKED_SUB }

1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, CHECKED_SUB }

samueltardieu · 2025-03-05T17:35:31Z

clippy_lints/src/manual_checked_sub.rs

+}
+
+impl<'tcx> SubExprVisitor<'_, 'tcx> {
+    fn build_suggession(&mut self, expr: &Expr<'tcx>) {


The name has several issues:

Typo (suggession → suggestion)

It doesn't build anything (a builder usually returns a value).

Maybe emit_lint() would be a better name.

samueltardieu · 2025-03-05T17:37:14Z

clippy_lints/src/methods/map_with_unused_argument_over_ranges.rs

-                    RangeLimits::HalfOpen => upper_bound - lower_bound,
-                    RangeLimits::Closed => (upper_bound - lower_bound).checked_add(1)?,
+                    RangeLimits::HalfOpen => result,
+                    RangeLimits::Closed => (result).checked_add(1)?,


Extra parenthesis in fix.

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

benacq · 2025-04-07T20:59:39Z

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 · 2025-04-07T21:31:04Z

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.

clippy_lints/src/manual_checked_sub.rs

…nerator function

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

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

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Apr 7, 2025

This comment has been minimized.

Sign in to view

benacq added 15 commits April 7, 2025 17:50

Add new lint [] init

42110d8

Lint test case false positives corrected

71a7ff4

Lint test case false positives corrected

e11ce77

Lint test case false positives corrected

fc2c9ab

Lint test case false positives corrected

3d541b5

Lint test case false positives corrected

ad4bccd

Lint test case false positives corrected

a8cc803

Lint test case false positives corrected

830891d

Lint test case false positives corrected

e865ea3

Lint test case false positives corrected

0f75ce5

Lint test case false positives corrected

91a3edd

Lint test case false positives corrected

931b25f

In progress

50dcdb2

manual_checked_sub implementation review feedback fixes

9e849a2

manual_checked_sub implementation review feedback fixes

0bbdebb

benacq force-pushed the manual_checked_sub branch from d6c0130 to 0bbdebb Compare April 7, 2025 17:50

rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Apr 7, 2025

benacq added 3 commits April 7, 2025 17:56

manual_checked_sub implementation review feedback fixes

000dd0a

manual_checked_sub implementation review feedback fixes - Improved ru…

0f0b13a

…stdoc example

manual_checked_sub implementation review feedback fixes - Improved ru…

5b2d8aa

…stdoc example

samueltardieu reviewed Apr 7, 2025

View reviewed changes

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

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

benacq added 4 commits April 8, 2025 07:24

manual_checked_sub implementation review feedback fixes - Variable ge…

76a0146

…nerator function

manual_checked_sub implementation review feedback fixes - Variable ge…

bb6f056

…nerator function

manual_checked_sub implementation review feedback fixes - Variable ge…

4199acf

…nerator function

manual_checked_sub implementation review feedback fixes - Variable ge…

1977f26

…nerator function

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 Mar 5, 2025

benacq Mar 8, 2025

samueltardieu Mar 8, 2025

benacq Mar 8, 2025

benacq Mar 8, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025 •

edited

Loading

benacq Mar 8, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025

samueltardieu Mar 5, 2025

samueltardieu commented Mar 5, 2025

benacq commented Mar 5, 2025

dswij commented Mar 27, 2025

samueltardieu commented Mar 27, 2025

This comment has been minimized.

benacq commented Apr 7, 2025

samueltardieu commented Apr 7, 2025

	1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, MANUAL_CHECKED_SUB }
	1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST, CHECKED_SUB }

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

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

samueltardieu Mar 5, 2025 • edited Loading

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

samueltardieu commented Mar 5, 2025

benacq commented Mar 5, 2025

dswij commented Mar 27, 2025

samueltardieu commented Mar 27, 2025

This comment has been minimized.

benacq commented Apr 7, 2025

samueltardieu commented Apr 7, 2025

Add new lint [`manual_checked_sub`] #14236

Add new lint [`manual_checked_sub`] #14236

samueltardieu Mar 5, 2025 •

edited

Loading