-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
repr(transparent) on generic type skips "exactly one non-zero-sized field" check #77841
Comments
Marked as bug because people might assume |
Agreed. |
Discussed in the @rust-lang/lang meeting.
|
To clarify:
This removes a restriction, but it now allows a new possibility that didn't exist before, so what are the rules meant to be for that case? (Well, it did exist because of generics, but presumably the rules didn't expect it to exist.) |
A possible rule is to use the ABI of the first 1-ZST Field if there are no non 1-ZST fields. |
AFAIK "zero-sized-field" for So a ZST with alignment greater 1 behaves like a non-ZST. I don't think that should change. |
If it's already permitted, that's brillaint. In which case my point can be reduced to a point of clarification, it would be good if the documentation noted that ("At most one non-zero-sized field or zero-sized field with an alignment greater than 1", wording could probably be changed, I'm much better writing normative text than text for users). If its not already permitted, adding it seems to be an extension of what is being discussed here. |
It's not a permission though, it's a restriction? For example, the following type is rejected, as it should: #[repr(transparent)]
struct Test((), i32, [i32; 0]); |
On the other hand, this is also rejected: #[repr(transparent)]
struct Test((), [i32; 0]); Even though it has exactly one non-1-ZST field. So looks like the actual check is more something like
|
This is what my point is. If we permit transparent types with only ZST's, it seems like it would be equally inconsistent if we don't allow a transparent type with exactly one ZST that isn't a 1-ZST (and any number of 1-ZSTs). The consistency issue raised above would apply here, if it's possible to have a generic transparent type which is monomorphized with |
Fully agreed. |
Point of clarification: What is a 1-ZST? A ZST with alignment 1? |
Yes. (This is terminology the UCG adopted at some point as it was very helpful for layout discussions.) |
OK, so, to restate the above point: We should check whether you can use generics to "sneak" in a ZST with alignment other than 1 (e.g., However, I don't think that you can. Based on some quick play experiments I believe we treat generic types "as if" they were a non-zero-sized field. Ah, ok, I think I understand now. The point is that we enforce (currently) a rule that says "any ZST must have alignment 1 in a repr(transparent) type" but that rule can be circumvented via generics. So, the proposed rule is something like. A
If that other field is present, then T behaves like type U for the purposes of ABIs. Otherwise it behaves like...? Is there a reason we can't just say |
As I mentioned, it may be reasonable to say the first member if all members are 1-ZSTs. I can't see a reason that it wouldn't work just saying |
@chorman0773 Ah, sorry, I missed that comment. I agree that "first member" is an option, though I have a mild preference for avoiding a reliance on the ordering that things were written. That said, I also have a mild preference for avoiding introducing an arbitrary type like |
I wouldn't call So, specifying |
That's a good point. So here is a proposed set of rules:
If that other field is present, then T behaves like type U for ABI purposes. Otherwise, T consists only of 1-ZST types, and it behaves like I'm going to go ahead and kick off an FCP request... @rfcbot fcp merge |
Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
+1 to not having anything order-dependent in a braced-struct. Hmm, the lint considers @rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
GCC-compatible compilers do support empty structs in C mode as an extension, treating as zero-sized types. As a result, ABIs often do document the behavior when C empty structs are passed or returned by value. Unfortunately, C++ has an entirely different interpretation of empty structs, where they get a padding byte to make them not zero-sized. This definitely changes their representation in memory, and sometimes also changes their representation when passing or returning them by value… depending on the ABI. As such, empty structs are best avoided entirely. |
Indeed, ZSTs exist though only in "non-standardized" extensions. The real question here is whether there exists some "special" 1-ZST that would be treated differently by the ABI than any other 1-ZST. I do not believe that is the case, which is why |
The final comment period, with a disposition to merge, 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. The RFC will be merged soon. |
@mahkoh are you interested in preparing a fix for this? |
Removing nomination as we have resolved this fr the lang-team's perspective, though we still need to do implementation work here. |
…compiler-errors repr(transparent): it's fine if the one non-1-ZST field is a ZST This code currently gets rejected: ```rust #[repr(transparent)] struct MyType([u16; 0]) ``` That clearly seems like a bug to me: `repr(transparent)` [got defined ](rust-lang#77841 (comment)) as having any number of 1-ZST fields plus optionally one more field; `MyType` clearly satisfies that definition. This PR changes the `repr(transparent)` logic to actually match that definition.
…errors repr(transparent): it's fine if the one non-1-ZST field is a ZST This code currently gets rejected: ```rust #[repr(transparent)] struct MyType([u16; 0]) ``` That clearly seems like a bug to me: `repr(transparent)` [got defined ](rust-lang/rust#77841 (comment)) as having any number of 1-ZST fields plus optionally one more field; `MyType` clearly satisfies that definition. This PR changes the `repr(transparent)` logic to actually match that definition.
…errors repr(transparent): it's fine if the one non-1-ZST field is a ZST This code currently gets rejected: ```rust #[repr(transparent)] struct MyType([u16; 0]) ``` That clearly seems like a bug to me: `repr(transparent)` [got defined ](rust-lang/rust#77841 (comment)) as having any number of 1-ZST fields plus optionally one more field; `MyType` clearly satisfies that definition. This PR changes the `repr(transparent)` logic to actually match that definition.
…errors repr(transparent): it's fine if the one non-1-ZST field is a ZST This code currently gets rejected: ```rust #[repr(transparent)] struct MyType([u16; 0]) ``` That clearly seems like a bug to me: `repr(transparent)` [got defined ](rust-lang/rust#77841 (comment)) as having any number of 1-ZST fields plus optionally one more field; `MyType` clearly satisfies that definition. This PR changes the `repr(transparent)` logic to actually match that definition.
But
The reference says
I do not see a reason for this restriction. Maybe simply replace "a single" by "at most one" and remove the check.
The text was updated successfully, but these errors were encountered: