Lokathor / bytemuck

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

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

Open LukasKalbertodt opened 3 years ago

LukasKalbertodt commented 3 years ago

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).

Lokathor commented 3 years ago

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.

LukasKalbertodt commented 3 years ago

I have a patch for this: https://github.com/Lokathor/bytemuck/compare/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: https://github.com/rust-lang/rust/issues/90869 (I just reported this issue).


So, how to proceed?

Lokathor commented 3 years ago

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.

LukasKalbertodt commented 2 years ago

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.

ratmice commented 2 years ago

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.