Auto merge of rust-lang#6360 - mlegner:mod_one, r=llogiq

bors · bors · commit a644930b2557 · 2020-11-28T07:07:25.000Z
Trigger modulo_one lint also on -1. Fixes rust-lang#6321. changelog: trigger `modulo_one` lint also on `-1`
diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs
@@ -18,7 +18,7 @@ use crate::utils::sugg::Sugg;
 use crate::utils::{
     get_item_name, get_parent_expr, higher, implements_trait, in_constant, is_integer_const, iter_input_pats,
     last_path_segment, match_qpath, match_trait_method, paths, snippet, snippet_opt, span_lint, span_lint_and_sugg,
-    span_lint_and_then, span_lint_hir_and_then, SpanlessEq,
+    span_lint_and_then, span_lint_hir_and_then, unsext, SpanlessEq,
 };
 
 declare_clippy_lint! {
@@ -139,23 +139,26 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for getting the remainder of a division by one.
+    /// **What it does:** Checks for getting the remainder of a division by one or minus
+    /// one.
     ///
-    /// **Why is this bad?** The result can only ever be zero. No one will write
-    /// such code deliberately, unless trying to win an Underhanded Rust
-    /// Contest. Even for that contest, it's probably a bad idea. Use something more
-    /// underhanded.
+    /// **Why is this bad?** The result for a divisor of one can only ever be zero; for
+    /// minus one it can cause panic/overflow (if the left operand is the minimal value of
+    /// the respective integer type) or results in zero. No one will write such code
+    /// deliberately, unless trying to win an Underhanded Rust Contest. Even for that
+    /// contest, it's probably a bad idea. Use something more underhanded.
     ///
     /// **Known problems:** None.
     ///
     /// **Example:**
     /// ```rust
     /// # let x = 1;
     /// let a = x % 1;
+    /// let a = x % -1;
     /// ```
     pub MODULO_ONE,
     correctness,
-    "taking a number modulo 1, which always returns 0"
+    "taking a number modulo +/-1, which can either panic/overflow or always returns 0"
 }
 
 declare_clippy_lint! {
@@ -378,60 +381,8 @@ impl<'tcx> LateLintPass<'tcx> for MiscLints {
                 return;
             },
             ExprKind::Binary(ref cmp, ref left, ref right) => {
-                let op = cmp.node;
-                if op.is_comparison() {
-                    check_nan(cx, left, expr);
-                    check_nan(cx, right, expr);
-                    check_to_owned(cx, left, right, true);
-                    check_to_owned(cx, right, left, false);
-                }
-                if (op == BinOpKind::Eq || op == BinOpKind::Ne) && (is_float(cx, left) || is_float(cx, right)) {
-                    if is_allowed(cx, left) || is_allowed(cx, right) {
-                        return;
-                    }
-
-                    // Allow comparing the results of signum()
-                    if is_signum(cx, left) && is_signum(cx, right) {
-                        return;
-                    }
-
-                    if let Some(name) = get_item_name(cx, expr) {
-                        let name = name.as_str();
-                        if name == "eq"
-                            || name == "ne"
-                            || name == "is_nan"
-                            || name.starts_with("eq_")
-                            || name.ends_with("_eq")
-                        {
-                            return;
-                        }
-                    }
-                    let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
-                    let (lint, msg) = get_lint_and_message(
-                        is_named_constant(cx, left) || is_named_constant(cx, right),
-                        is_comparing_arrays,
-                    );
-                    span_lint_and_then(cx, lint, expr.span, msg, |diag| {
-                        let lhs = Sugg::hir(cx, left, "..");
-                        let rhs = Sugg::hir(cx, right, "..");
-
-                        if !is_comparing_arrays {
-                            diag.span_suggestion(
-                                expr.span,
-                                "consider comparing them within some margin of error",
-                                format!(
-                                    "({}).abs() {} error_margin",
-                                    lhs - rhs,
-                                    if op == BinOpKind::Eq { '<' } else { '>' }
-                                ),
-                                Applicability::HasPlaceholders, // snippet
-                            );
-                        }
-                        diag.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`");
-                    });
-                } else if op == BinOpKind::Rem && is_integer_const(cx, right, 1) {
-                    span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0");
-                }
+                check_binary(cx, expr, cmp, left, right);
+                return;
             },
             _ => {},
         }
@@ -744,3 +695,74 @@ fn check_cast(cx: &LateContext<'_>, span: Span, e: &Expr<'_>, ty: &hir::Ty<'_>)
         }
     }
 }
+
+fn check_binary(
+    cx: &LateContext<'a>,
+    expr: &Expr<'_>,
+    cmp: &rustc_span::source_map::Spanned<rustc_hir::BinOpKind>,
+    left: &'a Expr<'_>,
+    right: &'a Expr<'_>,
+) {
+    let op = cmp.node;
+    if op.is_comparison() {
+        check_nan(cx, left, expr);
+        check_nan(cx, right, expr);
+        check_to_owned(cx, left, right, true);
+        check_to_owned(cx, right, left, false);
+    }
+    if (op == BinOpKind::Eq || op == BinOpKind::Ne) && (is_float(cx, left) || is_float(cx, right)) {
+        if is_allowed(cx, left) || is_allowed(cx, right) {
+            return;
+        }
+
+        // Allow comparing the results of signum()
+        if is_signum(cx, left) && is_signum(cx, right) {
+            return;
+        }
+
+        if let Some(name) = get_item_name(cx, expr) {
+            let name = name.as_str();
+            if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_") || name.ends_with("_eq") {
+                return;
+            }
+        }
+        let is_comparing_arrays = is_array(cx, left) || is_array(cx, right);
+        let (lint, msg) = get_lint_and_message(
+            is_named_constant(cx, left) || is_named_constant(cx, right),
+            is_comparing_arrays,
+        );
+        span_lint_and_then(cx, lint, expr.span, msg, |diag| {
+            let lhs = Sugg::hir(cx, left, "..");
+            let rhs = Sugg::hir(cx, right, "..");
+
+            if !is_comparing_arrays {
+                diag.span_suggestion(
+                    expr.span,
+                    "consider comparing them within some margin of error",
+                    format!(
+                        "({}).abs() {} error_margin",
+                        lhs - rhs,
+                        if op == BinOpKind::Eq { '<' } else { '>' }
+                    ),
+                    Applicability::HasPlaceholders, // snippet
+                );
+            }
+            diag.note("`f32::EPSILON` and `f64::EPSILON` are available for the `error_margin`");
+        });
+    } else if op == BinOpKind::Rem {
+        if is_integer_const(cx, right, 1) {
+            span_lint(cx, MODULO_ONE, expr.span, "any number modulo 1 will be 0");
+        }
+
+        if let ty::Int(ity) = cx.typeck_results().expr_ty(right).kind() {
+            if is_integer_const(cx, right, unsext(cx.tcx, -1, *ity)) {
+                span_lint(
+                    cx,
+                    MODULO_ONE,
+                    expr.span,
+                    "any number modulo -1 will panic/overflow or result in 0",
+                );
+            }
+        };
+    }
+}
diff --git a/tests/ui/modulo_one.rs b/tests/ui/modulo_one.rs
@@ -2,13 +2,22 @@
 #![allow(clippy::no_effect, clippy::unnecessary_operation)]
 
 static STATIC_ONE: usize = 2 - 1;
+static STATIC_NEG_ONE: i64 = 1 - 2;
 
 fn main() {
     10 % 1;
+    10 % -1;
     10 % 2;
+    i32::MIN % (-1); // also caught by rustc
 
     const ONE: u32 = 1 * 1;
+    const NEG_ONE: i64 = 1 - 2;
+    const INT_MIN: i64 = i64::MIN;
 
     2 % ONE;
-    5 % STATIC_ONE;
+    5 % STATIC_ONE; // NOT caught by lint
+    2 % NEG_ONE;
+    5 % STATIC_NEG_ONE; // NOT caught by lint
+    INT_MIN % NEG_ONE; // also caught by rustc
+    INT_MIN % STATIC_NEG_ONE; // ONLY caught by rustc
 }
diff --git a/tests/ui/modulo_one.stderr b/tests/ui/modulo_one.stderr
@@ -1,30 +1,74 @@
+error: this arithmetic operation will overflow
+  --> $DIR/modulo_one.rs:11:5
+   |
+LL |     i32::MIN % (-1); // also caught by rustc
+   |     ^^^^^^^^^^^^^^^ attempt to compute the remainder of `i32::MIN % -1_i32`, which would overflow
+   |
+   = note: `#[deny(arithmetic_overflow)]` on by default
+
+error: this arithmetic operation will overflow
+  --> $DIR/modulo_one.rs:21:5
+   |
+LL |     INT_MIN % NEG_ONE; // also caught by rustc
+   |     ^^^^^^^^^^^^^^^^^ attempt to compute the remainder of `i64::MIN % -1_i64`, which would overflow
+
+error: this arithmetic operation will overflow
+  --> $DIR/modulo_one.rs:22:5
+   |
+LL |     INT_MIN % STATIC_NEG_ONE; // ONLY caught by rustc
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^ attempt to compute the remainder of `i64::MIN % -1_i64`, which would overflow
+
 error: any number modulo 1 will be 0
-  --> $DIR/modulo_one.rs:7:5
+  --> $DIR/modulo_one.rs:8:5
    |
 LL |     10 % 1;
    |     ^^^^^^
    |
    = note: `-D clippy::modulo-one` implied by `-D warnings`
 
+error: any number modulo -1 will panic/overflow or result in 0
+  --> $DIR/modulo_one.rs:9:5
+   |
+LL |     10 % -1;
+   |     ^^^^^^^
+
+error: any number modulo -1 will panic/overflow or result in 0
+  --> $DIR/modulo_one.rs:11:5
+   |
+LL |     i32::MIN % (-1); // also caught by rustc
+   |     ^^^^^^^^^^^^^^^
+
 error: the operation is ineffective. Consider reducing it to `1`
-  --> $DIR/modulo_one.rs:10:22
+  --> $DIR/modulo_one.rs:13:22
    |
 LL |     const ONE: u32 = 1 * 1;
    |                      ^^^^^
    |
    = note: `-D clippy::identity-op` implied by `-D warnings`
 
 error: the operation is ineffective. Consider reducing it to `1`
-  --> $DIR/modulo_one.rs:10:22
+  --> $DIR/modulo_one.rs:13:22
    |
 LL |     const ONE: u32 = 1 * 1;
    |                      ^^^^^
 
 error: any number modulo 1 will be 0
-  --> $DIR/modulo_one.rs:12:5
+  --> $DIR/modulo_one.rs:17:5
    |
 LL |     2 % ONE;
    |     ^^^^^^^
 
-error: aborting due to 4 previous errors
+error: any number modulo -1 will panic/overflow or result in 0
+  --> $DIR/modulo_one.rs:19:5
+   |
+LL |     2 % NEG_ONE;
+   |     ^^^^^^^^^^^
+
+error: any number modulo -1 will panic/overflow or result in 0
+  --> $DIR/modulo_one.rs:21:5
+   |
+LL |     INT_MIN % NEG_ONE; // also caught by rustc
+   |     ^^^^^^^^^^^^^^^^^
+
+error: aborting due to 11 previous errors
 
