Skip to content

Commit ffe57fa

Browse files
committed
Auto merge of #4498 - sinkuu:checked_arithmetic_unwrap, r=flip1995
Add manual_saturating_arithmetic lint changelog: add `manual_saturating_arithmetic` lint Fixes #1557. This lint detects manual saturating arithmetics like `x.checked_add(10u32).unwrap_or(u32::max_value())` and suggests replacing with `x.saturating_add(10u32)`.
2 parents a2c4b2b + c6fb9c8 commit ffe57fa

9 files changed

+482
-2
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -1034,6 +1034,7 @@ Released 2018-09-13
10341034
[`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug
10351035
[`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
10361036
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
1037+
[`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
10371038
[`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap
10381039
[`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names
10391040
[`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone

README.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
88

9-
[There are 312 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
9+
[There are 313 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
1010

1111
We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
1212

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -793,6 +793,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
793793
methods::ITER_CLONED_COLLECT,
794794
methods::ITER_NTH,
795795
methods::ITER_SKIP_NEXT,
796+
methods::MANUAL_SATURATING_ARITHMETIC,
796797
methods::NEW_RET_NO_SELF,
797798
methods::OK_EXPECT,
798799
methods::OPTION_AND_THEN_SOME,
@@ -958,6 +959,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
958959
methods::INTO_ITER_ON_REF,
959960
methods::ITER_CLONED_COLLECT,
960961
methods::ITER_SKIP_NEXT,
962+
methods::MANUAL_SATURATING_ARITHMETIC,
961963
methods::NEW_RET_NO_SELF,
962964
methods::OK_EXPECT,
963965
methods::OPTION_MAP_OR_NONE,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
use crate::utils::{match_qpath, snippet_with_applicability, span_lint_and_sugg};
2+
use if_chain::if_chain;
3+
use rustc::hir;
4+
use rustc::lint::LateContext;
5+
use rustc_errors::Applicability;
6+
use rustc_target::abi::LayoutOf;
7+
use syntax::ast;
8+
9+
pub fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[&[hir::Expr]], arith: &str) {
10+
let unwrap_arg = &args[0][1];
11+
let arith_lhs = &args[1][0];
12+
let arith_rhs = &args[1][1];
13+
14+
let ty = cx.tables.expr_ty(arith_lhs);
15+
if !ty.is_integral() {
16+
return;
17+
}
18+
19+
let mm = if let Some(mm) = is_min_or_max(cx, unwrap_arg) {
20+
mm
21+
} else {
22+
return;
23+
};
24+
25+
if ty.is_signed() {
26+
use self::{MinMax::*, Sign::*};
27+
28+
let sign = if let Some(sign) = lit_sign(arith_rhs) {
29+
sign
30+
} else {
31+
return;
32+
};
33+
34+
match (arith, sign, mm) {
35+
("add", Pos, Max) | ("add", Neg, Min) | ("sub", Neg, Max) | ("sub", Pos, Min) => (),
36+
// "mul" is omitted because lhs can be negative.
37+
_ => return,
38+
}
39+
40+
let mut applicability = Applicability::MachineApplicable;
41+
span_lint_and_sugg(
42+
cx,
43+
super::MANUAL_SATURATING_ARITHMETIC,
44+
expr.span,
45+
"manual saturating arithmetic",
46+
&format!("try using `saturating_{}`", arith),
47+
format!(
48+
"{}.saturating_{}({})",
49+
snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability),
50+
arith,
51+
snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability),
52+
),
53+
applicability,
54+
);
55+
} else {
56+
match (mm, arith) {
57+
(MinMax::Max, "add") | (MinMax::Max, "mul") | (MinMax::Min, "sub") => (),
58+
_ => return,
59+
}
60+
61+
let mut applicability = Applicability::MachineApplicable;
62+
span_lint_and_sugg(
63+
cx,
64+
super::MANUAL_SATURATING_ARITHMETIC,
65+
expr.span,
66+
"manual saturating arithmetic",
67+
&format!("try using `saturating_{}`", arith),
68+
format!(
69+
"{}.saturating_{}({})",
70+
snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability),
71+
arith,
72+
snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability),
73+
),
74+
applicability,
75+
);
76+
}
77+
}
78+
79+
#[derive(PartialEq, Eq)]
80+
enum MinMax {
81+
Min,
82+
Max,
83+
}
84+
85+
fn is_min_or_max<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &hir::Expr) -> Option<MinMax> {
86+
// `T::max_value()` `T::min_value()` inherent methods
87+
if_chain! {
88+
if let hir::ExprKind::Call(func, args) = &expr.node;
89+
if args.is_empty();
90+
if let hir::ExprKind::Path(path) = &func.node;
91+
if let hir::QPath::TypeRelative(_, segment) = path;
92+
then {
93+
match &*segment.ident.as_str() {
94+
"max_value" => return Some(MinMax::Max),
95+
"min_value" => return Some(MinMax::Min),
96+
_ => {}
97+
}
98+
}
99+
}
100+
101+
let ty = cx.tables.expr_ty(expr);
102+
let ty_str = ty.to_string();
103+
104+
// `std::T::MAX` `std::T::MIN` constants
105+
if let hir::ExprKind::Path(path) = &expr.node {
106+
if match_qpath(path, &["core", &ty_str, "MAX"][..]) {
107+
return Some(MinMax::Max);
108+
}
109+
110+
if match_qpath(path, &["core", &ty_str, "MIN"][..]) {
111+
return Some(MinMax::Min);
112+
}
113+
}
114+
115+
// Literals
116+
let bits = cx.layout_of(ty).unwrap().size.bits();
117+
let (minval, maxval): (u128, u128) = if ty.is_signed() {
118+
let minval = 1 << (bits - 1);
119+
let mut maxval = !(1 << (bits - 1));
120+
if bits != 128 {
121+
maxval &= (1 << bits) - 1;
122+
}
123+
(minval, maxval)
124+
} else {
125+
(0, if bits == 128 { !0 } else { (1 << bits) - 1 })
126+
};
127+
128+
let check_lit = |expr: &hir::Expr, check_min: bool| {
129+
if let hir::ExprKind::Lit(lit) = &expr.node {
130+
if let ast::LitKind::Int(value, _) = lit.node {
131+
if value == maxval {
132+
return Some(MinMax::Max);
133+
}
134+
135+
if check_min && value == minval {
136+
return Some(MinMax::Min);
137+
}
138+
}
139+
}
140+
141+
None
142+
};
143+
144+
if let r @ Some(_) = check_lit(expr, !ty.is_signed()) {
145+
return r;
146+
}
147+
148+
if ty.is_signed() {
149+
if let hir::ExprKind::Unary(hir::UnNeg, val) = &expr.node {
150+
return check_lit(val, true);
151+
}
152+
}
153+
154+
None
155+
}
156+
157+
#[derive(PartialEq, Eq)]
158+
enum Sign {
159+
Pos,
160+
Neg,
161+
}
162+
163+
fn lit_sign(expr: &hir::Expr) -> Option<Sign> {
164+
if let hir::ExprKind::Unary(hir::UnNeg, inner) = &expr.node {
165+
if let hir::ExprKind::Lit(..) = &inner.node {
166+
return Some(Sign::Neg);
167+
}
168+
} else if let hir::ExprKind::Lit(..) = &expr.node {
169+
return Some(Sign::Pos);
170+
}
171+
172+
None
173+
}

clippy_lints/src/methods/mod.rs

+34
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
mod manual_saturating_arithmetic;
12
mod option_map_unwrap_or;
23
mod unnecessary_filter_map;
34

@@ -983,6 +984,33 @@ declare_clippy_lint! {
983984
"`MaybeUninit::uninit().assume_init()`"
984985
}
985986

987+
declare_clippy_lint! {
988+
/// **What it does:** Checks for `.checked_add/sub(x).unwrap_or(MAX/MIN)`.
989+
///
990+
/// **Why is this bad?** These can be written simply with `saturating_add/sub` methods.
991+
///
992+
/// **Example:**
993+
///
994+
/// ```rust
995+
/// # let y: u32 = 0;
996+
/// # let x: u32 = 100;
997+
/// let add = x.checked_add(y).unwrap_or(u32::max_value());
998+
/// let sub = x.checked_sub(y).unwrap_or(u32::min_value());
999+
/// ```
1000+
///
1001+
/// can be written using dedicated methods for saturating addition/subtraction as:
1002+
///
1003+
/// ```rust
1004+
/// # let y: u32 = 0;
1005+
/// # let x: u32 = 100;
1006+
/// let add = x.saturating_add(y);
1007+
/// let sub = x.saturating_sub(y);
1008+
/// ```
1009+
pub MANUAL_SATURATING_ARITHMETIC,
1010+
style,
1011+
"`.chcked_add/sub(x).unwrap_or(MAX/MIN)`"
1012+
}
1013+
9861014
declare_lint_pass!(Methods => [
9871015
OPTION_UNWRAP_USED,
9881016
RESULT_UNWRAP_USED,
@@ -1024,6 +1052,7 @@ declare_lint_pass!(Methods => [
10241052
INTO_ITER_ON_REF,
10251053
SUSPICIOUS_MAP,
10261054
UNINIT_ASSUMED_INIT,
1055+
MANUAL_SATURATING_ARITHMETIC,
10271056
]);
10281057

10291058
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
@@ -1076,6 +1105,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
10761105
["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]),
10771106
["count", "map"] => lint_suspicious_map(cx, expr),
10781107
["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr),
1108+
["unwrap_or", arith @ "checked_add"]
1109+
| ["unwrap_or", arith @ "checked_sub"]
1110+
| ["unwrap_or", arith @ "checked_mul"] => {
1111+
manual_saturating_arithmetic::lint(cx, expr, &arg_lists, &arith["checked_".len()..])
1112+
},
10791113
_ => {},
10801114
}
10811115

src/lintlist/mod.rs

+8-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ pub use lint::Lint;
66
pub use lint::LINT_LEVELS;
77

88
// begin lint list, do not remove this comment, it’s used in `update_lints`
9-
pub const ALL_LINTS: [Lint; 312] = [
9+
pub const ALL_LINTS: [Lint; 313] = [
1010
Lint {
1111
name: "absurd_extreme_comparisons",
1212
group: "correctness",
@@ -931,6 +931,13 @@ pub const ALL_LINTS: [Lint; 312] = [
931931
deprecation: None,
932932
module: "loops",
933933
},
934+
Lint {
935+
name: "manual_saturating_arithmetic",
936+
group: "style",
937+
desc: "`.chcked_add/sub(x).unwrap_or(MAX/MIN)`",
938+
deprecation: None,
939+
module: "methods",
940+
},
934941
Lint {
935942
name: "manual_swap",
936943
group: "complexity",
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// run-rustfix
2+
3+
#![allow(unused_imports)]
4+
5+
use std::{i128, i32, u128, u32};
6+
7+
fn main() {
8+
let _ = 1u32.saturating_add(1);
9+
let _ = 1u32.saturating_add(1);
10+
let _ = 1u8.saturating_add(1);
11+
let _ = 1u128.saturating_add(1);
12+
let _ = 1u32.checked_add(1).unwrap_or(1234); // ok
13+
let _ = 1u8.checked_add(1).unwrap_or(0); // ok
14+
let _ = 1u32.saturating_mul(1);
15+
16+
let _ = 1u32.saturating_sub(1);
17+
let _ = 1u32.saturating_sub(1);
18+
let _ = 1u8.saturating_sub(1);
19+
let _ = 1u32.checked_sub(1).unwrap_or(1234); // ok
20+
let _ = 1u8.checked_sub(1).unwrap_or(255); // ok
21+
22+
let _ = 1i32.saturating_add(1);
23+
let _ = 1i32.saturating_add(1);
24+
let _ = 1i8.saturating_add(1);
25+
let _ = 1i128.saturating_add(1);
26+
let _ = 1i32.saturating_add(-1);
27+
let _ = 1i32.saturating_add(-1);
28+
let _ = 1i8.saturating_add(-1);
29+
let _ = 1i128.saturating_add(-1);
30+
let _ = 1i32.checked_add(1).unwrap_or(1234); // ok
31+
let _ = 1i8.checked_add(1).unwrap_or(-128); // ok
32+
let _ = 1i8.checked_add(-1).unwrap_or(127); // ok
33+
34+
let _ = 1i32.saturating_sub(1);
35+
let _ = 1i32.saturating_sub(1);
36+
let _ = 1i8.saturating_sub(1);
37+
let _ = 1i128.saturating_sub(1);
38+
let _ = 1i32.saturating_sub(-1);
39+
let _ = 1i32.saturating_sub(-1);
40+
let _ = 1i8.saturating_sub(-1);
41+
let _ = 1i128.saturating_sub(-1);
42+
let _ = 1i32.checked_sub(1).unwrap_or(1234); // ok
43+
let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
44+
let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
45+
}
+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// run-rustfix
2+
3+
#![allow(unused_imports)]
4+
5+
use std::{i128, i32, u128, u32};
6+
7+
fn main() {
8+
let _ = 1u32.checked_add(1).unwrap_or(u32::max_value());
9+
let _ = 1u32.checked_add(1).unwrap_or(u32::MAX);
10+
let _ = 1u8.checked_add(1).unwrap_or(255);
11+
let _ = 1u128
12+
.checked_add(1)
13+
.unwrap_or(340_282_366_920_938_463_463_374_607_431_768_211_455);
14+
let _ = 1u32.checked_add(1).unwrap_or(1234); // ok
15+
let _ = 1u8.checked_add(1).unwrap_or(0); // ok
16+
let _ = 1u32.checked_mul(1).unwrap_or(u32::MAX);
17+
18+
let _ = 1u32.checked_sub(1).unwrap_or(u32::min_value());
19+
let _ = 1u32.checked_sub(1).unwrap_or(u32::MIN);
20+
let _ = 1u8.checked_sub(1).unwrap_or(0);
21+
let _ = 1u32.checked_sub(1).unwrap_or(1234); // ok
22+
let _ = 1u8.checked_sub(1).unwrap_or(255); // ok
23+
24+
let _ = 1i32.checked_add(1).unwrap_or(i32::max_value());
25+
let _ = 1i32.checked_add(1).unwrap_or(i32::MAX);
26+
let _ = 1i8.checked_add(1).unwrap_or(127);
27+
let _ = 1i128
28+
.checked_add(1)
29+
.unwrap_or(170_141_183_460_469_231_731_687_303_715_884_105_727);
30+
let _ = 1i32.checked_add(-1).unwrap_or(i32::min_value());
31+
let _ = 1i32.checked_add(-1).unwrap_or(i32::MIN);
32+
let _ = 1i8.checked_add(-1).unwrap_or(-128);
33+
let _ = 1i128
34+
.checked_add(-1)
35+
.unwrap_or(-170_141_183_460_469_231_731_687_303_715_884_105_728);
36+
let _ = 1i32.checked_add(1).unwrap_or(1234); // ok
37+
let _ = 1i8.checked_add(1).unwrap_or(-128); // ok
38+
let _ = 1i8.checked_add(-1).unwrap_or(127); // ok
39+
40+
let _ = 1i32.checked_sub(1).unwrap_or(i32::min_value());
41+
let _ = 1i32.checked_sub(1).unwrap_or(i32::MIN);
42+
let _ = 1i8.checked_sub(1).unwrap_or(-128);
43+
let _ = 1i128
44+
.checked_sub(1)
45+
.unwrap_or(-170_141_183_460_469_231_731_687_303_715_884_105_728);
46+
let _ = 1i32.checked_sub(-1).unwrap_or(i32::max_value());
47+
let _ = 1i32.checked_sub(-1).unwrap_or(i32::MAX);
48+
let _ = 1i8.checked_sub(-1).unwrap_or(127);
49+
let _ = 1i128
50+
.checked_sub(-1)
51+
.unwrap_or(170_141_183_460_469_231_731_687_303_715_884_105_727);
52+
let _ = 1i32.checked_sub(1).unwrap_or(1234); // ok
53+
let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
54+
let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
55+
}

0 commit comments

Comments
 (0)