Lokathor/bytemuck

`derive(Pod)` should work for `repr(c)` structs with generics fields where all fields have the same type

LukasKalbertodt opened this issue · 5 comments

According to this definition of repr(C) in Rust, structs where all fields have the same type are laid out like arrays (no padding anywhere). The reference doesn't directly state this, but it follows from the given layout algorithm (unless I'm wrong).

This seems accurate, if the generic is also Pod of course.

Edit: I would accept a PR for this but I am unlikely to do it myself because I'm not that big on proc-macros.

I have a patch for this: main...LukasKalbertodt:relax-derives-for-generics

The reason why I did not submit this as PR (yet) is because it makes a specific error messages notably worse: if a field's type does not implement Zeroable/Pod, the current error is something like this:

error[E0277]: the trait bound `Bar: Zeroable` is not satisfied
 --> src/main.rs:6:10
  |
6 |     bar: Bar,
  |          ^^^ the trait `Zeroable` is not implemented for `Bar`
  |
note: required by a bound in `assert_impl`
 --> src/main.rs:4:10
  |
4 | #[derive(Zeroable)]
  |          ^^^^^^^^ required by this bound in `assert_impl`
5 | struct Foo {
  | ------ required by a bound in this
  = note: this error originates in the derive macro `Zeroable` (in Nightly builds, run with -Z macro-backtrace for more info)

With this change, the error is:

error[E0277]: the trait bound `Bar: bytemuck::zeroable::Zeroable` is not satisfied
 --> examples/test.rs:4:23
  |
4 | #[derive(Clone, Copy, Zeroable, Pod)]
  |                       ^^^^^^^^ the trait `bytemuck::zeroable::Zeroable` is not implemented for `Bar`
  |
  = help: see issue #48214

This is basically due to this: rust-lang/rust#90869 (I just reported this issue).


So, how to proceed?

  • (a) Declare this error message unimportant and merge my changes now already
  • (b) Wait for the Rust diagnostic issue to be fixed
  • (c) Change my patch to only include the worse error message in the generic cases. This would be possible, doesn't make anything that works today worse, but would have more code and still has bad error messages for the generic case.

Option B seems fine if the error message is fixed in a timely manner, which is usually the case (thank you ekuber and co <3).

If the issue lingers we can go with Option A if we have to.

Since this doesn't actually block Pod from being implemented manually then it's probably fine to wait a little longer.

fixed in a timely manner, which is usually the case (thank you ekuber and co <3).

Hui, it was indeed fixed very quickly. Those people are amazing!

I will soon submit my changes as PR. Whenever I get to it. I guess it's fine to still wait a bit, maybe until the fix is on stable or so.

Nice work! A further relaxation might be type-layout,
I believe that the restrictions can be relaxed a little more to all non zero sized types have the same type.

there is this note, however I don't see how it applies to repr(C).

C++, in contrast, gives empty structs a size of 1

Since it seems relatively related, I thought I might mention it here, but should open up a different issue unless I'm missing something.