Allow custom derives for unions

[ghost]

Derived from this question:
https://users.rust-lang.org/t/why-are-proc-macro-derives-prohibited-for-unions/17065

Currently, we cannot apply custom derives to unions:

#[repr(C)]
#[derive(CustomDerive)]
union Value {
    a: u64,
    b: f64,
}

error: proc-macro derives may only be applied to struct/enum items
  --> src/value.rs:48:10
   |
48 | #[derive(CustomDerive)]
   |          ^^^^^^^^^^^^

This restriction should be removed if there is no technical issue.

[abonander]

Most derives are not expecting a union. Moreover, how useful would any derive be in isolation if you need an external value to tell which field is valid data?

The only derived traits I can see being useful are Clone and Copy but only for unions of exclusively Copy types since we can just bit-copy the union.

Maybe Default as well but then how do you define the default value of the union? Write the first field with the default value for its type? How will the user know which value is valid?

[ExpHP]

Most derives are not expecting a union.

I don't see the problem with this? If a custom derive doesn't expect a union, it can simply fail.

The only derived traits I can see being useful are Clone and Copy but only for unions of exclusively Copy types since we can just bit-copy the union.

Maybe Default as well but then how do you define the default value of the union? Write the first field with the default value for its type? How will the user know which value is valid?

These are all builtin derives. union already supports all of the builtin derives that it sensibly can (Copy and Clone). The issue is about custom derives.

[ExpHP]

After a brief investigation, it is tempting to say that this might be a one-line change. The linked snippet is a last-minute check on the item kind just moments before expanding the macro. (It is, of course, the check that produces this error message.)

Since custom derives are allowed to expand to arbitrary items, I can't imagine that there are any other diagnostics hiding around the corner anywhere that might be attempting to match this against a struct or enum. (But I'm still paranoid about it because if there is one, it'll probably give an ugly ICE if not updated)

[abonander]

If builtin derives are already allowed on unions then it's a discrepancy not to allow custom derives. I do agree that it likely just requires lifting that check in the line you linked.

[est31]

The use case seems to be

I want to create a derive to guarantee that all union fields implement a specific trait.
Such derives are safe for any unions because their fields are not accessed at run-time.

So why not use a proc macro here? They'll get stable soon if we are lucky.

[durka]

@est31 why not use a proc macro in place of every custom derive? Same answer -- derive attributes look better and don't prevent code parsing tools like rustfmt from seeing the item.

As for custom derives not expecting a union, it seems that at least syn is already equipped for this.

[est31]

derive attributes look better

I hope you do agree that

#[repr(C)]
#[derive(MakeSureTraitIsImplemented)]
union Value {
    a: u64,
    b: f64,
}

does not look better than

#[repr(C)]
#[make_sure_trait_is_implemented]
union Value {
    a: u64,
    b: f64,
}

I've meant the attribute proc macros, not the bang proc macros.

Still, the derive macros were introduced, are taught, are being extended, and have a syntax that points to their actual purpose: deriving traits for structs. Any other use is --technically-- abuse of the feature.

The only use case of this feature that I'm aware of is what @h2so5 mentioned in the thread linked: they wanted to make sure that every variant of the union implemented a trait. This is a solid use case, but it still is abusing the derive feature.

@abonander pointed out correctly that Clone and Copy are the only two traits that you can safely derive for unions, and even then you can only derive Clone if it also implements Copy (because obviously you can't dispatch).

If you are adding a feature where the only reasonable use case is its abuse, I think you've done a mistake in language design. I'm saying that as someone who loves abusing language quirks.

[durka]

Attribute proc macro makes more sense than bang proc macro here, yeah. I think there are legitimate uses for derive on unions though, even this isn't one of them.

…

On Thu, Apr 26, 2018 at 1:20 PM, est31 ***@***.***> wrote: derive attributes look better I hope you do agree that #[repr(C)] #[derive(MakeSureTraitIsImplemented)]union Value { a: u64, b: f64, } *does not* look better than #[repr(C)] #[make_sure_trait_is_implemented]union Value { a: u64, b: f64, } I've meant the attribute proc macros, not the bang proc macros. Still, the derive macros were introduced, are taught, are being extended <rust-lang/rfcs#2385>, and have a syntax that points to their actual purpose: deriving traits for structs. Any other use is --technically-- abuse of the feature. The only use case of this feature that I'm aware of is what @h2so5 <https://github.com/h2so5> mentioned in the thread linked: they wanted to make sure that every variant of the union implemented a trait. This is a solid use case, but it still is abusing the derive feature. @abonander <https://github.com/abonander> pointed out correctly that Clone and Copy are the only two traits that you can safely derive for unions, and even then you can only derive Clone if it also implements Copy (because obviously you can't dispatch). If you are adding a feature where the only reasonable use case is its abuse, I think you've done a mistake in language design. I'm saying that as someone who *loves* abusing language quirks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#50223 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n0gWLIFs5CgAgp6yJxYbJC5XyIU2ks5tsgHpgaJpZM4Tjerr> .

[ExpHP]

The only use case of this feature that I'm aware of is what @h2so5 mentioned in the thread linked: they wanted to make sure that every variant of the union implemented a trait. This is a solid use case, but it still is abusing the derive feature.

There are legitimate use cases.

• Marker traits
• Type-level programming

[ghost]

@est31
In my use case, the custom derive is used for applying a marker trait to struct/union recursively: #[derive(A)] adds trait A for struct/union if every field implements trait A. So it's not a workaround for unstable proc macro.

Custom marker traits for union make sense when we want to ensure the characteristics of its memory block independent of the type of variant.

For example:

• Struct/Union only contains abi-safe types or not
• Struct/Union may contain a pointer or not (for conservative GC)
• Struct/Union is relocatable or not

[abonander]

This appears to be an E-Easy change and I'm willing to mentor. If everyone concurs I'll post mentoring instructions.

[dtolnay]

All of the arguments against derive on unions so far also work as arguments against letting users handwrite trait implementations for their unions.

Given that we already permit builtin derives on unions, and given that we allow users to handwrite trait implementations for a union type, I believe custom derive needs to be supported.

[est31]

Sure, go with it. I don't feel too strongly about it.

[stevepentland]

@abonander I'd like to take care of this one as my first contribution to Rust once the mentoring instructions go up.

[abonander]

@stevepentland It should be as simple as adding ItemKind::Union(..) to this match, which does exactly what it looks like it does.

Of course you'll need to implement a test derive, which would involve adding two files to src/test/run-pass-fulldeps/proc-macro; one implementing the derive and one to use it. The file implementing the derive goes in the auxiliary/ subfolder, I recommend using derive-a.rs as a template for that. Then in your test file you signal it to build your derive file by adding these comments (the second is because of an unrelated issue):

// aux-build:derive-filename.rs
// ignore-stage1

And you import it with #[macro_use] extern crate derive_filename; like any derive crate.

The derive itself can be as simple as asserting the contents of the input and outputting nothing ("".parse().unwrap()). Make sure to pick informative names, you could name the file after this issue or something like derive-union.rs.

Don't forget the license headers, of course. You can copy them from one of the other files in the directory.

To test it, run x.py test run-pass-fulldeps --test-args test-filename which will run just that one test so you don't have to wait for all the others to complete. Unfortunately you will have to wait for the stage2 compiler to build which could take a while.

I don't have reviewer permissions so someone else will review the PR when you submit it, possibly @dtolnay or @petrochenkov or maybe someone else. Feel free to @-mention me though if you want me to look it over.