recognize x.saturating_sub(y).unwrap_or_default() as well

Author: ada4a

clippy_lints/src/methods/manual_saturating_arithmetic.rs

@@ -3,18 +3,19 @@ use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::{path_res, sym};
 use rustc_ast::ast;
 use rustc_errors::Applicability;
-use rustc_hir as hir;
 use rustc_hir::def::Res;
+use rustc_hir::{self as hir, Expr};
 use rustc_lint::LateContext;
+use rustc_middle::ty::Ty;
 use rustc_middle::ty::layout::LayoutOf;
 use rustc_span::Symbol;
 
-pub fn check(
+pub fn check_unwrap_or(
     cx: &LateContext<'_>,
-    expr: &hir::Expr<'_>,
-    arith_lhs: &hir::Expr<'_>,
-    arith_rhs: &hir::Expr<'_>,
-    unwrap_arg: &hir::Expr<'_>,
+    expr: &Expr<'_>,
+    arith_lhs: &Expr<'_>,
+    arith_rhs: &Expr<'_>,
+    unwrap_arg: &Expr<'_>,
     arith: Symbol,
 ) {
     let ty = cx.typeck_results().expr_ty(arith_lhs);
@@ -30,6 +31,42 @@ pub fn check(
         return;
     };
 
+    check(cx, expr, arith_lhs, arith_rhs, ty, mm, checked_arith);
+}
+
+/// Precondition: should be called on `x.saturating_sub(y).unwrap_or_default()`
+pub(super) fn check_unwrap_or_default(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    arith_lhs: &Expr<'_>,
+    arith_rhs: &Expr<'_>,
+) {
+    let ty = cx.typeck_results().expr_ty(arith_lhs);
+    if !ty.is_integral() {
+        return;
+    }
+
+    let mm = if ty.is_signed() {
+        return; // iN::default() is 0, which is neither MIN nor MAX
+    } else {
+        MinMax::Min // uN::default() is 0, which is also the MIN
+    };
+
+    // See precondition
+    let checked_arith = CheckedArith::Sub;
+
+    check(cx, expr, arith_lhs, arith_rhs, ty, mm, checked_arith);
+}
+
+fn check(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    arith_lhs: &Expr<'_>,
+    arith_rhs: &Expr<'_>,
+    ty: Ty<'_>,
+    mm: MinMax,
+    checked_arith: CheckedArith,
+) {
     if ty.is_signed() {
         use self::MinMax::{Max, Min};
         use self::Sign::{Neg, Pos};
@@ -39,15 +76,15 @@ pub fn check(
             return;
         };
 
-        match (&checked_arith, sign, mm) {
+        match (checked_arith, sign, mm) {
             (Add, Pos, Max) | (Add, Neg, Min) | (Sub, Neg, Max) | (Sub, Pos, Min) => (),
             // "mul" is omitted because lhs can be negative.
             _ => return,
         }
     } else {
         use self::MinMax::{Max, Min};
         use CheckedArith::{Add, Mul, Sub};
-        match (mm, &checked_arith) {
+        match (mm, checked_arith) {
             (Max, Add | Mul) | (Min, Sub) => (),
             _ => return,
         }
@@ -70,6 +107,7 @@ pub fn check(
     );
 }
 
+#[derive(Clone, Copy)]
 enum CheckedArith {
     Add,
     Sub,
@@ -87,7 +125,7 @@ impl CheckedArith {
         Some(res)
     }
 
-    fn as_saturating(&self) -> &'static str {
+    fn as_saturating(self) -> &'static str {
         match self {
             Self::Add => "saturating_add",
             Self::Sub => "saturating_sub",
@@ -102,7 +140,7 @@ enum MinMax {
     Max,
 }
 
-fn is_min_or_max(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<MinMax> {
+fn is_min_or_max(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<MinMax> {
     // `T::max_value()` `T::min_value()` inherent methods
     if let hir::ExprKind::Call(func, []) = &expr.kind
         && let hir::ExprKind::Path(hir::QPath::TypeRelative(_, segment)) = &func.kind
@@ -140,7 +178,7 @@ fn is_min_or_max(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<MinMax> {
         (0, if bits == 128 { !0 } else { (1 << bits) - 1 })
     };
 
-    let check_lit = |expr: &hir::Expr<'_>, check_min: bool| {
+    let check_lit = |expr: &Expr<'_>, check_min: bool| {
         if let hir::ExprKind::Lit(lit) = &expr.kind
             && let ast::LitKind::Int(value, _) = lit.node
         {
@@ -175,7 +213,7 @@ enum Sign {
     Neg,
 }
 
-fn lit_sign(expr: &hir::Expr<'_>) -> Option<Sign> {
+fn lit_sign(expr: &Expr<'_>) -> Option<Sign> {
     if let hir::ExprKind::Unary(hir::UnOp::Neg, inner) = &expr.kind {
         if let hir::ExprKind::Lit(..) = &inner.kind {
             return Some(Sign::Neg);

clippy_lints/src/methods/mod.rs

@@ -5488,7 +5488,7 @@ impl Methods {
                 (sym::unwrap_or, [u_arg]) => {
                     match method_call(recv) {
                         Some((arith @ (sym::checked_add | sym::checked_sub | sym::checked_mul), lhs, [rhs], _, _)) => {
-                            manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, arith);
+                            manual_saturating_arithmetic::check_unwrap_or(cx, expr, lhs, rhs, u_arg, arith);
                         },
                         Some((sym::map, m_recv, [m_arg], span, _)) => {
                             option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span, self.msrv);
@@ -5502,6 +5502,9 @@ impl Methods {
                 },
                 (sym::unwrap_or_default, []) => {
                     match method_call(recv) {
+                        Some((sym::checked_sub, lhs, [rhs], _, _)) => {
+                            manual_saturating_arithmetic::check_unwrap_or_default(cx, expr, lhs, rhs);
+                        },
                         Some((sym::map, m_recv, [arg], span, _)) => {
                             manual_is_variant_and::check(cx, expr, m_recv, arg, span, self.msrv);
                         },

tests/ui/manual_saturating_arithmetic.fixed

@@ -58,3 +58,13 @@ fn main() {
     let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
     let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
 }
+
+fn issue15655() {
+    let _ = 5u32.saturating_sub(1u32); //~ manual_saturating_arithmetic
+    let _ = 5u32.checked_add(1u32).unwrap_or_default(); // ok
+    let _ = 5u32.checked_mul(1u32).unwrap_or_default(); // ok
+
+    let _ = 5i32.checked_sub(1i32).unwrap_or_default(); // ok
+    let _ = 5i32.checked_add(1i32).unwrap_or_default(); // ok
+    let _ = 5i32.checked_mul(1i32).unwrap_or_default(); // ok
+}

tests/ui/manual_saturating_arithmetic.rs

@@ -73,3 +73,13 @@ fn main() {
     let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
     let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
 }
+
+fn issue15655() {
+    let _ = 5u32.checked_sub(1u32).unwrap_or_default(); //~ manual_saturating_arithmetic
+    let _ = 5u32.checked_add(1u32).unwrap_or_default(); // ok
+    let _ = 5u32.checked_mul(1u32).unwrap_or_default(); // ok
+
+    let _ = 5i32.checked_sub(1i32).unwrap_or_default(); // ok
+    let _ = 5i32.checked_add(1i32).unwrap_or_default(); // ok
+    let _ = 5i32.checked_mul(1i32).unwrap_or_default(); // ok
+}

tests/ui/manual_saturating_arithmetic.stderr

@@ -165,5 +165,11 @@ LL | |         .checked_sub(-1)
 LL | |         .unwrap_or(170_141_183_460_469_231_731_687_303_715_884_105_727);
    | |_______________________________________________________________________^ help: consider using `saturating_sub`: `1i128.saturating_sub(-1)`
 
-error: aborting due to 24 previous errors
+error: manual saturating arithmetic
+  --> tests/ui/manual_saturating_arithmetic.rs:78:13
+   |
+LL |     let _ = 5u32.checked_sub(1u32).unwrap_or_default();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_sub`: `5u32.saturating_sub(1u32)`
+
+error: aborting due to 25 previous errors