[`arithmetic_side_effects`] Fix #15943 #15950

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

clippy_lints/src/operators/arithmetic_side_effects.rs

-    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

clippy_lints/src/operators/arithmetic_side_effects.rs

-            && 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

clippy_lints/src/operators/arithmetic_side_effects.rs

+
+/// 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

clippy_lints/src/operators/arithmetic_side_effects.rs

+    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

clippy_lints/src/operators/arithmetic_side_effects.rs

+    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

clippy_lints/src/operators/arithmetic_side_effects.rs

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


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

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

clippy_lints/src/operators/arithmetic_side_effects.rs

+/// 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

clippy_lints/src/operators/arithmetic_side_effects.rs

        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

clippy_lints/src/operators/arithmetic_side_effects.rs

+        let has_valid_op = 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 (
-                Self::literal_integer(cx, actual_lhs),
-                Self::literal_integer(cx, actual_rhs),
-            ) {
+            match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {
                (None, None) => false,
                (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 => true,
                    // 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)
                    => true,
                    _ => false,
                },
                (Some(n), None) => match (&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)
                    => true,
                    _ => false,
                },
                (Some(_), Some(_)) => {
                    matches!((lhs_ref_counter, rhs_ref_counter), (0, 0))
                },
            }
        } else {
            false
        };
        if !has_valid_op {
            self.issue_lint(cx, expr);
        }


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

clippy_lints/src/operators/arithmetic_side_effects.rs

+/// 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.

samueltardieu · 2025-10-27T18:30:24Z

clippy_lints/src/operators/arithmetic_side_effects.rs

@@ -1,12 +1,13 @@
 use super::ARITHMETIC_SIDE_EFFECTS;
+use crate::clippy_utils::res::MaybeQPath;


Please add as _ since you only need the trait for its methods.

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

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

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

		@@ -1,12 +1,13 @@
		use super::ARITHMETIC_SIDE_EFFECTS;
		use crate::clippy_utils::res::MaybeQPath;

Uh oh!

[arithmetic_side_effects] Fix #15943 #15950

Are you sure you want to change the base?

[arithmetic_side_effects] Fix #15943 #15950

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

[`arithmetic_side_effects`] Fix #15943 #15950

[`arithmetic_side_effects`] Fix #15943 #15950

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