Auto merge of rust-lang#5820 - ThibsG:FixSuspiciousArithmeticImpl, r=…

…flip1995

Fix FP for `suspicious_arithmetic_impl` from `suspicious_trait_impl` …

As discussed in rust-lang#3215, the `suspicious_trait_impl` lint causes too many false positives, as it is complex to find out if binary operations are suspicious or not.

This PR restricts the number of binary operations to at most one, otherwise we don't lint.
This can be seen as very conservative, but at least FP can be reduced to bare minimum.

Fixes: rust-lang#3215

changelog: limit the `suspicious_arithmetic_impl` lint to one binop, to avoid many FPs

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
     }

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)
+    }
+}