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

[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.

[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".

[SimonSapin]

#62330 has landed. Are there remaining changes specified in the RFC that are not implemented yet?

[joshtriplett]

It sounds like this has been fully implemented, and the current state of unions is roughly where we want them to be. ManuallyDrop is available, and unions (in stable) don't allow Drop/non-Copy fields.

We'd like to close the remainder of this, and just not allow non-Copy union fields in general, changing that from a feature gate to an error.

@rfcbot close

[rfcbot]

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

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[rfcbot]

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

[RalfJung]

FWIW, when the untagged_unions feature is removed, the AssignToDroppingUnionField unsafety kind can also be removed -- it is impossible to trigger without that feature.

[RalfJung]

In #97995 I am suggesting to allow a few more types for union fields on stable (specifically: mutable references), before we entirely remove the untagged_unions feature.

[rfcbot]

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[raphaelcohn]

@joshtriplett @RalfJung I think removing this has exposed an overlooked use case that should work, but now doesn't.

struct StackWithoutLength<T, const N: usize>(ManuallyDrop<MaybeUninit<[T; N]>>);

union StackWithoutLengthOrHeap<T, const N: usize>
{
	stack_without_length: StackWithoutLength<T, N>,

         copyable: u32
}

This now fails to compile with error[E0740]: unions cannot contain fields that may need dropping (details below), when it seems like it should; trivial analysis suggests that StackWithoutLength` is a new type that can only be manually dropped...

error[E0740]: unions cannot contain fields that may need dropping
 --> swiss-army-knife/src/const_small_vec/StackWithoutLengthOrHeap.rs:7:2
  |
7 |     stack_without_length: StackWithoutLength<T, N>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: a type is guaranteed not to need dropping when it implements `Copy`, or when it is the special `ManuallyDrop<_>` type
help: when the type does not implement `Copy`, wrap it inside a `ManuallyDrop<_>` and ensure it is manually dropped
  |
7 |     stack_without_length: std::mem::ManuallyDrop<StackWithoutLength<T, N>>,
  |                           +++++++++++++++++++++++                        +

[RalfJung]

Never inspecting the fields of a struct for this check was a deliberate design decision, since if that struct was in a different crate, it might start needing drop in future versions of the crate.

In this case, it seems like the struct and union are in the same crate? Possibly an exception could be made for that case, though it would be an extension of the RFC. Please open a feature request issue.

[raphaelcohn]

@RalfJung thanks for the quick reply. The struct and union are in the same crate and both are private too it, as well.

If versions are correctly used, then a future version of the struct should then cause failure.

I won't be opening a feature request.