Fix FP for suspicious_arithmetic_impl from suspicious_trait_impl …

…lint
rust-lang · Jul 18, 2020 · 442c8ae · 442c8ae
1 parent ac85692
commit 442c8ae
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 20 deletions.
diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs
@@ -64,26 +64,22 @@ impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl {
                 | hir::BinOpKind::Gt => return,
                 _ => {},
             }
-            // Check if the binary expression is part of another bi/unary expression
-            // 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::UnOp::UnNeg, _)
-                        | hir::ExprKind::AssignOp(..) => return,
-                        _ => {},
+
+            // Check for more than one binary operation in the implemented function
+            // Linting when multiple operations are involved can result in false positives
+            if_chain! {
+                let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
+                if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn);
+                if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
+                let body = cx.tcx.hir().body(body_id);
+                let mut visitor = BinaryExprVisitor { nb_binops: 0 };
+
+                then {
+                    walk_expr(&mut visitor, &body.value);
+                    if visitor.nb_binops > 1 {
+                        return;
                     }
                 }
-                parent_expr = cx.tcx.hir().get_parent_node(parent_expr);
-            }
-            // as a parent node
-            let mut visitor = BinaryExprVisitor { in_binary_expr: false };
-            walk_expr(&mut visitor, expr);
-
-            if visitor.in_binary_expr {
-                return;
             }
 
             if let Some(impl_trait) = check_binop(
@@ -181,7 +177,7 @@ fn check_binop(
 }
 
 struct BinaryExprVisitor {
-    in_binary_expr: bool,
+    nb_binops: u32,
 }
 
 impl<'tcx> Visitor<'tcx> for BinaryExprVisitor {
@@ -191,12 +187,13 @@ impl<'tcx> Visitor<'tcx> for BinaryExprVisitor {
         match expr.kind {
             hir::ExprKind::Binary(..)
             | hir::ExprKind::Unary(hir::UnOp::UnNot | hir::UnOp::UnNeg, _)
-            | hir::ExprKind::AssignOp(..) => self.in_binary_expr = true,
+            | hir::ExprKind::AssignOp(..) => self.nb_binops += 1,
             _ => {},
         }
 
         walk_expr(self, expr);
     }
+
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
         NestedVisitorMap::None
     }

diff --git a/tests/ui/suspicious_arithmetic_impl.rs b/tests/ui/suspicious_arithmetic_impl.rs
@@ -88,3 +88,33 @@ fn main() {}
 fn do_nothing(x: u32) -> u32 {
     x
 }
+
+struct MultipleBinops(u32);
+
+impl Add for MultipleBinops {
+    type Output = MultipleBinops;
+
+    // OK: multiple Binops in `add` impl
+    fn add(self, other: Self) -> Self::Output {
+        let mut result = self.0 + other.0;
+        if result >= u32::max_value() {
+            result -= u32::max_value();
+        }
+        MultipleBinops(result)
+    }
+}
+
+impl Mul for MultipleBinops {
+    type Output = MultipleBinops;
+
+    // OK: multiple Binops in `mul` impl
+    fn mul(self, other: Self) -> Self::Output {
+        let mut result: u32 = 0;
+        let size = std::cmp::max(self.0, other.0) as usize;
+        let mut v = vec![0; size + 1];
+        for i in 0..size + 1 {
+            result *= i as u32;
+        }
+        MultipleBinops(result)
+    }
+}