Add ability to apply custom derive to union types. #50383

stevepentland · 2018-05-02T01:12:38Z

The Union item type has been included in the allowed types for a custom derive.
fyi @abonander

rust-highfive · 2018-05-02T01:12:48Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

abonander · 2018-05-02T01:19:54Z

Excellent! If you would, edit your PR to say "closes #50223" at the end, that will automatically close that issue when this is merged.

abonander · 2018-05-02T01:21:11Z

src/test/run-pass-fulldeps/proc-macro/auxiliary/derive-union.rs

+#[proc_macro_derive(Union)]
+pub fn derive(input: TokenStream) -> TokenStream {
+    let input = input.to_string();
+    assert_eq!(EXPECTED, input);


The convention is to put the actual value first (input) and the expected value second. Then you can inline the EXPECTED string without it being too ugly.

I was having problems with the formatting of the expected value when it was inline, I couldn't get the actual formatting of the incoming string (newlines, etc) to match up with the value

Other derive tests appear to just assert that the input contains certain lines: https://github.com/rust-lang/rust/blob/master/src/test/run-pass-fulldeps/proc-macro/auxiliary/derive-b.rs#L23

I don't think it affects the confidence interval of the test much since we're mostly just ensuring that derives are allowed to be applied to unions. I'll let @alexcrichton make the call on what he prefers to see.

abonander · 2018-05-02T01:22:31Z

src/test/run-pass-fulldeps/proc-macro/derive-union.rs

+extern crate derive_union;
+
+#[repr(C)]
+#[derive(Union)]


I would maybe name the derive UnionTest just to be perfectly clear. The filename is probably fine though.

abonander · 2018-05-02T01:23:07Z

src/libsyntax_ext/deriving/custom.rs

            _ => {
                ecx.span_err(span, "proc-macro derives may only be \
-                                    applied to struct/enum items");
+                                    applied to a struct, enum, or union item");


You can probably drop the word "item" from this and the previous message as it doesn't really add anything.

abonander · 2018-05-02T01:44:51Z

Looks all right to me! Now to see if it passes the build and the official review.

abonander · 2018-05-02T02:02:34Z

@stevepentland you don't have to do it right now but the reviewer will probably want you to squash your second commit onto the first. Do you know how to use git rebase?

stevepentland · 2018-05-02T02:04:58Z

you don't have to do it right now but the reviewer will probably want you to squash your second commit onto the first

Yeah, I've just didn't do it yet because of the stuff from highfive about adding them as extra commits to make tracking the changes easier. But since this is so small, I'll just do it right now.

abonander · 2018-05-02T02:11:18Z

It's normal to make a mess of the commit history during implementation/review and then squash at the end, that's why I said you didn't have to do it right away.

alexcrichton · 2018-05-02T02:59:04Z

The code changes and tests look good to me, thanks!

This'll be an insta-stable addition to the language so I've tagged this as T-lang and now could I entice someone from @rust-lang/lang to propose a merge on this?

abonander · 2018-05-02T03:01:52Z

Builtin derives are already allowed on unions so I consider it a discrepancy that custom derives aren't allowed. This was discussed in #50223.

cramertj · 2018-05-02T03:47:34Z

@rfcbot fcp merge

rfcbot · 2018-05-02T03:47:34Z

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

scottmcm · 2018-05-02T06:21:50Z

@alexcrichton Can you summarize the why for this one? Is it mostly just "well, we allow other derives on unions so might as well allow custom ones too"?

joshtriplett · 2018-05-02T08:13:08Z

@scottmcm I can think of any number of good reasons to use a custom derive on a union type, either for a trait or synthetically as a marker to add other functionality to a type. Why would we allow it on a struct and not on a union?

withoutboats · 2018-05-02T11:19:39Z

I think even the most minimal rational set of items that derives can be applied to (nominal type declarations) includes unions. I can't think of a name for the set that includes structs and enums but not unions.

alexcrichton · 2018-05-02T13:56:11Z

@scottmcm I'm mostly just the man in the middle, I'd ask that question to #50223

nikomatsakis · 2018-05-02T19:23:06Z

I think the reason we initially did not include union was that it can so easily go wrong. e.g., #[derive(Ord)] or something couldn't possible work. That said, I think that having the ability to derive things on unions makes sense.

I guess the question is: how do people write custom derives? Can we setup those libraries so that custom derives have to "opt in" to support union?

nikomatsakis · 2018-05-02T19:23:17Z

But that seems more like a "libs" problem, honestly.

withoutboats · 2018-05-02T19:35:39Z

I guess the question is: how do people write custom derives? Can we setup those libraries so that custom derives have to "opt in" to support union?

Its already set up like that. Anyone using syn (which AFAIK is everyone) receives the type represented like this; they'd have to explicitly describe how to handle a union if they want to, or panic otherwise (its possible there are derives already set up to handle unions by authors who don't realize it doesn't work).

pietroalbini · 2018-05-21T11:22:34Z

Ping from triage @aturon @nrc @pnkfelix @scottmcm! There are some checkboxes to check.

rfcbot · 2018-06-03T19:16:35Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2018-06-13T19:21:05Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

pietroalbini · 2018-06-18T09:39:04Z

The reviewer is currently away, can someone else from @rust-lang/compiler check in on this?

oli-obk · 2018-06-18T09:48:40Z

@bors r+

bors · 2018-06-18T09:48:41Z

📌 Commit 4be07af has been approved by oli-obk

bors · 2018-06-18T09:48:47Z

🔒 Merge conflict

pietroalbini · 2018-06-18T12:34:05Z

@stevepentland you need to rebase on top of the latest master, even if you don't see any conflicts. Sorry for the trouble!

The Union item type has been included in the allowed types for a custom derive. Closes rust-lang#50223

stevepentland · 2018-06-19T01:35:04Z

@pietroalbini done

pietroalbini · 2018-06-19T07:24:10Z

@oli-obk and now you need to approve this again...

oli-obk · 2018-06-19T07:28:40Z

@bors r+

bors · 2018-06-19T07:28:41Z

📌 Commit 14abb55 has been approved by oli-obk

bors · 2018-06-19T08:50:48Z

⌛ Testing commit 14abb55 with merge 1080203...

@abonander

Add ability to apply custom derive to union types. The Union item type has been included in the allowed types for a custom derive. fyi @abonander Closes #50223

bors · 2018-06-19T10:53:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 1080203 to master...

rust-highfive assigned alexcrichton May 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2018

abonander reviewed May 2, 2018

View reviewed changes

stevepentland force-pushed the union-derive branch from d07cc5e to ee71c91 Compare May 2, 2018 01:21

abonander reviewed May 2, 2018

View reviewed changes

stevepentland force-pushed the union-derive branch from bcafaee to 4be07af Compare May 2, 2018 02:06

alexcrichton added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 2, 2018

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 2, 2018

shepmaster added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2018

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jun 3, 2018

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jun 13, 2018

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 13, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2018

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 18, 2018

Add ability to apply custom derive to union types.

14abb55

The Union item type has been included in the allowed types for a custom derive. Closes rust-lang#50223

stevepentland force-pushed the union-derive branch from 4be07af to 14abb55 Compare June 19, 2018 01:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 19, 2018

bors merged commit 14abb55 into rust-lang:master Jun 19, 2018

mystor mentioned this pull request Nov 11, 2019

Support for derive on unions? mystor/synstructure#6

Closed

Add ability to apply custom derive to union types. #50383

Add ability to apply custom derive to union types. #50383

Conversation

stevepentland commented May 2, 2018 • edited Loading

rust-highfive commented May 2, 2018

abonander commented May 2, 2018

abonander May 2, 2018

Choose a reason for hiding this comment

stevepentland May 2, 2018

Choose a reason for hiding this comment

abonander May 2, 2018

Choose a reason for hiding this comment

abonander May 2, 2018

Choose a reason for hiding this comment

abonander May 2, 2018 • edited Loading

Choose a reason for hiding this comment

abonander commented May 2, 2018

abonander commented May 2, 2018

stevepentland commented May 2, 2018

abonander commented May 2, 2018

alexcrichton commented May 2, 2018

abonander commented May 2, 2018

cramertj commented May 2, 2018

rfcbot commented May 2, 2018 • edited by aturon Loading

scottmcm commented May 2, 2018

joshtriplett commented May 2, 2018 • edited Loading

withoutboats commented May 2, 2018

alexcrichton commented May 2, 2018

nikomatsakis commented May 2, 2018

nikomatsakis commented May 2, 2018

withoutboats commented May 2, 2018

pietroalbini commented May 21, 2018

rfcbot commented Jun 3, 2018

rfcbot commented Jun 13, 2018

pietroalbini commented Jun 18, 2018

oli-obk commented Jun 18, 2018

bors commented Jun 18, 2018

bors commented Jun 18, 2018

pietroalbini commented Jun 18, 2018

stevepentland commented Jun 19, 2018

pietroalbini commented Jun 19, 2018

oli-obk commented Jun 19, 2018

bors commented Jun 19, 2018

bors commented Jun 19, 2018

bors commented Jun 19, 2018

stevepentland commented May 2, 2018 •

edited

Loading

abonander May 2, 2018 •

edited

Loading

rfcbot commented May 2, 2018 •

edited by aturon

Loading

joshtriplett commented May 2, 2018 •

edited

Loading