Support for derive on unions?

[mystor]

I don't know what the correct behavior for synstructure should be here. In the update for syn 0.12, I intend to simply panic when being passed a union.

We should figure out what to do in that situation.

[dtolnay]

I expect most custom derives will not handle unions and just panic. I included them in DeriveInput because the compiler permits derive on unions. Something like typename_derive will want to support them.

[mystor]

Yeah, that seems like the most reasonable solution, at least for now.

[joshlf]

Now that syn supports Error::to_compiler_error, perhaps the error could be formatted using that instead of panicking? It'd also be cool if unions were supported - I'm working on a custom derive that I suspect will have consumers at some point that use unions.

[mystor]

synstructure was changed to use Error::to_compiler_error in #28, and handling unions generically probably isn't a good idea. Closing this issue.

[joshlf]

and handling unions generically probably isn't a good idea

I'm curious what the thinking is here? Do you mean handling unions at all, or does the word "generically" here mean something specific? The reason I ask is that I'd like to be able to support unions in my custom derive at some point. I can use something other than synstructure, of course - I just mean to illustrate that there are real use cases.

[mystor]

Support for custom derives on unions was added in rust-lang/rust#50383 for consistency with built-in derives, despite most derives not being compatible (rust-lang/rust#50383 (comment)). This is because union field accesses are unsafe, and the correctness of these accesses depends on external logic not visible to the derive implementer.

In effect, a derive which cares about the values of fields cannot support unions, which vastly limits the set of compatible derives. Only specific derives, such as the built-in #[derive(Copy, Clone)], can support unions, and derives which can won't benefit from synstructure, which provides helpers for matching against and accessing fields.

[mystor]

The reason I ask is that I'd like to be able to support unions in my custom derive at some point. I can use something other than synstructure, of course - I just mean to illustrate that there are real use cases.

If there are specific synstructure features which would be useful for your union derives, I'd be interested to hear about them. It may be worthwhile decoupling them from the Structure type, so that they can be supported there too. Perhaps another type which doesn't provide access to pattern matching or field information could be useful for traits which want to support union as well, which makes it clear that variant values, etc, are not available.

[joshlf]

Concretely, I'm interested in deriving the zerocopy traits, which are just unsafe marker traits that don't actually produce any code. For example, a derive of FromBytes for a union would just need to decide whether each individual variant was itself FromBytes. The utility is certainly limited compared to types where field access isn't unsafe, but it's still helpful (for example, to implement variable-length POSIX API structs).

The part of synstructure that we currently make use of is decl_derive!: https://docs.rs/zerocopy-derive/0.1.4/src/zerocopy_derive/lib.rs.html#38.

[mystor]

I didn't see any uses of Structure other than to extract the .ast() field. This DeriveInput object is easy to get with only syn, so it'd probably be best to just use it directly:

#[proc_macro_derive(FromBytes)]
pub fn derive_from_bytes(ts: proc_macro::TokenStream) -> proc_macro::TokenStream {
    let ast = syn::parse_macro_input!(ts as DeriveInput);
    match &ast.data {
        Data::Struct(strct) => derive_from_bytes_struct(&ast, strct),
        Data::Enum(enm) => derive_from_bytes_enum(&ast, enm),
        Data::Union(uni) => derive_from_bytes_union(&ast, uni),
    }
}

[joshlf]

Ah you're probably right. It's been a while since I wrote the code, so I honestly don't remember why I used synstructure. Certainly looks like, whatever the reason, we don't need it anymore.