Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: swap_with_temporary #14046

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Jan 20, 2025

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
Copy link
Collaborator

rustbot commented Jan 20, 2025

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 20, 2025
@samueltardieu samueltardieu force-pushed the push-nkqqkpnrplvv branch 4 times, most recently from befa2dc to b84eb55 Compare January 20, 2025 16:59
@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

Rebased.

@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

Rebased

Comment on lines 42 to 44
if target.span.from_expansion() {
ArgKind::FromExpansion
} else if target.is_syntactic_place_expr() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// ```
#[clippy::version = "1.86.0"]
pub SWAP_WITH_TEMPORARY,
perf,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Rebased

@samueltardieu
Copy link
Contributor Author

@y21 Anything else needs to be done?

@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

@y21 should I reassign?

@y21
Copy link
Member

y21 commented Mar 18, 2025

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +68 to +54
swap(&mut mac!(funcall func), z);
//~^ ERROR: swapping with a temporary value is inefficient
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 131 to 132
// 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 `()`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

swap(&mut x, &mut func());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint calling mem::swap on a reference
3 participants