Additionally suggest the semantic equal variant

diff --git a/clippy_lints/src/assign_ops.rs b/clippy_lints/src/assign_ops.rs
index fe6949566e4b84ab06dfff8b5c67793f5dd659e3..d285e71bd16644ecf76c4629544230b6098cb440 100644 (file)
--- 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 821f6a1a9bf3be8314104a9724c6e016122d4f5d..2d3adc2a661f945822a7b8db423e923cc42a3f74 100644 (file)
--- 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 992bb4079eab0c117a720e65791fd7a1d59a0835..2858af1f8c0b5493291ec1943867d97ba23b19df 100644 (file)
--- 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