Merge pull request #1113 from oli-obk/assign_op

lint `a += a + b` (possible mis-refactoring of `a = a + b`)

CHANGELOG.md

@@ -207,6 +207,7 @@ All notable changes to this project will be documented in this file.
 [`match_same_arms`]: https://github.com/Manishearth/rust-clippy/wiki#match_same_arms
 [`mem_forget`]: https://github.com/Manishearth/rust-clippy/wiki#mem_forget
 [`min_max`]: https://github.com/Manishearth/rust-clippy/wiki#min_max
+[`misrefactored_assign_op`]: https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op
 [`modulo_one`]: https://github.com/Manishearth/rust-clippy/wiki#modulo_one
 [`mut_mut`]: https://github.com/Manishearth/rust-clippy/wiki#mut_mut
 [`mutex_atomic`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic

README.md

@@ -17,7 +17,7 @@ Table of contents:
 
 ## Lints
 
-There are 159 lints included in this crate:
+There are 160 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -97,6 +97,7 @@ name
 [match_same_arms](https://github.com/Manishearth/rust-clippy/wiki#match_same_arms)                                   | warn    | `match` with identical arm bodies
 [mem_forget](https://github.com/Manishearth/rust-clippy/wiki#mem_forget)                                             | allow   | `mem::forget` usage on `Drop` types is likely to cause memory leaks
 [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max)                                                   | warn    | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant
+[misrefactored_assign_op](https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op)                   | warn    | having a variable on both sides of an assign op
 [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one)                                             | warn    | taking a number modulo 1, which always returns 0
 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut)                                                   | allow   | usage of double-mut refs, e.g. `&mut &mut ...`
 [mutex_atomic](https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic)                                         | warn    | using a mutex where an atomic value could be used instead

clippy_lints/src/assign_ops.rs

@@ -1,6 +1,6 @@
 use rustc::hir;
 use rustc::lint::*;
-use utils::{span_lint_and_then, span_lint, snippet_opt, SpanlessEq, get_trait_def_id, implements_trait};
+use utils::{span_lint_and_then, snippet_opt, SpanlessEq, get_trait_def_id, implements_trait};
 use utils::{higher, sugg};
 
 /// **What it does:** This lint checks for `+=` operations and similar.
@@ -38,12 +38,31 @@ declare_lint! {
     "assigning the result of an operation on a variable to that same variable"
 }
 
+/// **What it does:** Check for `a op= a op b` or `a op= b op a` patterns.
+///
+/// **Why is this bad?** Most likely these are bugs where one meant to write `a op= b`
+///
+/// **Known problems:** Someone might actually mean `a op= a op b`, but that should rather be written as `a = (2 * a) op b` where applicable.
+///
+/// **Example:**
+///
+/// ```rust
+/// let mut a = 5;
+/// ...
+/// a += a + b;
+/// ```
+declare_lint! {
+    pub MISREFACTORED_ASSIGN_OP,
+    Warn,
+    "having a variable on both sides of an assign op"
+}
+
 #[derive(Copy, Clone, Default)]
 pub struct AssignOps;
 
 impl LintPass for AssignOps {
     fn get_lints(&self) -> LintArray {
-        lint_array!(ASSIGN_OPS, ASSIGN_OP_PATTERN)
+        lint_array!(ASSIGN_OPS, ASSIGN_OP_PATTERN, MISREFACTORED_ASSIGN_OP)
     }
 }
 
@@ -59,6 +78,40 @@ impl LateLintPass for AssignOps {
                                        "replace it with",
                                        format!("{} = {}", lhs, sugg::make_binop(higher::binop(op.node), lhs, rhs)));
                 });
+                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 ty = cx.tcx.expr_ty(assignee);
+                            if ty.walk_shallow().next().is_some() {
+                                return; // implements_trait does not work with generics
+                            }
+                            let rty = cx.tcx.expr_ty(rhs);
+                            if rty.walk_shallow().next().is_some() {
+                                return; // implements_trait does not work with generics
+                            }
+                            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)) {
+                                                       db.span_suggestion(expr.span,
+                                                                          "replace it with",
+                                                                          format!("{} {}= {}", snip_a, op.node.as_str(), snip_r));
+                                                   }
+                                               });
+                        };
+                        // lhs op= l op r
+                        if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) {
+                            lint(lhs, r);
+                        }
+                        // lhs op= l commutative_op r
+                        if is_commutative(op.node) && SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, r) {
+                            lint(lhs, l);
+                        }
+                    }
+                }
             }
             hir::ExprAssign(ref assignee, ref e) => {
                 if let hir::ExprBinary(op, ref l, ref r) = e.node {
@@ -104,23 +157,18 @@ impl LateLintPass for AssignOps {
                                 BitXor: BiBitXor,
                                 Shr: BiShr,
                                 Shl: BiShl) {
-                            if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span),
+                            span_lint_and_then(cx,
-                                                                   snippet_opt(cx, rhs.span)) {
+                                               ASSIGN_OP_PATTERN,
-                                span_lint_and_then(cx,
+                                               expr.span,
-                                                   ASSIGN_OP_PATTERN,
+                                               "manual implementation of an assign operation",
-                                                   expr.span,
+                                               |db| {
-                                                   "manual implementation of an assign operation",
+                                                   if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span),
-                                                   |db| {
+                                                                                          snippet_opt(cx, rhs.span)) {
                                                        db.span_suggestion(expr.span,
                                                                           "replace it with",
                                                                           format!("{} {}= {}", snip_a, op.node.as_str(), snip_r));
-                                                   });
+                                                   }
-                            } else {
+                                               });
-                                span_lint(cx,
-                                          ASSIGN_OP_PATTERN,
-                                          expr.span,
-                                          "manual implementation of an assign operation");
-                            }
                         }
                     };
                     // a = a op b
@@ -143,3 +191,27 @@ impl LateLintPass for AssignOps {
         }
     }
 }
+
+fn is_commutative(op: hir::BinOp_) -> bool {
+    use rustc::hir::BinOp_::*;
+    match op {
+        BiAdd |
+        BiMul |
+        BiAnd |
+        BiOr |
+        BiBitXor |
+        BiBitAnd |
+        BiBitOr |
+        BiEq |
+        BiNe => true,
+        BiSub |
+        BiDiv |
+        BiRem |
+        BiShl |
+        BiShr |
+        BiLt |
+        BiLe |
+        BiGe |
+        BiGt => false,
+    }
+}

clippy_lints/src/lib.rs

@@ -300,6 +300,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         approx_const::APPROX_CONSTANT,
         array_indexing::OUT_OF_BOUNDS_INDEXING,
         assign_ops::ASSIGN_OP_PATTERN,
+        assign_ops::MISREFACTORED_ASSIGN_OP,
         attrs::DEPRECATED_SEMVER,
         attrs::INLINE_ALWAYS,
         bit_mask::BAD_BIT_MASK,

tests/compile-fail/assign_ops2.rs

@@ -0,0 +1,36 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#[allow(unused_assignments)]
+#[deny(misrefactored_assign_op)]
+fn main() {
+    let mut a = 5;
+    a += a + 1; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a += 1
+    a += 1 + a; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a += 1
+    a -= a - 1; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a -= 1
+    a *= a * 99; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a *= 99
+    a *= 42 * a; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a *= 42
+    a /= a / 2; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a /= 2
+    a %= a % 5; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a %= 5
+    a &= a & 1; //~ ERROR variable appears on both sides of an assignment operation
+    //~^ HELP replace it with
+    //~| SUGGESTION a &= 1
+    a -= 1 - a;
+    a /= 5 / a;
+    a %= 42 % a;
+    a <<= 6 << a;
+}