-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
Conversation
befa2dc
to
b84eb55
Compare
b84eb55
to
57f3cfb
Compare
Rebased |
57f3cfb
to
19ddfe2
Compare
Rebased. |
19ddfe2
to
67d0f0e
Compare
Rebased |
67d0f0e
to
b6a3a75
Compare
Rebased |
if target.span.from_expansion() { | ||
ArgKind::FromExpansion | ||
} else if target.is_syntactic_place_expr() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_lints/src/methods/mod.rs
Outdated
/// ``` | ||
#[clippy::version = "1.86.0"] | ||
pub SWAP_WITH_TEMPORARY, | ||
perf, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
4e6fe44
to
3016c0b
Compare
Rebased |
@y21 Anything else needs to be done? |
3016c0b
to
76e6421
Compare
Rebased |
@y21 should I reassign? |
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
swap(&mut mac!(funcall func), z); | ||
//~^ ERROR: swapping with a temporary value is inefficient |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
clippy_lints/src/methods/mod.rs
Outdated
/// *s = String::from("replaced"); | ||
/// } | ||
/// ``` | ||
#[clippy::version = "1.86.0"] |
There was a problem hiding this comment.
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
clippy_lints/src/methods/mod.rs
Outdated
/// 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. |
There was a problem hiding this comment.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// 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 `()`. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
76e6421
to
ed73e9d
Compare
This lint detects inefficient or useless
{std,core}::mem::swap()
calls such as:It also takes care of using a form appropriate for a
()
context ifswap()
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 lintClose #1968