From 7aa42073f220895e054987babe6f07148dad92ce Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 28 Apr 2017 17:03:47 +0200 Subject: [PATCH] Fix op_ref false positives --- clippy_lints/src/eq_op.rs | 167 ++++++++++++++++++++------------------ tests/ui/op_ref.stderr | 2 +- 2 files changed, 90 insertions(+), 79 deletions(-) diff --git a/clippy_lints/src/eq_op.rs b/clippy_lints/src/eq_op.rs index 764bcded5ed..51a11dd9dde 100644 --- 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 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for EqOp { 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 6e6ca457e03..5448429f164 100644 --- 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;