Skip to content

Commit

Permalink
Add checked_arith_unwrap_or lint
Browse files Browse the repository at this point in the history
  • Loading branch information
sinkuu committed Sep 4, 2019
1 parent 11da8c1 commit 54bf6f4
Show file tree
Hide file tree
Showing 9 changed files with 478 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,7 @@ Released 2018-09-13
[`char_lit_as_u8`]: https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8
[`chars_last_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_last_cmp
[`chars_next_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#chars_next_cmp
[`checked_arith_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#checked_arith_unwrap_or
[`checked_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#checked_conversions
[`clone_double_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_double_ref
[`clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

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

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

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

Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
mem_replace::MEM_REPLACE_OPTION_WITH_NONE,
methods::CHARS_LAST_CMP,
methods::CHARS_NEXT_CMP,
methods::CHECKED_ARITH_UNWRAP_OR,
methods::CLONE_DOUBLE_REF,
methods::CLONE_ON_COPY,
methods::EXPECT_FUN_CALL,
Expand Down Expand Up @@ -955,6 +956,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
matches::SINGLE_MATCH,
mem_replace::MEM_REPLACE_OPTION_WITH_NONE,
methods::CHARS_LAST_CMP,
methods::CHECKED_ARITH_UNWRAP_OR,
methods::INTO_ITER_ON_REF,
methods::ITER_CLONED_COLLECT,
methods::ITER_SKIP_NEXT,
Expand Down
173 changes: 173 additions & 0 deletions clippy_lints/src/methods/checked_arith_unwrap_or.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
use crate::utils::{match_qpath, snippet_with_applicability, span_lint_and_sugg};
use if_chain::if_chain;
use rustc::hir;
use rustc::lint::LateContext;
use rustc_errors::Applicability;
use rustc_target::abi::LayoutOf;
use syntax::ast;

pub fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr, args: &[&[hir::Expr]], arith: &str) {
let unwrap_arg = &args[0][1];
let arith_lhs = &args[1][0];
let arith_rhs = &args[1][1];

let ty = cx.tables.expr_ty(arith_lhs);
if !ty.is_integral() {
return;
}

let mm = if let Some(mm) = is_min_or_max(cx, unwrap_arg) {
mm
} else {
return;
};

if ty.is_signed() {
use self::{MinMax::*, Sign::*};

let sign = if let Some(sign) = lit_sign(arith_rhs) {
sign
} else {
return;
};

match (arith, sign, mm) {
("add", Pos, Max) | ("add", Neg, Min) | ("sub", Neg, Max) | ("sub", Pos, Min) => (),
// "mul" is omitted because lhs can be negative.
_ => return,
}

let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
super::CHECKED_ARITH_UNWRAP_OR,
expr.span,
"manual saturating arithmetic",
&format!("try using `saturating_{}`", arith),
format!(
"{}.saturating_{}({})",
snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability),
arith,
snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability),
),
applicability,
);
} else {
match (mm, arith) {
(MinMax::Max, "add") | (MinMax::Max, "mul") | (MinMax::Min, "sub") => (),
_ => return,
}

let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
super::CHECKED_ARITH_UNWRAP_OR,
expr.span,
"manual saturating arithmetic",
&format!("try using `saturating_{}`", arith),
format!(
"{}.saturating_{}({})",
snippet_with_applicability(cx, arith_lhs.span, "..", &mut applicability),
arith,
snippet_with_applicability(cx, arith_rhs.span, "..", &mut applicability),
),
applicability,
);
}
}

#[derive(PartialEq, Eq)]
enum MinMax {
Min,
Max,
}

fn is_min_or_max<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &hir::Expr) -> Option<MinMax> {
// `T::max_value()` `T::min_value()` inherent methods
if_chain! {
if let hir::ExprKind::Call(func, args) = &expr.node;
if args.is_empty();
if let hir::ExprKind::Path(path) = &func.node;
if let hir::QPath::TypeRelative(_, segment) = path;
then {
match &*segment.ident.as_str() {
"max_value" => return Some(MinMax::Max),
"min_value" => return Some(MinMax::Min),
_ => {}
}
}
}

let ty = cx.tables.expr_ty(expr);
let ty_str = ty.to_string();

// `std::T::MAX` `std::T::MIN` constants
if let hir::ExprKind::Path(path) = &expr.node {
if match_qpath(path, &["core", &ty_str, "MAX"][..]) {
return Some(MinMax::Max);
}

if match_qpath(path, &["core", &ty_str, "MIN"][..]) {
return Some(MinMax::Min);
}
}

// Literals
let bits = cx.layout_of(ty).unwrap().size.bits();
let (minval, maxval): (u128, u128) = if ty.is_signed() {
let minval = 1 << (bits - 1);
let mut maxval = !(1 << (bits - 1));
if bits != 128 {
maxval &= (1 << bits) - 1;
}
(minval, maxval)
} else {
(0, if bits == 128 { !0 } else { (1 << bits) - 1 })
};

let check_lit = |expr: &hir::Expr, check_min: bool| {
if let hir::ExprKind::Lit(lit) = &expr.node {
if let ast::LitKind::Int(value, _) = lit.node {
if value == maxval {
return Some(MinMax::Max);
}

if check_min && value == minval {
return Some(MinMax::Min);
}
}
}

None
};

if let r @ Some(_) = check_lit(expr, !ty.is_signed()) {
return r;
}

if ty.is_signed() {
if let hir::ExprKind::Unary(hir::UnNeg, val) = &expr.node {
return check_lit(val, true);
}
}

None
}

#[derive(PartialEq, Eq)]
enum Sign {
Pos,
Neg,
}

fn lit_sign(expr: &hir::Expr) -> Option<Sign> {
if let hir::ExprKind::Unary(hir::UnNeg, inner) = &expr.node {
if let hir::ExprKind::Lit(..) = &inner.node {
return Some(Sign::Neg);
}
} else if let hir::ExprKind::Lit(..) = &expr.node {
return Some(Sign::Pos);
}

None
}
32 changes: 32 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod checked_arith_unwrap_or;
mod option_map_unwrap_or;
mod unnecessary_filter_map;

Expand Down Expand Up @@ -983,6 +984,31 @@ declare_clippy_lint! {
"`MaybeUninit::uninit().assume_init()`"
}

declare_clippy_lint! {
/// **What it does:** Checks for `.checked_add/sub(x).unwrap_or(MAX/MIN)`.
///
/// **Why is this bad?** These can be written simply with `saturating_add/sub` methods.
///
/// **Example:**
///
/// ```rust
/// let x: u32 = 100;
///
/// let add = x.checked_add(y).unwrap_or(u32::max_value());
/// let sub = x.checked_sub(y).unwrap_or(u32::min_value());
/// ```
///
/// can be written using dedicated methods for saturating addition/subtraction as:
///
/// ```rust
/// let add = x.saturating_add(y);
/// let sub = x.saturating_sub(y);
/// ```
pub CHECKED_ARITH_UNWRAP_OR,
style,
"`.chcked_add/sub(x).unwrap_or(MAX/MIN)`"
}

declare_lint_pass!(Methods => [
OPTION_UNWRAP_USED,
RESULT_UNWRAP_USED,
Expand Down Expand Up @@ -1024,6 +1050,7 @@ declare_lint_pass!(Methods => [
INTO_ITER_ON_REF,
SUSPICIOUS_MAP,
UNINIT_ASSUMED_INIT,
CHECKED_ARITH_UNWRAP_OR,
]);

impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
Expand Down Expand Up @@ -1072,6 +1099,11 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]),
["count", "map"] => lint_suspicious_map(cx, expr),
["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr),
["unwrap_or", arith @ "checked_add"]
| ["unwrap_or", arith @ "checked_sub"]
| ["unwrap_or", arith @ "checked_mul"] => {
checked_arith_unwrap_or::lint(cx, expr, &arg_lists, &arith["checked_".len()..])
},
_ => {},
}

Expand Down
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 312] = [
pub const ALL_LINTS: [Lint; 313] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -189,6 +189,13 @@ pub const ALL_LINTS: [Lint; 312] = [
deprecation: None,
module: "methods",
},
Lint {
name: "checked_arith_unwrap_or",
group: "style",
desc: "`.chcked_add/sub(x).unwrap_or(MAX/MIN)`",
deprecation: None,
module: "methods",
},
Lint {
name: "checked_conversions",
group: "pedantic",
Expand Down
44 changes: 44 additions & 0 deletions tests/ui/checked_arith_unwrap_or.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// run-rustfix

#[allow(unused_imports)] // they become unnecessary after fix
use std::{i128, i32, u128, u32};

fn main() {
let _ = 1u32.saturating_add(1);
let _ = 1u32.saturating_add(1);
let _ = 1u8.saturating_add(1);
let _ = 1u128.saturating_add(1);
let _ = 1u32.checked_add(1).unwrap_or(1234); // ok
let _ = 1u8.checked_add(1).unwrap_or(0); // ok
let _ = 1u32.saturating_mul(1);

let _ = 1u32.saturating_sub(1);
let _ = 1u32.saturating_sub(1);
let _ = 1u8.saturating_sub(1);
let _ = 1u32.checked_sub(1).unwrap_or(1234); // ok
let _ = 1u8.checked_sub(1).unwrap_or(255); // ok

let _ = 1i32.saturating_add(1);
let _ = 1i32.saturating_add(1);
let _ = 1i8.saturating_add(1);
let _ = 1i128.saturating_add(1);
let _ = 1i32.saturating_add(-1);
let _ = 1i32.saturating_add(-1);
let _ = 1i8.saturating_add(-1);
let _ = 1i128.saturating_add(-1);
let _ = 1i32.checked_add(1).unwrap_or(1234); // ok
let _ = 1i8.checked_add(1).unwrap_or(-128); // ok
let _ = 1i8.checked_add(-1).unwrap_or(127); // ok

let _ = 1i32.saturating_sub(1);
let _ = 1i32.saturating_sub(1);
let _ = 1i8.saturating_sub(1);
let _ = 1i128.saturating_sub(1);
let _ = 1i32.saturating_sub(-1);
let _ = 1i32.saturating_sub(-1);
let _ = 1i8.saturating_sub(-1);
let _ = 1i128.saturating_sub(-1);
let _ = 1i32.checked_sub(1).unwrap_or(1234); // ok
let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
}
54 changes: 54 additions & 0 deletions tests/ui/checked_arith_unwrap_or.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// run-rustfix

#[allow(unused_imports)] // they become unnecessary after fix
use std::{i128, i32, u128, u32};

fn main() {
let _ = 1u32.checked_add(1).unwrap_or(u32::max_value());
let _ = 1u32.checked_add(1).unwrap_or(u32::MAX);
let _ = 1u8.checked_add(1).unwrap_or(255);
let _ = 1u128
.checked_add(1)
.unwrap_or(340_282_366_920_938_463_463_374_607_431_768_211_455);
let _ = 1u32.checked_add(1).unwrap_or(1234); // ok
let _ = 1u8.checked_add(1).unwrap_or(0); // ok
let _ = 1u32.checked_mul(1).unwrap_or(u32::MAX);

let _ = 1u32.checked_sub(1).unwrap_or(u32::min_value());
let _ = 1u32.checked_sub(1).unwrap_or(u32::MIN);
let _ = 1u8.checked_sub(1).unwrap_or(0);
let _ = 1u32.checked_sub(1).unwrap_or(1234); // ok
let _ = 1u8.checked_sub(1).unwrap_or(255); // ok

let _ = 1i32.checked_add(1).unwrap_or(i32::max_value());
let _ = 1i32.checked_add(1).unwrap_or(i32::MAX);
let _ = 1i8.checked_add(1).unwrap_or(127);
let _ = 1i128
.checked_add(1)
.unwrap_or(170_141_183_460_469_231_731_687_303_715_884_105_727);
let _ = 1i32.checked_add(-1).unwrap_or(i32::min_value());
let _ = 1i32.checked_add(-1).unwrap_or(i32::MIN);
let _ = 1i8.checked_add(-1).unwrap_or(-128);
let _ = 1i128
.checked_add(-1)
.unwrap_or(-170_141_183_460_469_231_731_687_303_715_884_105_728);
let _ = 1i32.checked_add(1).unwrap_or(1234); // ok
let _ = 1i8.checked_add(1).unwrap_or(-128); // ok
let _ = 1i8.checked_add(-1).unwrap_or(127); // ok

let _ = 1i32.checked_sub(1).unwrap_or(i32::min_value());
let _ = 1i32.checked_sub(1).unwrap_or(i32::MIN);
let _ = 1i8.checked_sub(1).unwrap_or(-128);
let _ = 1i128
.checked_sub(1)
.unwrap_or(-170_141_183_460_469_231_731_687_303_715_884_105_728);
let _ = 1i32.checked_sub(-1).unwrap_or(i32::max_value());
let _ = 1i32.checked_sub(-1).unwrap_or(i32::MAX);
let _ = 1i8.checked_sub(-1).unwrap_or(127);
let _ = 1i128
.checked_sub(-1)
.unwrap_or(170_141_183_460_469_231_731_687_303_715_884_105_727);
let _ = 1i32.checked_sub(1).unwrap_or(1234); // ok
let _ = 1i8.checked_sub(1).unwrap_or(127); // ok
let _ = 1i8.checked_sub(-1).unwrap_or(-128); // ok
}
Loading

0 comments on commit 54bf6f4

Please sign in to comment.