From 2a4dae368c109d0029e12787a78b6c028b61ca08 Mon Sep 17 00:00:00 2001 From: Caio Date: Thu, 18 Apr 2024 17:24:47 -0300 Subject: [PATCH] [arithmetic_side_effects] Fix #12318 --- .../src/operators/arithmetic_side_effects.rs | 53 +++++++++++++------ tests/ui/arithmetic_side_effects.rs | 10 ++++ tests/ui/arithmetic_side_effects.stderr | 14 ++++- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/clippy_lints/src/operators/arithmetic_side_effects.rs b/clippy_lints/src/operators/arithmetic_side_effects.rs index 96ea063aa74..7d6f26cde3e 100644 --- a/clippy_lints/src/operators/arithmetic_side_effects.rs +++ b/clippy_lints/src/operators/arithmetic_side_effects.rs @@ -7,14 +7,13 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, Ty}; use rustc_session::impl_lint_pass; -use rustc_span::source_map::Spanned; use rustc_span::symbol::sym; use rustc_span::{Span, Symbol}; use {rustc_ast as ast, rustc_hir as hir}; const HARD_CODED_ALLOWED_BINARY: &[[&str; 2]] = &[["f32", "f32"], ["f64", "f64"], ["std::string::String", "str"]]; const HARD_CODED_ALLOWED_UNARY: &[&str] = &["f32", "f64", "std::num::Saturating", "std::num::Wrapping"]; -const INTEGER_METHODS: &[Symbol] = &[ +const DISALLOWED_INT_METHODS: &[Symbol] = &[ sym::saturating_div, sym::wrapping_div, sym::wrapping_rem, @@ -27,8 +26,8 @@ pub struct ArithmeticSideEffects { allowed_unary: FxHashSet, // Used to check whether expressions are constants, such as in enum discriminants and consts const_span: Option, + disallowed_int_methods: FxHashSet, expr_span: Option, - integer_methods: FxHashSet, } impl_lint_pass!(ArithmeticSideEffects => [ARITHMETIC_SIDE_EFFECTS]); @@ -53,8 +52,8 @@ impl ArithmeticSideEffects { allowed_binary, allowed_unary, const_span: None, + disallowed_int_methods: DISALLOWED_INT_METHODS.iter().copied().collect(), expr_span: None, - integer_methods: INTEGER_METHODS.iter().copied().collect(), } } @@ -91,10 +90,10 @@ fn has_allowed_unary(&self, ty: Ty<'_>) -> bool { fn has_specific_allowed_type_and_operation<'tcx>( cx: &LateContext<'tcx>, lhs_ty: Ty<'tcx>, - op: &Spanned, + op: hir::BinOpKind, rhs_ty: Ty<'tcx>, ) -> bool { - let is_div_or_rem = matches!(op.node, hir::BinOpKind::Div | hir::BinOpKind::Rem); + let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem); let is_non_zero_u = |cx: &LateContext<'tcx>, ty: Ty<'tcx>| { let tcx = cx.tcx; @@ -166,13 +165,35 @@ fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option { None } + /// Methods like `add_assign` are send to their `BinOps` references. + fn manage_sugar_methods<'tcx>( + &mut self, + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + lhs: &'tcx hir::Expr<'_>, + ps: &hir::PathSegment<'_>, + rhs: &'tcx hir::Expr<'_>, + ) { + if ps.ident.name == sym::add || ps.ident.name == sym::add_assign { + self.manage_bin_ops(cx, expr, hir::BinOpKind::Add, lhs, rhs); + } else if ps.ident.name == sym::div || ps.ident.name == sym::div_assign { + self.manage_bin_ops(cx, expr, hir::BinOpKind::Div, lhs, rhs); + } else if ps.ident.name == sym::mul || ps.ident.name == sym::mul_assign { + self.manage_bin_ops(cx, expr, hir::BinOpKind::Mul, lhs, rhs); + } else if ps.ident.name == sym::rem || ps.ident.name == sym::rem_assign { + self.manage_bin_ops(cx, expr, hir::BinOpKind::Rem, lhs, rhs); + } else if ps.ident.name == sym::sub || ps.ident.name == sym::sub_assign { + self.manage_bin_ops(cx, expr, hir::BinOpKind::Sub, lhs, rhs); + } + } + /// Manages when the lint should be triggered. Operations in constant environments, hard coded - /// types, custom allowed types and non-constant operations that won't overflow are ignored. + /// types, custom allowed types and non-constant operations that don't overflow are ignored. fn manage_bin_ops<'tcx>( &mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>, - op: &Spanned, + op: hir::BinOpKind, lhs: &'tcx hir::Expr<'_>, rhs: &'tcx hir::Expr<'_>, ) { @@ -180,7 +201,7 @@ fn manage_bin_ops<'tcx>( return; } if !matches!( - op.node, + op, hir::BinOpKind::Add | hir::BinOpKind::Div | hir::BinOpKind::Mul @@ -204,7 +225,7 @@ fn manage_bin_ops<'tcx>( return; } let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) { - if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op.node { + if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op { // At least for integers, shifts are already handled by the CTFE return; } @@ -213,7 +234,7 @@ fn manage_bin_ops<'tcx>( Self::literal_integer(cx, actual_rhs), ) { (None, None) => false, - (None, Some(n)) => match (&op.node, n) { + (None, Some(n)) => match (&op, n) { // Division and module are always valid if applied to non-zero integers (hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => true, // Adding or subtracting zeros is always a no-op @@ -223,7 +244,7 @@ fn manage_bin_ops<'tcx>( => true, _ => false, }, - (Some(n), None) => match (&op.node, n) { + (Some(n), None) => match (&op, n) { // Adding or subtracting zeros is always a no-op (hir::BinOpKind::Add | hir::BinOpKind::Sub, 0) // Multiplication by 1 or 0 will never overflow @@ -249,6 +270,7 @@ fn manage_method_call<'tcx>( &mut self, args: &'tcx [hir::Expr<'_>], cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, ps: &'tcx hir::PathSegment<'_>, receiver: &'tcx hir::Expr<'_>, ) { @@ -262,7 +284,8 @@ fn manage_method_call<'tcx>( if !Self::is_integral(instance_ty) { return; } - if !self.integer_methods.contains(&ps.ident.name) { + self.manage_sugar_methods(cx, expr, receiver, ps, arg); + if !self.disallowed_int_methods.contains(&ps.ident.name) { return; } let (actual_arg, _) = peel_hir_expr_refs(arg); @@ -310,10 +333,10 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) { } match &expr.kind { hir::ExprKind::AssignOp(op, lhs, rhs) | hir::ExprKind::Binary(op, lhs, rhs) => { - self.manage_bin_ops(cx, expr, op, lhs, rhs); + self.manage_bin_ops(cx, expr, op.node, lhs, rhs); }, hir::ExprKind::MethodCall(ps, receiver, args, _) => { - self.manage_method_call(args, cx, ps, receiver); + self.manage_method_call(args, cx, expr, ps, receiver); }, hir::ExprKind::Unary(un_op, un_expr) => { self.manage_unary_ops(cx, expr, un_expr, *un_op); diff --git a/tests/ui/arithmetic_side_effects.rs b/tests/ui/arithmetic_side_effects.rs index b454c29aef4..e6951826b2f 100644 --- a/tests/ui/arithmetic_side_effects.rs +++ b/tests/ui/arithmetic_side_effects.rs @@ -521,4 +521,14 @@ fn example_rem(x: Wrapping, maybe_zero: Wrapping) -> Wrapping { example_rem(x, maybe_zero); } +pub fn issue_12318() { + use core::ops::{AddAssign, DivAssign, MulAssign, RemAssign, SubAssign}; + let mut one: i32 = 1; + one.add_assign(1); + one.div_assign(1); + one.mul_assign(1); + one.rem_assign(1); + one.sub_assign(1); +} + fn main() {} diff --git a/tests/ui/arithmetic_side_effects.stderr b/tests/ui/arithmetic_side_effects.stderr index 741c892a52c..8039c0bfa24 100644 --- a/tests/ui/arithmetic_side_effects.stderr +++ b/tests/ui/arithmetic_side_effects.stderr @@ -715,5 +715,17 @@ error: arithmetic operation that can potentially result in unexpected side-effec LL | x % maybe_zero | ^^^^^^^^^^^^^^ -error: aborting due to 119 previous errors +error: arithmetic operation that can potentially result in unexpected side-effects + --> tests/ui/arithmetic_side_effects.rs:527:5 + | +LL | one.add_assign(1); + | ^^^^^^^^^^^^^^^^^ + +error: arithmetic operation that can potentially result in unexpected side-effects + --> tests/ui/arithmetic_side_effects.rs:531:5 + | +LL | one.sub_assign(1); + | ^^^^^^^^^^^^^^^^^ + +error: aborting due to 121 previous errors