From f191e916bdbb98ef91a35b3c82376b3c517654fb Mon Sep 17 00:00:00 2001 From: mgr-inz-rafal Date: Thu, 26 Dec 2019 13:34:18 +0100 Subject: [PATCH] Add new lint (modulo_arithmetic) --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/lib.rs | 4 + clippy_lints/src/modulo_arithmetic.rs | 149 ++++++++++++++++++++++++++ src/lintlist/mod.rs | 9 +- 5 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/modulo_arithmetic.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index f0684ea44cc..50a7f44ad8e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index be5433029de..01fc20f0f27 100644 --- 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 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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c83be8e2c19..8b8f6b27a95 100644 --- a/clippy_lints/src/lib.rs +++ b/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), diff --git a/clippy_lints/src/modulo_arithmetic.rs b/clippy_lints/src/modulo_arithmetic.rs new file mode 100644 index 00000000000..308bb00fe76 --- /dev/null +++ b/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, + is_negative: bool, + is_integral: bool, +} + +fn analyze_operand(operand: &Expr, cx: &LateContext<'_, '_>, expr: &Expr) -> Option { + 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>(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); + } + } + }; + }, + _ => {}, + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index bdbe06c5f27..08cbff40444 100644 --- 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; 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",