Merge pull request #984 from Manishearth/fix-swap

Fix wrong suggestion with `MANUAL_SWAP` and slices

clippy_lints/Cargo.toml

@@ -16,6 +16,7 @@ license = "MPL-2.0"
 keywords = ["clippy", "lint", "plugin"]
 
 [dependencies]
+matches = "0.1.2"
 regex-syntax = "0.3.0"
 semver = "0.2.1"
 toml = "0.1"

clippy_lints/src/block_in_if_condition.rs

@@ -47,10 +47,7 @@ impl<'v> Visitor<'v> for ExVisitor<'v> {
             let complex = {
                 if block.stmts.is_empty() {
                     if let Some(ref ex) = block.expr {
-                        match ex.node {
+                        matches!(ex.node, ExprBlock(_))
-                            ExprBlock(_) => true,
-                            _ => false,
-                        }
                     } else {
                         false
                     }

clippy_lints/src/derive.rs

@@ -70,9 +70,7 @@ impl LintPass for Derive {
 
 impl LateLintPass for Derive {
     fn check_item(&mut self, cx: &LateContext, item: &Item) {
-        if_let_chain! {[
+        if let ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node {
-            let ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node
-        ], {
             let ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(item.id)).ty;
             let is_automatically_derived = item.attrs.iter().any(is_automatically_derived);
 
@@ -81,7 +79,7 @@ impl LateLintPass for Derive {
             if !is_automatically_derived {
                 check_copy_clone(cx, item, trait_ref, ty);
             }
-        }}
+        }
     }
 }
 

clippy_lints/src/lib.rs

@@ -42,6 +42,9 @@ extern crate rustc_plugin;
 extern crate rustc_const_eval;
 extern crate rustc_const_math;
 
+#[macro_use]
+extern crate matches as matches_macro;
+
 macro_rules! declare_restriction_lint {
     { pub $name:tt, $description:tt } => {
         declare_lint! { pub $name, Allow, $description }

clippy_lints/src/methods.rs

@@ -1023,11 +1023,7 @@ impl OutType {
             (&OutType::Bool, &hir::Return(ref ty)) if is_bool(ty) => true,
             (&OutType::Any, &hir::Return(ref ty)) if ty.node != hir::TyTup(vec![].into()) => true,
             (&OutType::Ref, &hir::Return(ref ty)) => {
-                if let hir::TyRptr(_, _) = ty.node {
+                matches!(ty.node, hir::TyRptr(_, _))
-                    true
-                } else {
-                    false
-                }
             }
             _ => false,
         }
@@ -1036,11 +1032,10 @@ impl OutType {
 
 fn is_bool(ty: &hir::Ty) -> bool {
     if let hir::TyPath(None, ref p) = ty.node {
-        if match_path(p, &["bool"]) {
+        match_path(p, &["bool"])
-            return true;
+    } else {
-        }
+        false
     }
-    false
 }
 
 fn is_copy<'a, 'ctx>(cx: &LateContext<'a, 'ctx>, ty: ty::Ty<'ctx>, item: &hir::Item) -> bool {

clippy_lints/src/misc.rs

@@ -189,11 +189,7 @@ fn is_allowed(cx: &LateContext, expr: &Expr) -> bool {
 }
 
 fn is_float(cx: &LateContext, expr: &Expr) -> bool {
-    if let ty::TyFloat(_) = walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty {
+    matches!(walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty, ty::TyFloat(_))
-        true
-    } else {
-        false
-    }
 }
 
 /// **What it does:** This lint checks for conversions to owned values just for the sake of a comparison.
@@ -283,11 +279,7 @@ fn check_to_owned(cx: &LateContext, expr: &Expr, other: &Expr, left: bool, op: S
 
 fn is_str_arg(cx: &LateContext, args: &[P<Expr>]) -> bool {
     args.len() == 1 &&
-    if let ty::TyStr = walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty {
+        matches!(walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty, ty::TyStr)
-        true
-    } else {
-        false
-    }
 }
 
 /// **What it does:** This lint checks for getting the remainder of a division by one.
@@ -449,10 +441,7 @@ fn is_used(cx: &LateContext, expr: &Expr) -> bool {
 fn in_attributes_expansion(cx: &LateContext, expr: &Expr) -> bool {
     cx.sess().codemap().with_expn_info(expr.span.expn_id, |info_opt| {
         info_opt.map_or(false, |info| {
-            match info.callee.format {
+            matches!(info.callee.format, ExpnFormat::MacroAttribute(_))
-                ExpnFormat::MacroAttribute(_) => true,
-                _ => false,
-            }
         })
     })
 }

clippy_lints/src/swap.rs

@@ -1,7 +1,8 @@
-use rustc::lint::*;
 use rustc::hir::*;
+use rustc::lint::*;
+use rustc::ty;
 use syntax::codemap::mk_sp;
-use utils::{differing_macro_contexts, snippet_opt, span_lint_and_then, SpanlessEq};
+use utils::{differing_macro_contexts, match_type, paths, snippet, snippet_opt, span_lint_and_then, walk_ptrs_ty, SpanlessEq};
 
 /// **What it does:** This lints manual swapping.
 ///
@@ -79,10 +80,40 @@ fn check_manual_swap(cx: &LateContext, block: &Block) {
             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)) {
+            fn check_for_slice<'a>(cx: &LateContext, lhs1: &'a Expr, lhs2: &'a Expr) -> Option<(&'a Expr, &'a Expr, &'a Expr)> {
-                (format!(" `{}` and `{}`", first, second), first, second)
+                if let ExprIndex(ref lhs1, ref idx1) = lhs1.node {
+                    if let ExprIndex(ref lhs2, ref idx2) = lhs2.node {
+                        if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, lhs2) {
+                            let ty = walk_ptrs_ty(cx.tcx.expr_ty(lhs1));
+
+                            if matches!(ty.sty, ty::TySlice(_)) ||
+                                matches!(ty.sty, ty::TyArray(_, _)) ||
+                                match_type(cx, ty, &paths::VEC) ||
+                                match_type(cx, ty, &paths::VEC_DEQUE) {
+                                    return Some((lhs1, idx1, idx2));
+                            }
+                        }
+                    }
+                }
+
+                None
+            }
+
+            let (replace, what, sugg) = if let Some((slice, idx1, idx2)) = check_for_slice(cx, lhs1, lhs2) {
+                if let Some(slice) = snippet_opt(cx, slice.span) {
+                    (false,
+                     format!(" elements of `{}`", slice),
+                     format!("{}.swap({}, {})",slice,  snippet(cx, idx1.span, ".."), snippet(cx, idx2.span, "..")))
+                } else {
+                    (false, "".to_owned(), "".to_owned())
+                }
             } else {
-                ("".to_owned(), "".to_owned(), "".to_owned())
+                 if let (Some(first), Some(second)) = (snippet_opt(cx, lhs1.span), snippet_opt(cx, rhs1.span)) {
+                    (true, format!(" `{}` and `{}`", first, second),
+                     format!("std::mem::swap(&mut {}, &mut {})", first, second))
+                } else {
+                    (true, "".to_owned(), "".to_owned())
+                }
             };
 
             let span = mk_sp(w[0].span.lo, second.span.hi);
@@ -92,10 +123,12 @@ fn check_manual_swap(cx: &LateContext, block: &Block) {
                                span,
                                &format!("this looks like you are swapping{} manually", what),
                                |db| {
-                                   if !what.is_empty() {
+                                   if !sugg.is_empty() {
-                                       db.span_suggestion(span, "try",
+                                       db.span_suggestion(span, "try", sugg);
-                                                          format!("std::mem::swap(&mut {}, &mut {})", lhs, rhs));
+
-                                       db.note("or maybe you should use `std::mem::replace`?");
+                                       if replace {
+                                           db.note("or maybe you should use `std::mem::replace`?");
+                                       }
                                    }
                                });
         }}

tests/compile-fail/swap.rs

@@ -2,11 +2,51 @@
 #![plugin(clippy)]
 
 #![deny(clippy)]
-#![allow(unused_assignments)]
+#![allow(blacklisted_name, unused_assignments)]
 
 struct Foo(u32);
 
+fn array() {
+    let mut foo = [1, 2];
+    let temp = foo[0];
+    foo[0] = foo[1];
+    foo[1] = temp;
+    //~^^^ ERROR this looks like you are swapping elements of `foo` manually
+    //~| HELP try
+    //~| SUGGESTION foo.swap(0, 1);
+
+    foo.swap(0, 1);
+}
+
+fn slice() {
+    let foo = &mut [1, 2];
+    let temp = foo[0];
+    foo[0] = foo[1];
+    foo[1] = temp;
+    //~^^^ ERROR this looks like you are swapping elements of `foo` manually
+    //~| HELP try
+    //~| SUGGESTION foo.swap(0, 1);
+
+    foo.swap(0, 1);
+}
+
+fn vec() {
+    let mut foo = vec![1, 2];
+    let temp = foo[0];
+    foo[0] = foo[1];
+    foo[1] = temp;
+    //~^^^ ERROR this looks like you are swapping elements of `foo` manually
+    //~| HELP try
+    //~| SUGGESTION foo.swap(0, 1);
+
+    foo.swap(0, 1);
+}
+
 fn main() {
+    array();
+    slice();
+    vec();
+
     let mut a = 42;
     let mut b = 1337;