Rollup merge of #5424 - jpospychala:suspicious_op_assign_impl, r=flip…

…1995 Incorrect suspicious_op_assign_impl fixes #5255 changelog: In suspicious_op_assign_impl ignore all operators in expression if it's part of AssignOp
rust-lang · Apr 8, 2020 · 8fc592a · 8fc592a
2 parents b42bd19 + 9c9af1d
commit 8fc592a
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 6 deletions.
diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs
@@ -54,7 +54,7 @@ declare_lint_pass!(SuspiciousImpl => [SUSPICIOUS_ARITHMETIC_IMPL, SUSPICIOUS_OP_
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for SuspiciousImpl {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>) {
-        if let hir::ExprKind::Binary(binop, _, _) = expr.kind {
+        if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind {
             match binop.node {
                 hir::BinOpKind::Eq
                 | hir::BinOpKind::Lt
@@ -65,14 +65,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for SuspiciousImpl {
                 _ => {},
             }
             // Check if the binary expression is part of another bi/unary expression
-            // as a child node
+            // or operator assignment as a child node
             let mut parent_expr = cx.tcx.hir().get_parent_node(expr.hir_id);
             while parent_expr != hir::CRATE_HIR_ID {
                 if let hir::Node::Expr(e) = cx.tcx.hir().get(parent_expr) {
                     match e.kind {
                         hir::ExprKind::Binary(..)
                         | hir::ExprKind::Unary(hir::UnOp::UnNot, _)
-                        | hir::ExprKind::Unary(hir::UnOp::UnNeg, _) => return,
+                        | hir::ExprKind::Unary(hir::UnOp::UnNeg, _)
+                        | hir::ExprKind::AssignOp(..) => return,
                         _ => {},
                     }
                 }
@@ -191,7 +192,8 @@ impl<'a, 'tcx> Visitor<'tcx> for BinaryExprVisitor {
         match expr.kind {
             hir::ExprKind::Binary(..)
             | hir::ExprKind::Unary(hir::UnOp::UnNot, _)
-            | hir::ExprKind::Unary(hir::UnOp::UnNeg, _) => self.in_binary_expr = true,
+            | hir::ExprKind::Unary(hir::UnOp::UnNeg, _)
+            | hir::ExprKind::AssignOp(..) => self.in_binary_expr = true,
             _ => {},
         }
 

diff --git a/tests/ui/suspicious_arithmetic_impl.rs b/tests/ui/suspicious_arithmetic_impl.rs
@@ -1,5 +1,5 @@
 #![warn(clippy::suspicious_arithmetic_impl)]
-use std::ops::{Add, AddAssign, Div, Mul, Sub};
+use std::ops::{Add, AddAssign, BitOrAssign, Div, DivAssign, Mul, MulAssign, Sub};
 
 #[derive(Copy, Clone)]
 struct Foo(u32);
@@ -18,6 +18,25 @@ impl AddAssign for Foo {
     }
 }
 
+impl BitOrAssign for Foo {
+    fn bitor_assign(&mut self, other: Foo) {
+        let idx = other.0;
+        self.0 |= 1 << idx; // OK: BinOpKind::Shl part of AssignOp as child node
+    }
+}
+
+impl MulAssign for Foo {
+    fn mul_assign(&mut self, other: Foo) {
+        self.0 /= other.0;
+    }
+}
+
+impl DivAssign for Foo {
+    fn div_assign(&mut self, other: Foo) {
+        self.0 /= other.0; // OK: BinOpKind::Div == DivAssign
+    }
+}
+
 impl Mul for Foo {
     type Output = Foo;
 

diff --git a/tests/ui/suspicious_arithmetic_impl.stderr b/tests/ui/suspicious_arithmetic_impl.stderr
@@ -14,5 +14,11 @@ LL |         *self = *self - other;
    |
    = note: `#[deny(clippy::suspicious_op_assign_impl)]` on by default
 
-error: aborting due to 2 previous errors
+error: Suspicious use of binary operator in `MulAssign` impl
+  --> $DIR/suspicious_arithmetic_impl.rs:30:16
+   |
+LL |         self.0 /= other.0;
+   |                ^^
+
+error: aborting due to 3 previous errors