Extend {implicit,inverted}_saturating_sub to expressions (rust-lang#14310)

llogiq · web-flow · commit 817853044f15 · 2025-03-01T05:51:35.000Z
changelog: [`implicit_saturating_sub`, `inverted_saturating_sub`]: extend lints from local variables to side-effect free expressions Noticed when rust-lang#14308 introduced an implicit `saturating_sub` operation and didn't get tagged.
diff --git a/clippy_lints/src/doc/mod.rs b/clippy_lints/src/doc/mod.rs
@@ -1010,11 +1010,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
                                 start -= 1;
                             }
 
-                            if start > range.start {
-                                start - range.start
-                            } else {
-                                0
-                            }
+                            start.saturating_sub(range.start)
                         }
                     } else {
                         0
diff --git a/clippy_lints/src/implicit_saturating_sub.rs b/clippy_lints/src/implicit_saturating_sub.rs
@@ -1,14 +1,14 @@
 use clippy_config::Conf;
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::source::snippet_opt;
+use clippy_utils::sugg::{Sugg, make_binop};
 use clippy_utils::{
-    SpanlessEq, higher, is_in_const_context, is_integer_literal, path_to_local, peel_blocks, peel_blocks_with_stmt,
+    SpanlessEq, eq_expr_value, higher, is_in_const_context, is_integer_literal, peel_blocks, peel_blocks_with_stmt,
 };
 use rustc_ast::ast::LitKind;
 use rustc_data_structures::packed::Pu128;
 use rustc_errors::Applicability;
-use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind, HirId, QPath};
+use rustc_hir::{BinOp, BinOpKind, Expr, ExprKind, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
 use rustc_span::Span;
@@ -170,22 +170,20 @@ fn check_gt(
     cx: &LateContext<'_>,
     condition_span: Span,
     expr_span: Span,
-    big_var: &Expr<'_>,
-    little_var: &Expr<'_>,
+    big_expr: &Expr<'_>,
+    little_expr: &Expr<'_>,
     if_block: &Expr<'_>,
     else_block: &Expr<'_>,
     msrv: Msrv,
     is_composited: bool,
 ) {
-    if let Some(big_var) = Var::new(big_var)
-        && let Some(little_var) = Var::new(little_var)
-    {
+    if is_side_effect_free(cx, big_expr) && is_side_effect_free(cx, little_expr) {
         check_subtraction(
             cx,
             condition_span,
             expr_span,
-            big_var,
-            little_var,
+            big_expr,
+            little_expr,
             if_block,
             else_block,
             msrv,
@@ -194,27 +192,17 @@ fn check_gt(
     }
 }
 
-struct Var {
-    span: Span,
-    hir_id: HirId,
-}
-
-impl Var {
-    fn new(expr: &Expr<'_>) -> Option<Self> {
-        path_to_local(expr).map(|hir_id| Self {
-            span: expr.span,
-            hir_id,
-        })
-    }
+fn is_side_effect_free(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    eq_expr_value(cx, expr, expr)
 }
 
 #[allow(clippy::too_many_arguments)]
 fn check_subtraction(
     cx: &LateContext<'_>,
     condition_span: Span,
     expr_span: Span,
-    big_var: Var,
-    little_var: Var,
+    big_expr: &Expr<'_>,
+    little_expr: &Expr<'_>,
     if_block: &Expr<'_>,
     else_block: &Expr<'_>,
     msrv: Msrv,
@@ -234,8 +222,8 @@ fn check_subtraction(
             cx,
             condition_span,
             expr_span,
-            little_var,
-            big_var,
+            little_expr,
+            big_expr,
             else_block,
             if_block,
             msrv,
@@ -247,17 +235,15 @@ fn check_subtraction(
         && let ExprKind::Binary(op, left, right) = if_block.kind
         && let BinOpKind::Sub = op.node
     {
-        let local_left = path_to_local(left);
-        let local_right = path_to_local(right);
-        if Some(big_var.hir_id) == local_left && Some(little_var.hir_id) == local_right {
+        if eq_expr_value(cx, left, big_expr) && eq_expr_value(cx, right, little_expr) {
             // This part of the condition is voluntarily split from the one before to ensure that
             // if `snippet_opt` fails, it won't try the next conditions.
-            if let Some(big_var_snippet) = snippet_opt(cx, big_var.span)
-                && let Some(little_var_snippet) = snippet_opt(cx, little_var.span)
-                && (!is_in_const_context(cx) || msrv.meets(cx, msrvs::SATURATING_SUB_CONST))
+            if (!is_in_const_context(cx) || msrv.meets(cx, msrvs::SATURATING_SUB_CONST))
+                && let Some(big_expr_sugg) = Sugg::hir_opt(cx, big_expr).map(Sugg::maybe_par)
+                && let Some(little_expr_sugg) = Sugg::hir_opt(cx, little_expr)
             {
                 let sugg = format!(
-                    "{}{big_var_snippet}.saturating_sub({little_var_snippet}){}",
+                    "{}{big_expr_sugg}.saturating_sub({little_expr_sugg}){}",
                     if is_composited { "{ " } else { "" },
                     if is_composited { " }" } else { "" }
                 );
@@ -271,11 +257,12 @@ fn check_subtraction(
                     Applicability::MachineApplicable,
                 );
             }
-        } else if Some(little_var.hir_id) == local_left
-            && Some(big_var.hir_id) == local_right
-            && let Some(big_var_snippet) = snippet_opt(cx, big_var.span)
-            && let Some(little_var_snippet) = snippet_opt(cx, little_var.span)
+        } else if eq_expr_value(cx, left, little_expr)
+            && eq_expr_value(cx, right, big_expr)
+            && let Some(big_expr_sugg) = Sugg::hir_opt(cx, big_expr)
+            && let Some(little_expr_sugg) = Sugg::hir_opt(cx, little_expr)
         {
+            let sugg = make_binop(BinOpKind::Sub, &big_expr_sugg, &little_expr_sugg);
             span_lint_and_then(
                 cx,
                 INVERTED_SATURATING_SUB,
@@ -284,12 +271,12 @@ fn check_subtraction(
                 |diag| {
                     diag.span_note(
                         if_block.span,
-                        format!("this subtraction underflows when `{little_var_snippet} < {big_var_snippet}`"),
+                        format!("this subtraction underflows when `{little_expr_sugg} < {big_expr_sugg}`"),
                     );
                     diag.span_suggestion(
                         if_block.span,
                         "try replacing it with",
-                        format!("{big_var_snippet} - {little_var_snippet}"),
+                        format!("{sugg}"),
                         Applicability::MaybeIncorrect,
                     );
                 },
diff --git a/tests/ui/implicit_saturating_sub.fixed b/tests/ui/implicit_saturating_sub.fixed
@@ -228,3 +228,27 @@ fn regression_13524(a: usize, b: usize, c: bool) -> usize {
         123
     } else { b.saturating_sub(a) }
 }
+
+fn with_side_effect(a: u64) -> u64 {
+    println!("a = {a}");
+    a
+}
+
+fn arbitrary_expression() {
+    let (a, b) = (15u64, 20u64);
+
+    let _ = (a * 2).saturating_sub(b);
+    //~^ implicit_saturating_sub
+
+    let _ = a.saturating_sub(b * 2);
+    //~^ implicit_saturating_sub
+
+    let _ = a.saturating_sub(b * 2);
+    //~^ implicit_saturating_sub
+
+    let _ = if with_side_effect(a) > a {
+        with_side_effect(a) - a
+    } else {
+        0
+    };
+}
diff --git a/tests/ui/implicit_saturating_sub.rs b/tests/ui/implicit_saturating_sub.rs
@@ -302,3 +302,27 @@ fn regression_13524(a: usize, b: usize, c: bool) -> usize {
         b - a
     }
 }
+
+fn with_side_effect(a: u64) -> u64 {
+    println!("a = {a}");
+    a
+}
+
+fn arbitrary_expression() {
+    let (a, b) = (15u64, 20u64);
+
+    let _ = if a * 2 > b { a * 2 - b } else { 0 };
+    //~^ implicit_saturating_sub
+
+    let _ = if a > b * 2 { a - b * 2 } else { 0 };
+    //~^ implicit_saturating_sub
+
+    let _ = if a < b * 2 { 0 } else { a - b * 2 };
+    //~^ implicit_saturating_sub
+
+    let _ = if with_side_effect(a) > a {
+        with_side_effect(a) - a
+    } else {
+        0
+    };
+}
diff --git a/tests/ui/implicit_saturating_sub.stderr b/tests/ui/implicit_saturating_sub.stderr
@@ -220,5 +220,23 @@ LL | |         b - a
 LL | |     }
    | |_____^ help: replace it with: `{ b.saturating_sub(a) }`
 
-error: aborting due to 24 previous errors
+error: manual arithmetic check found
+  --> tests/ui/implicit_saturating_sub.rs:314:13
+   |
+LL |     let _ = if a * 2 > b { a * 2 - b } else { 0 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `(a * 2).saturating_sub(b)`
+
+error: manual arithmetic check found
+  --> tests/ui/implicit_saturating_sub.rs:317:13
+   |
+LL |     let _ = if a > b * 2 { a - b * 2 } else { 0 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b * 2)`
+
+error: manual arithmetic check found
+  --> tests/ui/implicit_saturating_sub.rs:320:13
+   |
+LL |     let _ = if a < b * 2 { 0 } else { a - b * 2 };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `a.saturating_sub(b * 2)`
+
+error: aborting due to 27 previous errors
 
diff --git a/tests/ui/manual_arithmetic_check-2.rs b/tests/ui/manual_arithmetic_check-2.rs
@@ -24,6 +24,15 @@ fn main() {
     let result = if b <= a { 0 } else { a - b };
     //~^ inverted_saturating_sub
 
+    let result = if b * 2 <= a { 0 } else { a - b * 2 };
+    //~^ inverted_saturating_sub
+
+    let result = if b <= a * 2 { 0 } else { a * 2 - b };
+    //~^ inverted_saturating_sub
+
+    let result = if b + 3 <= a + 2 { 0 } else { (a + 2) - (b + 3) };
+    //~^ inverted_saturating_sub
+
     let af = 12f32;
     let bf = 13f32;
     // Should not lint!
diff --git a/tests/ui/manual_arithmetic_check-2.stderr b/tests/ui/manual_arithmetic_check-2.stderr
@@ -71,5 +71,41 @@ note: this subtraction underflows when `a < b`
 LL |     let result = if b <= a { 0 } else { a - b };
    |                                         ^^^^^
 
-error: aborting due to 6 previous errors
+error: inverted arithmetic check before subtraction
+  --> tests/ui/manual_arithmetic_check-2.rs:27:27
+   |
+LL |     let result = if b * 2 <= a { 0 } else { a - b * 2 };
+   |                           ^^                --------- help: try replacing it with: `b * 2 - a`
+   |
+note: this subtraction underflows when `a < b * 2`
+  --> tests/ui/manual_arithmetic_check-2.rs:27:45
+   |
+LL |     let result = if b * 2 <= a { 0 } else { a - b * 2 };
+   |                                             ^^^^^^^^^
+
+error: inverted arithmetic check before subtraction
+  --> tests/ui/manual_arithmetic_check-2.rs:30:23
+   |
+LL |     let result = if b <= a * 2 { 0 } else { a * 2 - b };
+   |                       ^^                    --------- help: try replacing it with: `b - a * 2`
+   |
+note: this subtraction underflows when `a * 2 < b`
+  --> tests/ui/manual_arithmetic_check-2.rs:30:45
+   |
+LL |     let result = if b <= a * 2 { 0 } else { a * 2 - b };
+   |                                             ^^^^^^^^^
+
+error: inverted arithmetic check before subtraction
+  --> tests/ui/manual_arithmetic_check-2.rs:33:27
+   |
+LL |     let result = if b + 3 <= a + 2 { 0 } else { (a + 2) - (b + 3) };
+   |                           ^^                    ----------------- help: try replacing it with: `b + 3 - (a + 2)`
+   |
+note: this subtraction underflows when `a + 2 < b + 3`
+  --> tests/ui/manual_arithmetic_check-2.rs:33:49
+   |
+LL |     let result = if b + 3 <= a + 2 { 0 } else { (a + 2) - (b + 3) };
+   |                                                 ^^^^^^^^^^^^^^^^^
+
+error: aborting due to 9 previous errors
 

Original file line number	Diff line number	Diff line change
`@@ -1010,11 +1010,7 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize`
`1010`	`1010`	`start -= 1;`
`1011`	`1011`	`}`
`1012`	`1012`
`1013`		`- if start > range.start {`
`1014`		`- start - range.start`
`1015`		`- } else {`
`1016`		`- 0`
`1017`		`- }`
	`1013`	`+ start.saturating_sub(range.start)`
`1018`	`1014`	`}`
`1019`	`1015`	`} else {`
`1020`	`1016`	`0`