Lint needless_borrow and explicit_auto_deref on most union field accesses
#11508
Conversation
|
r? @blyxyas (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
It's still relevant. Both the PR and the issue in question seem to have some misunderstandings why the suggestion doesn't compile. Auto-deref only doesn't apply on |
|
☔ The latest upstream changes (presumably #11511) made this pull request unmergeable. Please resolve the merge conflicts. |
| let _ = &mut (&mut x.u).x; | ||
| let _ = &mut (&mut { x.u }).x; | ||
| let _ = &mut ({ &mut x.u }).x; | ||
|
|
||
| let mut x = U { | ||
| u: Wrap(ManuallyDrop::new(Foo { x: 0 })), | ||
| }; | ||
| let _ = &mut (&mut x.u).x; | ||
| let _ = &mut (&mut { x.u }).x; | ||
| let _ = &mut ({ &mut x.u }).x; | ||
|
|
||
| let mut x = U { u: Wrap(Foo { x: 0 }) }; | ||
| let _ = &mut (&mut x.u).x; | ||
| let _ = &mut (&mut { x.u }).x; | ||
| let _ = &mut ({ &mut x.u }).x; |
There was a problem hiding this comment.
Could you add //~ ERROR annotations?
There was a problem hiding this comment.
I thought we weren't going to enforce this until ui_test worked the way we wanted it.
There was a problem hiding this comment.
If I understand correctly, the conclusions from the Zulip thread talking about this were that we soft-mandated //~ERROR annotations (without a body). Even if they aren't obligated by the CI, I just find them to increase legibility 🐱
tests/ui/needless_borrow.stderr
Outdated
There was a problem hiding this comment.
Are we sure these brackets are necessary? Is there some special behaviour about references in blocks?
There was a problem hiding this comment.
GitHub isn't showing code here, so I assume you're referring to the mid expression blocks. The final expression in a block is moved before the parent expression runs. Since rustc only disallows x.y.z not any other form (e.g. { x.y }.z) tests around that are needed.
clippy_lints/src/dereference.rs
Outdated
| @@ -172,8 +172,7 @@ pub struct Dereferencing<'tcx> { | |||
| #[derive(Debug)] | |||
| struct StateData<'tcx> { | |||
| /// Span of the top level expression | |||
| if parent_id == data.first_expr.hir_id { | ||
| return; | ||
| } | ||
| (cx.tcx.hir().get(parent_id).expect_expr().span, true) |
There was a problem hiding this comment.
Could you replace this expect_expr with a fallible alternative? Or is there a guarantee that the parent id is an expression?
There was a problem hiding this comment.
There has to be at least one deref expression as direct parents.
| // Checks if the adjustments contains a deref of `ManuallyDrop<_>` | ||
| fn adjust_derefs_manually_drop<'tcx>(adjustments: &'tcx [Adjustment<'tcx>], mut ty: Ty<'tcx>) -> bool { | ||
| adjustments.iter().any(|a| { | ||
| let ty = mem::replace(&mut ty, a.target); | ||
| matches!(a.kind, Adjust::Deref(Some(ref op)) if op.mutbl == Mutability::Mut) && is_manually_drop(ty) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure about the logic of this function. The original ty passed into the function is never read because from the first iteration it is already replaced with an adjustment.
Also, the doc-comment isn't a doc-comment
There was a problem hiding this comment.
The result of mem::replace on the first iteration is the passed in value.
|
I think that this two commits can be squashed, as all changes made to |
|
☔ The latest upstream changes (presumably #11747) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The first commit made changes to some logic. The second commit uses the changed logic, but extracted into a function. Diffs sometimes do a bad job at showing things. |
|
@bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Changes both lints to follow rustc's rules around auto-deref through
ManuallyDropunion fields rather than just bailing on union fields.changelog: [
needless_borrow] & [explicit_auto_deref]: Lint on most union field accesses