Lint manual swaps

README.md

@@ -8,11 +8,12 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 126 lints included in this crate:
+There are 127 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 [absurd_extreme_comparisons](https://github.com/Manishearth/rust-clippy/wiki#absurd_extreme_comparisons)             | warn    | a comparison involving a maximum or minimum value involves a case that is always true or always false
+[almost_swapped](https://github.com/Manishearth/rust-clippy/wiki#almost_swapped)                                     | warn    | `foo = bar; bar = foo` sequence
 [approx_constant](https://github.com/Manishearth/rust-clippy/wiki#approx_constant)                                   | warn    | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant
 [bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask)                                         | warn    | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have)
 [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr)             | warn    | braces can be eliminated in conditions that are expressions, e.g `if { true } ...`
@@ -62,6 +63,7 @@ name
 [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return)                                     | warn    | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block
 [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value)                                     | warn    | creating a let binding to a value of unit type, which usually can't be used afterwards
 [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist)                                             | warn    | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a VecDeque
+[manual_swap](https://github.com/Manishearth/rust-clippy/wiki#manual_swap)                                           | warn    | manual swap
 [map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone)                                               | warn    | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead)
 [map_entry](https://github.com/Manishearth/rust-clippy/wiki#map_entry)                                               | warn    | use of `contains_key` followed by `insert` on a `HashMap` or `BTreeMap`
 [match_bool](https://github.com/Manishearth/rust-clippy/wiki#match_bool)                                             | warn    | a match on boolean expression; recommends `if..else` block instead
@@ -114,7 +116,6 @@ name
 [string_to_string](https://github.com/Manishearth/rust-clippy/wiki#string_to_string)                                 | warn    | calling `String::to_string` which is inefficient
 [suspicious_assignment_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_assignment_formatting) | warn    | suspicious formatting of `*=`, `-=` or `!=`
 [suspicious_else_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_else_formatting)             | warn    | suspicious formatting of `else if`
-[suspicious_swap](https://github.com/Manishearth/rust-clippy/wiki#suspicious_swap)                                   | warn    | `foo = bar; bar = foo` sequence
 [temporary_assignment](https://github.com/Manishearth/rust-clippy/wiki#temporary_assignment)                         | warn    | assignments to temporaries
 [toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg)                                 | warn    | An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take references with `&`.
 [trivial_regex](https://github.com/Manishearth/rust-clippy/wiki#trivial_regex)                                       | warn    | finds trivial regular expressions in `Regex::new(_)` invocations

src/lib.rs

@@ -287,7 +287,8 @@ pub fn plugin_registrar(reg: &mut Registry) {
         returns::LET_AND_RETURN,
         returns::NEEDLESS_RETURN,
         strings::STRING_LIT_AS_BYTES,
-        swap::SUSPICIOUS_SWAP,
+        swap::ALMOST_SWAPPED,
+        swap::MANUAL_SWAP,
         temporary_assignment::TEMPORARY_ASSIGNMENT,
         transmute::USELESS_TRANSMUTE,
         types::ABSURD_EXTREME_COMPARISONS,

src/swap.rs

@@ -1,7 +1,26 @@
 use rustc::lint::*;
 use rustc_front::hir::*;
-use utils::{differing_macro_contexts, snippet_opt, span_lint_and_then, SpanlessEq};
 use syntax::codemap::mk_sp;
+use utils::{differing_macro_contexts, snippet_opt, span_lint_and_then, SpanlessEq};
+
+/// **What it does:** This lints manual swapping.
+///
+/// **Why is this bad?** The `std::mem::swap` function exposes the intent better without
+/// deinitializing or copying either variable.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust,ignore
+/// let t = b;
+/// b = a;
+/// a = t;
+/// ```
+declare_lint! {
+    pub MANUAL_SWAP,
+    Warn,
+    "manual swap"
+}
 
 /// **What it does:** This lints `foo = bar; bar = foo` sequences.
 ///
@@ -15,7 +34,7 @@ use syntax::codemap::mk_sp;
 /// b = a;
 /// ```
 declare_lint! {
-    pub SUSPICIOUS_SWAP,
+    pub ALMOST_SWAPPED,
     Warn,
     "`foo = bar; bar = foo` sequence"
 }
@@ -25,42 +44,95 @@ pub struct Swap;
 
 impl LintPass for Swap {
     fn get_lints(&self) -> LintArray {
-        lint_array![SUSPICIOUS_SWAP]
+        lint_array![MANUAL_SWAP, ALMOST_SWAPPED]
     }
 }
 
 impl LateLintPass for Swap {
     fn check_block(&mut self, cx: &LateContext, block: &Block) {
-        for w in block.stmts.windows(2) {
+        check_manual_swap(cx, block);
-            if_let_chain!{[
+        check_suspicious_swap(cx, block);
-                let StmtSemi(ref first, _) = w[0].node,
+    }
-                let StmtSemi(ref second, _) = w[1].node,
+}
-                !differing_macro_contexts(first.span, second.span),
+
-                let ExprAssign(ref lhs0, ref rhs0) = first.node,
+/// Implementation of the `MANUAL_SWAP` lint.
-                let ExprAssign(ref lhs1, ref rhs1) = second.node,
+fn check_manual_swap(cx: &LateContext, block: &Block) {
-                SpanlessEq::new(cx).ignore_fn().eq_expr(lhs0, rhs1),
+    for w in block.stmts.windows(3) {
-                SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, rhs0)
+        if_let_chain!{[
-            ], {
+            // let t = foo();
-                let (what, lhs, rhs) = if let (Some(first), Some(second)) = (snippet_opt(cx, lhs0.span), snippet_opt(cx, rhs0.span)) {
+            let StmtDecl(ref tmp, _) = w[0].node,
-                    (format!(" `{}` and `{}`", first, second), first, second)
+            let DeclLocal(ref tmp) = tmp.node,
-                } else {
+            let Some(ref tmp_init) = tmp.init,
-                    ("".to_owned(), "".to_owned(), "".to_owned())
+            let PatKind::Ident(_, ref tmp_name, None) = tmp.pat.node,
-                };
+
-
+            // foo() = bar();
-                let span = mk_sp(first.span.lo, second.span.hi);
+            let StmtSemi(ref first, _) = w[1].node,
-
+            let ExprAssign(ref lhs1, ref rhs1) = first.node,
-                span_lint_and_then(cx,
+
-                                   SUSPICIOUS_SWAP,
+            // bar() = t;
-                                   span,
+            let StmtSemi(ref second, _) = w[2].node,
-                                   &format!("this looks like you are trying to swap{}", what),
+            let ExprAssign(ref lhs2, ref rhs2) = second.node,
-                                   |db| {
+            let ExprPath(None, ref rhs2) = rhs2.node,
-                                       if !what.is_empty() {
+            rhs2.segments.len() == 1,
-                                           db.span_suggestion(span, "try",
+
-                                                              format!("std::mem::swap({}, {})", lhs, rhs));
+            tmp_name.node.name.as_str() == rhs2.segments[0].identifier.name.as_str(),
-                                           db.fileline_note(span, "or maybe you should use `std::mem::replace`?");
+            SpanlessEq::new(cx).ignore_fn().eq_expr(tmp_init, lhs1),
-                                       }
+            SpanlessEq::new(cx).ignore_fn().eq_expr(rhs1, lhs2)
-                                   });
+        ], {
-            }}
+            let (what, lhs, rhs) = if let (Some(first), Some(second)) = (snippet_opt(cx, lhs1.span), snippet_opt(cx, rhs1.span)) {
-        }
+                (format!(" `{}` and `{}`", first, second), first, second)
+            } else {
+                ("".to_owned(), "".to_owned(), "".to_owned())
+            };
+
+            let span = mk_sp(tmp.span.lo, second.span.hi);
+
+            span_lint_and_then(cx,
+                               MANUAL_SWAP,
+                               span,
+                               &format!("this looks like you are swapping{} manually", what),
+                               |db| {
+                                   if !what.is_empty() {
+                                       db.span_suggestion(span, "try",
+                                                          format!("std::mem::swap(&mut {}, &mut {})", lhs, rhs));
+                                       db.fileline_note(span, "or maybe you should use `std::mem::replace`?");
+                                   }
+                               });
+        }}
+    }
+}
+
+/// Implementation of the `ALMOST_SWAPPED` lint.
+fn check_suspicious_swap(cx: &LateContext, block: &Block) {
+    for w in block.stmts.windows(2) {
+        if_let_chain!{[
+            let StmtSemi(ref first, _) = w[0].node,
+            let StmtSemi(ref second, _) = w[1].node,
+            !differing_macro_contexts(first.span, second.span),
+            let ExprAssign(ref lhs0, ref rhs0) = first.node,
+            let ExprAssign(ref lhs1, ref rhs1) = second.node,
+            SpanlessEq::new(cx).ignore_fn().eq_expr(lhs0, rhs1),
+            SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, rhs0)
+        ], {
+            let (what, lhs, rhs) = if let (Some(first), Some(second)) = (snippet_opt(cx, lhs0.span), snippet_opt(cx, rhs0.span)) {
+                (format!(" `{}` and `{}`", first, second), first, second)
+            } else {
+                ("".to_owned(), "".to_owned(), "".to_owned())
+            };
+
+            let span = mk_sp(first.span.lo, second.span.hi);
+
+            span_lint_and_then(cx,
+                               ALMOST_SWAPPED,
+                               span,
+                               &format!("this looks like you are trying to swap{}", what),
+                               |db| {
+                                   if !what.is_empty() {
+                                       db.span_suggestion(span, "try",
+                                                          format!("std::mem::swap(&mut {}, &mut {})", lhs, rhs));
+                                       db.fileline_note(span, "or maybe you should use `std::mem::replace`?");
+                                   }
+                               });
+        }}
     }
 }

tests/compile-fail/swap.rs

@@ -4,6 +4,8 @@
 #![deny(clippy)]
 #![allow(unused_assignments)]
 
+struct Foo(u32);
+
 fn main() {
     let mut a = 42;
     let mut b = 1337;
@@ -12,6 +14,31 @@ fn main() {
     b = a;
     //~^^ ERROR this looks like you are trying to swap `a` and `b`
     //~| HELP try
-    //~| SUGGESTION std::mem::swap(a, b);
+    //~| SUGGESTION std::mem::swap(&mut a, &mut b);
+    //~| NOTE or maybe you should use `std::mem::replace`?
+
+    let t = a;
+    a = b;
+    b = t;
+    //~^^^ ERROR this looks like you are swapping `a` and `b` manually
+    //~| HELP try
+    //~| SUGGESTION std::mem::swap(&mut a, &mut b);
+    //~| NOTE or maybe you should use `std::mem::replace`?
+
+    let mut c = Foo(42);
+
+    c.0 = a;
+    a = c.0;
+    //~^^ ERROR this looks like you are trying to swap `c.0` and `a`
+    //~| HELP try
+    //~| SUGGESTION std::mem::swap(&mut c.0, &mut a);
+    //~| NOTE or maybe you should use `std::mem::replace`?
+
+    let t = c.0;
+    c.0 = a;
+    a = t;
+    //~^^^ ERROR this looks like you are swapping `c.0` and `a` manually
+    //~| HELP try
+    //~| SUGGESTION std::mem::swap(&mut c.0, &mut a);
     //~| NOTE or maybe you should use `std::mem::replace`?
 }