From 9f70d04000aa7ca4c23d6839ac0e98d2074cf28b Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 5 Jun 2016 20:19:00 +0200 Subject: [PATCH 1/3] Fix wrong suggestion with `MANUAL_SWAP` and slices --- clippy_lints/Cargo.toml | 1 + clippy_lints/src/lib.rs | 3 +++ clippy_lints/src/swap.rs | 51 +++++++++++++++++++++++++++++++------- tests/compile-fail/swap.rs | 42 ++++++++++++++++++++++++++++++- 4 files changed, 87 insertions(+), 10 deletions(-) diff --git a/clippy_lints/Cargo.toml b/clippy_lints/Cargo.toml index e3cf22e00e1..877b9037d37 100644 --- a/clippy_lints/Cargo.toml +++ b/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" diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 944825e2bdc..857e11b0bd3 100644 --- a/clippy_lints/src/lib.rs +++ b/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 } diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index a518e15ea13..5a3adfee409 100644 --- a/clippy_lints/src/swap.rs +++ b/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)) { - (format!(" `{}` and `{}`", first, second), first, second) + fn check_for_slice<'a>(cx: &LateContext, lhs1: &'a Expr, lhs2: &'a Expr) -> Option<(&'a Expr, &'a Expr, &'a Expr)> { + 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() { - db.span_suggestion(span, "try", - format!("std::mem::swap(&mut {}, &mut {})", lhs, rhs)); - db.note("or maybe you should use `std::mem::replace`?"); + if !sugg.is_empty() { + db.span_suggestion(span, "try", sugg); + + if replace { + db.note("or maybe you should use `std::mem::replace`?"); + } } }); }} diff --git a/tests/compile-fail/swap.rs b/tests/compile-fail/swap.rs index cc0570e6c63..c4135467556 100644 --- a/tests/compile-fail/swap.rs +++ b/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; From 7211df5a177eaf58a8c328bf6e0448fe772cbd3e Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 5 Jun 2016 20:46:27 +0200 Subject: [PATCH 2/3] Remove useless `if_let_chain` --- clippy_lints/src/derive.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index f08522953aa..b941d9a7fca 100644 --- a/clippy_lints/src/derive.rs +++ b/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! {[ - let ItemImpl(_, _, _, Some(ref trait_ref), _, _) = item.node - ], { + if 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); } - }} + } } } From 7bc7c675f2fe1fc9176cd45baa3361037b487dec Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 5 Jun 2016 20:46:42 +0200 Subject: [PATCH 3/3] Cleanup, use `matches!` some more --- clippy_lints/src/block_in_if_condition.rs | 5 +---- clippy_lints/src/methods.rs | 13 ++++--------- clippy_lints/src/misc.rs | 17 +++-------------- 3 files changed, 8 insertions(+), 27 deletions(-) diff --git a/clippy_lints/src/block_in_if_condition.rs b/clippy_lints/src/block_in_if_condition.rs index c56cf4dcd29..b8f1bb71ad9 100644 --- a/clippy_lints/src/block_in_if_condition.rs +++ b/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 { - ExprBlock(_) => true, - _ => false, - } + matches!(ex.node, ExprBlock(_)) } else { false } diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index f9f557e7a9a..c37fbba9250 100644 --- a/clippy_lints/src/methods.rs +++ b/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 { - true - } else { - false - } + matches!(ty.node, hir::TyRptr(_, _)) } _ => 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"]) { - return true; - } + match_path(p, &["bool"]) + } else { + false } - false } fn is_copy<'a, 'ctx>(cx: &LateContext<'a, 'ctx>, ty: ty::Ty<'ctx>, item: &hir::Item) -> bool { diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 5f113cf47ce..53f4c972644 100644 --- a/clippy_lints/src/misc.rs +++ b/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 { - true - } else { - false - } + matches!(walk_ptrs_ty(cx.tcx.expr_ty(expr)).sty, ty::TyFloat(_)) } /// **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]) -> bool { args.len() == 1 && - if let ty::TyStr = walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty { - true - } else { - false - } + matches!(walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty, ty::TyStr) } /// **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 { - ExpnFormat::MacroAttribute(_) => true, - _ => false, - } + matches!(info.callee.format, ExpnFormat::MacroAttribute(_)) }) }) }