Skip to content

Commit 95834c9

Browse files
ada4aada4a
committed
recognize x.checked_sub(y).unwrap_or_default() as well
1 parent 3833ac3 commit 95834c9

File tree

5 files changed

+95
-32
lines changed

5 files changed

+95
-32
lines changed

‎clippy_lints/src/methods/manual_saturating_arithmetic.rs‎

Lines changed: 64 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,19 @@ use clippy_utils::source::snippet_with_applicability;
44
use clippy_utils::sym;
55
use rustc_ast::ast;
66
use rustc_errors::Applicability;
7-
use rustc_hir as hir;
87
use rustc_hir::def::Res;
8+
use rustc_hir::{self as hir, Expr};
99
use rustc_lint::LateContext;
10+
use rustc_middle::ty::Ty;
1011
use rustc_middle::ty::layout::LayoutOf;
1112
use rustc_span::Symbol;
1213

13-
pub fn check(
14+
pub fn check_unwrap_or(
1415
cx: &LateContext<'_>,
15-
expr: &hir::Expr<'_>,
16-
arith_lhs: &hir::Expr<'_>,
17-
arith_rhs: &hir::Expr<'_>,
18-
unwrap_arg: &hir::Expr<'_>,
16+
expr: &Expr<'_>,
17+
arith_lhs: &Expr<'_>,
18+
arith_rhs: &Expr<'_>,
19+
unwrap_arg: &Expr<'_>,
1920
arith: Symbol,
2021
) {
2122
let ty = cx.typeck_results().expr_ty(arith_lhs);
@@ -31,26 +32,58 @@ pub fn check(
3132
return;
3233
};
3334

34-
{
35-
use self::MinMax::{Max, Min};
36-
use self::Sign::{Neg, Pos};
37-
use CheckedArith::{Add, Mul, Sub};
38-
39-
if ty.is_signed() {
40-
let Some(sign) = lit_sign(arith_rhs) else {
41-
return;
42-
};
43-
44-
match (&checked_arith, sign, mm) {
45-
(Add, Pos, Max) | (Add, Neg, Min) | (Sub, Neg, Max) | (Sub, Pos, Min) => (),
46-
// "mul" is omitted because lhs can be negative.
47-
_ => return,
48-
}
49-
} else {
50-
match (mm, &checked_arith) {
51-
(Max, Add | Mul) | (Min, Sub) => (),
52-
_ => return,
53-
}
35+
check(cx, expr, arith_lhs, arith_rhs, ty, mm, checked_arith);
36+
}
37+
38+
pub(super) fn check_sub_unwrap_or_default(
39+
cx: &LateContext<'_>,
40+
expr: &Expr<'_>,
41+
arith_lhs: &Expr<'_>,
42+
arith_rhs: &Expr<'_>,
43+
) {
44+
let ty = cx.typeck_results().expr_ty(arith_lhs);
45+
if !ty.is_integral() {
46+
return;
47+
}
48+
49+
let mm = if ty.is_signed() {
50+
return; // iN::default() is 0, which is neither MIN nor MAX
51+
} else {
52+
MinMax::Min // uN::default() is 0, which is also the MIN
53+
};
54+
55+
let checked_arith = CheckedArith::Sub;
56+
57+
check(cx, expr, arith_lhs, arith_rhs, ty, mm, checked_arith);
58+
}
59+
60+
fn check(
61+
cx: &LateContext<'_>,
62+
expr: &Expr<'_>,
63+
arith_lhs: &Expr<'_>,
64+
arith_rhs: &Expr<'_>,
65+
ty: Ty<'_>,
66+
mm: MinMax,
67+
checked_arith: CheckedArith,
68+
) {
69+
use self::MinMax::{Max, Min};
70+
use self::Sign::{Neg, Pos};
71+
use CheckedArith::{Add, Mul, Sub};
72+
73+
if ty.is_signed() {
74+
let Some(sign) = lit_sign(arith_rhs) else {
75+
return;
76+
};
77+
78+
match (checked_arith, sign, mm) {
79+
(Add, Pos, Max) | (Add, Neg, Min) | (Sub, Neg, Max) | (Sub, Pos, Min) => (),
80+
// "mul" is omitted because lhs can be negative.
81+
_ => return,
82+
}
83+
} else {
84+
match (mm, checked_arith) {
85+
(Max, Add | Mul) | (Min, Sub) => (),
86+
_ => return,
5487
}
5588
}
5689

@@ -71,6 +104,7 @@ pub fn check(
71104
);
72105
}
73106

107+
#[derive(Clone, Copy)]
74108
enum CheckedArith {
75109
Add,
76110
Sub,
@@ -88,7 +122,7 @@ impl CheckedArith {
88122
Some(res)
89123
}
90124

91-
fn as_saturating(&self) -> &'static str {
125+
fn as_saturating(self) -> &'static str {
92126
match self {
93127
Self::Add => "saturating_add",
94128
Self::Sub => "saturating_sub",
@@ -103,7 +137,7 @@ enum MinMax {
103137
Max,
104138
}
105139

106-
fn is_min_or_max(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<MinMax> {
140+
fn is_min_or_max(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<MinMax> {
107141
// `T::max_value()` `T::min_value()` inherent methods
108142
if let hir::ExprKind::Call(func, []) = &expr.kind
109143
&& let hir::ExprKind::Path(hir::QPath::TypeRelative(_, segment)) = &func.kind
@@ -141,7 +175,7 @@ fn is_min_or_max(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<MinMax> {
141175
(0, if bits == 128 { !0 } else { (1 << bits) - 1 })
142176
};
143177

144-
let check_lit = |expr: &hir::Expr<'_>, check_min: bool| {
178+
let check_lit = |expr: &Expr<'_>, check_min: bool| {
145179
if let hir::ExprKind::Lit(lit) = &expr.kind
146180
&& let ast::LitKind::Int(value, _) = lit.node
147181
{
@@ -176,7 +210,7 @@ enum Sign {
176210
Neg,
177211
}
178212

179-
fn lit_sign(expr: &hir::Expr<'_>) -> Option<Sign> {
213+
fn lit_sign(expr: &Expr<'_>) -> Option<Sign> {
180214
if let hir::ExprKind::Unary(hir::UnOp::Neg, inner) = &expr.kind {
181215
if let hir::ExprKind::Lit(..) = &inner.kind {
182216
return Some(Sign::Neg);

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5507,7 +5507,7 @@ impl Methods {
55075507
(sym::unwrap_or, [u_arg]) => {
55085508
match method_call(recv) {
55095509
Some((arith @ (sym::checked_add | sym::checked_sub | sym::checked_mul), lhs, [rhs], _, _)) => {
5510-
manual_saturating_arithmetic::check(cx, expr, lhs, rhs, u_arg, arith);
5510+
manual_saturating_arithmetic::check_unwrap_or(cx, expr, lhs, rhs, u_arg, arith);
55115511
},
55125512
Some((sym::map, m_recv, [m_arg], span, _)) => {
55135513
option_map_unwrap_or::check(cx, expr, m_recv, m_arg, recv, u_arg, span, self.msrv);
@@ -5528,6 +5528,9 @@ impl Methods {
55285528
},
55295529
(sym::unwrap_or_default, []) => {
55305530
match method_call(recv) {
5531+
Some((sym::checked_sub, lhs, [rhs], _, _)) => {
5532+
manual_saturating_arithmetic::check_sub_unwrap_or_default(cx, expr, lhs, rhs);
5533+
},
55315534
Some((sym::map, m_recv, [arg], span, _)) => {
55325535
manual_is_variant_and::check(cx, expr, m_recv, arg, span, self.msrv);
55335536
},

‎tests/ui/manual_saturating_arithmetic.fixed‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,13 @@ fn main() {
5858
let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
5959
let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
6060
}
61+
62+
fn issue15655() {
63+
let _ = 5u32.saturating_sub(1u32); //~ manual_saturating_arithmetic
64+
let _ = 5u32.checked_add(1u32).unwrap_or_default(); // ok
65+
let _ = 5u32.checked_mul(1u32).unwrap_or_default(); // ok
66+
67+
let _ = 5i32.checked_sub(1i32).unwrap_or_default(); // ok
68+
let _ = 5i32.checked_add(1i32).unwrap_or_default(); // ok
69+
let _ = 5i32.checked_mul(1i32).unwrap_or_default(); // ok
70+
}

‎tests/ui/manual_saturating_arithmetic.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,13 @@ fn main() {
7373
let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
7474
let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
7575
}
76+
77+
fn issue15655() {
78+
let _ = 5u32.checked_sub(1u32).unwrap_or_default(); //~ manual_saturating_arithmetic
79+
let _ = 5u32.checked_add(1u32).unwrap_or_default(); // ok
80+
let _ = 5u32.checked_mul(1u32).unwrap_or_default(); // ok
81+
82+
let _ = 5i32.checked_sub(1i32).unwrap_or_default(); // ok
83+
let _ = 5i32.checked_add(1i32).unwrap_or_default(); // ok
84+
let _ = 5i32.checked_mul(1i32).unwrap_or_default(); // ok
85+
}

‎tests/ui/manual_saturating_arithmetic.stderr‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,5 +165,11 @@ LL | | .checked_sub(-1)
165165
LL | | .unwrap_or(170_141_183_460_469_231_731_687_303_715_884_105_727);
166166
| |_______________________________________________________________________^ help: consider using `saturating_sub`: `1i128.saturating_sub(-1)`
167167

168-
error: aborting due to 24 previous errors
168+
error: manual saturating arithmetic
169+
--> tests/ui/manual_saturating_arithmetic.rs:78:13
170+
|
171+
LL | let _ = 5u32.checked_sub(1u32).unwrap_or_default();
172+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `saturating_sub`: `5u32.saturating_sub(1u32)`
173+
174+
error: aborting due to 25 previous errors
169175

0 commit comments

Comments
 (0)