Fix op_ref false positives

diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs
index 764bcded5ede10baa73ab7490fa8e76f7e42109c..51a11dd9dde65b90d0ddb3a7176783b37ebfce0c 100644 (file)
--- a/clippy_lints/src/eq_op.rs
+++ b/clippy_lints/src/eq_op.rs
@@ -1,7 +1,6 @@
 use rustc::hir::*;
 use rustc::lint::*;
-use utils::{SpanlessEq, span_lint, span_lint_and_then, multispan_sugg, snippet, implements_trait};
-use utils::sugg::Sugg;
+use utils::{SpanlessEq, span_lint, span_lint_and_then, multispan_sugg, snippet, implements_trait, is_copy};
 
 /// **What it does:** Checks for equal operands to comparison, logical and
 /// bitwise, difference and division binary operators (`==`, `>`, etc., `&&`,
@@ -59,83 +58,95 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) {
                               EQ_OP,
                               e.span,
                               &format!("equal expressions as operands to `{}`", op.node.as_str()));
-                } else {
-                    let trait_id = match op.node {
-                        BiAdd => cx.tcx.lang_items.add_trait(),
-                        BiSub => cx.tcx.lang_items.sub_trait(),
-                        BiMul => cx.tcx.lang_items.mul_trait(),
-                        BiDiv => cx.tcx.lang_items.div_trait(),
-                        BiRem => cx.tcx.lang_items.rem_trait(),
-                        BiAnd | BiOr => None,
-                        BiBitXor => cx.tcx.lang_items.bitxor_trait(),
-                        BiBitAnd => cx.tcx.lang_items.bitand_trait(),
-                        BiBitOr => cx.tcx.lang_items.bitor_trait(),
-                        BiShl => cx.tcx.lang_items.shl_trait(),
-                        BiShr => cx.tcx.lang_items.shr_trait(),
-                        BiNe | BiEq => cx.tcx.lang_items.eq_trait(),
-                        BiLt | BiLe | BiGe | BiGt => cx.tcx.lang_items.ord_trait(),
-                    };
-                    if let Some(trait_id) = trait_id {
-                        #[allow(match_same_arms)]
-                        match (&left.node, &right.node) {
-                            // do not suggest to dereference literals
-                            (&ExprLit(..), _) |
-                            (_, &ExprLit(..)) => {},
-                            // &foo == &bar
-                            (&ExprAddrOf(_, ref l), &ExprAddrOf(_, ref r)) => {
-                                if implements_trait(cx, cx.tables.expr_ty(l), trait_id, &[cx.tables.expr_ty(r)], None) {
-                                    span_lint_and_then(cx,
-                                                       OP_REF,
-                                                       e.span,
-                                                       "taken reference of both operands, which is done automatically \
-                                                        by the operator anyway",
-                                                       |db| {
-                                        let lsnip = snippet(cx, l.span, "...").to_string();
-                                        let rsnip = snippet(cx, r.span, "...").to_string();
-                                        multispan_sugg(db,
-                                                       "use the values directly".to_string(),
-                                                       vec![(left.span, lsnip),
-                                                            (right.span, rsnip)]);
-                                    })
-                                }
-                            },
-                            // &foo == bar
-                            (&ExprAddrOf(_, ref l), _) => {
-                                if implements_trait(cx,
-                                                    cx.tables.expr_ty(l),
-                                                    trait_id,
-                                                    &[cx.tables.expr_ty(right)],
-                                                    None) {
-                                    span_lint_and_then(cx, OP_REF, e.span, "taken reference of left operand", |db| {
-                                        let lsnip = snippet(cx, l.span, "...").to_string();
-                                        let rsnip = Sugg::hir(cx, right, "...").deref().to_string();
-                                        multispan_sugg(db,
-                                                       "dereference the right operand instead".to_string(),
-                                                       vec![(left.span, lsnip),
-                                                            (right.span, rsnip)]);
-                                    })
-                                }
-                            },
-                            // foo == &bar
-                            (_, &ExprAddrOf(_, ref r)) => {
-                                if implements_trait(cx,
-                                                    cx.tables.expr_ty(left),
-                                                    trait_id,
-                                                    &[cx.tables.expr_ty(r)],
-                                                    None) {
-                                    span_lint_and_then(cx, OP_REF, e.span, "taken reference of right operand", |db| {
-                                        let lsnip = Sugg::hir(cx, left, "...").deref().to_string();
-                                        let rsnip = snippet(cx, r.span, "...").to_string();
-                                        multispan_sugg(db,
-                                                       "dereference the left operand instead".to_string(),
-                                                       vec![(left.span, lsnip),
-                                                            (right.span, rsnip)]);
-                                    })
-                                }
-                            },
-                            _ => {},
+                    return;
+                }
+            }
+            let (trait_id, requires_ref) = match op.node {
+                BiAdd => (cx.tcx.lang_items.add_trait(), false),
+                BiSub => (cx.tcx.lang_items.sub_trait(), false),
+                BiMul => (cx.tcx.lang_items.mul_trait(), false),
+                BiDiv => (cx.tcx.lang_items.div_trait(), false),
+                BiRem => (cx.tcx.lang_items.rem_trait(), false),
+                // don't lint short circuiting ops
+                BiAnd | BiOr => return,
+                BiBitXor => (cx.tcx.lang_items.bitxor_trait(), false),
+                BiBitAnd => (cx.tcx.lang_items.bitand_trait(), false),
+                BiBitOr => (cx.tcx.lang_items.bitor_trait(), false),
+                BiShl => (cx.tcx.lang_items.shl_trait(), false),
+                BiShr => (cx.tcx.lang_items.shr_trait(), false),
+                BiNe | BiEq => (cx.tcx.lang_items.eq_trait(), true),
+                BiLt | BiLe | BiGe | BiGt => (cx.tcx.lang_items.ord_trait(), true),
+            };
+            let parent = cx.tcx.hir.get_parent(e.id);
+            if let Some(trait_id) = trait_id {
+                #[allow(match_same_arms)]
+                match (&left.node, &right.node) {
+                    // do not suggest to dereference literals
+                    (&ExprLit(..), _) |
+                    (_, &ExprLit(..)) => {},
+                    // &foo == &bar
+                    (&ExprAddrOf(_, ref l), &ExprAddrOf(_, ref r)) => {
+                        let lty = cx.tables.expr_ty(l);
+                        let rty = cx.tables.expr_ty(r);
+                        let lcpy = is_copy(cx, lty, parent);
+                        let rcpy = is_copy(cx, rty, parent);
+                        // either operator autorefs or both args are copyable
+                        if (requires_ref || (lcpy && rcpy)) && implements_trait(cx, lty, trait_id, &[rty], None) {
+                            span_lint_and_then(cx,
+                                                OP_REF,
+                                                e.span,
+                                                "needlessly taken reference of both operands",
+                                                |db| {
+                                let lsnip = snippet(cx, l.span, "...").to_string();
+                                let rsnip = snippet(cx, r.span, "...").to_string();
+                                multispan_sugg(db,
+                                                "use the values directly".to_string(),
+                                                vec![(left.span, lsnip),
+                                                    (right.span, rsnip)]);
+                            })
+                        } else if lcpy && !rcpy && implements_trait(cx, lty, trait_id, &[cx.tables.expr_ty(right)], None) {
+                            span_lint_and_then(cx,
+                                                OP_REF,
+                                                e.span,
+                                                "needlessly taken reference of left operand",
+                                                |db| {
+                                let lsnip = snippet(cx, l.span, "...").to_string();
+                                db.span_suggestion(left.span, "use the left value directly", lsnip);
+                            })
+                        } else if !lcpy && rcpy && implements_trait(cx, cx.tables.expr_ty(left), trait_id, &[rty], None) {
+                            span_lint_and_then(cx,
+                                                OP_REF,
+                                                e.span,
+                                                "needlessly taken reference of right operand",
+                                                |db| {
+                                let rsnip = snippet(cx, r.span, "...").to_string();
+                                db.span_suggestion(right.span, "use the right value directly", rsnip);
+                            })
+                        }
+                    },
+                    // &foo == bar
+                    (&ExprAddrOf(_, ref l), _) => {
+                        let lty = cx.tables.expr_ty(l);
+                        let lcpy = is_copy(cx, lty, parent);
+                        if (requires_ref || lcpy) && implements_trait(cx, lty, trait_id, &[cx.tables.expr_ty(right)], None) {
+                            span_lint_and_then(cx, OP_REF, e.span, "needlessly taken reference of left operand", |db| {
+                                let lsnip = snippet(cx, l.span, "...").to_string();
+                                db.span_suggestion(left.span, "use the left value directly", lsnip);
+                            })
+                        }
+                    },
+                    // foo == &bar
+                    (_, &ExprAddrOf(_, ref r)) => {
+                        let rty = cx.tables.expr_ty(r);
+                        let rcpy = is_copy(cx, rty, parent);
+                        if (requires_ref || rcpy) && implements_trait(cx, cx.tables.expr_ty(left), trait_id, &[rty], None) {
+                            span_lint_and_then(cx, OP_REF, e.span, "taken reference of right operand", |db| {
+                                let rsnip = snippet(cx, r.span, "...").to_string();
+                                db.span_suggestion(left.span, "use the right value directly", rsnip);
+                            })
                         }
-                    }
+                    },
+                    _ => {},
                 }
             }
         }

diff --git a/tests/ui/op_ref.stderr b/tests/ui/op_ref.stderr
index 6e6ca457e03ec54cd7b1a5a75bde6fd20dbd9188..5448429f16471bab7633db5a3aec63cd71ed4628 100644 (file)
--- a/tests/ui/op_ref.stderr
+++ b/tests/ui/op_ref.stderr
@@ -1,4 +1,4 @@
-warning: taken reference of both operands, which is done automatically by the operator anyway
+warning: needlessly taken reference of both operands
   --> $DIR/op_ref.rs:13:15
    |
 13 |     let foo = &5 - &6;