From 4876882b29a1535779593d321fcb886a0c26c594 Mon Sep 17 00:00:00 2001 From: Caio Date: Fri, 30 Sep 2022 15:30:40 -0300 Subject: [PATCH 1/2] Fix #9544 --- .../src/operators/arithmetic_side_effects.rs | 37 ++-- tests/ui/arithmetic_side_effects.rs | 53 ++++- tests/ui/arithmetic_side_effects.stderr | 202 +++++++++++++++--- 3 files changed, 243 insertions(+), 49 deletions(-) diff --git a/clippy_lints/src/operators/arithmetic_side_effects.rs b/clippy_lints/src/operators/arithmetic_side_effects.rs index c8a374d90..d871b72db 100644 --- a/clippy_lints/src/operators/arithmetic_side_effects.rs +++ b/clippy_lints/src/operators/arithmetic_side_effects.rs @@ -16,8 +16,9 @@ const HARD_CODED_ALLOWED: &[&str] = &[ "f32", "f64", "std::num::Saturating", - "std::string::String", "std::num::Wrapping", + "std::string::String", + "&str", ]; #[derive(Debug)] @@ -77,6 +78,11 @@ impl ArithmeticSideEffects { ) } + // For example, 8i32 or &i64::MAX. + fn is_integral<'expr, 'tcx>(cx: &LateContext<'tcx>, expr: &'expr hir::Expr<'tcx>) -> bool { + cx.typeck_results().expr_ty(expr).peel_refs().is_integral() + } + // Common entry-point to avoid code duplication. fn issue_lint(&mut self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) { let msg = "arithmetic operation that can potentially result in unexpected side-effects"; @@ -88,24 +94,13 @@ impl ArithmeticSideEffects { /// * Is `expr` is a literal integer reference like `&199`, returns the literal integer without /// references. /// * If `expr` is anything else, returns `None`. - fn literal_integer<'expr, 'tcx>( - cx: &LateContext<'tcx>, - expr: &'expr hir::Expr<'tcx>, - ) -> Option<&'expr hir::Expr<'tcx>> { - let expr_refs = cx.typeck_results().expr_ty(expr).peel_refs(); - - if !expr_refs.is_integral() { - return None; - } - + fn literal_integer<'expr, 'tcx>(expr: &'expr hir::Expr<'tcx>) -> Option<&'expr hir::Expr<'tcx>> { if matches!(expr.kind, hir::ExprKind::Lit(_)) { return Some(expr); } - if let hir::ExprKind::AddrOf(.., inn) = expr.kind && let hir::ExprKind::Lit(_) = inn.kind { return Some(inn) } - None } @@ -134,14 +129,18 @@ impl ArithmeticSideEffects { ) { return; }; - if self.is_allowed_ty(cx, lhs) || self.is_allowed_ty(cx, rhs) { + if self.is_allowed_ty(cx, lhs) && self.is_allowed_ty(cx, rhs) { return; } - let has_valid_op = match (Self::literal_integer(cx, lhs), Self::literal_integer(cx, rhs)) { - (None, None) => false, - (None, Some(local_expr)) => Self::has_valid_op(op, local_expr), - (Some(local_expr), None) => Self::has_valid_op(op, local_expr), - (Some(_), Some(_)) => true, + let has_valid_op = if Self::is_integral(cx, lhs) && Self::is_integral(cx, rhs) { + match (Self::literal_integer(lhs), Self::literal_integer(rhs)) { + (None, None) => false, + (None, Some(local_expr)) => Self::has_valid_op(op, local_expr), + (Some(local_expr), None) => Self::has_valid_op(op, local_expr), + (Some(_), Some(_)) => true, + } + } else { + false }; if !has_valid_op { self.issue_lint(cx, expr); diff --git a/tests/ui/arithmetic_side_effects.rs b/tests/ui/arithmetic_side_effects.rs index 6741e1485..c9c0f8be6 100644 --- a/tests/ui/arithmetic_side_effects.rs +++ b/tests/ui/arithmetic_side_effects.rs @@ -12,6 +12,31 @@ use core::num::{Saturating, Wrapping}; +pub struct Custom; + +macro_rules! impl_arith { + ( $( $_trait:ident, $ty:ty, $method:ident; )* ) => { + $( + impl core::ops::$_trait<$ty> for Custom { + type Output = Self; + fn $method(self, _: $ty) -> Self::Output { Self } + } + )* + } +} + +impl_arith!( + Add, i32, add; + Div, i32, div; + Mul, i32, mul; + Sub, i32, sub; + + Add, f64, add; + Div, f64, div; + Mul, f64, mul; + Sub, f64, sub; +); + pub fn association_with_structures_should_not_trigger_the_lint() { enum Foo { Bar = -2, @@ -130,7 +155,7 @@ pub fn non_overflowing_ops_or_ops_already_handled_by_the_compiler_should_not_tri _n = -(-1); } -pub fn overflowing_runtime_ops() { +pub fn runtime_ops() { let mut _n = i32::MAX; // Assign @@ -163,6 +188,32 @@ pub fn overflowing_runtime_ops() { _n = 2 * _n; _n = &2 * _n; + // Custom + let _ = Custom + 0; + let _ = Custom + 1; + let _ = Custom + 2; + let _ = Custom + 0.0; + let _ = Custom + 1.0; + let _ = Custom + 2.0; + let _ = Custom - 0; + let _ = Custom - 1; + let _ = Custom - 2; + let _ = Custom - 0.0; + let _ = Custom - 1.0; + let _ = Custom - 2.0; + let _ = Custom / 0; + let _ = Custom / 1; + let _ = Custom / 2; + let _ = Custom / 0.0; + let _ = Custom / 1.0; + let _ = Custom / 2.0; + let _ = Custom * 0; + let _ = Custom * 1; + let _ = Custom * 2; + let _ = Custom * 0.0; + let _ = Custom * 1.0; + let _ = Custom * 2.0; + // Unary _n = -_n; _n = -&_n; diff --git a/tests/ui/arithmetic_side_effects.stderr b/tests/ui/arithmetic_side_effects.stderr index 4dce13b62..7870c942e 100644 --- a/tests/ui/arithmetic_side_effects.stderr +++ b/tests/ui/arithmetic_side_effects.stderr @@ -1,5 +1,5 @@ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:137:5 + --> $DIR/arithmetic_side_effects.rs:162:5 | LL | _n += 1; | ^^^^^^^ @@ -7,166 +7,310 @@ LL | _n += 1; = note: `-D clippy::arithmetic-side-effects` implied by `-D warnings` error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:138:5 + --> $DIR/arithmetic_side_effects.rs:163:5 | LL | _n += &1; | ^^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:139:5 + --> $DIR/arithmetic_side_effects.rs:164:5 | LL | _n -= 1; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:140:5 + --> $DIR/arithmetic_side_effects.rs:165:5 | LL | _n -= &1; | ^^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:141:5 + --> $DIR/arithmetic_side_effects.rs:166:5 | LL | _n /= 0; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:142:5 + --> $DIR/arithmetic_side_effects.rs:167:5 | LL | _n /= &0; | ^^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:143:5 + --> $DIR/arithmetic_side_effects.rs:168:5 | LL | _n %= 0; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:144:5 + --> $DIR/arithmetic_side_effects.rs:169:5 | LL | _n %= &0; | ^^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:145:5 + --> $DIR/arithmetic_side_effects.rs:170:5 | LL | _n *= 2; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:146:5 + --> $DIR/arithmetic_side_effects.rs:171:5 | LL | _n *= &2; | ^^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:149:10 + --> $DIR/arithmetic_side_effects.rs:174:10 | LL | _n = _n + 1; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:150:10 + --> $DIR/arithmetic_side_effects.rs:175:10 | LL | _n = _n + &1; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:151:10 + --> $DIR/arithmetic_side_effects.rs:176:10 | LL | _n = 1 + _n; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:152:10 + --> $DIR/arithmetic_side_effects.rs:177:10 | LL | _n = &1 + _n; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:153:10 + --> $DIR/arithmetic_side_effects.rs:178:10 | LL | _n = _n - 1; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:154:10 + --> $DIR/arithmetic_side_effects.rs:179:10 | LL | _n = _n - &1; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:155:10 + --> $DIR/arithmetic_side_effects.rs:180:10 | LL | _n = 1 - _n; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:156:10 + --> $DIR/arithmetic_side_effects.rs:181:10 | LL | _n = &1 - _n; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:157:10 + --> $DIR/arithmetic_side_effects.rs:182:10 | LL | _n = _n / 0; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:158:10 + --> $DIR/arithmetic_side_effects.rs:183:10 | LL | _n = _n / &0; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:159:10 + --> $DIR/arithmetic_side_effects.rs:184:10 | LL | _n = _n % 0; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:160:10 + --> $DIR/arithmetic_side_effects.rs:185:10 | LL | _n = _n % &0; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:161:10 + --> $DIR/arithmetic_side_effects.rs:186:10 | LL | _n = _n * 2; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:162:10 + --> $DIR/arithmetic_side_effects.rs:187:10 | LL | _n = _n * &2; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:163:10 + --> $DIR/arithmetic_side_effects.rs:188:10 | LL | _n = 2 * _n; | ^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:164:10 + --> $DIR/arithmetic_side_effects.rs:189:10 | LL | _n = &2 * _n; | ^^^^^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:167:10 + --> $DIR/arithmetic_side_effects.rs:192:13 + | +LL | let _ = Custom + 0; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:193:13 + | +LL | let _ = Custom + 1; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:194:13 + | +LL | let _ = Custom + 2; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:195:13 + | +LL | let _ = Custom + 0.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:196:13 + | +LL | let _ = Custom + 1.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:197:13 + | +LL | let _ = Custom + 2.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:198:13 + | +LL | let _ = Custom - 0; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:199:13 + | +LL | let _ = Custom - 1; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:200:13 + | +LL | let _ = Custom - 2; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:201:13 + | +LL | let _ = Custom - 0.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:202:13 + | +LL | let _ = Custom - 1.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:203:13 + | +LL | let _ = Custom - 2.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:204:13 + | +LL | let _ = Custom / 0; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:205:13 + | +LL | let _ = Custom / 1; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:206:13 + | +LL | let _ = Custom / 2; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:207:13 + | +LL | let _ = Custom / 0.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:208:13 + | +LL | let _ = Custom / 1.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:209:13 + | +LL | let _ = Custom / 2.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:210:13 + | +LL | let _ = Custom * 0; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:211:13 + | +LL | let _ = Custom * 1; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:212:13 + | +LL | let _ = Custom * 2; + | ^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:213:13 + | +LL | let _ = Custom * 0.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:214:13 + | +LL | let _ = Custom * 1.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:215:13 + | +LL | let _ = Custom * 2.0; + | ^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:218:10 | LL | _n = -_n; | ^^^ error: arithmetic operation that can potentially result in unexpected side-effects - --> $DIR/arithmetic_side_effects.rs:168:10 + --> $DIR/arithmetic_side_effects.rs:219:10 | LL | _n = -&_n; | ^^^^ -error: aborting due to 28 previous errors +error: aborting due to 52 previous errors From 99363fef657f399eca30b49df5f7b68d8b8a0ee8 Mon Sep 17 00:00:00 2001 From: Caio Date: Mon, 3 Oct 2022 18:36:12 -0300 Subject: [PATCH 2/2] Address comments --- .../src/operators/arithmetic_side_effects.rs | 24 +++++++++---------- tests/ui/arithmetic_side_effects.stderr | 24 ++++++++++++++++--- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/operators/arithmetic_side_effects.rs b/clippy_lints/src/operators/arithmetic_side_effects.rs index d871b72db..fc2a828bf 100644 --- a/clippy_lints/src/operators/arithmetic_side_effects.rs +++ b/clippy_lints/src/operators/arithmetic_side_effects.rs @@ -9,6 +9,7 @@ use rustc_ast as ast; use rustc_data_structures::fx::FxHashSet; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; use rustc_session::impl_lint_pass; use rustc_span::source_map::{Span, Spanned}; @@ -67,20 +68,14 @@ impl ArithmeticSideEffects { } /// 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( - cx.typeck_results() - .expr_ty(expr) - .to_string() - .split('<') - .next() - .unwrap_or_default(), - ) + fn is_allowed_ty(&self, ty: Ty<'_>) -> bool { + self.allowed + .contains(ty.to_string().split('<').next().unwrap_or_default()) } // For example, 8i32 or &i64::MAX. - fn is_integral<'expr, 'tcx>(cx: &LateContext<'tcx>, expr: &'expr hir::Expr<'tcx>) -> bool { - cx.typeck_results().expr_ty(expr).peel_refs().is_integral() + fn is_integral(ty: Ty<'_>) -> bool { + ty.peel_refs().is_integral() } // Common entry-point to avoid code duplication. @@ -129,10 +124,13 @@ impl ArithmeticSideEffects { ) { return; }; - if self.is_allowed_ty(cx, lhs) && self.is_allowed_ty(cx, rhs) { + let lhs_ty = cx.typeck_results().expr_ty(lhs); + let rhs_ty = cx.typeck_results().expr_ty(rhs); + let lhs_and_rhs_have_the_same_ty = lhs_ty == rhs_ty; + if lhs_and_rhs_have_the_same_ty && self.is_allowed_ty(lhs_ty) && self.is_allowed_ty(rhs_ty) { return; } - let has_valid_op = if Self::is_integral(cx, lhs) && Self::is_integral(cx, rhs) { + let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) { match (Self::literal_integer(lhs), Self::literal_integer(rhs)) { (None, None) => false, (None, Some(local_expr)) => Self::has_valid_op(op, local_expr), diff --git a/tests/ui/arithmetic_side_effects.stderr b/tests/ui/arithmetic_side_effects.stderr index 7870c942e..8cabd05c2 100644 --- a/tests/ui/arithmetic_side_effects.stderr +++ b/tests/ui/arithmetic_side_effects.stderr @@ -1,10 +1,28 @@ +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:78:13 + | +LL | let _ = String::new() + ""; + | ^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::arithmetic-side-effects` implied by `-D warnings` + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:86:27 + | +LL | let inferred_string = string + ""; + | ^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> $DIR/arithmetic_side_effects.rs:90:13 + | +LL | let _ = inferred_string + ""; + | ^^^^^^^^^^^^^^^^^^^^ + error: arithmetic operation that can potentially result in unexpected side-effects --> $DIR/arithmetic_side_effects.rs:162:5 | LL | _n += 1; | ^^^^^^^ - | - = note: `-D clippy::arithmetic-side-effects` implied by `-D warnings` error: arithmetic operation that can potentially result in unexpected side-effects --> $DIR/arithmetic_side_effects.rs:163:5 @@ -312,5 +330,5 @@ error: arithmetic operation that can potentially result in unexpected side-effec LL | _n = -&_n; | ^^^^ -error: aborting due to 52 previous errors +error: aborting due to 55 previous errors