New lint: `swap_with_temporary` #14046

samueltardieu · 2025-01-20T13:34:24Z

This lint detects inefficient or useless {std,core}::mem::swap() calls such as:

    // Should be `a = temp();`
    swap(&mut a, &mut temp());
    // Should be `*b = temp();`
    swap(b, &mut temp());
    // Should be `temp1(); temp2();` if we want to keep the side effects
    swap(&mut temp1(), &mut temp2());

It also takes care of using a form appropriate for a () context if swap() is part of a larger expression (don't ask me why this wouldn't happen, I have no idea), by suggesting { x = y; } (statement in block) or {std,core}::mem::drop((temp1(), temp2()).

changelog: [swap_with_temporary]: new lint

Close #1968

rustbot · 2025-01-20T13:34:30Z

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

samueltardieu · 2025-01-27T22:27:21Z

Rebased

samueltardieu · 2025-01-30T20:54:41Z

Rebased.

samueltardieu · 2025-02-12T14:33:35Z

Rebased

samueltardieu · 2025-02-12T16:32:16Z

Rebased

y21 · 2025-02-15T10:49:50Z

clippy_lints/src/methods/swap_with_temporary.rs

+        if target.span.from_expansion() {
+            ArgKind::FromExpansion
+        } else if target.is_syntactic_place_expr() {


Are there any cases where linting if the borrowed value is from an expansion would result in FPs? Not linting in this case prevents linting something like swap(&mut v, &mut vec![1]); which seems like it could be useful

I was worried that some macros may sometimes return a temporary value and some other times return a non-temporary value. But indeed, if I don't check this, many more cases get linted.

I'll do a test run where I remove the expansion check in a second commit just to show how it behaves, and to get lintcheck results.

Ok, that went well, I've squashed the commits. This lint now supports expanded arguments as well, thanks for the idea.

y21 · 2025-02-15T11:14:51Z

clippy_lints/src/methods/mod.rs

+    /// ```
+    #[clippy::version = "1.86.0"]
+    pub SWAP_WITH_TEMPORARY,
+    perf,


This seems more like a complexity lint to me since it's just an unnecessarily complicated way to assign or drop. Does this actually make a difference in performance?

Probably not, at least not a significant one. I'll switch it to complexity.

samueltardieu · 2025-02-21T22:32:49Z

Rebased

samueltardieu · 2025-02-23T11:17:54Z

@y21 Anything else needs to be done?

samueltardieu · 2025-02-28T23:01:32Z

Rebased

samueltardieu · 2025-03-16T22:58:03Z

@y21 should I reassign?

y21 · 2025-03-18T00:41:15Z

Sorry, I've been meaning to get to this. I'll try to look into this again in the next few days!

y21 · 2025-03-18T16:39:05Z

clippy_lints/src/methods/swap_with_temporary.rs

+                        (left_temp.span.between(right_temp.span), String::from("; ")),
+                        (expr.span.with_lo(right_temp.span.hi()), String::new()),
+                    ],
+                    Applicability::MachineApplicable,


I'm not sure if this should be MachineApplicable. I've not seen code like it before but I feel like swapping two temporaries would just be a bug and it doesn't make any sense to write. So if the user gets this lint there's a chance that they might want to double check, perhaps they did want to actually swap something and the lint helped uncover a bug.

Done. The lint now suggests an autofix only when this is:

an inefficient way of doing an assignment;

and the source of the assignment can be found in source without being dereferenced (otherwise we might dereference a temporary value and get an error);

and the swap() was a statement or the last expression in a block.

All cases are still linted, but do not suggest an autofix. Also, the note has been specialized: &mut expr will indicate that expr is a temporary expression, while another (non &mut separable) expression will indicate that the expression is a mutable reference to a temporary expression.

y21 · 2025-03-18T17:44:28Z

tests/ui/swap_with_temporary.rs

+    swap(&mut mac!(funcall func), z);
+    //~^ ERROR: swapping with a temporary value is inefficient


Can you also add a test where both swap arguments are macro calls, i.e. swap(&mut mac!(funcall func), &mut mac!(funcall func))

y21 · 2025-03-18T17:47:45Z

clippy_lints/src/methods/mod.rs

+    ///     *s = String::from("replaced");
+    /// }
+    /// ```
+    #[clippy::version = "1.86.0"]


Outdated now; don't need to change this yet since we still have the FCP waiting period, but I'm creating this comment as a reminder for later (also for myself) to make sure it's up to date before merging

y21 · 2025-03-18T18:21:44Z

clippy_lints/src/methods/mod.rs

+    /// Storing a new value in place of a temporary value which will
+    /// be dropped right after the `swap` is inefficient. The same
+    /// result can be achieved by using an assignment, or dropping
+    /// the swap arguments.


Given that this is now a complexity lint, maybe also specifically mention that this is an unnecessarily complex/harder-to-read way of assigning, and that swapping two temporary values is possibly also a mistake/bug.

We could also add an example for a bug that this catches where the user accidentally has one layer of references too much and (perhaps unknowingly) is swapping references rather than the referent

fn bug(v1: &mut [i32], v2: &mut [i32]) { // Incorrect: swapping temporary references (`&mut &mut` passed to swap) std::mem::swap(&mut v1.last_mut().unwrap(), &mut v2.last_mut().unwrap()); // Correct std::mem::swap(v1.last_mut().unwrap(), v2.last_mut().unwrap()); }

(Would also be an example for my earlier comment that this maybe shouldn't be MachineApplicable)

y21 · 2025-03-18T18:25:18Z

clippy_lints/src/methods/swap_with_temporary.rs

+            // If the `swap()` is a statement by itself, propose to replace it by `a = b`. Otherwise, when part
+            // of a larger expression, surround the assignment with a block to make it `()`.


Assignment expressions also always evaluate to () so I don't think wrapping it in a block is needed?

Removed, as we only consider the statement and latest expression in a block when autofixing. Other cases require manual examination, as they have probably been written for a reason.

y21 · 2025-03-18T18:50:14Z

tests/ui/swap_with_temporary.rs

+    swap(&mut func(), &mut y);
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    swap(&mut x, &mut func());


This shouldn't be an issue with the current implementation but I think it would be good to have some other projection test cases that create places to future-proof where it shouldn't lint, like indexing (swap(&mut slice1[0], &mut slice2[0])) and field access (swap(&mut s1.x, &mut s2.x)).

rustbot assigned y21 Jan 20, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 20, 2025

samueltardieu force-pushed the push-nkqqkpnrplvv branch 4 times, most recently from befa2dc to b84eb55 Compare January 20, 2025 16:59

samueltardieu force-pushed the push-nkqqkpnrplvv branch from b84eb55 to 57f3cfb Compare January 27, 2025 22:27

samueltardieu force-pushed the push-nkqqkpnrplvv branch from 57f3cfb to 19ddfe2 Compare January 30, 2025 20:54

samueltardieu force-pushed the push-nkqqkpnrplvv branch from 19ddfe2 to 67d0f0e Compare February 12, 2025 14:33

samueltardieu force-pushed the push-nkqqkpnrplvv branch from 67d0f0e to b6a3a75 Compare February 12, 2025 16:32

y21 reviewed Feb 15, 2025

View reviewed changes

samueltardieu force-pushed the push-nkqqkpnrplvv branch 5 times, most recently from 4e6fe44 to 3016c0b Compare February 21, 2025 22:32

samueltardieu force-pushed the push-nkqqkpnrplvv branch from 3016c0b to 76e6421 Compare February 28, 2025 23:01

y21 reviewed Mar 18, 2025

View reviewed changes

New lint: swap_with_temporary

ed73e9d

samueltardieu force-pushed the push-nkqqkpnrplvv branch from 76e6421 to ed73e9d Compare March 19, 2025 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: `swap_with_temporary` #14046

New lint: `swap_with_temporary` #14046

samueltardieu commented Jan 20, 2025 •

edited

Loading

rustbot commented Jan 20, 2025

samueltardieu commented Jan 27, 2025

samueltardieu commented Jan 30, 2025

samueltardieu commented Feb 12, 2025

samueltardieu commented Feb 12, 2025

y21 Feb 15, 2025

samueltardieu Feb 15, 2025

samueltardieu Feb 15, 2025

y21 Feb 15, 2025

samueltardieu Feb 15, 2025

samueltardieu commented Feb 21, 2025

samueltardieu commented Feb 23, 2025

samueltardieu commented Feb 28, 2025

samueltardieu commented Mar 16, 2025

y21 commented Mar 18, 2025

y21 Mar 18, 2025

samueltardieu Mar 19, 2025

y21 Mar 18, 2025

samueltardieu Mar 19, 2025

y21 Mar 18, 2025

y21 Mar 18, 2025

samueltardieu Mar 19, 2025

y21 Mar 18, 2025

samueltardieu Mar 19, 2025

y21 Mar 18, 2025

samueltardieu Mar 19, 2025

		swap(&mut mac!(funcall func), z);
		//~^ ERROR: swapping with a temporary value is inefficient

		// If the `swap()` is a statement by itself, propose to replace it by `a = b`. Otherwise, when part
		// of a larger expression, surround the assignment with a block to make it `()`.

New lint: swap_with_temporary #14046

Are you sure you want to change the base?

New lint: swap_with_temporary #14046

Conversation

samueltardieu commented Jan 20, 2025 • edited Loading

rustbot commented Jan 20, 2025

samueltardieu commented Jan 27, 2025

samueltardieu commented Jan 30, 2025

samueltardieu commented Feb 12, 2025

samueltardieu commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueltardieu commented Feb 21, 2025

samueltardieu commented Feb 23, 2025

samueltardieu commented Feb 28, 2025

samueltardieu commented Mar 16, 2025

y21 commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

New lint: `swap_with_temporary` #14046

New lint: `swap_with_temporary` #14046

samueltardieu commented Jan 20, 2025 •

edited

Loading