do not apply DerefMut on union field#75584
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
src/test/ui/union/union-deref.stderr
Outdated
There was a problem hiding this comment.
The span for this is slightly wrong, I'd expect it to only cover u.f.0. I don't know how this is happening, since the span is right above.
There was a problem hiding this comment.
I would guess this has to do with the special handling of multiple tuple field accesses in a row. Perhaps 0.0 is a float token.
There was a problem hiding this comment.
I didn't know there was such special handling.^^ So this would be a problem elsewhere, in the code that sets up the span for these places?
736a46c to
85cb4a2
Compare
This comment has been minimized.
This comment has been minimized.
dabedc3 to
66b340f
Compare
|
Rebased to resolve conflict (but it was a clean rebase); @matthewjasper this is ready for another round of review. |
|
@bors r+ |
|
📌 Commit 66b340f has been approved by |
…asper do not apply DerefMut on union field This implements the part of [RFC 2514](https://github.com/rust-lang/rfcs/blob/master/text/2514-union-initialization-and-drop.md) about `DerefMut`. Unlike described in the RFC, we only apply this warning specifically when doing `DerefMut` of a `ManuallyDrop` field; that is really the case we are worried about here. @matthewjasper suggested I patch `convert_place_derefs_to_mutable` and `convert_place_op_to_mutable` for this, but I could not find anything to do in `convert_place_op_to_mutable` and this is sufficient to make the test pass. However, maybe there are some other cases this misses? I have no familiarity with this code. This is a breaking change *in theory*, if someone used `ManuallyDrop<T>` in a union field and relied on automatic `DerefMut`. But on stable this means `T: Copy`, so the `ManuallyDrop` is rather pointless. Cc rust-lang#55149
…asper do not apply DerefMut on union field This implements the part of [RFC 2514](https://github.com/rust-lang/rfcs/blob/master/text/2514-union-initialization-and-drop.md) about `DerefMut`. Unlike described in the RFC, we only apply this warning specifically when doing `DerefMut` of a `ManuallyDrop` field; that is really the case we are worried about here. @matthewjasper suggested I patch `convert_place_derefs_to_mutable` and `convert_place_op_to_mutable` for this, but I could not find anything to do in `convert_place_op_to_mutable` and this is sufficient to make the test pass. However, maybe there are some other cases this misses? I have no familiarity with this code. This is a breaking change *in theory*, if someone used `ManuallyDrop<T>` in a union field and relied on automatic `DerefMut`. But on stable this means `T: Copy`, so the `ManuallyDrop` is rather pointless. Cc rust-lang#55149
|
⌛ Testing commit 66b340f with merge ea590e7b64fbc953a5d33618e5af7c32915f465e... |
|
💔 Test failed - checks-actions |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Clang installation failure. |
|
☀️ Test successful - checks-actions, checks-azure |
This implements the part of RFC 2514 about
DerefMut. Unlike described in the RFC, we only apply this warning specifically when doingDerefMutof aManuallyDropfield; that is really the case we are worried about here.@matthewjasper suggested I patch
convert_place_derefs_to_mutableandconvert_place_op_to_mutablefor this, but I could not find anything to do inconvert_place_op_to_mutableand this is sufficient to make the test pass. However, maybe there are some other cases this misses? I have no familiarity with this code.This is a breaking change in theory, if someone used
ManuallyDrop<T>in a union field and relied on automaticDerefMut. But on stable this meansT: Copy, so theManuallyDropis rather pointless.Cc #55149