Modulo arithmetic

CHANGELOG.md

@@ -1186,6 +1186,7 @@ Released 2018-09-13
 [`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals
 [`module_inception`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_inception
 [`module_name_repetitions`]: https://rust-lang.github.io/rust-clippy/master/index.html#module_name_repetitions
+[`modulo_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#modulo_arithmetic
 [`modulo_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#modulo_one
 [`multiple_crate_versions`]: https://rust-lang.github.io/rust-clippy/master/index.html#multiple_crate_versions
 [`multiple_inherent_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#multiple_inherent_impl

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 341 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 342 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:
 

clippy_lints/src/lib.rs

@@ -239,6 +239,7 @@ pub mod misc_early;
 pub mod missing_const_for_fn;
 pub mod missing_doc;
 pub mod missing_inline;
+pub mod modulo_arithmetic;
 pub mod mul_add;
 pub mod multiple_crate_versions;
 pub mod mut_key;
@@ -667,6 +668,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         &missing_const_for_fn::MISSING_CONST_FOR_FN,
         &missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS,
         &missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS,
+        &modulo_arithmetic::MODULO_ARITHMETIC,
         &mul_add::MANUAL_MUL_ADD,
         &multiple_crate_versions::MULTIPLE_CRATE_VERSIONS,
         &mut_key::MUTABLE_KEY_TYPE,
@@ -943,6 +945,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
     store.register_late_pass(|| box comparison_chain::ComparisonChain);
     store.register_late_pass(|| box mul_add::MulAddCheck);
     store.register_late_pass(|| box mut_key::MutableKeyType);
+    store.register_late_pass(|| box modulo_arithmetic::ModuloArithmetic);
     store.register_early_pass(|| box reference::DerefAddrOf);
     store.register_early_pass(|| box reference::RefInDeref);
     store.register_early_pass(|| box double_parens::DoubleParens);
@@ -1003,6 +1006,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
         LintId::of(&misc::FLOAT_CMP_CONST),
         LintId::of(&missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS),
         LintId::of(&missing_inline::MISSING_INLINE_IN_PUBLIC_ITEMS),
+        LintId::of(&modulo_arithmetic::MODULO_ARITHMETIC),
         LintId::of(&panic_unimplemented::PANIC),
         LintId::of(&panic_unimplemented::TODO),
         LintId::of(&panic_unimplemented::UNIMPLEMENTED),

clippy_lints/src/modulo_arithmetic.rs

@@ -0,0 +1,149 @@
+use crate::consts::{constant, Constant};
+use crate::utils::{sext, span_lint_and_then};
+use if_chain::if_chain;
+use rustc::declare_lint_pass;
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::ty::{self};
+use rustc_session::declare_tool_lint;
+use std::fmt::Display;
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for modulo arithemtic.
+    ///
+    /// **Why is this bad?** The results of modulo (%) operation might differ
+    /// depending on the language, when negative numbers are involved.
+    /// If you interop with different languages it might be beneficial
+    /// to double check all places that use modulo arithmetic.
+    ///
+    /// For example, in Rust `17 % -3 = 2`, but in Python `17 % -3 = -1`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// let x = -17 % 3;
+    /// ```
+    pub MODULO_ARITHMETIC,
+    restriction,
+    "any modulo arithmetic statement"
+}
+
+declare_lint_pass!(ModuloArithmetic => [MODULO_ARITHMETIC]);
+
+struct OperandInfo {
+    string_representation: Option<String>,
+    is_negative: bool,
+    is_integral: bool,
+}
+
+fn analyze_operand(operand: &Expr<'_>, cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> Option<OperandInfo> {
+    match constant(cx, cx.tables, operand) {
+        Some((Constant::Int(v), _)) => match cx.tables.expr_ty(expr).kind {
+            ty::Int(ity) => {
+                let value = sext(cx.tcx, v, ity);
+                return Some(OperandInfo {
+                    string_representation: Some(value.to_string()),
+                    is_negative: value < 0,
+                    is_integral: true,
+                });
+            },
+            ty::Uint(_) => {
+                return Some(OperandInfo {
+                    string_representation: None,
+                    is_negative: false,
+                    is_integral: true,
+                });
+            },
+            _ => {},
+        },
+        Some((Constant::F32(f), _)) => {
+            return Some(floating_point_operand_info(&f));
+        },
+        Some((Constant::F64(f), _)) => {
+            return Some(floating_point_operand_info(&f));
+        },
+        _ => {},
+    }
+    None
+}
+
+fn floating_point_operand_info<T: Display + PartialOrd + From<f32>>(f: &T) -> OperandInfo {
+    OperandInfo {
+        string_representation: Some(format!("{:.3}", *f)),
+        is_negative: *f < 0.0.into(),
+        is_integral: false,
+    }
+}
+
+fn might_have_negative_value(t: &ty::TyS<'_>) -> bool {
+    t.is_signed() || t.is_floating_point()
+}
+
+fn check_const_operands<'a, 'tcx>(
+    cx: &LateContext<'a, 'tcx>,
+    expr: &'tcx Expr<'_>,
+    lhs_operand: &OperandInfo,
+    rhs_operand: &OperandInfo,
+) {
+    if lhs_operand.is_negative ^ rhs_operand.is_negative {
+        span_lint_and_then(
+            cx,
+            MODULO_ARITHMETIC,
+            expr.span,
+            &format!(
+                "you are using modulo operator on constants with different signs: `{} % {}`",
+                lhs_operand.string_representation.as_ref().unwrap(),
+                rhs_operand.string_representation.as_ref().unwrap()
+            ),
+            |db| {
+                db.note("double check for expected result especially when interoperating with different languages");
+                if lhs_operand.is_integral {
+                    db.note("or consider using `rem_euclid` or similar function");
+                }
+            },
+        );
+    }
+}
+
+fn check_non_const_operands<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>, operand: &Expr<'_>) {
+    let operand_type = cx.tables.expr_ty(operand);
+    if might_have_negative_value(operand_type) {
+        span_lint_and_then(
+            cx,
+            MODULO_ARITHMETIC,
+            expr.span,
+            "you are using modulo operator on types that might have different signs",
+            |db| {
+                db.note("double check for expected result especially when interoperating with different languages");
+                if operand_type.is_integral() {
+                    db.note("or consider using `rem_euclid` or similar function");
+                }
+            },
+        );
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ModuloArithmetic {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
+        match &expr.kind {
+            ExprKind::Binary(op, lhs, rhs) | ExprKind::AssignOp(op, lhs, rhs) => {
+                if let BinOpKind::Rem = op.node {
+                    let lhs_operand = analyze_operand(lhs, cx, expr);
+                    let rhs_operand = analyze_operand(rhs, cx, expr);
+                    if_chain! {
+                        if let Some(lhs_operand) = lhs_operand;
+                        if let Some(rhs_operand) = rhs_operand;
+                        then {
+                            check_const_operands(cx, expr, &lhs_operand, &rhs_operand);
+                        }
+                        else {
+                            check_non_const_operands(cx, expr, lhs);
+                        }
+                    }
+                };
+            },
+            _ => {},
+        }
+    }
+}

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; 341] = [
+pub const ALL_LINTS: [Lint; 342] = [
     Lint {
         name: "absurd_extreme_comparisons",
         group: "correctness",
@@ -1183,6 +1183,13 @@ pub const ALL_LINTS: [Lint; 341] = [
         deprecation: None,
         module: "enum_variants",
     },
+    Lint {
+        name: "modulo_arithmetic",
+        group: "restriction",
+        desc: "any modulo arithmetic statement",
+        deprecation: None,
+        module: "modulo_arithmetic",
+    },
     Lint {
         name: "modulo_one",
         group: "correctness",

tests/ui/modulo_arithmetic_float.rs

@@ -0,0 +1,36 @@
+#![warn(clippy::modulo_arithmetic)]
+#![allow(
+    unused,
+    clippy::shadow_reuse,
+    clippy::shadow_unrelated,
+    clippy::no_effect,
+    clippy::unnecessary_operation,
+    clippy::modulo_one
+)]
+
+fn main() {
+    // Lint when both sides are const and of the opposite sign
+    -1.6 % 2.1;
+    1.6 % -2.1;
+    (1.1 - 2.3) % (1.1 + 2.3);
+    (1.1 + 2.3) % (1.1 - 2.3);
+
+    // Lint on floating point numbers
+    let a_f32: f32 = -1.6;
+    let mut b_f32: f32 = 2.1;
+    a_f32 % b_f32;
+    b_f32 % a_f32;
+    b_f32 %= a_f32;
+
+    let a_f64: f64 = -1.6;
+    let mut b_f64: f64 = 2.1;
+    a_f64 % b_f64;
+    b_f64 % a_f64;
+    b_f64 %= a_f64;
+
+    // No lint when both sides are const and of the same sign
+    1.6 % 2.1;
+    -1.6 % -2.1;
+    (1.1 + 2.3) % (-1.1 + 2.3);
+    (-1.1 - 2.3) % (1.1 - 2.3);
+}

tests/ui/modulo_arithmetic_float.stderr

@@ -0,0 +1,83 @@
+error: you are using modulo operator on constants with different signs: `-1.600 % 2.100`
+  --> $DIR/modulo_arithmetic_float.rs:13:5
+   |
+LL |     -1.6 % 2.1;
+   |     ^^^^^^^^^^
+   |
+   = note: `-D clippy::modulo-arithmetic` implied by `-D warnings`
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on constants with different signs: `1.600 % -2.100`
+  --> $DIR/modulo_arithmetic_float.rs:14:5
+   |
+LL |     1.6 % -2.1;
+   |     ^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on constants with different signs: `-1.200 % 3.400`
+  --> $DIR/modulo_arithmetic_float.rs:15:5
+   |
+LL |     (1.1 - 2.3) % (1.1 + 2.3);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on constants with different signs: `3.400 % -1.200`
+  --> $DIR/modulo_arithmetic_float.rs:16:5
+   |
+LL |     (1.1 + 2.3) % (1.1 - 2.3);
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on types that might have different signs
+  --> $DIR/modulo_arithmetic_float.rs:21:5
+   |
+LL |     a_f32 % b_f32;
+   |     ^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on types that might have different signs
+  --> $DIR/modulo_arithmetic_float.rs:22:5
+   |
+LL |     b_f32 % a_f32;
+   |     ^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on types that might have different signs
+  --> $DIR/modulo_arithmetic_float.rs:23:5
+   |
+LL |     b_f32 %= a_f32;
+   |     ^^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on types that might have different signs
+  --> $DIR/modulo_arithmetic_float.rs:27:5
+   |
+LL |     a_f64 % b_f64;
+   |     ^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on types that might have different signs
+  --> $DIR/modulo_arithmetic_float.rs:28:5
+   |
+LL |     b_f64 % a_f64;
+   |     ^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: you are using modulo operator on types that might have different signs
+  --> $DIR/modulo_arithmetic_float.rs:29:5
+   |
+LL |     b_f64 %= a_f64;
+   |     ^^^^^^^^^^^^^^
+   |
+   = note: double check for expected result especially when interoperating with different languages
+
+error: aborting due to 10 previous errors
+