Add checked_arith_unwrap_or lint

sinkuu · sinkuu · commit 54bf6f4c0380 · 2019-09-04T18:19:16.000+09:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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
diff --git a/README.md b/README.md
@@ -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:
 
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -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,
@@ -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,
diff --git a/clippy_lints/src/methods/checked_arith_unwrap_or.rs b/clippy_lints/src/methods/checked_arith_unwrap_or.rs
@@ -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
+}
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -1,3 +1,4 @@
+mod checked_arith_unwrap_or;
 mod option_map_unwrap_or;
 mod unnecessary_filter_map;
 
@@ -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,
@@ -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 {
@@ -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()..])
+            },
             _ => {},
         }
 
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
@@ -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",
@@ -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",
diff --git a/tests/ui/checked_arith_unwrap_or.fixed b/tests/ui/checked_arith_unwrap_or.fixed
@@ -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
+}
diff --git a/tests/ui/checked_arith_unwrap_or.rs b/tests/ui/checked_arith_unwrap_or.rs
@@ -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
+}
diff --git a/tests/ui/checked_arith_unwrap_or.stderr b/tests/ui/checked_arith_unwrap_or.stderr
