From bd421cb5a5937ce42ddddd392ec743c8154f5d56 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 30 Jan 2018 17:45:35 +0100 Subject: [PATCH] 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 fe6949566e4..d285e71bd16 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 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { { 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 @@ macro_rules! ops { ); } }; - // 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 821f6a1a9bf..2d3adc2a661 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 992bb4079ea..2858af1f8c0 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 -- 2.44.0