From b7cb0752ff5ba28b4aa8e4bb8befc6b8977513d4 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 30 Jan 2018 14:58:38 +0100 Subject: [PATCH 1/2] Improved suggestion on misrefactored_assign_op lint. Fixes #1239 --- clippy_lints/src/assign_ops.rs | 12 ++++--- tests/ui/assign_ops2.rs | 1 + tests/ui/assign_ops2.stderr | 60 +++++++++++++++++++++++++++++----- 3 files changed, 60 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index 790c2884273e..fe6949566e4b 100644 --- a/clippy_lints/src/assign_ops.rs +++ b/clippy_lints/src/assign_ops.rs @@ -87,19 +87,23 @@ 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, ".."); 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, + snip_a, sugg::make_binop(higher::binop(op.node), a, r)), + format!("{} {}= {}", snip_a, op.node.as_str(), snip_r) ); }, ); diff --git a/tests/ui/assign_ops2.rs b/tests/ui/assign_ops2.rs index 8d6ef827f52d..821f6a1a9bf3 100644 --- a/tests/ui/assign_ops2.rs +++ b/tests/ui/assign_ops2.rs @@ -13,6 +13,7 @@ fn main() { a /= a / 2; a %= a % 5; a &= a & 1; + a *= a * 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..992bb4079eab 100644 --- a/tests/ui/assign_ops2.stderr +++ b/tests/ui/assign_ops2.stderr @@ -2,51 +2,93 @@ 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; + | ^^^^^^ 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; + | ^^^^^^ 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; + | ^^^^^^ 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; + | ^^^^^^^ 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; + | ^^^^^^^ 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; + | ^^^^^^ 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; + | ^^^^^^ 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; + | ^^^^^^ -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; + | ^^^^^^ + +error: aborting due to 9 previous errors From bd421cb5a5937ce42ddddd392ec743c8154f5d56 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 30 Jan 2018 17:45:35 +0100 Subject: [PATCH 2/2] Additionally suggest the semantic equal variant --- clippy_lints/src/assign_ops.rs | 75 +++++++++++++++++++++++++--------- tests/ui/assign_ops2.rs | 5 ++- tests/ui/assign_ops2.stderr | 36 ++++++++++++++++ 3 files changed, 96 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs index fe6949566e4b..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}; @@ -98,13 +99,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps { { 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, - &format!("Did you mean {} = {} {} {} or {} = {}? Consider replacing it with", + &format!("Did you mean {} = {} {} {} or {}? Consider replacing it with", snip_a, snip_a, op.node.as_str(), snip_r, - snip_a, sugg::make_binop(higher::binop(op.node), a, r)), + long), format!("{} {}= {}", snip_a, op.node.as_str(), snip_r) ); + db.span_suggestion( + expr.span, + "or", + long + ); }, ); }; @@ -193,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); + }, + _ => {}, + } } } } @@ -226,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 821f6a1a9bf3..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; @@ -14,6 +14,9 @@ fn main() { 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 992bb4079eab..2858af1f8c0b 100644 --- a/tests/ui/assign_ops2.stderr +++ b/tests/ui/assign_ops2.stderr @@ -9,6 +9,10 @@ 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 @@ -19,6 +23,10 @@ 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 @@ -29,6 +37,10 @@ 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 @@ -39,6 +51,10 @@ 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 @@ -49,6 +65,10 @@ 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 @@ -59,6 +79,10 @@ 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 @@ -69,6 +93,10 @@ 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 @@ -79,6 +107,10 @@ 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: variable appears on both sides of an assignment operation --> $DIR/assign_ops2.rs:16:5 @@ -89,6 +121,10 @@ 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