do not apply DerefMut on union field #75584

RalfJung · 2020-08-16T08:35:37Z

This implements the part of RFC 2514 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 #55149

rust-highfive · 2020-08-16T08:35:41Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

RalfJung · 2020-08-16T08:36:11Z

src/test/ui/union/union-deref.stderr

+  --> $DIR/union-deref.rs:18:14
+   |
+LL |     unsafe { u.f.0.0 = Vec::new() };
+   |              ^^^^^^^


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.

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.

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?

petrochenkov · 2020-08-16T09:34:58Z

r? @matthewjasper

matthewjasper

This looks good.

src/test/ui/union/union-deref.rs

RalfJung · 2020-08-31T11:03:40Z

Rebased to resolve conflict (but it was a clean rebase); @matthewjasper this is ready for another round of review.

matthewjasper · 2020-09-04T18:55:54Z

@bors r+

bors · 2020-09-04T18:55:56Z

📌 Commit 66b340f has been approved by matthewjasper

@matthewjasper

…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

@matthewjasper

…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

bors · 2020-09-05T09:32:28Z

⌛ Testing commit 66b340f with merge ea590e7b64fbc953a5d33618e5af7c32915f465e...

bors · 2020-09-05T09:35:14Z

💔 Test failed - checks-actions

rust-log-analyzer · 2020-09-05T09:35:39Z

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 @rust-lang/infra. (Feature Requests)

RalfJung · 2020-09-05T09:36:38Z

Clang installation failure.
@bors retry

bors · 2020-09-05T11:47:07Z

⌛ Testing commit 66b340f with merge 81a769f...

bors · 2020-09-05T13:55:21Z

☀️ Test successful - checks-actions, checks-azure
Approved by: matthewjasper
Pushing 81a769f to master...

rust-highfive assigned petrochenkov Aug 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2020

RalfJung commented Aug 16, 2020

View reviewed changes

RalfJung force-pushed the union-no-deref branch from 736a46c to 85cb4a2 Compare August 16, 2020 09:19

rust-highfive assigned matthewjasper and unassigned petrochenkov Aug 16, 2020

matthewjasper reviewed Aug 30, 2020

View reviewed changes

src/test/ui/union/union-deref.rs Show resolved Hide resolved

Centril mentioned this pull request Aug 30, 2020

Tracking issue for RFC 2514, "Union initialization and Drop" #55149

Closed

7 tasks

This comment has been minimized.

Sign in to view

RalfJung added 4 commits August 31, 2020 10:14

do not apply DerefMut on union field

ec0924f

also detect DerefMut in nested union fields

44defae

only emit error for ManuallyDrop derefs

97974e3

test more ways of mutably accessing a place

66b340f

RalfJung force-pushed the union-no-deref branch from dabedc3 to 66b340f Compare August 31, 2020 08:15

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2020

Dylan-DPC-zz mentioned this pull request Sep 4, 2020

Rollup of 14 pull requests #76348

Closed

Dylan-DPC-zz mentioned this pull request Sep 4, 2020

Rollup of 13 pull requests #76350

Closed

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 5, 2020

bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 5, 2020

bors merged commit 81a769f into rust-lang:master Sep 5, 2020

rustbot added this to the 1.48.0 milestone Sep 5, 2020

RalfJung deleted the union-no-deref branch September 8, 2020 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not apply DerefMut on union field #75584

do not apply DerefMut on union field #75584

RalfJung commented Aug 16, 2020 •

edited

Loading

rust-highfive commented Aug 16, 2020

RalfJung Aug 16, 2020

joshtriplett Aug 30, 2020

RalfJung Aug 30, 2020

petrochenkov commented Aug 16, 2020

matthewjasper left a comment

This comment has been minimized.

RalfJung commented Aug 31, 2020

matthewjasper commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 5, 2020

bors commented Sep 5, 2020

rust-log-analyzer commented Sep 5, 2020

RalfJung commented Sep 5, 2020

bors commented Sep 5, 2020

bors commented Sep 5, 2020

do not apply DerefMut on union field #75584

do not apply DerefMut on union field #75584

Conversation

RalfJung commented Aug 16, 2020 • edited Loading

rust-highfive commented Aug 16, 2020

RalfJung Aug 16, 2020

Choose a reason for hiding this comment

joshtriplett Aug 30, 2020

Choose a reason for hiding this comment

RalfJung Aug 30, 2020

Choose a reason for hiding this comment

petrochenkov commented Aug 16, 2020

matthewjasper left a comment

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung commented Aug 31, 2020

matthewjasper commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 5, 2020

bors commented Sep 5, 2020

rust-log-analyzer commented Sep 5, 2020

RalfJung commented Sep 5, 2020

bors commented Sep 5, 2020

bors commented Sep 5, 2020

RalfJung commented Aug 16, 2020 •

edited

Loading