needless_borrows_for_generic_args: Fix for &mut #12892
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
meithecatte
force-pushed
the
needless-borrows-mutrefs
branch
2 times, most recently
from
1ce1b47 to
2f21dcf
Compare
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the lint has been changed, to avoid suggestions that introduce a move. The motivation in the commit message is quite poor (if the detection for significant drops is not sufficient because it's not transitive, the proper fix would be to make it transitive). However, rust-lang#12454, the linked issue, provides a good reason for the change — if the value being borrowed is bound to a variable, then moving it will only introduce friction into future refactorings. Thus rust-lang#12706 changes the logic so that the lint triggers if the value being borrowed is Copy, or is the result of a function call, simplifying the logic to the point where analysing "is this the only use of this value" isn't necessary. However, said PR also introduces an undocumented carveout, where referents that themselves are mutable references are treated as Copy, to catch some cases that we do want to lint against. However, that is not sound — it's possible to consume a mutable reference by moving it. To avoid emitting false suggestions, this PR reintroduces the referent_used_exactly_once logic and runs that check for referents that are themselves mutable references. Thinking about the code shape of &mut x, where x: &mut T, raises the point that while removing the &mut outright won't work, the extra indirection is still undesirable, and perhaps instead we should suggest reborrowing: &mut *x. That, however, is left as possible future work. Fixes rust-lang#12856
meithecatte
force-pushed
the
needless-borrows-mutrefs
branch
from
2f21dcf to
c6cc160
Compare
|
Hey, this is triage: It looks like @Jarcho is currently busy, let's pick a new reviewer. r? clippy |
|
Hey @dswij, rustbot has chosen you as a new reviewer. If you don't have the time, you can reassign it with |
|
Hey, this is triage: It looks like @Jarcho is currently busy, let's pick a new reviewer. r? xFrednet |
| && let ExprKind::AddrOf(_, _, inner) = reference.kind | ||
| && !matches!(inner.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)) | ||
| if !(is_copy(cx, referent_ty) | ||
| || referent_ty.is_ref() && referent_used_exactly_once(cx, possible_borrowers, reference) |
There was a problem hiding this comment.
In the PR description you mention:
To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_oncelogic and runs that check for referents that are themselves mutable references.
Where is the mutability checked?
Alos, when is this check ever run? &T/&mut T should both be copy, which should short circit the || on this line. 🤔
There was a problem hiding this comment.
&mut T is not copy, as it must be unique. Observe how #[derive(Clone, Copy)] struct Meow<'a>(&'a mut i32); doesn't compile.
In most contexts it does behave like it is Copy, but under the hood it's actually a reborrow – and when passing a &mut to a function that takes impl Trait, the coercion that causes the reborrow doesn't happen.
There was a problem hiding this comment.
Ahh, interesting TIL. Then this checks out
Co-authored-by: Fridtjof Stoldt <[email protected]>
meithecatte
force-pushed
the
needless-borrows-mutrefs
branch
from
5778e49 to
e63061d
Compare
|
LGTM, thanks! Roses are red, |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
A fix for this appears to be coming soon: rust-lang/rust-clippy#12892 Signed-off-by: J Robert Ray <[email protected]>
Jarcho
added
the
beta-nominated
|
Don't know if this made it into the 1.81 beta. |
This will probably be fixed in clippy 1.81 (rust-lang/rust-clippy#12892)
Jarcho
added
beta-accepted
[beta] Clippy backports r? `@Mark-Simulacrum` Backports: - rust-lang/rust-clippy#12892 - rust-lang/rust-clippy#13168 - rust-lang/rust-clippy#13290 Both 12892 and 13290 are fixes for stable regressions. The first is a little large, but mostly just reverts the change that caused the regression. 13168 is to handle the `Error` trait moving to core in 1.81.
xFrednet
removed
the
beta-accepted
This commit fixes a bug introduced in #12706, where the behavior of the lint has been changed, to avoid suggestions that introduce a move. The motivation in the commit message is quite poor (if the detection for significant drops is not sufficient because it's not transitive, the proper fix would be to make it transitive). However, #12454, the linked issue, provides a good reason for the change — if the value being borrowed is bound to a variable, then moving it will only introduce friction into future refactorings.
Thus #12706 changes the logic so that the lint triggers if the value being borrowed is Copy, or is the result of a function call, simplifying the logic to the point where analysing "is this the only use of this value" isn't necessary.
However, said PR also introduces an undocumented carveout, where referents that themselves are mutable references are treated as Copy, to catch some cases that we do want to lint against. However, that is not sound — it's possible to consume a mutable reference by moving it.
To avoid emitting false suggestions, this PR reintroduces the referent_used_exactly_once logic and runs that check for referents that are themselves mutable references.
Thinking about the code shape of &mut x, where x: &mut T, raises the point that while removing the &mut outright won't work, the extra indirection is still undesirable, and perhaps instead we should suggest reborrowing: &mut *x. That, however, is left as possible future work.
Fixes #12856
changelog: none