Fields of enums and unions should be allowed to overlap

[Darksonn]

This fixes a bug that we currently say that fields of enums and unions must not overlap.

As mentioned in #2166 (comment) there is currently no exception for uninhabited types. We should determine whether such an exception should be added, but this is a pre-existing problem so let's not block this PR on that.

This PR does not add any statement about overlap for fields of the same variant of an enum because the uninhabited type concern should be addressed first before we say anything about enums.

[RalfJung]

LGTM with the nit fixed -- the only change here is fixing what clearly looks like an editorial issue.

[RalfJung]

Lol this seems to have been wrong for years (#1152 landed in 2022) and nobody complained in all that time that union fields apparently must be disjoint?^^

[Darksonn]

Any chance I could get a review for this one?

[scottmcm]

Makes sense to me!

And I guess, Ralf, further evidence that nobody really cares about repr(Rust) unions and only ever uses repr(C) ones...

View changes since this review

[RalfJung]

I'm a bit surprised this even needs lang team discussion -- it doesn't add any new guarantees, it just removes the accidental guarantee that enum/union fields do not overlap (which is obviously nonsense). So I had expected this to be treated as an editorial change.

[ehuss]

This PR does not add any statement about overlap for fields of the same variant of an enum because the uninhabited type concern should be addressed first before we say anything about enums.

I'm not quite following, why would an enum variant be different from a struct in this regard?

[Darksonn]

The only difference is that the reference does not already contain a statement about enums, and this was intended to be an editorial change only.

[RalfJung]

We have so far deliberately left open the possibility for this type to have size 0:

enum E {
  A(i32, !),
  B
}

[ehuss]

The only difference is that the reference does not already contain a statement about enums, and this was intended to be an editorial change only.

I thought that was the purpose of f9ec1d2, to make this also cover struct-like variants and possibly also tuple-like variants (which I consider just a special case where the struct fields have numbers instead of identifiers)?

We have so far deliberately left open the possibility for this type to have size 0:

That seems like it is more of a question of: Do uninhabited variants count for the purpose of determining the number of variants (which isn't relevant right now because that is an unstable feature) and does a single-variant fieldless enum have size zero (which doesn't seem to be in conflict here?). I'm not quite following how the field layout guarantees would be any different here. That is, I wouldn't expect E::A to have different requirements than a tuple of the same type (i32, !), and E::B has no fields (which would be no different than enum Foo { V }).

[RalfJung]

I thought that was the purpose of f9ec1d2, to make this also cover struct-like variants and possibly also tuple-like variants (which I consider just a special case where the struct fields have numbers instead of identifiers)?

The way it is currently written it also covers enums and unions. Not enum variants, but enums themselves. That's clearly nonsense.

That seems like it is more of a question of: Do uninhabited variants count for the purpose of determining the number of variants (which isn't relevant right now because that is an unstable feature) and does a single-variant fieldless enum have size zero (which doesn't seem to be in conflict here?). I'm not quite following how the field layout guarantees would be any different here. That is, I wouldn't expect E::A to have different requirements than a tuple of the same type (i32, !), and E::B has no fields (which would be no different than enum Foo { V }).

If we say that all the fields of a tuple variant fit inside the surrounding enum, we'd force my example enum to be at least 4 bytes large.

[ehuss]

The way it is currently written it also covers enums and unions. Not enum variants, but enums themselves. That's clearly nonsense.

IIRC covering enum variants was the intent. The text could certainly be clarified to say that. I didn't think about unions at the time because they are the stepchild that gets ignored all the time.

If we say that all the fields of a tuple variant fit inside the surrounding enum, we'd force my example enum to be at least 4 bytes large.

When never types are stabilized, we can add a rule to say that uninhabited variants have different properties. I'm not sure how that is relevant right now.

[RalfJung]

Uninhabited types already exist, I just used ! as a shortcut:

enum Void {}

enum E {
  A(i32, Void),
  B
}

So, this is not something we can delegate to some time in the future.

[RalfJung]

IIRC covering enum variants was the intent.

The lang team very explicitly did not want to rule out this layout optimization in the discussion around offset_of! on enums. So I highly doubt they would ever intentionally approve a Reference change that would imply that all fields of an enum fit inside the enum.

We could try to do some careful wording where we specify the layout of enum variants and them separately say how that gets turned into the layout of the entire enum, but no such discussion is currently present, and absent that discussion it is very confusing to talk about the layout of enum variants as if they were a thing in the language that had a layout on its own right.

[Darksonn]

Even if we were to do that, I think it should be a separate PR. I split this out from #2166 in the hopes that this was something that could easily be merged (as it makes no new promises), and then I planned to rebase #2166 so that it no longer makes multiple changes in one PR, as per the review.