Skip to content
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

Closed
mahkoh opened this issue Oct 12, 2020 · 26 comments · Fixed by #86279
Closed

repr(transparent) on generic type skips "exactly one non-zero-sized field" check #77841

mahkoh opened this issue Oct 12, 2020 · 26 comments · Fixed by #86279
Labels
A-layout Area: Memory layout of types A-zst Area: Zero-sized types (ZST). C-bug Category: This is a bug. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@mahkoh
Copy link
Contributor

mahkoh commented Oct 12, 2020

#[repr(transparent)]
struct X(()); // ERROR: needs exactly one non-zero-sized field, but has 0

But

#[repr(transparent)]
struct X<T>(T);

println!("{}", mem::align_of_val(&X(())));  // 1
println!("{}", mem::size_of_val(&X(())));   // 0

The reference says

The transparent representation can only be used on a struct or an enum with a single variant that has:

  • a single field with non-zero size, and
  • any number of fields with size 0 and alignment 1 (e.g. PhantomData).

I do not see a reason for this restriction. Maybe simply replace "a single" by "at most one" and remove the check.

@mahkoh mahkoh added the C-bug Category: This is a bug. label Oct 12, 2020
@mahkoh
Copy link
Contributor Author

mahkoh commented Oct 12, 2020

Marked as bug because people might assume X<T> values have size > 0 in unsafe code. Unlikely to be the case in real code.

@jyn514 jyn514 added A-layout Area: Memory layout of types T-lang Relevant to the language team, which will review and decide on the PR/issue. A-zst Area: Zero-sized types (ZST). labels Oct 12, 2020
@RalfJung
Copy link
Member

I do not see a reason for this restriction. Maybe simply replace "a single" by "at most one" and remove the check.

Agreed.
Cc @rust-lang/wg-unsafe-code-guidelines

@nikomatsakis
Copy link
Contributor

Discussed in the @rust-lang/lang meeting.

  • We agree with the sense of this but would like a specific semantics -- i.e., if all fields are ZSTs, then we need to define what the ABI behavior of the type is (if there is a single non-ZST field f, then it behaves like f, the "real value" -- but what does it act like if there are no real values?).
  • Perhaps the answer is just "whatever happens if you try to pass ()".
  • If someone can supply a concrete rule that makes sense, we would like to approve it =)

@nikomatsakis
Copy link
Contributor

To clarify:

I do not see a reason for this restriction. Maybe simply replace "a single" by "at most one" and remove the check.

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

@chorman0773
Copy link
Contributor

A possible rule is to use the ABI of the first 1-ZST Field if there are no non 1-ZST fields.
One question I have, what about types with size 0 and alignment >1 (0 sized arrays like [u16;0] are examples). Under this rule, these types would still seem to be excluded. That could probably be added to the first part at the same time as it's lifted to allow zero of such types. If a ZST that has an alignment >1 is in a repr(transparent) struct, the ABI follows that ZST specifically.

@RalfJung
Copy link
Member

AFAIK "zero-sized-field" for repr(transparent) means 1-ZST. Everything else would be buggy I think...

So a ZST with alignment greater 1 behaves like a non-ZST. I don't think that should change.

@chorman0773
Copy link
Contributor

AFAIK "zero-sized-field" for repr(transparent) means 1-ZST

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.

@RalfJung
Copy link
Member

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]);

@RalfJung
Copy link
Member

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

  • All fields except for 1 must be zero-sized
  • And additionally all zero-sized fields must have alignment 1

@chorman0773
Copy link
Contributor

On the other hand, this is also rejected

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 [i32;0], but its not permitted to include [i32;0] directly, even if all other fields are 1-ZSTs.

@RalfJung
Copy link
Member

The consistency issue raised above would apply here, if it's possible to have a generic transparent type which is monomorphized with [i32;0], but its not permitted to include [i32;0] directly, even if all other fields are 1-ZSTs.

Fully agreed.

@nikomatsakis
Copy link
Contributor

Point of clarification: What is a 1-ZST? A ZST with alignment 1?

@RalfJung
Copy link
Member

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

@nikomatsakis
Copy link
Contributor

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., [i32; 0]).

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 repr(transparent) type T must meet the following rules:

  • It may have any number of 1-ZST fields
  • In addition, it may have at most one other field of type U

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 ()?

@chorman0773
Copy link
Contributor

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 (), though.

@nikomatsakis
Copy link
Contributor

@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 doubt it matters much in practice since I imagine that all 1-ZSTs basically behave the same with respect to the ABI, though.

@RalfJung
Copy link
Member

I wouldn't call () "arbitrary". It is basically the canonical "informationless type" in Rust, or "final element" in category theory speak -- similar to how ! is the canonical "uninhabited type" or "initial element".

So, specifying () as a canonical representative of 1-ZST makes a lot of sense IMO.

@nikomatsakis
Copy link
Contributor

That's a good point. So here is a proposed set of rules:

  • A repr(transparent) type T must meet the following rules:
    • It may have any number of 1-ZST fields
    • In addition, it may have at most one other field of type U

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 (), which is the "canonical" 1-ZST.

I'm going to go ahead and kick off an FCP request...

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 26, 2020

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 26, 2020
@scottmcm
Copy link
Member

scottmcm commented Oct 26, 2020

+1 to not having anything order-dependent in a braced-struct.

Hmm, the lint considers () an improper_ctype. But then an empty struct is UB(!?) in C, so I guess there isn't a canonical ABI for this.

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 26, 2020
@rfcbot
Copy link

rfcbot commented Oct 26, 2020

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 26, 2020
@comex
Copy link
Contributor

comex commented Oct 26, 2020

Hmm, the lint considers () an improper_ctype. But then an empty struct is UB(!?) in C, so I guess there isn't a canonical ABI for this.

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.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 26, 2020

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 () is kind of "as good as any other". (There are some C structs, as I understand it, that are treated somewhat specially by ABIs, but very few -- I think maybe having to do with complex types? Can't remember.)

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 5, 2020
@rfcbot
Copy link

rfcbot commented Nov 5, 2020

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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Nov 5, 2020
@nikomatsakis
Copy link
Contributor

@mahkoh are you interested in preparing a fix for this?

@nikomatsakis
Copy link
Contributor

Removing nomination as we have resolved this fr the lang-team's perspective, though we still need to do implementation work here.

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Nov 12, 2020
@bors bors closed this as completed in 456a032 Jun 24, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 17, 2023
…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.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 19, 2023
…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.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…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.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Area: Memory layout of types A-zst Area: Zero-sized types (ZST). C-bug Category: This is a bug. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants