Lokathor / bytemuck

A crate for mucking around with piles of bytes
https://docs.rs/bytemuck
Apache License 2.0
738 stars 80 forks source link

Support zeroable and pod traits for fieldless enums #239

Closed jozanza closed 6 months ago

jozanza commented 6 months ago

Hey folks! I was looking at this PR https://github.com/Lokathor/bytemuck/pull/233 and decided to take things a step further so we could finally derive the Pod trait for fieldless enums. Hopefully, this is something this project is interested in supporting. If so, I'm happy to make further updates to clean up this PR. Cheers!

Also related to this issue: https://github.com/Lokathor/bytemuck/issues/230

zachs18 commented 6 months ago

Pod/AnyBitPattern can only be implemented for an enum if every possible value of the discriminant has a valid variant associated with it. For example, the following code compiles under this PR, but has undefined behavior, since x[2] is not a valid instance of Thing.

use bytemuck::{Zeroable, Pod};

#[derive(Clone, Copy, Debug, Zeroable, Pod)]
#[repr(u8)]
enum Thing {
    A = 0,
    B = 1,
}

fn main() {
    let x: &[Thing] = bytemuck::cast_slice(&[0_u8, 1, 2]);
    dbg!(x);
}
Miri output on the above ```rust $ cargo +nightly miri run // ignoring cargo's output [src/main.rs:12:5] x = [ A, B, error: Undefined Behavior: enum value has invalid tag: 0x02 --> src/main.rs:3:23 | 3 | #[derive(Clone, Copy, Debug, Zeroable, Pod)] | ^^^^^ enum value has invalid tag: 0x02 | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information = note: BACKTRACE: = note: inside `::fmt` at src/main.rs:3:23: 3:28 = note: inside `<&Thing as std::fmt::Debug>::fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2343:62: 2343:82 = note: inside closure at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/builders.rs:648:35: 648:47 = note: inside closure at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/builders.rs:424:17: 424:39 = note: inside `std::result::Result::<(), std::fmt::Error>::and_then::<(), {closure@bytemuck::core::fmt::builders::DebugInner<'_, '_>::entry_with<{closure@std::fmt::DebugList<'_, '_>::entry::{closure#0}}>::{closure#0}}>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:1321:22: 1321:27 = note: inside `bytemuck::core::fmt::builders::DebugInner::<'_, '_>::entry_with::<{closure@std::fmt::DebugList<'_, '_>::entry::{closure#0}}>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/builders.rs:416:23: 432:11 = note: inside `std::fmt::DebugList::<'_, '_>::entry` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/builders.rs:648:9: 648:48 = note: inside `std::fmt::DebugList::<'_, '_>::entries::<&Thing, std::slice::Iter<'_, Thing>>` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/builders.rs:695:13: 695:31 = note: inside `<[Thing] as std::fmt::Debug>::fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2571:9: 2571:44 = note: inside `<&[Thing] as std::fmt::Debug>::fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2343:62: 2343:82 = note: inside `<&&[Thing] as std::fmt::Debug>::fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:2343:62: 2343:82 = note: inside `bytemuck::core::fmt::rt::Argument::<'_>::fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/rt.rs:165:63: 165:82 = note: inside `bytemuck::core::fmt::run` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:1207:14: 1207:28 = note: inside `std::fmt::write` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/fmt/mod.rs:1174:26: 1174:61 = note: inside ` as std::io::Write>::write_fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/mod.rs:1835:15: 1835:43 = note: inside `<&std::io::Stderr as std::io::Write>::write_fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1019:9: 1019:36 = note: inside `::write_fmt` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:993:9: 993:33 = note: inside `std::io::stdio::print_to::` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1117:21: 1117:47 = note: inside `std::io::_eprint` at /home/zachary/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/stdio.rs:1205:5: 1205:37 note: inside `main` --> src/main.rs:12:5 | 12 | dbg!(x); | ^^^^^^^ = note: this error originates in the derive macro `Debug` which comes from the expansion of the macro `dbg` (in Nightly builds, run with -Z macro-backtrace for more info) note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace error: aborting due to 1 previous error; 1 warning emitted ```

This PR could be updated to check if there are exactly 2**int_repr_bitsize defined variants, or IMO it'd probably be "good enough" to only support u8 and i8 with exactly 256 variants; having a #[repr(u16)] 65536-variant enum probably already takes too long to compile for anyone to want to do it (takes ~4 seconds on my machine to compile a hello world with such an enum), and a #[repr(u32)] 4294967296-variant enum won't compile at all due to source file length restrictions IIRC.

Lokathor commented 6 months ago

Zeroable we could maybe support in more cases though, if we can determine that zero-bits is a valid value

Lokathor commented 6 months ago

Hmm, if this passed miri then maybe we need more miri tests? dunno if miri supports "should fail miri" the same as normal tests do

jozanza commented 6 months ago

Okay I see we want to avoid that case of undefined behavior. I can definitely add a check for all 256 u8 variants, however, that kills the ergonomics and definitely causes Pod to be infeasible for other integer types.

Is it possible we could just make the enum Pod derive not do the exhaustiveness check and make another trait (let's call it Exhaustive) that enforces the exhaustiveness for Pod enums -- basically requiring repr u8 and all 256 variants to be defined as was mentioned?

I'm personally fine with the undefined behavior for out of range Pod enum variants by default as I just want to use enums in Pod structs and to serialize to/from a u8.

But either way, I'm happy to move forward with whatever works for your project.

Lokathor commented 6 months ago

I'm personally fine with the undefined behavior for out of range Pod enum variants by default

You're the first rust programmer I've ever seen say this.

But no, we don't accept code that leads to UB.

You could look at implementing TryFrom on your enum so that it can convert from an int to the enum type.

jozanza commented 6 months ago

LOL fair enough! Someone needs to take away my Rust card 😂

zachs18 commented 6 months ago

You may want to look into derive(CheckedBitPattern) which does not require that all bit patterns are valid for an enum.

Lokathor commented 6 months ago

Since we fundamentally can't accept this change, I'm gonna close it out.