Make Pod and Zeroable derives work for some generic structs and improve docs
#83
Conversation
Before, the struct itself already needed to have `T: Zeroable` bounds in order for the derive to work. However, we can also add those bounds to the `Zeroable` impl itself. Meaning that the inner types only need to be zeroable in order to implement `Zeroable` for the outer type. This is in line with how derives like `Clone` work.
Before, the check that all struct field types implement `Pod` was done in some unnamed `const`. With this commit, those checks are `where` bounds on the generated impl instead. This is mainly a preparation for allowing generic parameters for `derive(Pod)` in a few situations. But it is also slightly simpler than the previous version.
|
I thought I had replied last week but I guess it didn't post, very sorry. I like what I see but I literally don't use proc-macros at all so I'd like at least one other person to have a look at this and check that things seem right. |
|
The error message for when the types are different looks to be wrong now. Because the derive can be done for generic types, just not ones that have different types. |
| /// The macro ensures that the struct follows all the safety requirements for | ||
| /// the `Zeroable` trait. If your struct has generic fields, the generated | ||
| /// `impl` will have a `T: Zeroable` bound for each generic type `T` of your | ||
| /// struct. |
There was a problem hiding this comment.
This says it will add a T: Zeroable bound, but for a struct like struct Foo<T: Iterator> { foo: T::Item } it looks like it will actually generate a T::Item: Zeroable bound (which is more correct, but not what derives generally do).
There was a problem hiding this comment.
This. I personally prefer what this derive does, but it's important that whatever choice is made, it's done being aware that it's not the stdlib's approach (cc @Lokathor).
I personally prefer the approach here, because the stdlib one is what makes:
#[derive(PartialEq, Eq)]
struct Ptr<T>(*const T);only yield comparable pointers when the pointees are.
There was a problem hiding this comment.
This is a tough one.
- I'd generally prefer that the derives be "extra picky" because this is an unsafe trait after all, and you can always just implement manually if you think you know better than the derive's reasoning.
- I do agree that the standard library is a little bit wrong on this one, and I've been bit before by a standard library derive being too conservative (when I wrote
VolAddress<T,R,W>).
However, I think that since this is becoming an increasingly used crate we should have the derives work like the stdlib derives. Don't want people to have any sudden surprises happening.
There was a problem hiding this comment.
The advantage of the default approach will be that we'll have correctly-spanned error messages even in older rust versions 🙂
There was a problem hiding this comment.
Nice initiative 🙂
- I'd not recommend enabling
syn's extra-traits, at least not for a syntactical type comparison which is nonetheless brittle; - I'd thus delegate to a static assertion that the types are equal (if you still want the nice error message, then you can use
.to_string()to perform comparison withoutextra-traits, but there should be a const assertion that the types are nonetheless equal). - Regarding
Pod, it could be nice to also supportrepr(C)/repr(transparent)wrappers, as well asrepr(packed)structs (for a follow-up PR, I imagine).- Not necessarily a good thing, but it is possible to soundly implement
Podfor a fully generic struct, by relying on post-monomorphization errors: those are better than panics, but worse than classic trait const assertions. Indeed, if the code "links" (performs codegen) correctly, then it won't panic; but it can still fail to codegen despitecargo checkpassing.
If you are interested, I got a proof-of-concept for anAsBytestrait (which is the part thatPodis checking, there), in here.
- Not necessarily a good thing, but it is possible to soundly implement
| @@ -15,7 +15,7 @@ name = "bytemuck_derive" | |||
| proc-macro = true | |||
|
|
|||
| [dependencies] | |||
| syn = "1" | |||
| syn = { version = "1", features = ["extra-traits"] } | |||
There was a problem hiding this comment.
| syn = { version = "1", features = ["extra-traits"] } | |
| syn = "1" |
extra-traits lengthens compile-time quite significantly, so it should only be enabled through an opt-in feature, never unconditionally. I think it's used for a type comparison below, so another approach will be needed (emit code that asserts type equality, rather than use the syntactical input)
|
|
||
| // Merge existing bounds with our bounds. | ||
| let mut where_clause = original_where_clause | ||
| .map(|w| if w.predicates.trailing_punct() { quote! { #w } } else { quote! { #w, }}) |
There was a problem hiding this comment.
| .map(|w| if w.predicates.trailing_punct() { quote! { #w } } else { quote! { #w, }}) | |
| .cloned() |
(combined with a .predicates.extend(<iterator of extra bounds>))
| let field_types = get_struct_fields(input)?; | ||
| let all_fields_same_type = field_types.iter() | ||
| .zip(field_types.iter().skip(1)) | ||
| .all(|(f0, f1)| f0.ty == f1.ty); |
There was a problem hiding this comment.
I suspect this will be unsound in the presence of hygienic macros / def_site spans: Foo and Foo could refer to distinct types should the spans be up for it (Playground).
| /// The macro ensures that the struct follows all the safety requirements for | ||
| /// the `Zeroable` trait. If your struct has generic fields, the generated | ||
| /// `impl` will have a `T: Zeroable` bound for each generic type `T` of your | ||
| /// struct. |
There was a problem hiding this comment.
The advantage of the default approach will be that we'll have correctly-spanned error messages even in older rust versions 🙂
|
Sorry for the late reply! Unfortunately, I don't need this change anymore and don't really have the energy to work on this right now. Since this PR would require a non-trivial rebase and has several unresolved problems, I will simply close it now. I still think the general direction of this PR is good and that the derives should be generalized. But yeah, closing, sorry! |
Fixes #75
Fixes #51
derive(Zeroable)now works for structs with generic fields. Theimplis bounded withT: Zeroablefor each struct fieldT.derive(Pod)now works for structs where all fields have the same generic type. Again, theimplis bounded withT: Podthen.Note: the code emitted by the derive macros is slightly different with this PR. This has the unfortunate consequence that some compiler errors (for when a struct's field does not implement
Pod) are worse than before. However, this was due to a Rust issue which has been fixed and will be released on January 13 as 1.58. See #75 for more details.