cleanups and fixes for #[derive] #32139

durka · 2016-03-08T23:51:38Z

This contains a bunch of little cleanups and fixes to #[derive]. There are more extensive comments on each individual commit.

hygiene cleanups
use discriminant_value instead of variant index in #[derive(Hash)]
~~don't move out of borrowed content in #[derive(PartialOrd, PartialEq)]~~
use intrinsics::unreachable() instead of unreachable!()

I don't believe there are any breaking changes in here, but I do want some more eyes on that.

Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...).
Fixes #21714 (cc @apasel422).
~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss).
Fixes #31714 (cc @alexcrichton @bluss).
Fixes #31886 (cc @oli-obk).

durka · 2016-03-09T00:02:50Z

High five? No high five? ✋ 😿

r? @alexcrichton

bors · 2016-03-09T00:14:11Z

☔ The latest upstream changes (presumably #32009) made this pull request unmergeable. Please resolve the merge conflicts.

durka · 2016-03-09T03:15:49Z

The failing test is here and I don't really know how to interpret it. I guess the &s I added to the output of #[derive(PartialEq)] affected the monomorphizations that trans had to generate?

bluss · 2016-03-09T03:59:12Z

I think that issue (#24047) should not be worked around in derive but it may be fixed directly instead.

durka · 2016-03-09T04:21:15Z

I can back out that commit and leave it for later.

On Tue, Mar 8, 2016 at 10:59 PM, bluss notifications@github.com wrote:

I think that issue (#24047
#24047) should not be worked
around in derive but it may be fixed directly instead.

—
Reply to this email directly or view it on GitHub
#32139 (comment).

alexcrichton · 2016-03-09T18:31:47Z

src/libsyntax_ext/deriving/cmp/partial_eq.rs

+                let eq = cx.expr_binary(span,
+                                        BinOpKind::Ne,
+                                        cx.expr_addr_of(span, self_f),
+                                        cx.expr_addr_of(span, other_f.clone()));


In the past we've had subtle bugs where fields didn't actually implement some trait you were deriving but it worked anyway, so I'm curious why the extra & is needed here? In theory that should all happen automatically, right?

It's because of this: http://is.gd/Dgj5u2

Hm I'm somewhat confused, each of those lines works independently? Perhaps this is a case where UFCS should be used?

Yeah maybe we should use UFCS in the derive expansion, but there's a deeper issue (which is why @bluss suggested not working around it here)... line 17 should be exactly the same as line 19, right?

Perhaps yeah? I'm not too familiar with auto-ref and how that works

alexcrichton · 2016-03-09T18:37:41Z

Fixes #24047

Wait this is an issue that can be fixed? PartialEq isn't object safe?

alexcrichton · 2016-03-09T18:38:25Z

Oh then I keep reading and realize it's PartialEq for Trait...

bluss · 2016-03-09T18:42:08Z

Yes it can be fixed. The code snippet I showed in that issue is a bug that's independent of derive itself.

durka · 2016-03-10T05:31:52Z

OK I addressed the comments and removed the two commits related to #24047 (don't worry I still have them in another branch!). Let's see if it builds!

durka · 2016-03-10T05:33:14Z

src/test/auxiliary/derive-no-std.rs

@@ -8,32 +8,33 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-#![feature(rand, collections, rustc_private)]
-#![no_std]
+// no-prefer-dynamic


For the record I have no idea what this does, but without it it asks for the lang items (even though this is supposed to be a lib).

Yeah building a dylib requires lang items to be resolved, but as an rlib (what no-prefer-dynamic asks for) means that they can stay dangling.

It already says #![crate_type = "rlib"] though.

Yeah but that's ignored as --crate-type dylib is passed on the command line

Got it. I'll take that out.

Actually #![crate_type = "rlib"] and // no-prefer-dynamic are required for it not to try and resolve the lang items.

alexcrichton · 2016-03-10T18:16:52Z

src/libsyntax_ext/deriving/mod.rs

+/// Construct a name for the inner type parameter that can't collide with any type parameters of
+/// the item. This is achieved by starting with a base and then concatenating the names of all
+/// other type parameters.
+fn hygienic_type_parameter(item: &Annotatable, base: &str) -> String {


I feel like this may want to continue to have a FIXME for doing something "more correct" here, this seems like it's still kinda a hack to get the job done?

It's a hack yes (but a better one than before IMO). I'll add a FIXME.

alexcrichton · 2016-03-10T18:18:30Z

Thanks @durka! Just a comment to be added I think but other than that looks good to me.

durka · 2016-03-10T18:29:01Z

Added the FIXME comment.

durka · 2016-03-10T19:22:45Z

cc @talchas

The #31886 test was going to fail on 32-bit platforms because I changed it from assuming i32 to assuming isize but that's no different. Now it assumes i64. Hopefully this has little to no impact on codegen? (I don't expect so because discriminant_value returns u64 already.)

alexcrichton · 2016-03-11T00:53:13Z

@bors: r+ 8cccdd0

Thanks!

@apasel422

cleanups and fixes for #[derive] This contains a bunch of little cleanups and fixes to `#[derive]`. There are more extensive comments on each individual commit. - hygiene cleanups - use `discriminant_value` instead of variant index in `#[derive(Hash)]` - ~~don't move out of borrowed content in `#[derive(PartialOrd, PartialEq)]`~~ - use `intrinsics::unreachable()` instead of `unreachable!()` I don't believe there are any breaking changes in here, but I do want some more eyes on that. Fixes #2810 (!), I believe (we still assume that "std" or "core" is the standard library but so does the rest of rustc...). Fixes #21714 (cc @apasel422). ~~Fixes~~ (postponed) #24047 (cc @withoutboats @bluss). Fixes #31714 (cc @alexcrichton @bluss). Fixes #31886 (cc @oli-obk).

bors · 2016-03-13T19:24:41Z

💔 Test failed - auto-mac-32-opt

alexcrichton · 2016-03-13T19:30:12Z

@bors: retry

On Sun, Mar 13, 2016 at 12:24 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-32-opt
http://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/8396

—
Reply to this email directly or view it on GitHub
#32139 (comment).

bors · 2016-03-13T22:28:04Z

⌛ Testing commit a037073 with merge fb9b646...

alexcrichton · 2016-03-13T22:31:40Z

@bors: retry force

bors · 2016-03-13T22:34:59Z

⌛ Testing commit a037073 with merge afacbfd...

bors · 2016-03-13T23:53:08Z

💔 Test failed - auto-mac-32-opt

durka · 2016-03-14T00:29:30Z

Okay what the heck is going on.

eddyb · 2016-03-14T07:51:33Z

@bors retry

bors · 2016-03-14T11:35:47Z

⌛ Testing commit a037073 with merge 97339bc...

bors · 2016-03-14T12:44:22Z

💔 Test failed - auto-mac-32-opt

durka · 2016-03-14T14:39:49Z

This is starting to seem legitimate just by repetition! I'm thinking I
should close this and resubmit the fixes individually to see which one
isn't harmless.
On Mar 14, 2016 08:44, "bors" notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-32-opt
http://buildbot.rust-lang.org/builders/auto-mac-32-opt/builds/8406

—
Reply to this email directly or view it on GitHub
#32139 (comment).

alexcrichton · 2016-03-14T20:14:41Z

Something seems to be getting oddly mixed up as it's referencing stage1 dylibs? Probably couldn't hurt to break apart though I guess?

@alexcrichton

derive: use intrinsics::unreachable over unreachable!() derive: use intrinsics::unreachable over unreachable!() Fixes #31574. Spawned from #32139. r? @alexcrichton

@alexcrichton

derive: clean up hygiene derive: clean up hygiene Fixes #2810. Spawned from #32139. r? @alexcrichton

@alexcrichton

derive: assume enum repr defaults to isize derive: assume enum repr defaults to isize Fixes #31886. Spawned from #32139. r? @alexcrichton

@alexcrichton

derive: use discriminant_value in #[derive(Hash)] derive: use discriminant_value in #[derive(Hash)] Fixes #21714. Spawned from #32139. r? @alexcrichton

@alexcrichton

derive: use discriminant_value in #[derive(Hash)] derive: use discriminant_value in #[derive(Hash)] Fixes #21714. Spawned from #32139. r? @alexcrichton

@alexcrichton

derive: use discriminant_value in #[derive(Hash)] derive: use discriminant_value in #[derive(Hash)] Fixes rust-lang#21714. Spawned from rust-lang#32139. r? @alexcrichton

durka force-pushed the derive-fix branch from 0611aef to 255f833 Compare March 9, 2016 00:18

alexcrichton reviewed Mar 9, 2016
View reviewed changes

alexcrichton self-assigned this Mar 10, 2016

durka force-pushed the derive-fix branch from 255f833 to 6e98896 Compare March 10, 2016 05:19

durka reviewed Mar 10, 2016
View reviewed changes

durka force-pushed the derive-fix branch 2 times, most recently from 3831056 to 47900e6 Compare March 10, 2016 06:55

alexcrichton reviewed Mar 10, 2016
View reviewed changes

durka force-pushed the derive-fix branch from 47900e6 to 3b4575a Compare March 10, 2016 18:28

durka force-pushed the derive-fix branch from 3b4575a to 8cccdd0 Compare March 10, 2016 19:21

Manishearth mentioned this pull request Mar 12, 2016

Rollup of 8 pull requests #32209

Closed

durka closed this Mar 14, 2016

This was referenced Mar 14, 2016

derive: use intrinsics::unreachable over unreachable!() #32250

Merged

derive: clean up hygiene #32251

Merged

derive: use discriminant_value in #[derive(Hash)] #32252

Merged

derive: assume enum repr defaults to isize #32253

Merged

bors added a commit that referenced this pull request Mar 15, 2016

Auto merge of #32251 - durka:derive-2810, r=alexcrichton

1efa752

derive: clean up hygiene derive: clean up hygiene Fixes #2810. Spawned from #32139. r? @alexcrichton

bors added a commit that referenced this pull request Mar 22, 2016

Auto merge of #32253 - durka:derive-31886, r=alexcrichton

6cc502c

derive: assume enum repr defaults to isize derive: assume enum repr defaults to isize Fixes #31886. Spawned from #32139. r? @alexcrichton

bors added a commit that referenced this pull request Mar 27, 2016

Auto merge of #32252 - durka:derive-21714, r=alexcrichton

4369eef

derive: use discriminant_value in #[derive(Hash)] derive: use discriminant_value in #[derive(Hash)] Fixes #21714. Spawned from #32139. r? @alexcrichton

bors added a commit that referenced this pull request Mar 27, 2016

Auto merge of #32252 - durka:derive-21714, r=alexcrichton

deee0f7

derive: use discriminant_value in #[derive(Hash)] derive: use discriminant_value in #[derive(Hash)] Fixes #21714. Spawned from #32139. r? @alexcrichton

cleanups and fixes for #[derive] #32139

cleanups and fixes for #[derive] #32139

Conversation

durka commented Mar 8, 2016

durka commented Mar 9, 2016

bors commented Mar 9, 2016

durka commented Mar 9, 2016

bluss commented Mar 9, 2016

durka commented Mar 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 9, 2016

alexcrichton commented Mar 9, 2016

bluss commented Mar 9, 2016

durka commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 10, 2016

durka commented Mar 10, 2016

durka commented Mar 10, 2016

alexcrichton commented Mar 11, 2016

bors commented Mar 13, 2016

alexcrichton commented Mar 13, 2016

bors commented Mar 13, 2016

alexcrichton commented Mar 13, 2016

bors commented Mar 13, 2016

bors commented Mar 13, 2016

durka commented Mar 14, 2016

eddyb commented Mar 14, 2016

bors commented Mar 14, 2016

bors commented Mar 14, 2016

durka commented Mar 14, 2016

alexcrichton commented Mar 14, 2016