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

[Centril]

This is a tracking issue for the RFC "Union initialization and Drop" (rust-lang/rfcs#2514).

Successor of #32836.

Steps:

• Implement the RFC (Mentoring instructions: Tracking issue for RFC 2514, "Union initialization and Drop" #55149 (comment)): Change untagged_unions to not allow union fields with drop #62330
• Adjust documentation (see instructions on forge)
• Stabilization PR (see instructions on forge)

Unresolved questions:

• There should be more tests in particular for all move-related behaviors (1, 2, 3, 4). Done in unions: test move behavior of non-Copy fields #75559.

• Should we try to avoid the DerefMut-related pitfall? And if yes, should we maybe try harder, e.g. lint against using * below a union type when describing a place? That would make people write let v = &mut u.f; *v = Vec::new();. It is not clear that this helps in terms of pointing out that an automatic drop may be happening. (Implementation at do not apply DerefMut on union field #75584.)

• We could allow moving out of a union field even if the union implements Drop. That would have the effect of making the union considered uninitialized, i.e., it would not be dropped implicitly when it goes out of scope. However, it might be useful to not let people do this accidentally. The same effect can always be achieved by having a dropless union wrapped in a newtype struct with the desired Drop.

• Should we allow impl Copy for Union even when the union has non-Copy fields? (Proposed here.)

Implementation history

• Change untagged_unions to not allow union fields with drop #62330: union fields must be no-drop
• unions: test move behavior of non-Copy fields #75559: more tests for moving out of non-Copy union fields
• do not apply DerefMut on union field #75584: don't implicitly DerefMut in unions
• stabilize union with 'ManuallyDrop' fields and 'impl Drop for Union' #77547: stabilize ManuallyDrop<_> union fields

[RalfJung]

can anyone write up mentoring instructions

I have no idea, unfortunately.

[SimonSapin]

Is there anything else tracked in #32836 that is not covered here? Does implementing this RFC mean fully stabilizing the untagged_unions feature?

[Centril]

@SimonSapin I just make the issues, so I'll leave the question up to @RalfJung :)

[RalfJung]

Stabilization is separate, right? It has its own checkmark above. :) Implementing the RFC means restricting unions with non-Copy types to the point where I think they can be stabilized, yes.

I am not fully aware of everything that exists behind the untagged_union feature gate though... if e.g. there is such a thing as union patterns, they are not affected by this directly.

Quoting from the other tracking issue:

When moving out of one field of a union, are the others considered invalidated?

This is answered by this RFC.

Under what conditions can you implement Copy for a union? For example, what if some variants are of non-Copy type? All variants?

The RFC doesn't say anything about that, but I think indeed the answer should be "all variants".

What interaction is there between unions and enum layout optimizations?

This is topic of a future unsafe code guidelines discussion, and unrelated to this RFC.

[Aaron1011]

I'm interested in working on implementing this.

[Gankra]

cc'ing myself on this since this is a pre-req for unforking serde_derive in firefox

[Centril]

@gankro GitHub has a "subscribe" button (to the right, above "participants") so you don't have to write "cc me" and such in issues.

[eddyb]

This is the part that needs to be changed:

rust/src/librustc/middle/stability.rs

Lines 768 to 773 in 757d6cc

	let param_env = self.tcx.param_env(def_id);
	if !param_env.can_type_implement_copy(self.tcx, ty).is_ok() {
	emit_feature_err(&self.tcx.sess.parse_sess,
	"untagged_unions", item.span, GateIssue::Language,
	"unions with non-`Copy` fields are unstable");
	}

It should be possible to change it to ty.needs_drop(self.tcx, param_env).

(note that this will remain valid only as long as nobody changes the implementation of needs_drop to return false for unions without checking! but if we add tests we should be fine)

Turns out... I was wrong from the start, needs_drop always returns false for unions.
Which means we need this:

adt_def.non_enum_variant().fields.iter().any(|field| {
    self.tcx.type_of(field.did).needs_drop(self.tcx, param_env)
})

Also, this shouldn't be nested in the else for the has_dtor check.

[RalfJung]

Behavior if the check failed should then also change from "require feature flag" to "hard error".