Auto merge of #12692 - c410-f3r:arrrr, r=Manishearth

[arithmetic_side_effects] Fix #12318

Fix #12318

changelog: [arithmetic_side_effects]: Consider method calls that correspond to arithmetic symbols

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_data_structures::fx::{FxHashMap, FxHashSet};
 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<String>,
     // Used to check whether expressions are constants, such as in enum discriminants and consts
     const_span: Option<Span>,
+    disallowed_int_methods: FxHashSet<Symbol>,
     expr_span: Option<Span>,
-    integer_methods: FxHashSet<Symbol>,
 }
 
 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 @@ impl ArithmeticSideEffects {
     fn has_specific_allowed_type_and_operation<'tcx>(
         cx: &LateContext<'tcx>,
         lhs_ty: Ty<'tcx>,
-        op: &Spanned<hir::BinOpKind>,
+        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 @@ impl ArithmeticSideEffects {
         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<hir::BinOpKind>,
+        op: hir::BinOpKind,
         lhs: &'tcx hir::Expr<'_>,
         rhs: &'tcx hir::Expr<'_>,
     ) {
@@ -180,7 +201,7 @@ impl ArithmeticSideEffects {
             return;
         }
         if !matches!(
-            op.node,
+            op,
             hir::BinOpKind::Add
                 | hir::BinOpKind::Div
                 | hir::BinOpKind::Mul
@@ -204,7 +225,7 @@ impl ArithmeticSideEffects {
             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 @@ impl ArithmeticSideEffects {
                 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 @@ impl ArithmeticSideEffects {
                     => 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 @@ impl ArithmeticSideEffects {
         &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 @@ impl ArithmeticSideEffects {
         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 @@ impl<'tcx> LateLintPass<'tcx> for ArithmeticSideEffects {
         }
         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 @@ pub fn issue_11393() {
     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