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

Support zeroable and pod traits for fieldless enums #239

Closed
wants to merge 1 commit into from

Conversation

jozanza
Copy link

@jozanza jozanza commented May 9, 2024

Hey folks! I was looking at this PR #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: #230

@zachs18
Copy link
Contributor

zachs18 commented May 9, 2024

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
$ 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 `<Thing as std::fmt::Debug>::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 `<std::io::StderrLock<'_> 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 `<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:993:9: 993:33
   = note: inside `std::io::stdio::print_to::<std::io::Stderr>` 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
Copy link
Owner

Lokathor commented May 9, 2024

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

@Lokathor
Copy link
Owner

Lokathor commented May 9, 2024

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
Copy link
Author

jozanza commented May 11, 2024

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
Copy link
Owner

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
Copy link
Author

jozanza commented May 11, 2024

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

@zachs18
Copy link
Contributor

zachs18 commented May 11, 2024

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

@Lokathor Lokathor closed this May 28, 2024
@Lokathor
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants