Unions 1.2

[petrochenkov]

The existing implementation, guarantees and tradeoffs are described in more detail.
Future directions are outlined.
Few corner cases are permitted (empty unions, union patterns with zero fields and ..).

This RFC also provides documentation necessary for stabilization of the feature.
The "Overview" section can be copy-pasted into the book and "Detailed design" into the reference.

Rendered

[kennytm]

Do you mean one cannot derive PartialEq alone? I don't see how can you derive Eq without deriving PartialEq.

[petrochenkov]

derive(PartialEq) needs to actually generate code while derive(Eq) is just an assertion like derive(Copy). Otherwise #1897 (comment)

[kennytm]

What happens when we write to an inactive field with Drop, UB due to a read? Unresolved question?

union U {
    a: NoisyDrop1,
    b: NoisyDrop2,
}
fn main() {
    unsafe {
        let mut u = U { a: NoisyDrop1 };
        u.b = NoisyDrop2;  // calls `NoisyDrop2::drop(&mut u.b)` (UB)? or something else?
    }
}

[petrochenkov]

Calls NoisyDrop2::drop if u contains a valid value of NoisyDrop2, UB due to a read otherwise.
I will add this example to the text.

[nagisa]

can overwrite

will overwrite

[petrochenkov]

If we write into an empty field, e.g. (), it will not overwrite anything (#1897 (comment)).

[nagisa]

I would expect Eq to be derivable just fine for union that implements PartialEq manually. This requirement seems overly restrictive to me – and also doesn’t really match behaviour of derive(Eq) elsewhere IIRC.

[petrochenkov]

Maybe the wording is bad? This

I would expect Eq to be derivable just fine for union that implements PartialEq manually.

is exactly the case.

[nagisa]

What’s the point of this guarantee?

[petrochenkov]

To support this.
(This guarantee was in the original RFC as well.)

[nagisa]

This guarantee only makes sense for specified layouts, then.

[nagisa]

Needs spec wrt Drop when writen to an inactive fragment.

[petrochenkov]

#1897 (comment)

[nagisa]

s/struct/union/

[petrochenkov]

The next sentence is about unions, this one is actually about structs.

[burdges]

Your is_zero example needs to place the whole match into an unsafe block, when only the accesses to U are unsafe. Is there any way to avoid this? I suppose you could call safe closures. Would it make any sense to push the unsafe keyword inside the pattern? Say maybe:

fn is_zero(v: Value) -> bool {
    match v {
        Value { tag: I, u: U { unsafe i: 0 } } => true,
        Value { tag: F, u: U { unsafe f: 0.0 } } => true,
        _ => false,
    }
}

[aturon]

cc @joshtriplett @retep998

[sgrif]

Probably don't need the , or at the end of each item here since it's a list. I though there was a markdown rendering error at first.

[Ericson2314]

Re the technical details, this is wonderful. I didn't follow the original thread very closely, and had no idea this much static analysis was in the works.

There's a lot of "unions are like structs" phrasing however, which makes me worried. Don't get me wrong, I'm perfectly happy with the current syntax of unions as a intentional legacy homage / enabler of basic "copy-paste" FFI, but I fail to see how the semantics of union are at all like structs, except in that they are both "aggregate things" (and so are enums). To anyone learning Rust before C, I think this would be quite confusing.

[ubsan]

This feels... weird, as an RFC. There's a lot of language that makes me feel like it is supposed to be written in a standardese-y sort of way, and then it says stuff like

Reading a fragment f having type T from of a union value u is permitted if and only if the bit pattern of u contains a valid value of type T starting from offset(t) and this value has no uninitialized bits (except for maybe in padding), otherwise such read results in UB. (Note that writes may require reads too if destructor is called.)

which... okay, I get what you're trying to go for here, but it's not well specified at all. I'd much rather go the C++ route of saying you can read the common initial subset, but otherwise you get unspecified behavior.

[ubsan]

And I realize most of that is due to the unsafe code guidelines team not having finished very much, but... this feels a bit like overstepping the bounds of guarantees. At most, we should guarantee what C or C++ guarantee, and we may add more guarantees later as part of the unsafe code guidelines team.

[ubsan]

And I realize it may have been in the original, but in the cases where the f2 field is active,

unsafe {
    match u {
      MyUnion { f1: 10 } => { println!("ten"); }
      MyUnion { f2 } => { println!("{}", f2); }
    }
}

will always read an invalid member in cases where u = MyUnion { f1 : (not 10) } or u = MyUnion { f2 : (anything) }.

I'm also not certain of the tagged union case. I'd assume that matchers (conceptually) read the whole value, and only then test whether the thing holds. Therefore, if you match on a tagged union like the following:

unsafe {
  match v {
    Value { tag: I, u: U { i: 0 } } => true,
    Value { tag: F, u: U { f: 0.0 } } => true,
    _ => false,
  }
}

In each branch, you're reading the entire thing, and then checking whether tag is I or F; which means that you're reading the conceptually uninitialized i field, and only then checking if tag = I.

[ubsan]

The section "Active fragment and active field" absolutely squicks me out. This is nowhere near the rules that I would write:

There should be one active variant of a union. There are two ways to change the currently active variant;

The general case: u = MyUnion { d: 0 }. This works for all types, including !trivially_destructible types.

A specific case, for trivially destructible types: u.d = 0.

Writes into a non-active variant (like u.a.x = 0; from the proposal) are undefined behavior. (This may be changed later by the unsafe code guidelines team).

[joshtriplett]

Writes into a non-active variant (like u.a.x = 0; from the proposal) are undefined behavior.

If I understand what you mean by that correctly (which I may well not have): making that UB would break well-established use cases for unions.

[ubsan]

@joshtriplett If the current active variant is b, then writing into a is UB, unless a is trivially destructible and you write to the whole thing at once. We might slowly ease up on this, but for now, I would rather not. We may add an exception for compatible initial sequences.

[ubsan]

Also, this RFC requires an editor to go over the language used. It's unclear and/or ambiguous in some parts.

[joshtriplett]

@ubsan I agree about the "trivially destructible" part. However, I think this needs to work:

#![feature(untagged_unions)]

struct X {
    a: u32,
    b: u32,
}

union U {
    full: u64,
    halves: X
}

fn main() {
    let mut value = U { full: 0x0123456789ABCDEF };
    unsafe { value.halves.a = 42; }
    println!("{:#x} {:#x}", unsafe { value.halves.b }, unsafe { value.full });
}

Runnable playground version: https://is.gd/GNHHgL

Rough sketch of what makes that safe: every field impls Copy, none implement Drop, nothing reads padding, and all the types involved are "total" (every possible bit value has a safe interpretation without UB).

[comex]

@ubsan Both the C and C++ standards allow reading from the "wrong" field of a union as long as the memory contains a valid bit pattern for the type in question. (Whether it does depends on type representation, which is implementation defined, but that's no different from the case in this RFC.) There's no good reason for Rust to be stricter than C on the matter, especially given that the feature's main use case is FFI. The common initial subset rule in C that you allude to is separate, and kind of a mess / not actually implemented by compilers, but has to do with aliasing pointers to the field types in the union, not accesses through the union type itself. Though even there, I can't imagine a world where the Rust unsafe code guidelines end up forbidding pointer transmutes, given the amount of existing code that relies on them.

Implied destructor calls should be treated no differently from any other call: valid if the memory has a valid bit pattern for the type being destructed.

However, your point about match patterns is well taken. Also, there is one area where this RFC would make stronger guarantees than C: the effect of writes to union fields on unused bits (i.e. bits that correspond to other fields but not the field in question). This RFC says they're unaffected; C says they take unspecified values. It may be worth weakening the guarantee to match C for now.

[comex]

Sigh, again I have to correct myself about the C++ standard. It does arguably prohibit reading anything other than the common initial subsequence, a concept it uses in a totally different way from the C standard… Though IMO only arguably. In any case, C explicitly allows it, and in practice it's common for code written in both language to depend on it as one of the "blessed" ways to perform bitcasts (the other is memcpy).

[ubsan]

@comex I'll be honest, I care very little about C. My interest is in C++. Therefore, I write from a C++ mindset.

[ubsan]

@joshtriplett mmmmmmmmhhhhheeehghhgiheghe

I can... kinda see how that might be useful, but it at least should be implementation defined. I'm made uncomfortable by it, but I can see the use for things like microsoft's LARGE_INTEGER type.

// note: https://msdn.microsoft.com/en-us/library/windows/desktop/aa383713(v=vs.85).aspx

[petrochenkov]

@ubsan

Therefore, I write from a C++ mindset.

Specifically for people with C++ mindset I wrote the Type punning section and listed examples that should work and should not be UB. This kind of stuff is used a lot in low-level code (emulation of hardware or interaction with it).

[petrochenkov]

Discussing stuff like compatible initial sequences and other layout compatibility at type level is the dead end IMO.
Of course, it's interesting and provides endless food for language lawyering, but it 1) doesn't meet requirements and 2) it's not necessary if we have better ownership based aliasing information and don't have to rely on TBAA like C++.
If some piece of memory is initialized and contains a valid value of T, then we can read it as T. Simple and intuitive.

[comex]

@ubsan Put it another way: The main reason not to allow it would be if a compiler can make optimizations based on treating it as UB, which matters if either (a) it can improve performance of Rust code, or (b) Rust may want to use existing compiler backends that bake in those optimizations. The fact that ~every C++ compiler is also a C compiler, along with the fact that union type punning is common in C++ code too (even if possibly unsanctioned by the standard), strongly suggests that (b) does not apply. (a) is dubious to start with for a FFI- and low-level-focused feature, and is even less likely given that the C standard is mostly designed to maximize optimization potential (with exceptions that don't apply here). Further, as I said, allowing type punning through unions pretty clearly seems to be 'easier' than allowing it through pointer transmutes, yet the latter are effectively guaranteed in Rust at this point due to existing code depending on them.

I suppose another reason to forbid it could be to allow sanitizers to complain about it, which could help code that uses unions for general storage purposes (not FFI) and doesn't intentionally type pun. But for that purpose, accesses of inactive variants should be prohibited altogether, without exceptions for initial common subsequences and such; and such a rule should be possible to opt out of, or more probably should be opt-in.

[kennytm]

“Trivial destructible” relies on Copy being mutually exclusive with Drop, but the wording of E0184 is not quite affirmative:

Explicitly implementing both Drop and Copy for a type is currently disallowed. This feature can make some sense in theory, but the current implementation is incorrect and can lead to memory unsafety (see issue #20126), so it has been disabled for now.

All point to that the restriction will be dropped if the implementation is fixed.

I think we should spell out the assumption that Copy means Copy + !Drop in this RFC, or make the restriction E0184 permanent.

[retep998]

Type punning through unions has to be legal enough for stuff like LARGE_INTEGER to work, but beyond that doesn't really matter to me.

[Mark-Simulacrum]

This feels somewhat inconsistent to me. Is there a specific reason for it?

It also feels like this is hard to track, since passing a reference to a constant is reasonable, and IIRC, we eventually will lose the 'constness' of the reference. Either way it seems like an unnecessary limitation unless I'm missing something.

[petrochenkov]

@burdges

the whole match into an unsafe block, when only the accesses to U are unsafe. Is there any way to avoid this?

Unsafety in pattern matching, unlike unsafety in expressions, is a new thing (unions are recently implemeted and unsafe borrowing of packed struct fields is still unimplemented) so it doesn't have any conveniences like unsafe blocks.
Maybe it's reasonable to add unsafe blocks to pattern syntax as well, but this is an idea orthogonal to unions. I think it needs to wait though, I'm not sure it will be used often enough to pull its weight, at least for now.

[petrochenkov]

@Mark-Simulacrum

This feels somewhat inconsistent to me. Is there a specific reason for it?

Mostly limitations of current const eval, it cannot reinterpret memory.
Changing this is listed in future directions (the last paragraph).

[gbutler69]

IGNORE: Missed the copy impl.

[aturon]

I feel bad about how little attention this has gotten from the lang team so far. Could use some help, so I'm raising the bat signal: @joshtriplett, @whitequark, @solson, @scottmcm, @Ixrec, any of you have time to catch up on this thread and summarize the status?

[Ixrec]

I have absolutely no experience with anything FFI-related or union-related, but here goes nothing...

With regards to making type punning legal, the high-level design presented here seems like the correct one. @joshtriplett's and @retep998's comments make it pretty clear that we need to "make unions like LARGE_INTEGER work" for FFI, and that seems to entail exactly the requirements listed by the RFC and no more.

The problems I see are:

1. This PR also contains so many smaller amendments, and changes so much of the original RFC's text, that I'm honestly not sure what changes are intended and what is meant to be "just cleanup" (e.g., was the union_field_drop lint supposed to get renamed to unions_with_drop_fields?).
2. The wording and organization of the RFC leaves a lot to be desired. I had to do a lot of back-tracking and double-checking in order to convince myself I understood what it was trying to say, and I'm fairly sure a lot of it could be more clearly written or even removed.

To expand on 2 a bit:

• The notion of "trivially destructible" appears to only be used when proposing that some union field assignments should be made safe. I think it could be defined in one or two sentences within the same section as that feature.
• I strongly suspect I'm missing something here, but all the parts of this RFC that talk about "fragments" seem like they could be rewritten to only talk about active/inactive union fields, and they'd mean the same thing in far fewer words. I couldn't find a place where the notion of an "active fragment" (of arbitrary "depth") seemed useful in specifying the guarantees. I understand that the implementation needs a more complex notion of fragment to deal with unions-in-structs-in-unions, but even the current wording of this RFC explicitly handwaves away much of that, so it's clearly not trying to be a formal specification. If there is a place where we need the notion of "fragments", it needs to be much more obvious, and "fragments" probably need to be defined much closer to it.
• There are a number of places where it's not entirely clear whether the RFC is proposing something, suggesting something we might do in the future but isn't proposing yet, or merely describing something that we're unlikely to ever do.
• I'm not even sure the wording is up to date anymore, since a) the line "All accesses to union fields (both reads and writes, and also borrows) require unsafe blocks" seems to be contradicted both by the proposal later in the RFC to make some of them safe and by the recent comments on #2095 b) There's a reference to NoDrop which I'm guessing is an old name for ManuallyDrop? c) we just stabilized a subset of unions, so the original PR comment that "This RFC also provides documentation necessary for stabilization of the feature." seems no longer correct?
• In general I agree with the comments that a lot of the wording is too "standardese-y" (i.e. hard to follow in a way that doesn't necessarily make it more precise) and some of it seems to be trying to specify things better left to the unsafe code guidelines project.

Currently, I think the type punning issue would be better off as its own self-contained RFC. In particular, I really liked the "Requirements and desirable properties" section of this PR, but the fact that it's 2/3rds of the way down means that a lot of the RFC feels unmotivated or doesn't make much sense until you get there. I'd rather see an RFC where "Motivation" describes the LARGE_INTEGER use case, then "Guide-Level Explanation" is mostly the same text as "Requirements and desirable properties", then "Reference-Level Explanation" lists exactly the requirements/guarantees we're adding to support these use cases and nothing else. Right now I think I know what those requirements are, but I'm not entirely sure because they're scattered throughout the RFC, and I'm not at all clear on whether they're supposed to apply to all unions or just #[repr(C)] unions.

If we do that, the smaller changes like allowing empty unions, .. syntax, making trivially-destructible assignments safe, the clarification that any union field becoming (un)initialized makes all sibling fields (un)initialized, and the clarification that dynamically-sized types are not allowed in unions (those are all the smaller changes in this PR I'm confident are intentional) seem like they'd only be a few dozen lines in total, so we can probably handle that all of that as a single amendment. As far as I know, the only objection to any of them is the semver compatibility hazard pointed out on the union tracking issue, which seems like something we can hash out here.

---

Smaller questions, some of which may become irrelevant if we do break this up:

• This was in the original unions RFC, but the examples in the pattern matching section don't make any sense to me either. The first example seems like guaranteed UB unless the second match arm is dead code. The second example seems like guaranteed UB unless we provide some kind of "short-circuiting" guarantee that we do not attempt to read the union part of the pattern until the tag has been read and compared (do patterns already have these guarantees? does this make UB-ness of patterns depend on whether you write the tag field first or last?)

• Is defining the term "layout-compatible" as "whatever C does" good enough for FFI purposes?

• What's the motivation for allowing empty unions? I've got nothing against it, but it seems weird to add it without at least saying "this makes macros simpler" or something.

• I'm also confused by the wording about deriving Eq without PartialEq. I thought that was a logical contradiction?

[petrochenkov]

@aturon

I feel bad about how little attention this has gotten from the lang team so far.

I feel bad about how little attention this RFC has gotten from me in the recent months. I still need to incorporate the feedback given so far and address unresolved questions.

[joshtriplett]

@petrochenkov Honestly, I do feel like the feedback about this feeling like a full rewrite in a way that makes it hard to compare to the original is accurate. It's difficult to walk through this RFC and figure out what has changed, what is the same but clarified (e.g. things you discovered while implementing, or nuances that came up later), and what is just rewritten/reworded without any clear rationale for doing so.

I feel like those are three independent goals, and doing them all in one giant change/rewrite makes it a challenge to review. I do absolutely think there's value in clarifying/strengthening the specification to match the code; that's one goal. I also think there's value in adding additional union features that haven't yet been implemented; that seems like it should be a separate step, and reviewed separately.

[nikomatsakis]

I'm nominating this for discussion in the @rust-lang/lang meeting. I propose that we assign someone to do a deep read of this document and summarize the pros/cons.

@petrochenkov, I'm sorry for the lack of attention this RFC has gotten. If it helps, I think the primary problem is that you did too good of a job! I printed it out once and found that it was going to take me some time to digest and never got back to it.

[nikomatsakis]

@rfcbot fcp postpone

We talked this over in the @rust-lang/lang meeting, and the sense was that this RFC is a great start, but it's been long enough that we ought to revisit the topic. In particular, the presentation of the RFC as edits and so forth makes it hard to understand precisely what is being proposed, so it might make sense to try and write a "from scratch" version that lays out the underlying philosophical approach to unions (about which there was some debate, iirc) and then digs more down into the details.

[rfcbot]

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

• @aturon
• @cramertj
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, 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.

[petrochenkov]

the presentation of the RFC as edits and so forth makes it hard to understand precisely what is being proposed, so it might make sense to try and write a "from scratch" version

This text is mostly "from scratch" already, but I guess I'll just drop all the fragments from the old text and make it a new numbered RFC.
I still plan to revise it sometime... this year.

[rfcbot]

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

[rfcbot]

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

[rfcbot]

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

[RalfJung]

Oh wow, I had no idea there's a discussion about type punning happening for, like, a year now! This is clearly unsafe-code-guidelines related but somehow it didn't appear on my radar, sorry for that.

My two cents: AFAIK, union type punning restrictions in C(++) are closely tied to type-based alias analysis. Essentially, there are two ways to get a "bad" pointer (pointing to data "of the wrong type") In C: Pointer manipulations (like casting or arithmetic), and unions. Both have to be restricted to make type-based alias analysis legal. Rust doesn't (want to) use TBAA, so we shouldn't need any magic type punning rules.
I see type punning as being equivalent to a transmute. The full rules for transmute aren't written down yet, but I feel like standardizing that union type punning does the same thing makes a lot of sense. Given that the unsafe code guidelines team hasn't made a single decision yet, I'm not sure what kind of process we'd go through to see if the entire team can agree on that :D

[ubsan]

@RalfJung that seems entirely reasonable to me - although it's more equivalent to transmute_copy.

[rfcbot]

The final comment period is now complete.

[aturon]

Closing as postponed. Thanks again @petrochenkov for your extensive work here; we look forward to a revised RFC!