Auto merge of #83213 - rylev:update-lints-to-errors, r=nikomatsakis

[rust.git] / clippy_lints / src / suspicious_trait_impl.rs

diff --git a/clippy_lints/src/suspicious_trait_impl.rs b/clippy_lints/src/suspicious_trait_impl.rs
index e39a7b5e2ab6023275c72e18dc30d3753422640c..512abde11a63433a827f797f95002b6489ac6a20 100644 (file)
--- a/clippy_lints/src/suspicious_trait_impl.rs
+++ b/clippy_lints/src/suspicious_trait_impl.rs
@@ -1,9 +1,10 @@
-use crate::utils::{get_trait_def_id, span_lint, trait_ref_of_method};
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::{get_trait_def_id, paths, trait_ref_of_method};
 use if_chain::if_chain;
-use rustc::hir::map::Map;
 use rustc_hir as hir;
 use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
 use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::hir::map::Map;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
@@ -52,9 +53,9 @@
 
 declare_lint_pass!(SuspiciousImpl => [SUSPICIOUS_ARITHMETIC_IMPL, SUSPICIOUS_OP_ASSIGN_IMPL]);
 
-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 {
+impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
+        if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind {
             match binop.node {
                 hir::BinOpKind::Eq
                 | hir::BinOpKind::Lt
@@ -64,45 +65,48 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>)
                 | hir::BinOpKind::Gt => return,
                 _ => {},
             }
-            // Check if the binary expression is part of another bi/unary expression
-            // 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,
-                        _ => {},
+
+            // Check for more than one binary operation in the implemented function
+            // Linting when multiple operations are involved can result in false positives
+            let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
+            if_chain! {
+                if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn);
+                if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
+                then {
+                    let body = cx.tcx.hir().body(body_id);
+                    let mut visitor = BinaryExprVisitor { nb_binops: 0 };
+                    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(
                 cx,
                 expr,
                 binop.node,
-                &["Add", "Sub", "Mul", "Div"],
+                &[
+                    "Add", "Sub", "Mul", "Div", "Rem", "BitAnd", "BitOr", "BitXor", "Shl", "Shr",
+                ],
                 &[
                     hir::BinOpKind::Add,
                     hir::BinOpKind::Sub,
                     hir::BinOpKind::Mul,
                     hir::BinOpKind::Div,
+                    hir::BinOpKind::Rem,
+                    hir::BinOpKind::BitAnd,
+                    hir::BinOpKind::BitOr,
+                    hir::BinOpKind::BitXor,
+                    hir::BinOpKind::Shl,
+                    hir::BinOpKind::Shr,
                 ],
             ) {
                 span_lint(
                     cx,
                     SUSPICIOUS_ARITHMETIC_IMPL,
                     binop.span,
-                    &format!(r#"Suspicious use of binary operator in `{}` impl"#, impl_trait),
+                    &format!("suspicious use of binary operator in `{}` impl", impl_trait),
                 );
             }
 
@@ -139,7 +143,7 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>)
                     cx,
                     SUSPICIOUS_OP_ASSIGN_IMPL,
                     binop.span,
-                    &format!(r#"Suspicious use of binary operator in `{}` impl"#, impl_trait),
+                    &format!("suspicious use of binary operator in `{}` impl", impl_trait),
                 );
             }
         }
@@ -147,14 +151,14 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>)
 }
 
 fn check_binop(
-    cx: &LateContext<'_, '_>,
+    cx: &LateContext<'_>,
     expr: &hir::Expr<'_>,
     binop: hir::BinOpKind,
     traits: &[&'static str],
     expected_ops: &[hir::BinOpKind],
 ) -> Option<&'static str> {
     let mut trait_ids = vec![];
-    let [krate, module] = crate::utils::paths::OPS_MODULE;
+    let [krate, module] = paths::OPS_MODULE;
 
     for &t in traits {
         let path = [krate, module, t];
@@ -181,22 +185,23 @@ fn check_binop(
 }
 
 struct BinaryExprVisitor {
-    in_binary_expr: bool,
+    nb_binops: u32,
 }
 
-impl<'a, 'tcx> Visitor<'tcx> for BinaryExprVisitor {
+impl<'tcx> Visitor<'tcx> for BinaryExprVisitor {
     type Map = Map<'tcx>;
 
     fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
         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::Not | hir::UnOp::Neg, _)
+            | hir::ExprKind::AssignOp(..) => self.nb_binops += 1,
             _ => {},
         }
 
         walk_expr(self, expr);
     }
+
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
         NestedVisitorMap::None
     }