diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index b278171abce..b792423917e 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; @@ -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,49 +105,115 @@ 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 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 { 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_with_applicability(cx, idx1.span, "..", &mut applicability), + snippet_with_applicability(cx, idx2.span, "..", &mut applicability), + ), + ) } 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, + ); - if replace { - db.note("or maybe you should use `std::mem::replace`?"); - } - } - }); + if replace { + db.note("or maybe you should use `std::mem::replace`?"); + } + } + } + ); } } } } +enum Slice<'a> { + /// `slice.swap(idx1, idx2)` can be used + /// + /// ## Example + /// + /// ```rust + /// # let mut a = vec![0, 1]; + /// 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 mut a = [vec![1, 2], vec![3, 4]]; + /// 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) { diff --git a/tests/ui/swap.fixed b/tests/ui/swap.fixed new file mode 100644 index 00000000000..82931bf91fd --- /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 b508c1ee009..4582db4b40e 100644 --- a/tests/ui/swap.rs +++ b/tests/ui/swap.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::all)] #![allow( clippy::blacklisted_name, @@ -46,6 +48,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]; @@ -60,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 b45187b5805..f49bcfedf3a 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:51: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:71: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:80: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:68: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:77:5 + --> $DIR/swap.rs:89:5 | LL | / c.0 = a; LL | | a = c.0;