From 76004306ccf3e3069bdf0928a70895916bea00ff Mon Sep 17 00:00:00 2001 From: mcarton Date: Sun, 28 Feb 2016 00:46:02 +0100 Subject: [PATCH] Lint manual swaps --- README.md | 5 +- src/lib.rs | 3 +- src/swap.rs | 140 ++++++++++++++++++++++++++++--------- tests/compile-fail/swap.rs | 29 +++++++- 4 files changed, 139 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index f7527372585a..f421cbdb7c9c 100644 --- a/README.md +++ b/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 diff --git a/src/lib.rs b/src/lib.rs index 217110e2bd84..5debe2ed50ce 100644 --- a/src/lib.rs +++ b/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, diff --git a/src/swap.rs b/src/swap.rs index 8e1b1e781f4a..6d7212233fbb 100644 --- a/src/swap.rs +++ b/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) { - 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, - SUSPICIOUS_SWAP, - 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({}, {})", lhs, rhs)); - db.fileline_note(span, "or maybe you should use `std::mem::replace`?"); - } - }); - }} - } + check_manual_swap(cx, block); + check_suspicious_swap(cx, block); + } +} + +/// Implementation of the `MANUAL_SWAP` lint. +fn check_manual_swap(cx: &LateContext, block: &Block) { + for w in block.stmts.windows(3) { + if_let_chain!{[ + // let t = foo(); + let StmtDecl(ref tmp, _) = w[0].node, + let DeclLocal(ref tmp) = tmp.node, + let Some(ref tmp_init) = tmp.init, + let PatKind::Ident(_, ref tmp_name, None) = tmp.pat.node, + + // foo() = bar(); + let StmtSemi(ref first, _) = w[1].node, + let ExprAssign(ref lhs1, ref rhs1) = first.node, + + // bar() = t; + let StmtSemi(ref second, _) = w[2].node, + let ExprAssign(ref lhs2, ref rhs2) = second.node, + let ExprPath(None, ref rhs2) = rhs2.node, + rhs2.segments.len() == 1, + + tmp_name.node.name.as_str() == rhs2.segments[0].identifier.name.as_str(), + 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`?"); + } + }); + }} } } diff --git a/tests/compile-fail/swap.rs b/tests/compile-fail/swap.rs index be2c785eb412..cc0570e6c631 100755 --- a/tests/compile-fail/swap.rs +++ b/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`? }