diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index 790c2884273e..d285e71bd166 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -1,4 +1,5 @@ use rustc::hir; +use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc::lint::*; use syntax::ast; use utils::{get_trait_def_id, implements_trait, snippet_opt, span_lint_and_then, SpanlessEq}; @@ -87,19 +88,29 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps { }); if let hir::ExprBinary(binop, ref l, ref r) = rhs.node { if op.node == binop.node { - let lint = |assignee: &hir::Expr, rhs: &hir::Expr| { + let lint = |assignee: &hir::Expr, rhs_other: &hir::Expr| { span_lint_and_then( cx, MISREFACTORED_ASSIGN_OP, expr.span, "variable appears on both sides of an assignment operation", |db| if let (Some(snip_a), Some(snip_r)) = - (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span)) + (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) { + let a = &sugg::Sugg::hir(cx, assignee, ".."); + let r = &sugg::Sugg::hir(cx, rhs, ".."); + let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r)); db.span_suggestion( expr.span, - "replace it with", - format!("{} {}= {}", snip_a, op.node.as_str(), snip_r), + &format!("Did you mean {} = {} {} {} or {}? Consider replacing it with", + snip_a, snip_a, op.node.as_str(), snip_r, + long), + format!("{} {}= {}", snip_a, op.node.as_str(), snip_r) + ); + db.span_suggestion( + expr.span, + "or", + long ); }, ); @@ -189,23 +200,34 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps { ); } }; - // a = a op b - if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) { - lint(assignee, r); - } - // a = b commutative_op a - if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r) { - match op.node { - hir::BiAdd | - hir::BiMul | - hir::BiAnd | - hir::BiOr | - hir::BiBitXor | - hir::BiBitAnd | - hir::BiBitOr => { - lint(assignee, l); - }, - _ => {}, + + let mut visitor = ExprVisitor { + assignee: assignee, + counter: 0, + cx: cx + }; + + walk_expr(&mut visitor, e); + + if visitor.counter == 1 { + // a = a op b + if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) { + lint(assignee, r); + } + // a = b commutative_op a + if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r) { + match op.node { + hir::BiAdd | + hir::BiMul | + hir::BiAnd | + hir::BiOr | + hir::BiBitXor | + hir::BiBitAnd | + hir::BiBitOr => { + lint(assignee, l); + }, + _ => {}, + } } } } @@ -222,3 +244,22 @@ fn is_commutative(op: hir::BinOp_) -> bool { BiSub | BiDiv | BiRem | BiShl | BiShr | BiLt | BiLe | BiGe | BiGt => false, } } + +struct ExprVisitor<'a, 'tcx: 'a> { + assignee: &'a hir::Expr, + counter: u8, + cx: &'a LateContext<'a, 'tcx>, +} + +impl<'a, 'tcx: 'a> Visitor<'tcx> for ExprVisitor<'a, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx hir::Expr) { + if SpanlessEq::new(self.cx).ignore_fn().eq_expr(self.assignee, &expr) { + self.counter += 1; + } + + walk_expr(self, expr); + } + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} diff --git a/tests/ui/assign_ops2.rs b/tests/ui/assign_ops2.rs index 8d6ef827f52d..2d3adc2a661f 100644 --- a/tests/ui/assign_ops2.rs +++ b/tests/ui/assign_ops2.rs @@ -2,7 +2,7 @@ #[allow(unused_assignments)] -#[warn(misrefactored_assign_op)] +#[warn(misrefactored_assign_op, assign_op_pattern)] fn main() { let mut a = 5; a += a + 1; @@ -13,6 +13,10 @@ fn main() { a /= a / 2; a %= a % 5; a &= a & 1; + a *= a * a; + a = a * a * a; + a = a * 42 * a; + a = a * 2 + a; a -= 1 - a; a /= 5 / a; a %= 42 % a; diff --git a/tests/ui/assign_ops2.stderr b/tests/ui/assign_ops2.stderr index 0ff211259c03..2858af1f8c0b 100644 --- a/tests/ui/assign_ops2.stderr +++ b/tests/ui/assign_ops2.stderr @@ -2,51 +2,129 @@ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:8:5 | 8 | a += a + 1; - | ^^^^^^^^^^ help: replace it with: `a += 1` + | ^^^^^^^^^^ | = note: `-D misrefactored-assign-op` implied by `-D warnings` +help: Did you mean a = a + 1 or a = a + a + 1? Consider replacing it with + | +8 | a += 1; + | ^^^^^^ +help: or + | +8 | a = a + a + 1; + | ^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:9:5 | 9 | a += 1 + a; - | ^^^^^^^^^^ help: replace it with: `a += 1` + | ^^^^^^^^^^ +help: Did you mean a = a + 1 or a = a + 1 + a? Consider replacing it with + | +9 | a += 1; + | ^^^^^^ +help: or + | +9 | a = a + 1 + a; + | ^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:10:5 | 10 | a -= a - 1; - | ^^^^^^^^^^ help: replace it with: `a -= 1` + | ^^^^^^^^^^ +help: Did you mean a = a - 1 or a = a - (a - 1)? Consider replacing it with + | +10 | a -= 1; + | ^^^^^^ +help: or + | +10 | a = a - (a - 1); + | ^^^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:11:5 | 11 | a *= a * 99; - | ^^^^^^^^^^^ help: replace it with: `a *= 99` + | ^^^^^^^^^^^ +help: Did you mean a = a * 99 or a = a * a * 99? Consider replacing it with + | +11 | a *= 99; + | ^^^^^^^ +help: or + | +11 | a = a * a * 99; + | ^^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:12:5 | 12 | a *= 42 * a; - | ^^^^^^^^^^^ help: replace it with: `a *= 42` + | ^^^^^^^^^^^ +help: Did you mean a = a * 42 or a = a * 42 * a? Consider replacing it with + | +12 | a *= 42; + | ^^^^^^^ +help: or + | +12 | a = a * 42 * a; + | ^^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:13:5 | 13 | a /= a / 2; - | ^^^^^^^^^^ help: replace it with: `a /= 2` + | ^^^^^^^^^^ +help: Did you mean a = a / 2 or a = a / (a / 2)? Consider replacing it with + | +13 | a /= 2; + | ^^^^^^ +help: or + | +13 | a = a / (a / 2); + | ^^^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:14:5 | 14 | a %= a % 5; - | ^^^^^^^^^^ help: replace it with: `a %= 5` + | ^^^^^^^^^^ +help: Did you mean a = a % 5 or a = a % (a % 5)? Consider replacing it with + | +14 | a %= 5; + | ^^^^^^ +help: or + | +14 | a = a % (a % 5); + | ^^^^^^^^^^^^^^^ error: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:15:5 | 15 | a &= a & 1; - | ^^^^^^^^^^ help: replace it with: `a &= 1` + | ^^^^^^^^^^ +help: Did you mean a = a & 1 or a = a & a & 1? Consider replacing it with + | +15 | a &= 1; + | ^^^^^^ +help: or + | +15 | a = a & a & 1; + | ^^^^^^^^^^^^^ -error: aborting due to 8 previous errors +error: variable appears on both sides of an assignment operation + --> $DIR/assign_ops2.rs:16:5 + | +16 | a *= a * a; + | ^^^^^^^^^^ +help: Did you mean a = a * a or a = a * a * a? Consider replacing it with + | +16 | a *= a; + | ^^^^^^ +help: or + | +16 | a = a * a * a; + | ^^^^^^^^^^^^^ + +error: aborting due to 9 previous errors