From d3232b0fce41bce8f184841e3c613df1c1a43035 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 3 Dec 2019 13:18:45 +0100 Subject: [PATCH 1/5] Add regression test for manual_swap --- tests/ui/swap.rs | 9 +++++++++ tests/ui/swap.stderr | 10 +++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/ui/swap.rs b/tests/ui/swap.rs index b508c1ee0096..f5e9182bed1e 100644 --- a/tests/ui/swap.rs +++ b/tests/ui/swap.rs @@ -46,6 +46,15 @@ fn slice() { foo.swap(0, 1); } +fn unswappable_slice() { + let foo = &mut [vec![1, 2], vec![3, 4]]; + let temp = foo[0][1]; + foo[0][1] = foo[1][0]; + foo[1][0] = temp; + + // swap(foo[0][1], foo[1][0]) would fail +} + fn vec() { let mut foo = vec![1, 2]; let temp = foo[0]; diff --git a/tests/ui/swap.stderr b/tests/ui/swap.stderr index b45187b58050..f3622230042b 100644 --- a/tests/ui/swap.stderr +++ b/tests/ui/swap.stderr @@ -17,7 +17,7 @@ LL | | foo[1] = temp; | |_________________^ help: try: `foo.swap(0, 1)` error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:51:5 + --> $DIR/swap.rs:60:5 | LL | / let temp = foo[0]; LL | | foo[0] = foo[1]; @@ -25,7 +25,7 @@ LL | | foo[1] = temp; | |_________________^ help: try: `foo.swap(0, 1)` error: this looks like you are swapping `a` and `b` manually - --> $DIR/swap.rs:71:7 + --> $DIR/swap.rs:80:7 | LL | ; let t = a; | _______^ @@ -36,7 +36,7 @@ LL | | b = t; = note: or maybe you should use `std::mem::replace`? error: this looks like you are swapping `c.0` and `a` manually - --> $DIR/swap.rs:80:7 + --> $DIR/swap.rs:89:7 | LL | ; let t = c.0; | _______^ @@ -47,7 +47,7 @@ LL | | a = t; = note: or maybe you should use `std::mem::replace`? error: this looks like you are trying to swap `a` and `b` - --> $DIR/swap.rs:68:5 + --> $DIR/swap.rs:77:5 | LL | / a = b; LL | | b = a; @@ -57,7 +57,7 @@ LL | | b = a; = note: or maybe you should use `std::mem::replace`? error: this looks like you are trying to swap `c.0` and `a` - --> $DIR/swap.rs:77:5 + --> $DIR/swap.rs:86:5 | LL | / c.0 = a; LL | | a = c.0; From d1d5f790f575c4872f500287d54135a3f0361623 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 3 Dec 2019 13:20:42 +0100 Subject: [PATCH 2/5] Fix FP in manual_swap lint with slice-like types --- clippy_lints/src/swap.rs | 79 ++++++++++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index b278171abceb..c7a0f0058f96 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -97,29 +97,6 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { if SpanlessEq::new(cx).ignore_fn().eq_expr(tmp_init, lhs1); if SpanlessEq::new(cx).ignore_fn().eq_expr(rhs1, lhs2); then { - fn check_for_slice<'a>( - cx: &LateContext<'_, '_>, - lhs1: &'a Expr, - lhs2: &'a Expr, - ) -> Option<(&'a Expr, &'a Expr, &'a Expr)> { - if let ExprKind::Index(ref lhs1, ref idx1) = lhs1.kind { - if let ExprKind::Index(ref lhs2, ref idx2) = lhs2.kind { - if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, lhs2) { - let ty = walk_ptrs_ty(cx.tables.expr_ty(lhs1)); - - if matches!(ty.kind, ty::Slice(_)) || - matches!(ty.kind, ty::Array(_, _)) || - is_type_diagnostic_item(cx, ty, Symbol::intern("vec_type")) || - match_type(cx, ty, &paths::VEC_DEQUE) { - return Some((lhs1, idx1, idx2)); - } - } - } - } - - None - } - if let ExprKind::Field(ref lhs1, _) = lhs1.kind { if let ExprKind::Field(ref lhs2, _) = lhs2.kind { if lhs1.hir_id.owner_def_id() == lhs2.hir_id.owner_def_id() { @@ -128,7 +105,11 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { } } - let (replace, what, sugg) = if let Some((slice, idx1, idx2)) = check_for_slice(cx, lhs1, lhs2) { + let slice = check_for_slice(cx, lhs1, lhs2); + + let (replace, what, sugg) = if let Slice::NotSwappable = slice { + return; + } else if let Slice::Swappable(slice, idx1, idx2) = slice { if let Some(slice) = Sugg::hir_opt(cx, slice) { (false, format!(" elements of `{}`", slice), @@ -171,6 +152,56 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { } } +enum Slice<'a> { + /// `slice.swap(idx1, idx2)` can be used + /// + /// ## Example + /// + /// ```rust + /// let t = a[1]; + /// a[1] = a[0]; + /// a[0] = t; + /// // can be written as + /// a.swap(0, 1); + /// ``` + Swappable(&'a Expr, &'a Expr, &'a Expr), + /// The `swap` function cannot be used. + /// + /// ## Example + /// + /// ```rust + /// let t = a[0][1]; + /// a[0][1] = a[1][0]; + /// a[1][0] = t; + /// ``` + NotSwappable, + /// Not a slice + None, +} + +/// Checks if both expressions are index operations into "slice-like" types. +fn check_for_slice<'a>(cx: &LateContext<'_, '_>, lhs1: &'a Expr, lhs2: &'a Expr) -> Slice<'a> { + if let ExprKind::Index(ref lhs1, ref idx1) = lhs1.kind { + if let ExprKind::Index(ref lhs2, ref idx2) = lhs2.kind { + if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, lhs2) { + let ty = walk_ptrs_ty(cx.tables.expr_ty(lhs1)); + + if matches!(ty.kind, ty::Slice(_)) + || matches!(ty.kind, ty::Array(_, _)) + || is_type_diagnostic_item(cx, ty, Symbol::intern("vec_type")) + || match_type(cx, ty, &paths::VEC_DEQUE) + { + return Slice::Swappable(lhs1, idx1, idx2); + } + } else { + return Slice::NotSwappable; + } + } + } + + Slice::None +} + /// Implementation of the `ALMOST_SWAPPED` lint. fn check_suspicious_swap(cx: &LateContext<'_, '_>, block: &Block) { for w in block.stmts.windows(2) { From bd39a608a8b09ad8207c87f6e976c85d6a407966 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 3 Dec 2019 13:21:00 +0100 Subject: [PATCH 3/5] Formatting --- clippy_lints/src/swap.rs | 59 +++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index c7a0f0058f96..bb294c467fa8 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -111,42 +111,51 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { return; } else if let Slice::Swappable(slice, idx1, idx2) = slice { if let Some(slice) = Sugg::hir_opt(cx, slice) { - (false, - format!(" elements of `{}`", slice), - format!("{}.swap({}, {})", - slice.maybe_par(), - snippet(cx, idx1.span, ".."), - snippet(cx, idx2.span, ".."))) + ( + false, + format!(" elements of `{}`", slice), + format!( + "{}.swap({}, {})", + slice.maybe_par(), + snippet(cx, idx1.span, ".."), + snippet(cx, idx2.span, ".."), + ), + ) } else { (false, String::new(), String::new()) } } else if let (Some(first), Some(second)) = (Sugg::hir_opt(cx, lhs1), Sugg::hir_opt(cx, rhs1)) { - (true, format!(" `{}` and `{}`", first, second), - format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr())) + ( + true, + format!(" `{}` and `{}`", first, second), + format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()), + ) } else { (true, String::new(), String::new()) }; let span = w[0].span.to(second.span); - span_lint_and_then(cx, - MANUAL_SWAP, - span, - &format!("this looks like you are swapping{} manually", what), - |db| { - if !sugg.is_empty() { - db.span_suggestion( - span, - "try", - sugg, - Applicability::Unspecified, - ); + span_lint_and_then( + cx, + MANUAL_SWAP, + span, + &format!("this looks like you are swapping{} manually", what), + |db| { + if !sugg.is_empty() { + db.span_suggestion( + span, + "try", + sugg, + Applicability::Unspecified, + ); - if replace { - db.note("or maybe you should use `std::mem::replace`?"); - } - } - }); + if replace { + db.note("or maybe you should use `std::mem::replace`?"); + } + } + } + ); } } } From 1e3b24de4389491dac9ce630668c710df5950c70 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 3 Dec 2019 13:42:05 +0100 Subject: [PATCH 4/5] Make manual_swap autofixable --- clippy_lints/src/swap.rs | 13 ++++--- tests/ui/swap.fixed | 83 ++++++++++++++++++++++++++++++++++++++++ tests/ui/swap.rs | 3 ++ tests/ui/swap.stderr | 14 +++---- 4 files changed, 100 insertions(+), 13 deletions(-) create mode 100644 tests/ui/swap.fixed diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index bb294c467fa8..77288869ce0e 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -1,7 +1,7 @@ use crate::utils::sugg::Sugg; use crate::utils::{ - differing_macro_contexts, is_type_diagnostic_item, match_type, paths, snippet, span_lint_and_then, walk_ptrs_ty, - SpanlessEq, + differing_macro_contexts, is_type_diagnostic_item, match_type, paths, snippet_with_applicability, + span_lint_and_then, walk_ptrs_ty, SpanlessEq, }; use if_chain::if_chain; use matches::matches; @@ -105,8 +105,9 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { } } - let slice = check_for_slice(cx, lhs1, lhs2); + let mut applicability = Applicability::MachineApplicable; + let slice = check_for_slice(cx, lhs1, lhs2); let (replace, what, sugg) = if let Slice::NotSwappable = slice { return; } else if let Slice::Swappable(slice, idx1, idx2) = slice { @@ -117,8 +118,8 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { format!( "{}.swap({}, {})", slice.maybe_par(), - snippet(cx, idx1.span, ".."), - snippet(cx, idx2.span, ".."), + snippet_with_applicability(cx, idx1.span, "..", &mut applicability), + snippet_with_applicability(cx, idx2.span, "..", &mut applicability), ), ) } else { @@ -147,7 +148,7 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) { span, "try", sugg, - Applicability::Unspecified, + applicability, ); if replace { diff --git a/tests/ui/swap.fixed b/tests/ui/swap.fixed new file mode 100644 index 000000000000..82931bf91fdf --- /dev/null +++ b/tests/ui/swap.fixed @@ -0,0 +1,83 @@ +// run-rustfix + +#![warn(clippy::all)] +#![allow( + clippy::blacklisted_name, + clippy::no_effect, + clippy::redundant_clone, + redundant_semicolon, + unused_assignments +)] + +struct Foo(u32); + +#[derive(Clone)] +struct Bar { + a: u32, + b: u32, +} + +fn field() { + let mut bar = Bar { a: 1, b: 2 }; + + let temp = bar.a; + bar.a = bar.b; + bar.b = temp; + + let mut baz = vec![bar.clone(), bar.clone()]; + let temp = baz[0].a; + baz[0].a = baz[1].a; + baz[1].a = temp; +} + +fn array() { + let mut foo = [1, 2]; + foo.swap(0, 1); + + foo.swap(0, 1); +} + +fn slice() { + let foo = &mut [1, 2]; + foo.swap(0, 1); + + foo.swap(0, 1); +} + +fn unswappable_slice() { + let foo = &mut [vec![1, 2], vec![3, 4]]; + let temp = foo[0][1]; + foo[0][1] = foo[1][0]; + foo[1][0] = temp; + + // swap(foo[0][1], foo[1][0]) would fail +} + +fn vec() { + let mut foo = vec![1, 2]; + foo.swap(0, 1); + + foo.swap(0, 1); +} + +#[rustfmt::skip] +fn main() { + field(); + array(); + slice(); + unswappable_slice(); + vec(); + + let mut a = 42; + let mut b = 1337; + + std::mem::swap(&mut a, &mut b); + + ; std::mem::swap(&mut a, &mut b); + + let mut c = Foo(42); + + std::mem::swap(&mut c.0, &mut a); + + ; std::mem::swap(&mut c.0, &mut a); +} diff --git a/tests/ui/swap.rs b/tests/ui/swap.rs index f5e9182bed1e..4582db4b40e2 100644 --- a/tests/ui/swap.rs +++ b/tests/ui/swap.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::all)] #![allow( clippy::blacklisted_name, @@ -69,6 +71,7 @@ fn main() { field(); array(); slice(); + unswappable_slice(); vec(); let mut a = 42; diff --git a/tests/ui/swap.stderr b/tests/ui/swap.stderr index f3622230042b..f49bcfedf3a1 100644 --- a/tests/ui/swap.stderr +++ b/tests/ui/swap.stderr @@ -1,5 +1,5 @@ error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:33:5 + --> $DIR/swap.rs:35:5 | LL | / let temp = foo[0]; LL | | foo[0] = foo[1]; @@ -9,7 +9,7 @@ LL | | foo[1] = temp; = note: `-D clippy::manual-swap` implied by `-D warnings` error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:42:5 + --> $DIR/swap.rs:44:5 | LL | / let temp = foo[0]; LL | | foo[0] = foo[1]; @@ -17,7 +17,7 @@ LL | | foo[1] = temp; | |_________________^ help: try: `foo.swap(0, 1)` error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:60:5 + --> $DIR/swap.rs:62:5 | LL | / let temp = foo[0]; LL | | foo[0] = foo[1]; @@ -25,7 +25,7 @@ LL | | foo[1] = temp; | |_________________^ help: try: `foo.swap(0, 1)` error: this looks like you are swapping `a` and `b` manually - --> $DIR/swap.rs:80:7 + --> $DIR/swap.rs:83:7 | LL | ; let t = a; | _______^ @@ -36,7 +36,7 @@ LL | | b = t; = note: or maybe you should use `std::mem::replace`? error: this looks like you are swapping `c.0` and `a` manually - --> $DIR/swap.rs:89:7 + --> $DIR/swap.rs:92:7 | LL | ; let t = c.0; | _______^ @@ -47,7 +47,7 @@ LL | | a = t; = note: or maybe you should use `std::mem::replace`? error: this looks like you are trying to swap `a` and `b` - --> $DIR/swap.rs:77:5 + --> $DIR/swap.rs:80:5 | LL | / a = b; LL | | b = a; @@ -57,7 +57,7 @@ LL | | b = a; = note: or maybe you should use `std::mem::replace`? error: this looks like you are trying to swap `c.0` and `a` - --> $DIR/swap.rs:86:5 + --> $DIR/swap.rs:89:5 | LL | / c.0 = a; LL | | a = c.0; From aa2381d9d0c83ce4a2c0a6de6ac472d3fef4c7a1 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 3 Dec 2019 16:29:05 +0100 Subject: [PATCH 5/5] Fix rustdoc examples --- clippy_lints/src/swap.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index 77288869ce0e..b792423917ea 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -168,6 +168,7 @@ enum Slice<'a> { /// ## Example /// /// ```rust + /// # let mut a = vec![0, 1]; /// let t = a[1]; /// a[1] = a[0]; /// a[0] = t; @@ -180,6 +181,7 @@ enum Slice<'a> { /// ## Example /// /// ```rust + /// # let mut a = [vec![1, 2], vec![3, 4]]; /// let t = a[0][1]; /// a[0][1] = a[1][0]; /// a[1][0] = t;