Auto merge of rust-lang#12944 - Jarcho:overflow_check, r=y21

Fix and rename `overflow_check_conditional`

fixes rust-lang#2457

Other changes:
* Limit the lint to unsigned types.
* Actually check if the operands are the same rather than using only the first part of the path.
* Allow the repeated expression to be anything as long as there are no side effects.

changelog: Rename `overflow_check_conditional` to `panicking_overflow_check` and move to `correctness`

CHANGELOG.md

@@ -5705,6 +5705,7 @@ Released 2018-09-13
 [`panic`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic
 [`panic_in_result_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_in_result_fn
 [`panic_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#panic_params
+[`panicking_overflow_checks`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_overflow_checks
 [`panicking_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#panicking_unwrap
 [`partial_pub_fields`]: https://rust-lang.github.io/rust-clippy/master/index.html#partial_pub_fields
 [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl

clippy_lints/src/declared_lints.rs

@@ -587,12 +587,12 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::operators::VERBOSE_BIT_MASK_INFO,
     crate::option_env_unwrap::OPTION_ENV_UNWRAP_INFO,
     crate::option_if_let_else::OPTION_IF_LET_ELSE_INFO,
-    crate::overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL_INFO,
     crate::panic_in_result_fn::PANIC_IN_RESULT_FN_INFO,
     crate::panic_unimplemented::PANIC_INFO,
     crate::panic_unimplemented::TODO_INFO,
     crate::panic_unimplemented::UNIMPLEMENTED_INFO,
     crate::panic_unimplemented::UNREACHABLE_INFO,
+    crate::panicking_overflow_checks::PANICKING_OVERFLOW_CHECKS_INFO,
     crate::partial_pub_fields::PARTIAL_PUB_FIELDS_INFO,
     crate::partialeq_ne_impl::PARTIALEQ_NE_IMPL_INFO,
     crate::partialeq_to_none::PARTIALEQ_TO_NONE_INFO,

clippy_lints/src/lib.rs

@@ -280,9 +280,9 @@ mod only_used_in_recursion;
 mod operators;
 mod option_env_unwrap;
 mod option_if_let_else;
-mod overflow_check_conditional;
 mod panic_in_result_fn;
 mod panic_unimplemented;
+mod panicking_overflow_checks;
 mod partial_pub_fields;
 mod partialeq_ne_impl;
 mod partialeq_to_none;
@@ -790,7 +790,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     let format_args = format_args_storage.clone();
     store.register_late_pass(move |_| Box::new(format::UselessFormat::new(format_args.clone())));
     store.register_late_pass(|_| Box::new(swap::Swap));
-    store.register_late_pass(|_| Box::new(overflow_check_conditional::OverflowCheckConditional));
+    store.register_late_pass(|_| Box::new(panicking_overflow_checks::PanickingOverflowChecks));
     store.register_late_pass(|_| Box::<new_without_default::NewWithoutDefault>::default());
     store.register_late_pass(move |_| Box::new(disallowed_names::DisallowedNames::new(disallowed_names)));
     store.register_late_pass(move |_| {

clippy_lints/src/panicking_overflow_checks.rs

@@ -0,0 +1,86 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::eq_expr_value;
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty;
+use rustc_session::declare_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Detects C-style underflow/overflow checks.
+    ///
+    /// ### Why is this bad?
+    /// These checks will, by default, panic in debug builds rather than check
+    /// whether the operation caused an overflow.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # let a = 1i32;
+    /// # let b = 2i32;
+    /// if a + b < a {
+    ///     // handle overflow
+    /// }
+    /// ```
+    ///
+    /// Use instead:
+    /// ```no_run
+    /// # let a = 1i32;
+    /// # let b = 2i32;
+    /// if a.checked_add(b).is_none() {
+    ///     // handle overflow
+    /// }
+    /// ```
+    ///
+    /// Or:
+    /// ```no_run
+    /// # let a = 1i32;
+    /// # let b = 2i32;
+    /// if a.overflowing_add(b).1 {
+    ///     // handle overflow
+    /// }
+    /// ```
+    #[clippy::version = "pre 1.29.0"]
+    pub PANICKING_OVERFLOW_CHECKS,
+    correctness,
+    "overflow checks which will panic in debug mode"
+}
+
+declare_lint_pass!(PanickingOverflowChecks => [PANICKING_OVERFLOW_CHECKS]);
+
+impl<'tcx> LateLintPass<'tcx> for PanickingOverflowChecks {
+    // a + b < a, a > a + b, a < a - b, a - b > a
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if let ExprKind::Binary(op, lhs, rhs) = expr.kind
+            && let (lt, gt) = match op.node {
+                BinOpKind::Lt => (lhs, rhs),
+                BinOpKind::Gt => (rhs, lhs),
+                _ => return,
+            }
+            && let ctxt = expr.span.ctxt()
+            && let (op_lhs, op_rhs, other, commutative) = match (&lt.kind, &gt.kind) {
+                (&ExprKind::Binary(op, lhs, rhs), _) if op.node == BinOpKind::Add && ctxt == lt.span.ctxt() => {
+                    (lhs, rhs, gt, true)
+                },
+                (_, &ExprKind::Binary(op, lhs, rhs)) if op.node == BinOpKind::Sub && ctxt == gt.span.ctxt() => {
+                    (lhs, rhs, lt, false)
+                },
+                _ => return,
+            }
+            && let typeck = cx.typeck_results()
+            && let ty = typeck.expr_ty(op_lhs)
+            && matches!(ty.kind(), ty::Uint(_))
+            && ty == typeck.expr_ty(op_rhs)
+            && ty == typeck.expr_ty(other)
+            && !in_external_macro(cx.tcx.sess, expr.span)
+            && (eq_expr_value(cx, op_lhs, other) || (commutative && eq_expr_value(cx, op_rhs, other)))
+        {
+            span_lint(
+                cx,
+                PANICKING_OVERFLOW_CHECKS,
+                expr.span,
+                "you are trying to use classic C overflow conditions that will fail in Rust",
+            );
+        }
+    }
+}

clippy_lints/src/renamed_lints.rs

@@ -26,6 +26,7 @@ pub static RENAMED_LINTS: &[(&str, &str)] = &[
     ("clippy::option_map_unwrap_or", "clippy::map_unwrap_or"),
     ("clippy::option_map_unwrap_or_else", "clippy::map_unwrap_or"),
     ("clippy::option_unwrap_used", "clippy::unwrap_used"),
+    ("clippy::overflow_check_conditional", "clippy::panicking_overflow_checks"),
     ("clippy::ref_in_deref", "clippy::needless_borrow"),
     ("clippy::result_expect_used", "clippy::expect_used"),
     ("clippy::result_map_unwrap_or_else", "clippy::map_unwrap_or"),

tests/ui/panicking_overflow_checks.rs

@@ -0,0 +1,27 @@
+#![warn(clippy::panicking_overflow_checks)]
+#![allow(clippy::needless_if)]
+
+fn test(a: u32, b: u32, c: u32) {
+    if a + b < a {} //~ panicking_overflow_checks
+    if a > a + b {} //~ panicking_overflow_checks
+    if a + b < b {} //~ panicking_overflow_checks
+    if b > a + b {} //~ panicking_overflow_checks
+    if a - b > b {}
+    if b < a - b {}
+    if a - b > a {} //~ panicking_overflow_checks
+    if a < a - b {} //~ panicking_overflow_checks
+    if a + b < c {}
+    if c > a + b {}
+    if a - b < c {}
+    if c > a - b {}
+    let i = 1.1;
+    let j = 2.2;
+    if i + j < i {}
+    if i - j < i {}
+    if i > i + j {}
+    if i - j < i {}
+}
+
+fn main() {
+    test(1, 2, 3)
+}

tests/ui/panicking_overflow_checks.stderr

@@ -0,0 +1,41 @@
+error: you are trying to use classic C overflow conditions that will fail in Rust
+  --> tests/ui/panicking_overflow_checks.rs:5:8
+   |
+LL |     if a + b < a {}
+   |        ^^^^^^^^^
+   |
+   = note: `-D clippy::panicking-overflow-checks` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::panicking_overflow_checks)]`
+
+error: you are trying to use classic C overflow conditions that will fail in Rust
+  --> tests/ui/panicking_overflow_checks.rs:6:8
+   |
+LL |     if a > a + b {}
+   |        ^^^^^^^^^
+
+error: you are trying to use classic C overflow conditions that will fail in Rust
+  --> tests/ui/panicking_overflow_checks.rs:7:8
+   |
+LL |     if a + b < b {}
+   |        ^^^^^^^^^
+
+error: you are trying to use classic C overflow conditions that will fail in Rust
+  --> tests/ui/panicking_overflow_checks.rs:8:8
+   |
+LL |     if b > a + b {}
+   |        ^^^^^^^^^
+
+error: you are trying to use classic C overflow conditions that will fail in Rust
+  --> tests/ui/panicking_overflow_checks.rs:11:8
+   |
+LL |     if a - b > a {}
+   |        ^^^^^^^^^
+
+error: you are trying to use classic C overflow conditions that will fail in Rust
+  --> tests/ui/panicking_overflow_checks.rs:12:8
+   |
+LL |     if a < a - b {}
+   |        ^^^^^^^^^
+
+error: aborting due to 6 previous errors
+

tests/ui/rename.fixed

@@ -24,6 +24,7 @@
 #![allow(clippy::expect_used)]
 #![allow(clippy::map_unwrap_or)]
 #![allow(clippy::unwrap_used)]
+#![allow(clippy::panicking_overflow_checks)]
 #![allow(clippy::needless_borrow)]
 #![allow(clippy::single_char_add_str)]
 #![allow(clippy::module_name_repetitions)]
@@ -78,6 +79,7 @@
 #![warn(clippy::map_unwrap_or)]
 #![warn(clippy::map_unwrap_or)]
 #![warn(clippy::unwrap_used)]
+#![warn(clippy::panicking_overflow_checks)]
 #![warn(clippy::needless_borrow)]
 #![warn(clippy::expect_used)]
 #![warn(clippy::map_unwrap_or)]