Fields must fit in the type, even for repr(Rust)#2166
Fields must fit in the type, even for repr(Rust)#2166Darksonn wants to merge 3 commits intorust-lang:masterfrom
Conversation
|
@rustbot label I-lang-nominated |
src/type-layout.md
Outdated
|
|
||
| 1. The fields are properly aligned. | ||
| 2. The fields do not overlap. | ||
| 2. The fields do not overlap and fit in the struct. |
There was a problem hiding this comment.
The surrounding text is about " All user-defined composite types (structs, enums, and unions) " (layout.repr.intro). If some part of this text is meant to apply only to structs, that needs to be made clear upfront, not in the middle of a section.
There was a problem hiding this comment.
Surely this section cannot apply to unions. It says the fields are not allowed to overlap.
There was a problem hiding this comment.
It clearly shouldn't apply to unions, but I can't find anything in the wording that would prevent it from applying to unions.
There was a problem hiding this comment.
Clearly if this was for structs only, the section identifier should make that explicit. But it is layout.repr.rust.layout, no "struct" anywhere.
There was a problem hiding this comment.
How about something like:
| 2. The fields do not overlap and fit in the struct. | |
| 2. The fields fit in the type: | |
| a. For structs, this means the fields do not overlap. | |
| b. For enums and unions, this means that fields from the same variant do not overlap (but fields from different variants can overlap). |
I'm not sure if it's worth splitting enum variants and union fields into separate sentences.
There was a problem hiding this comment.
Alternate wording:
The only data layout guarantees made by this representation are those required for soundness. They are:
1. The fields are properly aligned.
- 2. The fields do not overlap and fit in the struct.
- 3. The alignment of the type is at least the maximum alignment of its fields.
+ 2. The alignment of the type is at least the maximum alignment of its fields.
+ 3. The fields fit in the type.
+ 4. Fields that can co-exist do not overlap.
r[layout.repr.rust.alignment]
Formally, the first guarantee means that the offset of any field is divisible by that field's alignment.
r[layout.repr.rust.field-storage]
-The second guarantee means that the fields can be ordered such that the offset plus the size of each field is less than or equal to the offset of the next field in the ordering, or for the last field, the size of the struct. The ordering does not have to be the same as the order in which the fields are specified in the declaration of the type.
+The third guarantee means that for any field, the field's offset plus its size must be less than or equal to the size of the type.
+
+r[layout.repr.rust.field-overlap]
+Fields are said to overlap unless the size plus offset of one field is less than or equal to the offset of the other field. Fields are said to co-exist if the type can contain both fields at the same time, i.e. all fields of a struct co-exist, only fields of the same enum variant co-exist, and fields never co-exist in a union.
-Be aware that the second guarantee does not imply that the fields have distinct addresses: zero-sized types may have the same address as other fields in the same struct.
+Be aware that this does not imply that the fields have distinct addresses: zero-sized types may have the same address as other fields without overlapping with them.
r[layout.repr.rust.unspecified]
There are no other guarantees of data layout made by this representation.There was a problem hiding this comment.
Fields are said to overlap unless the size plus offset of one field is less than or equal to the offset of the other field.
I'd prefer a positive definition instead of a negative one:
Two fields are said to overlap if the size plus offset of one field is strictly within the range of the other field.
Or use the opposite term:
Two fields are non-overlapping if the size plus offset of one field is less than or equal to the offset of the other field (field1.offset + field1.size <= field2.offset || field2.offset + field2.size <= field1.offset).
(It took me reading this multiple times until I realized you hid a disjunction here so I think the condition is worth spelling out formally, not just in English.)
There was a problem hiding this comment.
Maybe the best is to keep the "you can order fields such that" wording ...
Two fields are said to overlap if the size plus offset of one field is strictly within the range of the other field.
This definition does not work because it does not cover non-ZST fields of the same size and offset.
There was a problem hiding this comment.
Does using a niche for an enum discriminant change how we want to word the overlap guarantee?
I assume discriminants are not considered fields, but unless we say that somewhere, there could be ambiguity.
There was a problem hiding this comment.
Yeah for the purpose of this we do not consider the discriminant to be a field.
At least @scottmcm seems to agree that for structs we should guarantee that even uninhabited structs have space for all their fields (and presumably, the fields remain disjoint). However, as currently worded the text is not just about structs but also about unions (where the no-overlap guarantee makes absolutely no sense) and enums. This is almost certainly also an accident. Not sure if it's best to discuss both of these at once -- I assume a PR that just rearranges the text to make it about structs only would not need much discussion. |
|
True, but I think we do want the other existing guarantees for union and enums:
|
|
I filed a PR to fix the struct/union/enum issue: |
This comment has been minimized.
This comment has been minimized.
2cfcc57 to
c2e6c9a
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
c2e6c9a to
be8acd0
Compare
src/type-layout.md
Outdated
|
|
||
| A field is considered constructible if it is possible to create a value of the type containing the field. | ||
|
|
||
| For example, given `enum E { A { x: u32 }, B { y: u32, z: ! } }`, the field `x` is constructible because you can create the value `E::A { x: 0 }`, but the fields `y` and `z` are not constructible because the type of `z` is uninhabited, so it is impossible to create an `E::B` value. |
There was a problem hiding this comment.
It seems easier to define this as
- all fields of a struct and union are constructible
- for an enum, fields of a variant are constructible if all fields of that variant are inhabited
Is there a definition of "inhabited" we can refer to?
There was a problem hiding this comment.
Your proposed definition makes the guarantee apply to structs even if there are ! fields. But if a struct has a field of type !, surely none of the fields are constructible? It seems like changing the guarantee is a better fix than redefining constructible to mean something less natural.
There was a problem hiding this comment.
Oh, I didn't realize you meant to exclude structs with uninhabited fields. I think we do want to guarantee that fields are disjoint and fit in the type even for such structs -- that seems pretty much settled to me.
|
The PR title seems outdated now? The PR aims to make new guarantees not just for structs. |
src/type-layout.md
Outdated
|
|
||
| r[layout.repr.rust.layout.constructible] | ||
|
|
||
| A field is considered constructible if it is possible to create a value of the type containing the field. |
There was a problem hiding this comment.
| A field is considered constructible if it is possible to create a value of the type containing the field. | |
| A field is considered constructible if it is possible to create a value containing the field. |
Below the type is E; there is no type E::B. So the next line doesn't follow from the existing definition.
|
General vibes: the change to add the "constructible" concept seems great, and reserves enough space for things we might want to be able to do. We can then separately nail down when we do and don't want to mandate constructibility. |
src/type-layout.md
Outdated
|
|
||
| r[layout.repr.rust.layout.constructible] | ||
|
|
||
| A field is considered constructible if it is possible to create a value of the type containing the field. |
There was a problem hiding this comment.
How about:
| A field is considered constructible if it is possible to create a value of the type containing the field. | |
| A field is considered constructible if it is part of an inhabited struct or enum variant, or part of a union. |
There was a problem hiding this comment.
On second thought speaking in terms of individual fields feels weird, e.g. I could imagine:
enum Foo {
A { x: u32 },
B { x: u32, y: ! },
}and I could imagine (I kinda want even) that we add a language feature whereby we can write foo.x because that field exists in all variants. Is x constructible then? Well no because x refers to two "real" fields.
So I'd rather say: a type has space for its inhabited variants, and an inhabited variant has space for its fields.
There was a problem hiding this comment.
I think under this section of the reference, those xs have to be separate fields because otherwise a field might have multiple offsets, and this section talks about fields as if they have a single offset.
src/type-layout.md
Outdated
|
|
||
| r[layout.repr.rust.layout.constructible] | ||
|
|
||
| A field is considered constructible if it is possible to create a value of the type containing the field. |
There was a problem hiding this comment.
I think this definition isn't quite right, because in the example E is the type and you can create a value of that type, which would mean that y is still "constructible" because it's talking about type.
Perhaps this should be talking about variants instead? (At least in the compiler even structs have variants, just exactly one variant, so the reference might want to adopt that approach.)
There was a problem hiding this comment.
You can create values of type E, but you can't create a value of type E that contains the field y.
|
I thought a bit more about it, and here is a different possible wording.
This drops the 'constructible' wording, which I liked, but I think the above wording has several advantages:
|
This clarifies the
repr(Rust)layout rules slightly by specifying that fields must fit in their struct. That is, the offset of a field plus its size must be less than the size of the struct.Accidental stabilization. One question that needs to be addressed before we can merge this is the fact that we have guaranteed that fields of
repr(Rust)do not overlap, even though one can imagine structs where this is not the case when the struct is uninhabited. For example:So as part of this PR I am posing the following question to the lang team:
In any case, I think we should definitely guarantee that fields fit in the struct for inhabited
repr(Rust)structs.Context: #t-lang > ZST Uninhabited types with non-ZST fields @ 💬