Consider type conversion that won't overflow by c410-f3r · Pull Request #15950 · rust-lang/rust-clippy

c410-f3r · 2025-10-25T17:17:44Z

pub fn from_days(days: u8) -> Duration {
    Duration::from_secs(86400 * u64::from(days))
}

An initial foundation that can be used for future improvements. It is been a while since I touched Clippy so feel free to indicate better designs if applicable.

Looks like #15342 will help but it is unclear when it will be finished. This PR provides a partial solution for the current time.

changelog: [arithmetic_side_effects]: Consider type conversion that won't overflow

rustbot · 2025-10-25T17:17:48Z

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

c410-f3r · 2025-10-25T17:19:22Z

-    fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<u128> {
-        let actual = peel_hir_expr_unary(expr).0;
-        if let hir::ExprKind::Lit(lit) = actual.kind
-            && let ast::LitKind::Int(n, _) = lit.node
-        {
-            return Some(n.get());
-        }
-        if let Some(Constant::Int(n)) = ConstEvalCtxt::new(cx).eval(expr) {
-            return Some(n);
-        }
-        None
-    }


Not sure why I placed these non-self mehods inside the impl block. It it better to put them outside IMO.

github-actions · 2025-10-25T17:36:30Z

Lintcheck changes for b7f9216

Lint	Added	Removed	Changed
`clippy::arithmetic_side_effects`	0	24	3

This comment will be updated if you push new changes

c410-f3r · 2025-10-26T11:37:17Z

r? clippy

samueltardieu

This currently removes too many rightful instances of the lint. This is not safe to broadly assume that any function taking an u8 and returning a u64 will limit the return values to the range of u8. For example, the following code overflows and this PR suppresses the warning from Clippy:

use std::time::Duration;

fn shift(x: u8) -> u64 {
    1 << u64::from(x)
}

fn main() {
    _ = Duration::from_secs(86400 * shift(1));
}

View changes since this review

samueltardieu · 2025-10-27T07:24:04Z

-            && Self::is_non_zero_u(cx, receiver_ty)
-            && let Some(1) = Self::literal_integer(cx, actual_rhs)
+            && is_non_zero_u(cx, receiver_ty)
+            && let Some(1) = literal_integer(cx, actual_rhs)


Suggested change

&& let Some(1) = literal_integer(cx, actual_rhs)

&& literal_integer(cx, actual_rhs) == Some(1)

Not my code -> a745e2c

samueltardieu · 2025-10-27T07:24:34Z

+
+/// For example, if the variable `foo` of type `u32` comes from another variable `bar` of type
+/// `u8` like `let foo = u64::from(bar)`.
+fn find_original_primitive_ty<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option<Ty<'tcx>> {


samueltardieu · 2025-10-27T07:25:46Z

+    rhs_ty: Ty<'tcx>,
+) -> bool {
+    let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
+    let is_sat_or_wrap = |ty: Ty<'_>| ty.is_diag_item(cx, sym::Saturating) || ty.is_diag_item(cx, sym::Wrapping);


You can do one lookup instead of two by getting the diag item name and matching against it.

samueltardieu · 2025-10-27T07:27:15Z

+    let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
+    let is_sat_or_wrap = |ty: Ty<'_>| ty.is_diag_item(cx, sym::Saturating) || ty.is_diag_item(cx, sym::Wrapping);
+
+    // If the RHS is `NonZero<u*>`, then division or module by zero will never occur.


Please use proper doc comments /// to document functions (instead of simple code comments), so that the documentation can pop up in IDEs.

Not my code -> a51fc2a

samueltardieu · 2025-10-27T07:27:59Z

+    false
+}
+
+// For example, `i8` or `u128`


This should also mention that it matches &&i8 for example.

samueltardieu · 2025-10-27T07:34:13Z

+/// smaller type. `0` and `1` suffixes indicate different sides.
+///
+/// For example, `1000u64 + u64::from(some_runtime_variable_of_type_u8)`.
+fn manage_integer_types(


manage is unclear, especially when it looks like it does not perform any side effect operation and only returns a boolean.

samueltardieu · 2025-10-27T07:39:25Z

        if self.has_allowed_binary(lhs_ty, rhs_ty) {
            return;
        }
-        if Self::has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {
+        if has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {
            return;
        }
-
-        let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {
+        if manage_integer_types(cx, op, (actual_lhs, lhs_ty), actual_rhs) {
+            return;
+        }
+        if manage_integer_types(cx, op, (actual_rhs, rhs_ty), actual_lhs) {
+            return;
+        }


Suggested change

if self.has_allowed_binary(lhs_ty, rhs_ty) {

return;

}

if Self::has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {

if has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {

return;

}

let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {

if manage_integer_types(cx, op, (actual_lhs, lhs_ty), actual_rhs) {

return;

}

if manage_integer_types(cx, op, (actual_rhs, rhs_ty), actual_lhs) {

return;

}

if self.has_allowed_binary(lhs_ty, rhs_ty)

|| has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty)

|| manage_integer_types(cx, op, (actual_lhs, lhs_ty), actual_rhs)

|| manage_integer_types(cx, op, (actual_rhs, rhs_ty), actual_lhs)

{

return;

}

samueltardieu · 2025-10-27T07:49:20Z

Since early returns are used throughout this lint, it makes sense to keep using them instead of going through a has_valid_op boolean variable.

Suggested change

if is_integer(lhs_ty) && is_integer(rhs_ty) {

if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op {

// At least for integers, shifts are already handled by the CTFE

return;

}

match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {

(None, Some(n)) => match (&op, n) {

// Division and module are always valid if applied to non-zero integers

(hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => return,

// Adding or subtracting zeros is always a no-op

(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)

// Multiplication by 1 or 0 will never overflow

| (hir::BinOpKind::Mul, 0 | 1)

=> return,

_ => (),

},

(Some(n), None)

if matches!(

(&op, n),

// Adding or subtracting zeros is always a no-op

(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)

// Multiplication by 1 or 0 will never overflow

| (hir::BinOpKind::Mul, 0 | 1)

) =>

{

return;

},

(Some(_), Some(_)) if matches!((lhs_ref_counter, rhs_ref_counter), (0, 0)) => return,

_ => (),

}

}

self.issue_lint(cx, expr);

samueltardieu · 2025-10-27T07:50:56Z

You also need to remove the issue number from the commit message. And why did you ask for a reassignment of this PR?

c410-f3r · 2025-10-27T13:26:30Z

This currently removes too many rightful instances of the lint. This is not safe to broadly assume that any function taking an u8 and returning a u64 will limit the return values to the range of u8

Ops... Fixed.

You also need to remove the issue number from the commit message.

Done

And why did you ask for a reassignment of this PR?

It is not rare to wait weeks or months for a review and I personally experimented many occasions where the assigned reviewer passed the task to another member after a long hiatus. Most individuals are volunteers with limited free time, therefore, it is reasonable to seek help from reviewers that are more active at the current time unless they are too overloaded.

And I also would like to tackle more features like signed integers in a following PR instead of delivering everything at once to make things a little easier.

c410-f3r · 2025-10-27T13:29:50Z

The move of non-self methods was meant to be a verbatim operation. Maybe I should have made that clear in the description or created a separate PR.

Regardless, all concerns were addressed. Let me know if there is anything left.

samueltardieu · 2025-10-27T13:35:57Z

+/// example, `let foo = u64::from(bar)`.
+fn find_original_primitive_ty<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option<Ty<'tcx>> {
+    if let hir::ExprKind::Call(path, [arg]) = &expr.kind
+        && let hir::ExprKind::Path(hir::QPath::TypeRelative(_, segment)) = &path.kind


It will be easy to design a BitShift::from(u8) -> u64 which will again work as a counter example while passing through this check.

You should probably check whether this is a call to an implementation of From::from.

let [sym::core, sym::convert, sym::From, sym::from] = cx.get_def_path(def_id).as_slice() works as intended but I don't know of a better solution

path.res(cx).opt_def_id().is_diag_item(&cx.tcx, sym::from_fn) should work

Can you add a test with another from (not from the From trait) to see that it works as expected?

samueltardieu · 2025-10-27T13:37:35Z

The move of non-self methods was meant to be a verbatim operation. Maybe I should have made that clear in the description or created a separate PR.

A separate commit (in the same PR) would have make this clearer.

c410-f3r · 2025-11-04T19:30:20Z

ping @samueltardieu

samueltardieu · 2025-11-04T22:43:30Z

ping @samueltardieu

When review comments have been adressed, you should re-request a review which will change the S-waiting-on-author label to the S-waiting-on-review one, or use "@rustbot review".

I'll add this to my todo list.

samueltardieu · 2025-11-05T00:07:59Z

@rustbot blocked by #16032

rustbot assigned dswij Oct 25, 2025

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

This comment has been minimized.

Sign in to view

c410-f3r commented Oct 25, 2025

View reviewed changes

c410-f3r force-pushed the dasdaswe branch from 6726be8 to 6b7a22e Compare October 25, 2025 17:26

This comment has been minimized.

Sign in to view

rustbot assigned llogiq and samueltardieu and unassigned dswij and llogiq Oct 26, 2025

samueltardieu requested changes Oct 27, 2025

View reviewed changes

rustbot 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 Oct 27, 2025

c410-f3r force-pushed the dasdaswe branch from 6b7a22e to 2f184cc Compare October 27, 2025 13:14

samueltardieu requested changes Oct 27, 2025

View reviewed changes

c410-f3r force-pushed the dasdaswe branch 5 times, most recently from 22ec98f to d6c29ac Compare October 27, 2025 18:28

samueltardieu reviewed Oct 27, 2025

View reviewed changes

Comment thread clippy_lints/src/operators/arithmetic_side_effects.rs Outdated

Consider type conversion that won't overflow

b7f9216

c410-f3r force-pushed the dasdaswe branch from d6c29ac to b7f9216 Compare October 27, 2025 18:41

samueltardieu 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 Nov 4, 2025

samueltardieu approved these changes Nov 4, 2025

View reviewed changes

samueltardieu changed the title ~~[arithmetic_side_effects] Fix #15943~~ Consider type conversion that won't overflow Nov 4, 2025

samueltardieu enabled auto-merge November 4, 2025 23:33

samueltardieu added this pull request to the merge queue Nov 4, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 4, 2025

rustbot added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 5, 2025

samueltardieu added this pull request to the merge queue Nov 5, 2025

Merged via the queue into rust-lang:master with commit 4eac58a Nov 5, 2025
13 checks passed

	&& let Some(1) = literal_integer(cx, actual_rhs)
	&& literal_integer(cx, actual_rhs) == Some(1)

+        if is_integer(lhs_ty) && is_integer(rhs_ty) {
+            if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op {
+                // At least for integers, shifts are already handled by the CTFE
+                return;
+            }
+            match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {
+                (None, Some(n)) => match (&op, n) {
+                    // Division and module are always valid if applied to non-zero integers
+                    (hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => return,
+                    // Adding or subtracting zeros is always a no-op
+                    (hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
+                    // Multiplication by 1 or 0 will never overflow
+                    | (hir::BinOpKind::Mul, 0 | 1)
+                    => return,
+                    _ => (),
+                },
+                (Some(n), None)
+                    if matches!(
+                        (&op, n),
+                        // Adding or subtracting zeros is always a no-op
+                        (hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
+                        // Multiplication by 1 or 0 will never overflow
+                        | (hir::BinOpKind::Mul, 0 | 1)
+                    ) =>
+                {
+                    return;
+                },
+                (Some(_), Some(_)) if matches!((lhs_ref_counter, rhs_ref_counter), (0, 0)) => return,
+                _ => (),
+            }
+        }
+        self.issue_lint(cx, expr);

Uh oh!

Uh oh!

Conversation

c410-f3r commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 25, 2025

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions Bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c410-f3r commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Oct 27, 2025

Uh oh!

c410-f3r commented Oct 27, 2025

Uh oh!

c410-f3r commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Oct 27, 2025

Uh oh!

Uh oh!

c410-f3r commented Nov 4, 2025

Uh oh!

samueltardieu commented Nov 4, 2025

Uh oh!

Uh oh!

samueltardieu commented Nov 5, 2025

Uh oh!

Uh oh!

c410-f3r commented Oct 25, 2025 •

edited

Loading

github-actions Bot commented Oct 25, 2025 •

edited

Loading

c410-f3r commented Oct 26, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

c410-f3r commented Oct 27, 2025 •

edited

Loading