From dba5adae6e626e0d709be1146fae19b6801e1655 Mon Sep 17 00:00:00 2001 From: Caio Date: Thu, 15 Sep 2022 13:28:18 -0300 Subject: [PATCH] [arithmetic-side-effects] Finish non-overflowing ops --- .../src/operators/arithmetic_side_effects.rs | 41 ++++---- tests/ui/arithmetic_side_effects.rs | 70 ++++++++----- tests/ui/arithmetic_side_effects.stderr | 98 ++++++++++++------- 3 files changed, 127 insertions(+), 82 deletions(-) diff --git a/clippy_lints/src/operators/arithmetic_side_effects.rs b/clippy_lints/src/operators/arithmetic_side_effects.rs index 21c2b76439b..4629ea1f65b 100644 --- a/clippy_lints/src/operators/arithmetic_side_effects.rs +++ b/clippy_lints/src/operators/arithmetic_side_effects.rs @@ -42,26 +42,23 @@ pub fn new(mut allowed: FxHashSet) -> Self { } } - /// Checks assign operators (+=, -=, *=, /=) of integers in a non-constant environment that - /// won't overflow. - fn has_valid_assign_op(op: &Spanned, rhs: &hir::Expr<'_>, rhs_refs: Ty<'_>) -> bool { - if !Self::is_literal_integer(rhs, rhs_refs) { - return false; - } + /// Assuming that `expr` is a literal integer, checks assign operators (+=, -=, *=, /=) in a + /// non-constant environment that won't overflow. + fn has_valid_assign_op(op: &Spanned, expr: &hir::Expr<'_>) -> bool { if let hir::BinOpKind::Add | hir::BinOpKind::Sub = op.node - && let hir::ExprKind::Lit(ref lit) = rhs.kind + && let hir::ExprKind::Lit(ref lit) = expr.kind && let ast::LitKind::Int(0, _) = lit.node { return true; } if let hir::BinOpKind::Div | hir::BinOpKind::Rem = op.node - && let hir::ExprKind::Lit(ref lit) = rhs.kind + && let hir::ExprKind::Lit(ref lit) = expr.kind && !matches!(lit.node, ast::LitKind::Int(0, _)) { return true; } if let hir::BinOpKind::Mul = op.node - && let hir::ExprKind::Lit(ref lit) = rhs.kind + && let hir::ExprKind::Lit(ref lit) = expr.kind && let ast::LitKind::Int(0 | 1, _) = lit.node { return true; @@ -69,12 +66,6 @@ fn has_valid_assign_op(op: &Spanned, rhs: &hir::Expr<'_>, rhs_re false } - /// Checks "raw" binary operators (+, -, *, /) of integers in a non-constant environment - /// already handled by the CTFE. - fn has_valid_bin_op(lhs: &hir::Expr<'_>, lhs_refs: Ty<'_>, rhs: &hir::Expr<'_>, rhs_refs: Ty<'_>) -> bool { - Self::is_literal_integer(lhs, lhs_refs) && Self::is_literal_integer(rhs, rhs_refs) - } - /// Checks if the given `expr` has any of the inner `allowed` elements. fn is_allowed_ty(&self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool { self.allowed.contains( @@ -95,7 +86,8 @@ fn is_literal_integer(expr: &hir::Expr<'_>, expr_refs: Ty<'_>) -> bool { } fn issue_lint(&mut self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) { - span_lint(cx, ARITHMETIC_SIDE_EFFECTS, expr.span, "arithmetic detected"); + let msg = "arithmetic operation that can potentially result in unexpected side-effects"; + span_lint(cx, ARITHMETIC_SIDE_EFFECTS, expr.span, msg); self.expr_span = Some(expr.span); } @@ -127,13 +119,18 @@ fn manage_bin_ops( if self.is_allowed_ty(cx, lhs) || self.is_allowed_ty(cx, rhs) { return; } - let lhs_refs = cx.typeck_results().expr_ty(lhs).peel_refs(); - let rhs_refs = cx.typeck_results().expr_ty(rhs).peel_refs(); - let has_valid_assign_op = Self::has_valid_assign_op(op, rhs, rhs_refs); - if has_valid_assign_op || Self::has_valid_bin_op(lhs, lhs_refs, rhs, rhs_refs) { - return; + let has_valid_op = match ( + Self::is_literal_integer(lhs, cx.typeck_results().expr_ty(rhs).peel_refs()), + Self::is_literal_integer(rhs, cx.typeck_results().expr_ty(rhs).peel_refs()), + ) { + (true, true) => true, + (true, false) => Self::has_valid_assign_op(op, lhs), + (false, true) => Self::has_valid_assign_op(op, rhs), + (false, false) => false, + }; + if !has_valid_op { + self.issue_lint(cx, expr); } - self.issue_lint(cx, expr); } } diff --git a/tests/ui/arithmetic_side_effects.rs b/tests/ui/arithmetic_side_effects.rs index a51c2514283..c07425be50a 100644 --- a/tests/ui/arithmetic_side_effects.rs +++ b/tests/ui/arithmetic_side_effects.rs @@ -1,7 +1,10 @@ #![allow( clippy::assign_op_pattern, - unconditional_panic, - clippy::unnecessary_owned_empty_strings + clippy::erasing_op, + clippy::identity_op, + clippy::unnecessary_owned_empty_strings, + arithmetic_overflow, + unconditional_panic )] #![feature(inline_const, saturating_int_impl)] #![warn(clippy::arithmetic_side_effects)] @@ -30,7 +33,7 @@ pub fn hard_coded_allowed() { } #[rustfmt::skip] -pub fn non_overflowing_ops() { +pub fn non_overflowing_const_ops() { const _: i32 = { let mut n = 1; n += 1; n }; let _ = const { let mut n = 1; n += 1; n }; @@ -41,33 +44,54 @@ pub fn non_overflowing_ops() { let _ = const { let mut n = 1; n = 1 + n; n }; const _: i32 = 1 + 1; - let _ = 1 + 1; let _ = const { 1 + 1 }; +} - let mut _a = 1; - _a += 0; - _a -= 0; - _a /= 99; - _a %= 99; - _a *= 0; - _a *= 1; +pub fn non_overflowing_runtime_ops() { + let mut _n = i32::MAX; + + // Assign + _n += 0; + _n -= 0; + _n /= 99; + _n %= 99; + _n *= 0; + _n *= 1; + + // Binary + _n = _n + 0; + _n = 0 + _n; + _n = _n - 0; + _n = 0 - _n; + _n = _n / 99; + _n = _n % 99; + _n = _n * 0; + _n = 0 * _n; + _n = _n * 1; + _n = 1 * _n; + _n = 23 + 85; } #[rustfmt::skip] -pub fn overflowing_ops() { - let mut _a = 1; _a += 1; +pub fn overflowing_runtime_ops() { + let mut _n = i32::MAX; - let mut _b = 1; _b = _b + 1; + // Assign + _n += 1; + _n -= 1; + _n /= 0; + _n %= 0; + _n *= 2; - let mut _c = 1; _c = 1 + _c; - - let mut _a = 1; - _a += 1; - _a -= 1; - _a /= 0; - _a %= 0; - _a *= 2; - _a *= 3; + // Binary + _n = _n + 1; + _n = 1 + _n; + _n = _n - 1; + _n = 1 - _n; + _n = _n / 0; + _n = _n % 0; + _n = _n * 2; + _n = 2 * _n; } fn main() {} diff --git a/tests/ui/arithmetic_side_effects.stderr b/tests/ui/arithmetic_side_effects.stderr index 6652a1f8ca8..2f953d26699 100644 --- a/tests/ui/arithmetic_side_effects.stderr +++ b/tests/ui/arithmetic_side_effects.stderr @@ -1,58 +1,82 @@ -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:58:21 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:80:5 | -LL | let mut _a = 1; _a += 1; - | ^^^^^^^ +LL | _n += 1; + | ^^^^^^^ | = note: `-D clippy::arithmetic-side-effects` implied by `-D warnings` -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:60:26 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:81:5 | -LL | let mut _b = 1; _b = _b + 1; - | ^^^^^^ - -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:62:26 - | -LL | let mut _c = 1; _c = 1 + _c; - | ^^^^^^ - -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:65:5 - | -LL | _a += 1; +LL | _n -= 1; | ^^^^^^^ -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:66:5 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:82:5 | -LL | _a -= 1; +LL | _n /= 0; | ^^^^^^^ -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:67:5 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:83:5 | -LL | _a /= 0; +LL | _n %= 0; | ^^^^^^^ -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:68:5 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:84:5 | -LL | _a %= 0; +LL | _n *= 2; | ^^^^^^^ -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:69:5 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:87:10 | -LL | _a *= 2; - | ^^^^^^^ +LL | _n = _n + 1; + | ^^^^^^ -error: arithmetic detected - --> $DIR/arithmetic_side_effects.rs:70:5 +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:88:10 | -LL | _a *= 3; - | ^^^^^^^ +LL | _n = 1 + _n; + | ^^^^^^ -error: aborting due to 9 previous errors +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:89:10 + | +LL | _n = _n - 1; + | ^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:90:10 + | +LL | _n = 1 - _n; + | ^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:91:10 + | +LL | _n = _n / 0; + | ^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:92:10 + | +LL | _n = _n % 0; + | ^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:93:10 + | +LL | _n = _n * 2; + | ^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:94:10 + | +LL | _n = 2 * _n; + | ^^^^^^ + +error: aborting due to 13 previous errors