add support for deriving NoUninit on enums with fields

[Freax13]

We check for padding like we do for structs except that we also consider the enum discriminant when calculating the unpadded size.

[hannahfluch]

any updates on this?

[Lokathor]

good question. I'd totally forgotten about this PR before putting out the derive update yesterday. I'll try to sit down and review it soon.

[stegaBOB]

Would love to get this in! One thing that might be missing here is the docs for the NoUninit trait itself. They currently say you can't have an enum with fields (not sure if theres a reason for that other than not having proc macro support?). It also says only repr(Int) and not repr(C) enums are allowed.

bytemuck/src/no_uninit.rs

Line 51 in 028ff3b

/// * Enums must have only fieldless variants

[Lokathor]

I believe that comment is based on the fact that, in the general case, if there is an enum variant with a field, the tag byte will always be initialized but the field bytes might not be initialized if it's currently been tagged as some other variant

consider:

pub enum Foo {
  TagEmpty,
  TagField(u8),
}

the byte the u8 sits in is only sometimes initialized.

[stegaBOB]

That makes sense. This wouldn't be the case if all variants have the same size and there's no padding though, right?

[Lokathor]

I think so. Actually I don't know how the tag bytes are considered to work if there's more than one tag byte for alignment.

pub enum ColorUsize {
  Red(usize),
  Green(usize),
}

In an enum like this, assuming 64-bit for sake of argument, the tag will be 8 bytes so that the usize fields are aligned to 8. In such a case, is the tag 1 byte and then there's 7 uninit bytes, or is the tag the full 8 bytes and so all tag bytes are initialized. I hope it's the latter, but we should confirm that somewhere. Unfortunately I won't have much Rust time until the weekend, so if someone else can confirm that before then that would be great.

[zachs18]

pub enum ColorUsize {
  Red(usize),
  Green(usize),
}

In an enum like this, assuming 64-bit for sake of argument, the tag will be 8 bytes so that the usize fields are aligned to 8. In such a case, is the tag 1 byte and then there's 7 uninit bytes, or is the tag the full 8 bytes and so all tag bytes are initialized. I hope it's the latter, but we should confirm that somewhere. Unfortunately I won't have much Rust time until the weekend, so if someone else can confirm that before then that would be great.

As written, this enum is (implicitly) repr(Rust) (and is not "option-like"), so it has essentially no guarantees about its tag/discriminant representation. In practice, the tag will be a usize, but that is not guaranteed and may change in newer compiler versions (or under things like -Zrandomize-layout), so should not be relied upon by unsafe code.

If the enum was #[repr(usize)], (or #[repr(u64)] on 64-bit, etc), then it would have a fully defined layout, which for this particular enum does not have any padding. Likewise with #[repr(C, usize)] (which would give the same layout as #[repr(usize)] for this enum in particular).

[Lokathor]

Alright, then we can absolutely adjust the wording on the trait to allow in additional types of enum that we do get the guarantees for.

[fu5ha]

Finally have some bandwidth towards open source again recently, so I'm gonna try to review this in the next few days :)

[fu5ha]

As discussed, I believe the full requirements are:

• All variants must be themselves NoUninit
• All variants must be the same size
• All variants must have no extra padding required when padded to the alignment of the highest aligned variant
• The highest aligned variant must have alignment <= the alignment of the discriminant (thus the discriminant must also be explicitly annotated)

[Freax13]

As discussed, I believe the full requirements are:

• All variants must be themselves NoUninit

• All variants must be the same size

• All variants must have no extra padding required when padded to the alignment of the highest aligned variant

• The highest aligned variant must have alignment <= the alignment of the discriminant (thus the discriminant must also be explicitly annotated)

The implementation provided in this PR checks all of these requirements by checking that a packed struct of <DISCRIMINANT>, <VARIANT FIELDS...> for each variant has the exact same size as the enum itself and that every field implements NoUninit. These are the same checks we use for normal structs (except for the discriminant field, obviously). I believe that covers all these requirements.

... (thus the discriminant must also be explicitly annotated)

Hmm, when I originally wrote the code for this PR, I made core::ffi::c_int the default if no explicit discriminant type is specified, but now I can't find a guarantee for this. Looking at rust-lang/rust#107592, it seems that this is the default for most targets, but there are some targets where 8-bit discriminants are used instead.
Maybe instead of hard-coding core::ffi::c_int, we can just declare a helper field-less enum tagged with #[repr(C)]. If we assume that discriminant has the same size for all #[repr(C)] enums, we can use the helper enum instead of core::ffi::c_int.

[Freax13]

... (thus the discriminant must also be explicitly annotated)

Hmm, when I originally wrote the code for this PR, I made core::ffi::c_int the default if no explicit discriminant type is specified, but now I can't find a guarantee for this. Looking at rust-lang/rust#107592, it seems that this is the default for most targets, but there are some targets where 8-bit discriminants are used instead.

I added some code that computes the integer type of the correct size for the discriminant of #[repr(C)] enums at compile time and replaced the uses of core::ffi::c_int with that. This assumes that the discriminant for all #[repr(C)] enums has the same size, but I think that's a reasonable assumption to make.

[Lokathor]

i think we should error out and require a specific integer type instead of guessing at the probable integer type.

In general, the derive should always error out if we're not 100% sure. Any user can "override" our hesitation by just writing the manual impl, if they're that confident we've been too conservative.

[Freax13]

The alternative would be to not assume the discriminant size and derive it by duplicating the type, removing the fields, but keeping any explicit discriminant values, and then looking at the resulting enum's size. That's literally the representation for #[repr(C)] enums as spec'ed. @Lokathor Would you be okay with an implementation based on that?

[Lokathor]

Yeah that would be good. If we're following a spec it's fine.

[Freax13]

I updated the desugaring to transform enums with fields into fieldless enums (as described in the reference) and use that to get the size of the discriminant.

[Lokathor]

@fu5ha do you want to do a final check on this before a merge?

[fu5ha]

@Lokathor yes, reviewing it now

[fu5ha]

A couple of minor comments. In addition to the suggestions/comments in the code, the docs for both the core NoUninit trait as well as the NoUninit derive macro need updating. They should enumerate exactly the requirements for when field-ful(?) enums are supported, and link to reference for further info.

[Freax13]

Thanks for the review @fu5ha !

[fu5ha]

Thanks, pushed a tiny update to the NoUninit docs, otherwise looks good.

My only hesitation left is the fact we're generating a non-hygenic BlahDiscriminant type for all #[repr(C)] enums now, but I don't immediately see a way to prevent that. We already do this with a BlahBits type for the CheckedBitPattern derive macro, so it's not un-heardof... @Lokathor what do you think?

[Freax13]

My only hesitation left is the fact we're generating a non-hygenic BlahDiscriminant type for all #[repr(C)] enums now, but I don't immediately see a way to prevent that. We already do this with a BlahBits type for the CheckedBitPattern derive macro, so it's not un-heardof... @Lokathor what do you think?

The NoUninit implementation is wrapped in an anonymous constant scope (const _: () = { ... };), so the BlahDiscriminant type can't be named outside that scope. This isn't true for CheckedBitPattern, though; in that case that's more of a feature because it's part of the BlahBits type as you already pointed out.

[fu5ha]

Oh! For some reason I thought the BlahDiscriminant existed outside that anonymous scope for the NoUninit case too, but you're right, it is contained inside. Excellent! Then ignore my hesitancy :D

LGTM!

[fu5ha]

Wait, hm. For the CheckedBitPattern case, could we not get rid of the BlahRawDiscriminant and just inline the current type expression into the BlahBits type instead? I think I would prefer that

[Freax13]

Wait, hm. For the CheckedBitPattern case, could we not get rid of the BlahRawDiscriminant and just inline the current type expression into the BlahBits type instead? I think I would prefer that

Yup, that works, good idea!

[fu5ha]

Awesome :) Thanks for working on this and applying feedback! Think we can merge whenever @Lokathor would like

[stegaBOB]

Hey @Lokathor would it be possible to publish a new version with this change?

[Lokathor]

Ah sorry, I didn't realize it'd been a month already. Let me see what I can do from my phone.

[Lokathor]

Should be all good now, 1.10