Union initialization and Drop #2514

RalfJung · 2018-08-03T15:35:13Z

This RFC realizes the second part of https://internals.rust-lang.org/t/pre-rfc-unions-drop-types-and-manuallydrop/8025: Changing unions to no longer allow fields with drop glue, and otherwise describing what we need to settle before unions can be fully stabilized. Unfortunately this got somewhat more complicated than I thought.

As usual I had trouble separating things between the guide-level and the reference-level explanation; I hope this makes sense!

Rendered
Tracking issue

petrochenkov · 2018-08-03T16:09:57Z

text/0000-union-initialization-and-drop.md

+
+// We can write into uninitialized inner fields:
+u.f2.1 = S(42);
+let _ = &u.f2.1; // This field is initialized now.


let _ is a bit of a footgun in terms of testing initialization.
_ pattern doesn't access the right side at all, so even something like this

let x: u8; let _ = x;

successfully compiles.
You need let _y = x; or just x; to test for x being initialized.

oops thanks!

EDIT: And fixed.

joshtriplett · 2018-08-03T16:17:35Z

text/0000-union-initialization-and-drop.md

+let _ = &u.f2.0;
+
+// Equivalently, we can assign the entire union:
+u = U { f2: S(42) };


This doesn't type-check; f2 has type (S, S).

Thanks, I also aligned the unions in the two examples.

petrochenkov · 2018-08-03T16:32:56Z

text/0000-union-initialization-and-drop.md

+# Summary
+[summary]: #summary
+
+Unions do not allow fields of types that require drop glue, but they may still


Nit: "drop glue" is such a rustc-specific jargon, perhaps "trivial ~~destructor~~ drop", or "drop not running any code", or something like this could be better.

I think "(doesn't) need(s) drop" or "noop drop" could also work as terms.

Don't we have a term for "the code that runs when something is dropped"? I feel that is not such a strange rustc-specific concept, all Rust implementations and also e.g. C++ have it.

Personally, I find "noop drop" much less clear than "does not have drop glue".^^

petrochenkov · 2018-08-03T16:33:45Z

text/0000-union-initialization-and-drop.md

+(which this RFC adapts from @petrochenkov's proposal) can sometimes be a little
+surprising when looking at individual fields: Whether `u.f2 = ...;` drops
+depends on whether `u.f1` has been previously initialized.  @petrochenkov hence
+proposes a lint to warn people that unions with drop-glue fields are not always


The lint was inherited from the original RFC rather than proposed by me :)

Ah, sorry. Fixed :)

petrochenkov · 2018-08-03T16:39:58Z

text/0000-union-initialization-and-drop.md

+the rules that are currently only applied to unions that `impl Drop`.  However,
+that does not actually help with the pitfall described above.  The more complex
+rules allow more code that many will reasonably expect to work, and do not seem
+to introduce any additional pitfalls.


Independent nested fields borrowed independently is probably the most natural expectation:

union U { a: (u8, u8), b: u16 } let x = &u.a.0; let y = &u.a.1;

so it's borrow checker that needs to work in per-field fashion first of all.
Move checker just mirrors what borrow checker does (for consistency and also because they share common infrastructure in the compiler).

Ah, I hadn't thought about borrowck here. So with the rules as stated, once a field is (partially) borrowed, its siblings all become completely blocked from borrowing?

Borrowing is unsafe, so this would not be necessary, but it still seems useful.

petrochenkov · 2018-08-03T16:44:17Z

LGTM.
It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.

This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.

dlight · 2018-08-04T21:10:18Z

The RFC summary should probably link to something that explains what a drop glue is.

(The Drop chapter of the Rust book doesn't contain the word "glue")

RalfJung · 2018-08-06T15:36:07Z

It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.

What would be a good example?

This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.

Yes. (I avoided calling them "Drop fields" because a field can need drop glue even if it does not impl Drop -- e.g. if it is a struct and one of its fields has impl Drop.)

The RFC summary should probably link to something that explains what a drop glue is.

I added a brief explanation.

joshtriplett · 2018-08-09T19:36:10Z

@rfcbot merge

Based on discussion in the lang team and with Ralf, I think this is ready to merge. Fields in unions that impl Drop have never been part of stable Rust, so this isn't a breaking change, and this fixes various concerns and painful corner cases that kept coming up.

rfcbot · 2018-08-09T19:36:11Z

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

~~pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box~~ resolved by Union initialization and Drop #2514 (comment)

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

text/0000-union-initialization-and-drop.md

cramertj · 2018-08-13T21:18:16Z

text/0000-union-initialization-and-drop.md

+}
+{
+    let u = U { f1: ManuallyDrop::new(Vec::new()) };
+    foo(u.f2);


This looks like moving out of a union that implements Drop-- how is this different from the let v = u.f1; case above?

You are right, I will fix the example.

The only way to deinitialize a union with drop is to move away the entire thing, just like with structs.

cramertj · 2018-08-13T21:20:56Z

text/0000-union-initialization-and-drop.md

+This requires `unsafe` because it desugars to `ManuallyDrop::deref_mut(&mut u.f).0`,
+and while writing to a union field is safe, taking a reference is not.
+
+For this reason, `DerefMut` auto-deref is not applied when working on a union or


Why ban just auto-deref rather than banning DerefMut entirely? Users could access the nested types using .as_ptr() or .as_mut_ptr() methods.

I felt that was too drastic, but sure.
Probably calling deref_mut manually is also still okay, just the sugar that lets you use * is not?

I also do not know what in technically feasible in this space.

Yeah, I'm not sure what the right balance is. You want to allow the deref when the value has been initialized, but ban it as much as possible in cases where it creates an &mut T to a partially or wholly uninitialized T.

Uh, I think I'd rather avoid such stateful lints. They are not complete enough IMHO (they cannot know which field you are now allowed to create a reference to).

pnkfelix · 2018-09-27T19:54:54Z

@rfcbot concern pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box

I've been swamped, but this is an area I sort of care about and I want to read this

rfcbot · 2018-10-04T19:22:01Z

🔔 This is now entering its final comment period, as per the review above. 🔔

daboross · 2018-10-10T21:11:23Z

text/0000-union-initialization-and-drop.md

+
+// Rejected
+union Example4<T> {
+    // `T` might have drop glue, and then `Cell<T>` would as well.


Should this mention RefCell<T>?

Why that? I didn't intend to list every type with interior mutability.

Specifically because the line below is f1: RefCell<T>. I assumed this comment was here to explain why, given that line, this struct is rejected.

Oh, I made a typo and didn't use the same type on both sides. Thanks for pointing that out! (GitHub's diff viw for comments embedded in the discussions is so bad...)

No problem - sorry I didn't point it out better initially! I'm still not entirely used to how github displays these things either.

crlf0710 · 2018-10-13T05:48:31Z

Sorry it's a bit late, but in prior art section: Just want to point out that actually C++ does have destructors for unions. (See https://en.cppreference.com/w/cpp/language/union). Maybe a small edit to the text will be good.

RalfJung · 2018-10-13T09:32:51Z

@crlf0710 Thanks for pointing that out! I fixed the text. Do you agree it is correct now?

crlf0710 · 2018-10-13T10:25:03Z

@RalfJung Looks good!

SimonSapin · 2018-10-13T11:37:37Z

Would this auto trait be an accurate expression of the “has no drop glue” concept?

pub auto trait NoDropGlue {}
impl<T> !NoDropGlue for T where T: Drop {}

If it is and we make it a lang item (in core::marker), generic unions could be extended to allow fields that are not necessarily Copy:

union Foo<T: NoDropGlue> {
    f: T,
}

RalfJung · 2018-10-13T14:12:14Z

I think it would begin to express that concept if it worked, but AFAIK those kind of negative bounds don't work...at least I recall that was the result of someone checking last time.

We'd however also need

impl<T> NoDropGlue for ManuallyDrop<T>

and then what about unions themselves? Do they get auto traits the same way everything else does, or are auto traits just never implemented for them? Either way we'd want NoDropGlue implemented for all unions

SimonSapin · 2018-10-13T15:39:49Z

Good points. So leaving aside the auto-trait definition and assuming it can be implemented with ~compiler magic~ instead, would it be desirable for this trait to exist to enable such generic unions?

RalfJung · 2018-10-13T16:08:25Z

I would think so, yes. Copy has always been a bad approximation. IIRC either @eddyb or @nikomatsakis had other uses for such a trait as well.

However, introducing such a trait is firmly out of scope for this PR.

crlf0710 · 2018-10-13T16:20:18Z

i think the “has no drop glue” approximately corresponds to std::mem::needs_drop which is implemented using a intrinsic. As the documentation says it's conservative, so... maybe it will be a matter of making it accurate and const fn... Maybe it's also a good idea to lift it to a trait(that's a common problem for all const fns, i think).

rfcbot · 2018-10-14T19:27:42Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

Centril · 2018-10-17T14:25:13Z

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#55149

RalfJung added 2 commits August 3, 2018 16:52

union-initialization-and-drop

04c66db

edits

6bb3c6b

petrochenkov reviewed Aug 3, 2018

View reviewed changes

clarifications

b7c64f2

joshtriplett reviewed Aug 3, 2018

View reviewed changes

don't use 'let _ ='

5cff374

scottmcm added the T-lang Relevant to the language team, which will review and decide on the RFC. label Aug 3, 2018

fix code typo, and make unions more consistent

d4d6e38

petrochenkov reviewed Aug 3, 2018

View reviewed changes

RalfJung added 2 commits August 6, 2018 17:31

fix misattribution

134ca0b

explain drop glue, briefly

290ceaa

nikomatsakis assigned joshtriplett Aug 9, 2018

rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Aug 9, 2018

cramertj reviewed Aug 13, 2018

View reviewed changes

text/0000-union-initialization-and-drop.md Outdated Show resolved Hide resolved

cramertj reviewed Aug 13, 2018

View reviewed changes

fix example

41a02cf

hanna-kruppe mentioned this pull request Sep 5, 2018

Representation of unions rust-lang/unsafe-code-guidelines#13

Closed

unresolved question: move out of dropping union

0971718

rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Sep 27, 2018

rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Oct 4, 2018

rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Oct 4, 2018

daboross reviewed Oct 10, 2018

View reviewed changes

fix typo

f9321d9

fix what we say about C++

ecf6a2d

rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Oct 14, 2018

Centril mentioned this pull request Oct 17, 2018

Tracking issue for RFC 2514, "Union initialization and Drop" rust-lang/rust#55149

Closed

7 tasks

RFC 2514

e9f3184

Centril merged commit e5276df into rust-lang:master Oct 17, 2018

inquisitivecrystal mentioned this pull request Nov 3, 2018

Nightly requirements sdleffler/thunk-rs#1

Open

Centril added A-unions Proposals relating to union items A-drop Proposals relating to the Drop trait or drop semantics A-typesystem Type system related proposals & ideas A-expressions Term language related proposals & ideas A-data-types RFCs about data-types labels Nov 23, 2018

petrochenkov mentioned this pull request Feb 11, 2019

Adding a Drop impl to a type is a breaking change rust-lang/rust#58379

Closed

RalfJung deleted the union-initialization-and-drop branch September 16, 2019 18:38

nikomatsakis mentioned this pull request Oct 24, 2019

Untagged unions (tracking issue for RFC 1444) rust-lang/rust#32836

Closed

5 tasks

Union initialization and Drop #2514

Union initialization and Drop #2514

Conversation

RalfJung commented Aug 3, 2018 • edited by Centril Loading

Choose a reason for hiding this comment

RalfJung Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Aug 3, 2018

dlight commented Aug 4, 2018

RalfJung commented Aug 6, 2018

joshtriplett commented Aug 9, 2018

rfcbot commented Aug 9, 2018 • edited by nikomatsakis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Sep 27, 2018 • edited Loading

rfcbot commented Oct 4, 2018

Choose a reason for hiding this comment

RalfJung Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crlf0710 commented Oct 13, 2018

RalfJung commented Oct 13, 2018

crlf0710 commented Oct 13, 2018

SimonSapin commented Oct 13, 2018

RalfJung commented Oct 13, 2018

SimonSapin commented Oct 13, 2018

RalfJung commented Oct 13, 2018 • edited Loading

crlf0710 commented Oct 13, 2018

rfcbot commented Oct 14, 2018

Centril commented Oct 17, 2018

RalfJung commented Aug 3, 2018 •

edited by Centril

Loading

RalfJung Aug 3, 2018 •

edited

Loading

RalfJung Aug 6, 2018 •

edited

Loading

petrochenkov Aug 3, 2018 •

edited

Loading

rfcbot commented Aug 9, 2018 •

edited by nikomatsakis

Loading

pnkfelix commented Sep 27, 2018 •

edited

Loading

RalfJung Oct 11, 2018 •

edited

Loading

RalfJung commented Oct 13, 2018 •

edited

Loading