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) could accept singleton ZST with alignment > 1. #100954

Closed
pnkfelix opened this issue Aug 24, 2022 · 11 comments
Closed

repr(transparent) could accept singleton ZST with alignment > 1. #100954

pnkfelix opened this issue Aug 24, 2022 · 11 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@pnkfelix
Copy link
Member

I tried this code:

#[repr(transparent)]
struct R([usize; 0]);

I expected to see this happen: That code could be accepted.

Instead, this happened: The code is rejected, with the diagnostic:

error[[E0691]](https://doc.rust-lang.org/nightly/error-index.html#E0691): zero-sized field in transparent struct has alignment larger than 1
 --> src/lib.rs:2:10
  |
2 | struct R([usize; 0]);
  |          ^^^^^^^^^^ has alignment larger than 1

For more information about this error, try `rustc --explain E0691`.

That is, we could loosen the repr(transparent) rules to say that it "requires at most one field that occupies memory"; this is a loosening of our current rules on two levels:

  • It would allow for there to be no non-ZST fields in the struct. Today we require exactly one non-ZST field in a repr(transparent) struct
  • It would replace the predicate "ZST/non-ZST" with "occupies/does-not-occupy memory" -- the distinction there being that a ZST with alignment > 1 can cause padding to be injected, and thus memory to be allocated.

Hat tip to @RalfJung for pointing this oddity out as part of PR #99972

@pnkfelix pnkfelix added C-bug Category: This is a bug. C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Aug 24, 2022
@pnkfelix
Copy link
Member Author

pnkfelix commented Aug 24, 2022

Nominating for discussion at lang team meeting.

  1. Does this deserve an formal major change proposal, or is this a small enough tweak that we could just amend the RFC and the reference as well as revise the implementation?
  2. Is this worth doing at all?
    • I was about to claim that it seems like a minor issue, but then I realized of course that we accept #[repr(transparent)] struct R([usize; 1]);, so one can imagine getting annoyed by this corner case.

@rustbot label: +I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 24, 2022
@RalfJung
Copy link
Member

That is, we could loosen the repr(transparent) rules to say that it "requires at most one field that occupies memory"; this is a loosening of our current rules on two levels:

I think this formulation makes it not at all clear that alignment matters.

I would just say, it can have at most one field that is not a 1-ZST.

(Also Cc #96921)

@RalfJung
Copy link
Member

we accept #[repr(transparent)] struct R([usize; 1]);, so one can imagine getting annoyed by this corner case.

Yeah, and it is possible to construct repr(transparent) where all fields are ZST via generics. So if we have logic bugs in the compiler because of this (which I kind of expect we do, a bunch of code will be looking for "the first non-ZST field" and I am not sure if it always has code to handle the situation where no field is found), they are already exposed, just harder to hit.

@scottmcm
Copy link
Member

scottmcm commented Sep 6, 2022

I would just say, it can have at most one field that is not a 1-ZST.

I think this rule makes the most sense to me.

After all, as you said, one can do

#[repr(transparent)]
struct Rtemp<const N: usize>([usize; N]);
type R = Rtemp<0>;

which I think is fine.

Or, more generally, I think that repr(transparent) on a one-field struct should always work, since it does on single field of generic type.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 13, 2022

Discussed in lang-team meeting to general happiness and accord. Based on current attendance (not everyone), we would welcome a PR implementing this change and we could FCP that PR to confirm.

An alternate phrasing of the rule is:

  • If you ignore any 1-ZST fields, you can have at most one field in your struct remaining.

@nikomatsakis
Copy link
Contributor

@tmandry proposes:

"You can have at most one field in your struct, ignoring any 1-ZST fields."

@nikomatsakis
Copy link
Contributor

Anything that doesn't include a double negative works for me :P

@scottmcm
Copy link
Member

I was going to add an induction definition (one-field is fine; adding 1-ZST fields is fine), but it turns out that this is legal:

#[repr(transparent)]
struct Foo {}

Which means that the current rule isn't actually

Today we require exactly one non-ZST field in a repr(transparent) struct

And thus I'm even more in favour of the "at most one" version of the rule, since that seems to be what we're already doing in other situations.

@pnkfelix
Copy link
Member Author

removing nomination; we discussed this last week.

@rustbot label: -I-lang-nominated

@traviscross
Copy link
Contributor

Somewhere along the way, this was done, as the code in the description is accepted now.

@madsmtm
Copy link
Contributor

madsmtm commented Jul 16, 2024

And there's an existing test for it here:

#[repr(transparent)]
struct WrapsZstWithAlignment([i32; 0]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

No branches or pull requests

7 participants