Auto merge of #10177 - chansuke:almost_swapped, r=Alexendoo

Almost swapped

Take over from #8945

Fix #8151

---

changelog: enhancement: [`almost_swapped`]: Now detects almost swaps using `let` statements
[#10177](#10177)
<!-- changelog_checked -->

Author: bors

clippy_lints/src/swap.rs

@@ -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, Span};
+use rustc_span::{sym, symbol::Ident, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -174,53 +174,74 @@ fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) {
 
 /// Implementation of the `ALMOST_SWAPPED` lint.
 fn check_suspicious_swap(cx: &LateContext<'_>, block: &Block<'_>) {
-    for w in block.stmts.windows(2) {
-        if_chain! {
-            if let StmtKind::Semi(first) = w[0].kind;
-            if let StmtKind::Semi(second) = w[1].kind;
-            if first.span.ctxt() == second.span.ctxt();
-            if let ExprKind::Assign(lhs0, rhs0, _) = first.kind;
-            if let ExprKind::Assign(lhs1, rhs1, _) = second.kind;
-            if eq_expr_value(cx, lhs0, rhs1);
-            if eq_expr_value(cx, lhs1, rhs0);
-            then {
-                let lhs0 = Sugg::hir_opt(cx, lhs0);
-                let rhs0 = Sugg::hir_opt(cx, rhs0);
-                let (what, lhs, rhs) = if let (Some(first), Some(second)) = (lhs0, rhs0) {
-                    (
-                        format!(" `{first}` and `{second}`"),
-                        first.mut_addr().to_string(),
-                        second.mut_addr().to_string(),
-                    )
-                } else {
-                    (String::new(), String::new(), String::new())
-                };
+    for [first, second] in block.stmts.array_windows() {
+        if let Some((lhs0, rhs0)) = parse(first)
+            && let Some((lhs1, rhs1)) = parse(second)
+            && first.span.eq_ctxt(second.span)
+            && is_same(cx, lhs0, rhs1)
+            && is_same(cx, lhs1, rhs0)
+            && let Some(lhs_sugg) = match &lhs0 {
+                ExprOrIdent::Expr(expr) => Sugg::hir_opt(cx, expr),
+                ExprOrIdent::Ident(ident) => Some(Sugg::NonParen(ident.as_str().into())),
+            }
+            && let Some(rhs_sugg) = Sugg::hir_opt(cx, rhs0)
+        {
+            let span = first.span.to(rhs1.span);
+            let Some(sugg) = std_or_core(cx) else { return };
+            span_lint_and_then(
+                cx,
+                ALMOST_SWAPPED,
+                span,
+                &format!("this looks like you are trying to swap `{lhs_sugg}` and `{rhs_sugg}`"),
+                |diag| {
+                    diag.span_suggestion(
+                        span,
+                        "try",
+                        format!("{sugg}::mem::swap({}, {})", lhs_sugg.mut_addr(), rhs_sugg.mut_addr()),
+                        Applicability::MaybeIncorrect,
+                    );
+                    diag.note(format!("or maybe you should use `{sugg}::mem::replace`?"));
+                },
+            );
+        }
+    }
+}
+
+fn is_same(cx: &LateContext<'_>, lhs: ExprOrIdent<'_>, rhs: &Expr<'_>) -> bool {
+    match lhs {
+        ExprOrIdent::Expr(expr) => eq_expr_value(cx, expr, rhs),
+        ExprOrIdent::Ident(ident) => {
+            if let ExprKind::Path(QPath::Resolved(None, path)) = rhs.kind
+                && let [segment] = &path.segments
+                && segment.ident == ident
+            {
+                true
+            } else {
+                false
+            }
+        }
+    }
+}
 
-                let span = first.span.to(second.span);
-                let Some(sugg) = std_or_core(cx) else { return };
+#[derive(Debug, Clone, Copy)]
+enum ExprOrIdent<'a> {
+    Expr(&'a Expr<'a>),
+    Ident(Ident),
+}
 
-                span_lint_and_then(cx,
-                    ALMOST_SWAPPED,
-                    span,
-                    &format!("this looks like you are trying to swap{what}"),
-                    |diag| {
-                        if !what.is_empty() {
-                            diag.span_suggestion(
-                                span,
-                                "try",
-                                format!(
-                                    "{sugg}::mem::swap({lhs}, {rhs})",
-                                ),
-                                Applicability::MaybeIncorrect,
-                            );
-                            diag.note(
-                                format!("or maybe you should use `{sugg}::mem::replace`?")
-                            );
-                        }
-                    });
+fn parse<'a, 'hir>(stmt: &'a Stmt<'hir>) -> Option<(ExprOrIdent<'hir>, &'a Expr<'hir>)> {
+    if let StmtKind::Semi(expr) = stmt.kind {
+        if let ExprKind::Assign(lhs, rhs, _) = expr.kind {
+            return Some((ExprOrIdent::Expr(lhs), rhs));
+        }
+    } else if let StmtKind::Local(expr) = stmt.kind {
+        if let Some(rhs) = expr.init {
+            if let PatKind::Binding(_, _, ident_l, _) = expr.pat.kind {
+                return Some((ExprOrIdent::Ident(ident_l), rhs));
             }
         }
     }
+    None
 }
 
 /// Implementation of the xor case for `MANUAL_SWAP` lint.

tests/ui/swap.fixed

@@ -7,7 +7,8 @@
     clippy::redundant_clone,
     redundant_semicolons,
     dead_code,
-    unused_assignments
+    unused_assignments,
+    unused_variables
 )]
 
 struct Foo(u32);
@@ -121,6 +122,27 @@ fn main() {
     std::mem::swap(&mut c.0, &mut a);
 
     ; std::mem::swap(&mut c.0, &mut a);
+
+    std::mem::swap(&mut a, &mut b);
+
+    let mut c = 1;
+    let mut d = 2;
+    std::mem::swap(&mut d, &mut c);
+
+    let mut b = 1;
+    std::mem::swap(&mut a, &mut b);
+
+    let b = 1;
+    let a = 2;
+
+    let t = b;
+    let b = a;
+    let a = t;
+
+    let mut b = 1;
+    let mut a = 2;
+
+    std::mem::swap(&mut b, &mut a);
 }
 
 fn issue_8154() {

tests/ui/swap.rs

@@ -7,7 +7,8 @@
     clippy::redundant_clone,
     redundant_semicolons,
     dead_code,
-    unused_assignments
+    unused_assignments,
+    unused_variables
 )]
 
 struct Foo(u32);
@@ -143,6 +144,32 @@ fn main() {
     ; let t = c.0;
     c.0 = a;
     a = t;
+
+    let a = b;
+    let b = a;
+
+    let mut c = 1;
+    let mut d = 2;
+    d = c;
+    c = d;
+
+    let mut b = 1;
+    let a = b;
+    b = a;
+
+    let b = 1;
+    let a = 2;
+
+    let t = b;
+    let b = a;
+    let a = t;
+
+    let mut b = 1;
+    let mut a = 2;
+
+    let t = b;
+    b = a;
+    a = t;
 }
 
 fn issue_8154() {

tests/ui/swap.stderr

@@ -1,5 +1,5 @@
 error: this looks like you are swapping `bar.a` and `bar.b` manually
-  --> $DIR/swap.rs:24:5
+  --> $DIR/swap.rs:25:5
    |
 LL | /     let temp = bar.a;
 LL | |     bar.a = bar.b;
@@ -10,55 +10,55 @@ LL | |     bar.b = temp;
    = note: `-D clippy::manual-swap` implied by `-D warnings`
 
 error: this looks like you are swapping elements of `foo` manually
-  --> $DIR/swap.rs:36:5
+  --> $DIR/swap.rs:37: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 elements of `foo` manually
-  --> $DIR/swap.rs:45:5
+  --> $DIR/swap.rs:46: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 elements of `foo` manually
-  --> $DIR/swap.rs:64:5
+  --> $DIR/swap.rs:65: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
+  --> $DIR/swap.rs:76:5
    |
 LL | /     a ^= b;
 LL | |     b ^= a;
 LL | |     a ^= b;
    | |___________^ help: try: `std::mem::swap(&mut a, &mut b)`
 
 error: this looks like you are swapping `bar.a` and `bar.b` manually
-  --> $DIR/swap.rs:83:5
+  --> $DIR/swap.rs:84: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 looks like you are swapping elements of `foo` manually
-  --> $DIR/swap.rs:91:5
+  --> $DIR/swap.rs:92: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
+  --> $DIR/swap.rs:121:5
    |
 LL | /     let temp = foo[0][1];
 LL | |     foo[0][1] = bar[1][0];
@@ -68,7 +68,7 @@ LL | |     bar[1][0] = temp;
    = note: or maybe you should use `std::mem::replace`?
 
 error: this looks like you are swapping `a` and `b` manually
-  --> $DIR/swap.rs:134:7
+  --> $DIR/swap.rs:135:7
    |
 LL |       ; let t = a;
    |  _______^
@@ -79,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:143:7
+  --> $DIR/swap.rs:144:7
    |
 LL |       ; let t = c.0;
    |  _______^
@@ -89,8 +89,18 @@ LL | |     a = t;
    |
    = note: or maybe you should use `std::mem::replace`?
 
+error: this looks like you are swapping `b` and `a` manually
+  --> $DIR/swap.rs:170:5
+   |
+LL | /     let t = b;
+LL | |     b = a;
+LL | |     a = t;
+   | |_________^ help: try: `std::mem::swap(&mut b, &mut a)`
+   |
+   = note: or maybe you should use `std::mem::replace`?
+
 error: this looks like you are trying to swap `a` and `b`
-  --> $DIR/swap.rs:131:5
+  --> $DIR/swap.rs:132:5
    |
 LL | /     a = b;
 LL | |     b = a;
@@ -100,16 +110,43 @@ LL | |     b = a;
    = note: `-D clippy::almost-swapped` implied by `-D warnings`
 
 error: this looks like you are trying to swap `c.0` and `a`
-  --> $DIR/swap.rs:140:5
+  --> $DIR/swap.rs:141: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: this looks like you are trying to swap `a` and `b`
+  --> $DIR/swap.rs:148:5
+   |
+LL | /     let a = b;
+LL | |     let b = a;
+   | |_____________^ help: try: `std::mem::swap(&mut a, &mut b)`
+   |
+   = note: or maybe you should use `std::mem::replace`?
+
+error: this looks like you are trying to swap `d` and `c`
+  --> $DIR/swap.rs:153:5
+   |
+LL | /     d = c;
+LL | |     c = d;
+   | |_________^ help: try: `std::mem::swap(&mut d, &mut c)`
+   |
+   = note: or maybe you should use `std::mem::replace`?
+
+error: this looks like you are trying to swap `a` and `b`
+  --> $DIR/swap.rs:157:5
+   |
+LL | /     let a = b;
+LL | |     b = a;
+   | |_________^ help: try: `std::mem::swap(&mut a, &mut b)`
+   |
+   = note: or maybe you should use `std::mem::replace`?
+
 error: this looks like you are swapping `s.0.x` and `s.0.y` manually
-  --> $DIR/swap.rs:178:5
+  --> $DIR/swap.rs:205:5
    |
 LL | /     let t = s.0.x;
 LL | |     s.0.x = s.0.y;
@@ -118,5 +155,5 @@ LL | |     s.0.y = t;
    |
    = note: or maybe you should use `std::mem::replace`?
 
-error: aborting due to 13 previous errors
+error: aborting due to 17 previous errors