diff --git a/CHANGELOG.md b/CHANGELOG.md index 601fa83ad89..acbefc8064d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2900,7 +2900,6 @@ Released 2018-09-13 [`wrong_pub_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_pub_self_convention [`wrong_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention [`wrong_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_transmute -[`xor_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#xor_swap [`zero_divided_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_divided_by_zero [`zero_prefixed_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_prefixed_literal [`zero_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_ptr diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c4197bab82a..f49b382c5ea 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -922,7 +922,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, swap::ALMOST_SWAPPED, swap::MANUAL_SWAP, - swap::XOR_SWAP, tabs_in_doc_comments::TABS_IN_DOC_COMMENTS, temporary_assignment::TEMPORARY_ASSIGNMENT, to_digit_is_some::TO_DIGIT_IS_SOME, @@ -1420,7 +1419,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap::ALMOST_SWAPPED), LintId::of(swap::MANUAL_SWAP), - LintId::of(swap::XOR_SWAP), LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT), LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME), @@ -1649,7 +1647,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(strings::STRING_FROM_UTF8_AS_BYTES), LintId::of(strlen_on_c_strings::STRLEN_ON_C_STRINGS), LintId::of(swap::MANUAL_SWAP), - LintId::of(swap::XOR_SWAP), LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT), LintId::of(transmute::CROSSPOINTER_TRANSMUTE), LintId::of(transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS), diff --git a/clippy_lints/src/swap.rs b/clippy_lints/src/swap.rs index 6dceec65574..f126908e84b 100644 --- a/clippy_lints/src/swap.rs +++ b/clippy_lints/src/swap.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::source::snippet_with_applicability; use clippy_utils::sugg::Sugg; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{differing_macro_contexts, eq_expr_value}; +use clippy_utils::{can_mut_borrow_both, differing_macro_contexts, eq_expr_value}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind}; @@ -10,7 +10,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Spanned; -use rustc_span::sym; +use rustc_span::{sym, Span}; declare_clippy_lint! { /// ### What it does @@ -65,37 +65,7 @@ declare_clippy_lint! { "`foo = bar; bar = foo` sequence" } -declare_clippy_lint! { - /// **What it does:** Checks for uses of xor-based swaps. - /// - /// **Why is this bad?** The `std::mem::swap` function exposes the intent better - /// without deinitializing or copying either variable. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// - /// ```rust - /// let mut a = 1; - /// let mut b = 2; - /// - /// a ^= b; - /// b ^= a; - /// a ^= b; - /// ``` - /// - /// Use std::mem::swap() instead: - /// ```rust - /// let mut a = 1; - /// let mut b = 2; - /// std::mem::swap(&mut a, &mut b); - /// ``` - pub XOR_SWAP, - complexity, - "xor-based swap of two variables" -} - -declare_lint_pass!(Swap => [MANUAL_SWAP, ALMOST_SWAPPED, XOR_SWAP]); +declare_lint_pass!(Swap => [MANUAL_SWAP, ALMOST_SWAPPED]); impl<'tcx> LateLintPass<'tcx> for Swap { fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) { @@ -105,6 +75,63 @@ impl<'tcx> LateLintPass<'tcx> for Swap { } } +fn generate_swap_warning(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>, span: Span, is_xor_based: bool) { + let mut applicability = Applicability::MachineApplicable; + + if !can_mut_borrow_both(cx, e1, e2) { + if let ExprKind::Index(lhs1, idx1) = e1.kind { + if let ExprKind::Index(lhs2, idx2) = e2.kind { + if eq_expr_value(cx, lhs1, lhs2) { + let ty = cx.typeck_results().expr_ty(lhs1).peel_refs(); + + if matches!(ty.kind(), ty::Slice(_)) + || matches!(ty.kind(), ty::Array(_, _)) + || is_type_diagnostic_item(cx, ty, sym::vec_type) + || is_type_diagnostic_item(cx, ty, sym::vecdeque_type) + { + let slice = Sugg::hir_with_applicability(cx, lhs1, "", &mut applicability); + span_lint_and_sugg( + cx, + MANUAL_SWAP, + span, + &format!("this looks like you are swapping elements of `{}` manually", slice), + "try", + format!( + "{}.swap({}, {})", + slice.maybe_par(), + snippet_with_applicability(cx, idx1.span, "..", &mut applicability), + snippet_with_applicability(cx, idx2.span, "..", &mut applicability), + ), + applicability, + ); + } + } + } + } + return; + } + + let first = Sugg::hir_with_applicability(cx, e1, "..", &mut applicability); + let second = Sugg::hir_with_applicability(cx, e2, "..", &mut applicability); + span_lint_and_then( + cx, + MANUAL_SWAP, + span, + &format!("this looks like you are swapping `{}` and `{}` manually", first, second), + |diag| { + diag.span_suggestion( + span, + "try", + format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()), + applicability, + ); + if !is_xor_based { + diag.note("or maybe you should use `std::mem::replace`?"); + } + }, + ); +} + /// Implementation of the `MANUAL_SWAP` lint. fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) { for w in block.stmts.windows(3) { @@ -128,123 +155,13 @@ fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) { if eq_expr_value(cx, tmp_init, lhs1); if eq_expr_value(cx, rhs1, lhs2); then { - if let ExprKind::Field(lhs1, _) = lhs1.kind { - if let ExprKind::Field(lhs2, _) = lhs2.kind { - if lhs1.hir_id.owner == lhs2.hir_id.owner { - return; - } - } - } - - 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_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()), - ) - } 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), - |diag| { - if !sugg.is_empty() { - diag.span_suggestion( - span, - "try", - sugg, - applicability, - ); - - if replace { - diag.note("or maybe you should use `std::mem::replace`?"); - } - } - } - ); + generate_swap_warning(cx, lhs1, lhs2, span, false); } } } } -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>, &'a Expr<'a>, &'a Expr<'a>), - /// 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(lhs1, idx1) = lhs1.kind { - if let ExprKind::Index(lhs2, idx2) = lhs2.kind { - if eq_expr_value(cx, lhs1, lhs2) { - let ty = cx.typeck_results().expr_ty(lhs1).peel_refs(); - - if matches!(ty.kind(), ty::Slice(_)) - || matches!(ty.kind(), ty::Array(_, _)) - || is_type_diagnostic_item(cx, ty, sym::vec_type) - || is_type_diagnostic_item(cx, ty, sym::vecdeque_type) - { - 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) { @@ -295,62 +212,20 @@ fn check_suspicious_swap(cx: &LateContext<'_>, block: &Block<'_>) { } } -/// Implementation of the `XOR_SWAP` lint. +/// Implementation of the xor case for `MANUAL_SWAP` lint. fn check_xor_swap(cx: &LateContext<'_>, block: &Block<'_>) { for window in block.stmts.windows(3) { if_chain! { if let Some((lhs0, rhs0)) = extract_sides_of_xor_assign(&window[0]); if let Some((lhs1, rhs1)) = extract_sides_of_xor_assign(&window[1]); if let Some((lhs2, rhs2)) = extract_sides_of_xor_assign(&window[2]); - if eq_expr_value(cx, lhs0, rhs1) - && eq_expr_value(cx, rhs1, lhs2) - && eq_expr_value(cx, rhs0, lhs1) - && eq_expr_value(cx, lhs1, rhs2); + if eq_expr_value(cx, lhs0, rhs1); + if eq_expr_value(cx, lhs2, rhs1); + if eq_expr_value(cx, lhs1, rhs0); + if eq_expr_value(cx, lhs1, rhs2); then { let span = window[0].span.to(window[2].span); - let mut applicability = Applicability::MachineApplicable; - match check_for_slice(cx, lhs0, rhs0) { - Slice::Swappable(slice, idx0, idx1) => { - if let Some(slice) = Sugg::hir_opt(cx, slice) { - span_lint_and_sugg( - cx, - XOR_SWAP, - span, - &format!( - "this xor-based swap of the elements of `{}` can be \ - more clearly expressed using `.swap`", - slice - ), - "try", - format!( - "{}.swap({}, {})", - slice.maybe_par(), - snippet_with_applicability(cx, idx0.span, "..", &mut applicability), - snippet_with_applicability(cx, idx1.span, "..", &mut applicability) - ), - applicability - ) - } - } - Slice::None => { - if let (Some(first), Some(second)) = (Sugg::hir_opt(cx, lhs0), Sugg::hir_opt(cx, rhs0)) { - span_lint_and_sugg( - cx, - XOR_SWAP, - span, - &format!( - "this xor-based swap of `{}` and `{}` can be \ - more clearly expressed using `std::mem::swap`", - first, second - ), - "try", - format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()), - applicability, - ); - } - } - Slice::NotSwappable => {} - } + generate_swap_warning(cx, lhs0, rhs0, span, true); } }; } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 59f878f8b20..e930338270c 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -558,6 +558,54 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio None } +/// This method will return tuple of projection stack and root of the expression, +/// used in `can_mut_borrow_both`. +/// +/// For example, if `e` represents the `v[0].a.b[x]` +/// this method will return a tuple, composed of a `Vec` +/// containing the `Expr`s for `v[0], v[0].a, v[0].a.b, v[0].a.b[x]` +/// and a `Expr` for root of them, `v` +fn projection_stack<'a, 'hir>(mut e: &'a Expr<'hir>) -> (Vec<&'a Expr<'hir>>, &'a Expr<'hir>) { + let mut result = vec![]; + let root = loop { + match e.kind { + ExprKind::Index(ep, _) | ExprKind::Field(ep, _) => { + result.push(e); + e = ep; + }, + _ => break e, + }; + }; + result.reverse(); + (result, root) +} + +/// Checks if two expressions can be mutably borrowed simultaneously +/// and they aren't dependent on borrowing same thing twice +pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -> bool { + let (s1, r1) = projection_stack(e1); + let (s2, r2) = projection_stack(e2); + if !eq_expr_value(cx, r1, r2) { + return true; + } + for (x1, x2) in s1.iter().zip(s2.iter()) { + match (&x1.kind, &x2.kind) { + (ExprKind::Field(_, i1), ExprKind::Field(_, i2)) => { + if i1 != i2 { + return true; + } + }, + (ExprKind::Index(_, i1), ExprKind::Index(_, i2)) => { + if !eq_expr_value(cx, i1, i2) { + return false; + } + }, + _ => return false, + } + } + false +} + /// Checks if the top level expression can be moved into a closure as is. pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool { match expr.kind { diff --git a/tests/ui/swap.fixed b/tests/ui/swap.fixed index e0d412b2198..ef518359ec5 100644 --- a/tests/ui/swap.fixed +++ b/tests/ui/swap.fixed @@ -6,6 +6,7 @@ clippy::no_effect, clippy::redundant_clone, redundant_semicolons, + dead_code, unused_assignments )] @@ -20,9 +21,7 @@ struct Bar { fn field() { let mut bar = Bar { a: 1, b: 2 }; - let temp = bar.a; - bar.a = bar.b; - bar.b = temp; + std::mem::swap(&mut bar.a, &mut bar.b); let mut baz = vec![bar.clone(), bar.clone()]; let temp = baz[0].a; @@ -51,6 +50,7 @@ fn unswappable_slice() { foo[1][0] = temp; // swap(foo[0][1], foo[1][0]) would fail + // this could use split_at_mut and mem::swap, but that is not much simpler. } fn vec() { @@ -95,20 +95,19 @@ fn xor_unswappable_slice() { foo[0][1] ^= foo[1][0]; foo[1][0] ^= foo[0][0]; foo[0][1] ^= foo[1][0]; + + // swap(foo[0][1], foo[1][0]) would fail + // this could use split_at_mut and mem::swap, but that is not much simpler. +} + +fn distinct_slice() { + let foo = &mut [vec![1, 2], vec![3, 4]]; + let bar = &mut [vec![1, 2], vec![3, 4]]; + std::mem::swap(&mut foo[0][1], &mut bar[1][0]); } #[rustfmt::skip] fn main() { - field(); - array(); - slice(); - unswappable_slice(); - vec(); - xor_swap_locals(); - xor_field_swap(); - xor_slice_swap(); - xor_no_swap(); - xor_unswappable_slice(); let mut a = 42; let mut b = 1337; diff --git a/tests/ui/swap.rs b/tests/ui/swap.rs index 6f8fd6e4c91..8518659ccf3 100644 --- a/tests/ui/swap.rs +++ b/tests/ui/swap.rs @@ -6,6 +6,7 @@ clippy::no_effect, clippy::redundant_clone, redundant_semicolons, + dead_code, unused_assignments )] @@ -55,6 +56,7 @@ fn unswappable_slice() { foo[1][0] = temp; // swap(foo[0][1], foo[1][0]) would fail + // this could use split_at_mut and mem::swap, but that is not much simpler. } fn vec() { @@ -107,20 +109,21 @@ fn xor_unswappable_slice() { foo[0][1] ^= foo[1][0]; foo[1][0] ^= foo[0][0]; foo[0][1] ^= foo[1][0]; + + // swap(foo[0][1], foo[1][0]) would fail + // this could use split_at_mut and mem::swap, but that is not much simpler. +} + +fn distinct_slice() { + let foo = &mut [vec![1, 2], vec![3, 4]]; + let bar = &mut [vec![1, 2], vec![3, 4]]; + let temp = foo[0][1]; + foo[0][1] = bar[1][0]; + bar[1][0] = temp; } #[rustfmt::skip] fn main() { - field(); - array(); - slice(); - unswappable_slice(); - vec(); - xor_swap_locals(); - xor_field_swap(); - xor_slice_swap(); - xor_no_swap(); - xor_unswappable_slice(); let mut a = 42; let mut b = 1337; diff --git a/tests/ui/swap.stderr b/tests/ui/swap.stderr index 55b470821e2..614d16ced40 100644 --- a/tests/ui/swap.stderr +++ b/tests/ui/swap.stderr @@ -1,15 +1,16 @@ -error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:35:5 +error: this looks like you are swapping `bar.a` and `bar.b` manually + --> $DIR/swap.rs:24:5 | -LL | / let temp = foo[0]; -LL | | foo[0] = foo[1]; -LL | | foo[1] = temp; - | |_________________^ help: try: `foo.swap(0, 1)` +LL | / let temp = bar.a; +LL | | bar.a = bar.b; +LL | | bar.b = temp; + | |________________^ help: try: `std::mem::swap(&mut bar.a, &mut bar.b)` | = note: `-D clippy::manual-swap` implied by `-D warnings` + = note: or maybe you should use `std::mem::replace`? error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:44:5 + --> $DIR/swap.rs:36:5 | LL | / let temp = foo[0]; LL | | foo[0] = foo[1]; @@ -17,41 +18,57 @@ LL | | foo[1] = temp; | |_________________^ help: try: `foo.swap(0, 1)` error: this looks like you are swapping elements of `foo` manually - --> $DIR/swap.rs:62:5 + --> $DIR/swap.rs:45:5 | LL | / let temp = foo[0]; LL | | foo[0] = foo[1]; LL | | foo[1] = temp; | |_________________^ help: try: `foo.swap(0, 1)` -error: this xor-based swap of `a` and `b` can be more clearly expressed using `std::mem::swap` - --> $DIR/swap.rs:73:5 +error: this looks like you are swapping elements of `foo` manually + --> $DIR/swap.rs:64:5 + | +LL | / let temp = foo[0]; +LL | | foo[0] = foo[1]; +LL | | foo[1] = temp; + | |_________________^ help: try: `foo.swap(0, 1)` + +error: this looks like you are swapping `a` and `b` manually + --> $DIR/swap.rs:75:5 | LL | / a ^= b; LL | | b ^= a; LL | | a ^= b; | |___________^ help: try: `std::mem::swap(&mut a, &mut b)` - | - = note: `-D clippy::xor-swap` implied by `-D warnings` -error: this xor-based swap of `bar.a` and `bar.b` can be more clearly expressed using `std::mem::swap` - --> $DIR/swap.rs:81:5 +error: this looks like you are swapping `bar.a` and `bar.b` manually + --> $DIR/swap.rs:83:5 | LL | / bar.a ^= bar.b; LL | | bar.b ^= bar.a; LL | | bar.a ^= bar.b; | |___________________^ help: try: `std::mem::swap(&mut bar.a, &mut bar.b)` -error: this xor-based swap of the elements of `foo` can be more clearly expressed using `.swap` - --> $DIR/swap.rs:89:5 +error: this looks like you are swapping elements of `foo` manually + --> $DIR/swap.rs:91:5 | LL | / foo[0] ^= foo[1]; LL | | foo[1] ^= foo[0]; LL | | foo[0] ^= foo[1]; | |_____________________^ help: try: `foo.swap(0, 1)` +error: this looks like you are swapping `foo[0][1]` and `bar[1][0]` manually + --> $DIR/swap.rs:120:5 + | +LL | / let temp = foo[0][1]; +LL | | foo[0][1] = bar[1][0]; +LL | | bar[1][0] = temp; + | |____________________^ help: try: `std::mem::swap(&mut foo[0][1], &mut bar[1][0])` + | + = note: or maybe you should use `std::mem::replace`? + error: this looks like you are swapping `a` and `b` manually - --> $DIR/swap.rs:131:7 + --> $DIR/swap.rs:134:7 | LL | ; let t = a; | _______^ @@ -62,7 +79,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:140:7 + --> $DIR/swap.rs:143:7 | LL | ; let t = c.0; | _______^ @@ -73,7 +90,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:128:5 + --> $DIR/swap.rs:131:5 | LL | / a = b; LL | | b = a; @@ -83,7 +100,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:137:5 + --> $DIR/swap.rs:140:5 | LL | / c.0 = a; LL | | a = c.0; @@ -91,5 +108,5 @@ LL | | a = c.0; | = note: or maybe you should use `std::mem::replace`? -error: aborting due to 10 previous errors +error: aborting due to 12 previous errors