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

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

Open
LukasKalbertodt opened this issue Sep 9, 2021 · 5 comments

Comments

@LukasKalbertodt
Copy link

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

Lokathor commented Sep 9, 2021

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

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.

@Lokathor
Copy link
Owner

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

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

ratmice commented Jan 7, 2022

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.

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 a pull request may close this issue.

3 participants