add xor-swap lint

Hrishi Dharam · HKalbasi · commit dbb10b87f8e4 · 2021-07-29T15:25:08.000+04:30
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2900,6 +2900,7 @@ 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
@@ -922,6 +922,7 @@ 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,
@@ -1419,6 +1420,7 @@ 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),
@@ -1647,6 +1649,7 @@ 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
@@ -1,14 +1,15 @@
-use clippy_utils::diagnostics::span_lint_and_then;
+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 if_chain::if_chain;
 use rustc_errors::Applicability;
-use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, StmtKind};
+use rustc_hir::{BinOpKind, Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind};
 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;
 
 declare_clippy_lint! {
@@ -64,12 +65,43 @@ declare_clippy_lint! {
     "`foo = bar; bar = foo` sequence"
 }
 
-declare_lint_pass!(Swap => [MANUAL_SWAP, ALMOST_SWAPPED]);
+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]);
 
 impl<'tcx> LateLintPass<'tcx> for Swap {
     fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
         check_manual_swap(cx, block);
         check_suspicious_swap(cx, block);
+        check_xor_swap(cx, block);
     }
 }
 
@@ -262,3 +294,82 @@ fn check_suspicious_swap(cx: &LateContext<'_>, block: &Block<'_>) {
         }
     }
 }
+
+/// Implementation of the `XOR_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);
+            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 => {}
+                }
+            }
+        };
+    }
+}
+
+/// Returns the lhs and rhs of an xor assignment statement.  
+fn extract_sides_of_xor_assign<'a, 'hir>(stmt: &'a Stmt<'hir>) -> Option<(&'a Expr<'hir>, &'a Expr<'hir>)> {
+    if let StmtKind::Semi(expr) = stmt.kind {
+        if let ExprKind::AssignOp(
+            Spanned {
+                node: BinOpKind::BitXor,
+                ..
+            },
+            lhs,
+            rhs,
+        ) = expr.kind
+        {
+            return Some((lhs, rhs));
+        }
+    }
+    None
+}
diff --git a/tests/ui/swap.fixed b/tests/ui/swap.fixed
@@ -60,13 +60,55 @@ fn vec() {
     foo.swap(0, 1);
 }
 
+fn xor_swap_locals() {
+    // This is an xor-based swap of local variables.
+    let mut a = 0;
+    let mut b = 1;
+    std::mem::swap(&mut a, &mut b)
+}
+
+fn xor_field_swap() {
+    // This is an xor-based swap of fields in a struct.
+    let mut bar = Bar { a: 0, b: 1 };
+    std::mem::swap(&mut bar.a, &mut bar.b)
+}
+
+fn xor_slice_swap() {
+    // This is an xor-based swap of a slice
+    let foo = &mut [1, 2];
+    foo.swap(0, 1)
+}
+
+fn xor_no_swap() {
+    // This is a sequence of xor-assignment statements that doesn't result in a swap.
+    let mut a = 0;
+    let mut b = 1;
+    let mut c = 2;
+    a ^= b;
+    b ^= c;
+    a ^= c;
+    c ^= a;
+}
+
+fn xor_unswappable_slice() {
+    let foo = &mut [vec![1, 2], vec![3, 4]];
+    foo[0][1] ^= foo[1][0];
+    foo[1][0] ^= foo[0][0];
+    foo[0][1] ^= foo[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
@@ -66,13 +66,61 @@ fn vec() {
     foo.swap(0, 1);
 }
 
+fn xor_swap_locals() {
+    // This is an xor-based swap of local variables.
+    let mut a = 0;
+    let mut b = 1;
+    a ^= b;
+    b ^= a;
+    a ^= b;
+}
+
+fn xor_field_swap() {
+    // This is an xor-based swap of fields in a struct.
+    let mut bar = Bar { a: 0, b: 1 };
+    bar.a ^= bar.b;
+    bar.b ^= bar.a;
+    bar.a ^= bar.b;
+}
+
+fn xor_slice_swap() {
+    // This is an xor-based swap of a slice
+    let foo = &mut [1, 2];
+    foo[0] ^= foo[1];
+    foo[1] ^= foo[0];
+    foo[0] ^= foo[1];
+}
+
+fn xor_no_swap() {
+    // This is a sequence of xor-assignment statements that doesn't result in a swap.
+    let mut a = 0;
+    let mut b = 1;
+    let mut c = 2;
+    a ^= b;
+    b ^= c;
+    a ^= c;
+    c ^= a;
+}
+
+fn xor_unswappable_slice() {
+    let foo = &mut [vec![1, 2], vec![3, 4]];
+    foo[0][1] ^= foo[1][0];
+    foo[1][0] ^= foo[0][0];
+    foo[0][1] ^= foo[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.stderr b/tests/ui/swap.stderr
@@ -24,8 +24,34 @@ 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
+   |
+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
+   |
+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
+   |
+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 `a` and `b` manually
-  --> $DIR/swap.rs:83:7
+  --> $DIR/swap.rs:131:7
    |
 LL |       ; let t = a;
    |  _______^
@@ -36,7 +62,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:92:7
+  --> $DIR/swap.rs:140:7
    |
 LL |       ; let t = c.0;
    |  _______^
@@ -47,7 +73,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:80:5
+  --> $DIR/swap.rs:128:5
    |
 LL | /     a = b;
 LL | |     b = a;
@@ -57,13 +83,13 @@ 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:89:5
+  --> $DIR/swap.rs:137:5
    |
 LL | /     c.0 = a;
 LL | |     a = c.0;
    | |___________^ help: try: `std::mem::swap(&mut c.0, &mut a)`
    |
    = note: or maybe you should use `std::mem::replace`?
 
-error: aborting due to 7 previous errors
+error: aborting due to 10 previous errors
 
