Remove destructor-related restrictions from unions #38934

Manishearth · 2017-01-08T23:06:14Z

They don't have drop glue.

This doesn't fix the rvalue promotion issues when trying to do things like static FOO: NoDrop<Bar> = NoDrop {inner: Bar}. I'm not sure if we should fix that.

rust-highfive · 2017-01-08T23:06:25Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2017-01-08T23:08:38Z

@bors r+

bors · 2017-01-08T23:08:39Z

📌 Commit efd1877 has been approved by eddyb

eddyb · 2017-01-09T00:28:27Z

@bors r- Travis failed, you need to maybe update compile-fail tests.

Manishearth · 2017-01-09T00:37:49Z

We have a test, union-copy.rs, that tests that you can't impl Copy on a union containing noncopy types. Which is sort of the opposite of what this PR is trying to do. I thought the fact that unions can't be Copy was a bug; maybe it's intentional?

Regardless, I feel that it should be possible to do this.

Manishearth · 2017-01-09T00:45:35Z

Ah. #36016 (comment) mentions that there should be a test for Copy on non-copy-contents unions.

#36016 (comment) is an opposition to that.

cc @nikomatsakis @petrochenkov I'd really like to support copy being implemented on unions (even if #[derive(Copy)] doesn't work, at least so that NoDrop can work. There needs to be a way to opt out of drop glue and !Copy for generics without needing to resort to const size_of arrays.

petrochenkov · 2017-01-09T06:37:42Z

Of course, the restriction is intentional, this change creates a hole in the current move/initialization semantics of unions and removes existing guarantees.
impl Copy for anything (including unions) with Copy fields should at least require unsafe.
I wanted to extend the union RFC with more details since it was implemented, maybe it's time to do it now. Can this PR wait for a week or so?

Manishearth · 2017-01-09T14:43:12Z

I can remove the Copy related bits if that's the case. My main goal here was to get rid of the needs_drop requirement, the copy bit can wait.

Got an explanation of the hole?

petrochenkov · 2017-01-09T15:07:18Z

@Manishearth

Got an explanation of the hole?

Now union can be thought as an enum with unknown discriminant, so, for example, you have a guarantee that union U { s: String } always contains a valid String that can be safely accessed. If Copy can be implemented for U then this guarantee is dropped and you have a way to create dangling Strings by copying U. With this hole other checks, like "union values must be initialized" make much less sense.

My main goal here was to get rid of the needs_drop requirement

Ha, this is much better. I believe the value returned by needs_drop intrinsic can be changed without affecting anything else.

Manishearth · 2017-01-09T15:14:55Z

Split it into two commits. Are we okay with just landing the first one?

petrochenkov · 2017-01-09T18:17:52Z

src/librustc/ty/contents.rs

@@ -235,6 +235,11 @@ impl<'a, 'tcx> ty::TyS<'tcx> {
                            })
                        });

+                    if def.is_union() {
+                        // unions don't have destructors regardless of the child types


Could you add a test checking that needs_drop still returns true on unions that implement Drop themselves?
I suspect the condition should be def.is_union() && !def.has_dtor() instead of just def.is_union().

The destructor is added below IIRC.

Nah, that bit gets re-added below.

petrochenkov · 2017-01-09T18:18:44Z

@Manishearth
I'm okay with the first commit modulo comment.

Manishearth · 2017-01-09T18:28:03Z

Done.

Manishearth · 2017-01-09T19:54:57Z

@eddyb r?

eddyb · 2017-01-09T19:58:44Z

@bors r+

bors · 2017-01-09T19:58:45Z

📌 Commit b9b0732 has been approved by eddyb

bors · 2017-01-09T21:37:30Z

⌛ Testing commit b9b0732 with merge 5b0ae4b...

nikomatsakis · 2017-01-09T22:38:04Z

Woah. I feel that this whole discussion went by awfully fast. Can someone summarize the questions (and answers) on the tracking issue, please? (This seems to get at the philosophical debate about how best to think of unions that @petrochenkov and I were pursuing some time back.)

bors · 2017-01-09T23:45:28Z

💔 Test failed - status-travis

Manishearth · 2017-01-09T23:46:41Z

@bors retry

arm stalled

Spurious timeout on Android when starting tests #38851

Manishearth · 2017-01-09T23:48:36Z

FWIW, the current PR only changes the fact that unions containing dtors are needs_drop (and can't be put in statics). It doesn't change the Copy thing at all, although it initially intended to.

bors · 2017-01-10T02:45:13Z

⌛ Testing commit b9b0732 with merge 26dc969...

Remove destructor-related restrictions from unions They don't have drop glue. This doesn't fix the rvalue promotion issues when trying to do things like `static FOO: NoDrop<Bar> = NoDrop {inner: Bar}`. I'm not sure if we should fix that.

bors · 2017-01-10T04:43:31Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 26dc969 to master...

nikomatsakis · 2017-01-10T22:02:52Z

FWIW, the current PR only changes the fact that unions containing dtors are needs_drop (and can't be put in statics). It doesn't change the Copy thing at all, although it initially intended to.

OK, that seems pretty harmless.

rust-highfive assigned nikomatsakis Jan 8, 2017

Manishearth force-pushed the nodrop branch from ea78642 to efd1877 Compare January 8, 2017 23:07

Manishearth force-pushed the nodrop branch from efd1877 to 418710b Compare January 9, 2017 00:35

Manishearth force-pushed the nodrop branch from 418710b to 7eb46d2 Compare January 9, 2017 15:14

petrochenkov reviewed Jan 9, 2017

View reviewed changes

Manishearth force-pushed the nodrop branch from 7eb46d2 to 1a255a0 Compare January 9, 2017 18:24

Make unions never have needs_drop

b9b0732

Manishearth force-pushed the nodrop branch from 1a255a0 to b9b0732 Compare January 9, 2017 18:44

bors merged commit b9b0732 into rust-lang:master Jan 10, 2017

Manishearth deleted the nodrop branch January 10, 2017 05:12

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove destructor-related restrictions from unions #38934

Remove destructor-related restrictions from unions #38934

Manishearth commented Jan 8, 2017

rust-highfive commented Jan 8, 2017

eddyb commented Jan 8, 2017

bors commented Jan 8, 2017

eddyb commented Jan 9, 2017

Manishearth commented Jan 9, 2017

Manishearth commented Jan 9, 2017

petrochenkov commented Jan 9, 2017

Manishearth commented Jan 9, 2017

petrochenkov commented Jan 9, 2017 •

edited

Loading

Manishearth commented Jan 9, 2017

petrochenkov Jan 9, 2017

eddyb Jan 9, 2017

Manishearth Jan 9, 2017

petrochenkov commented Jan 9, 2017

Manishearth commented Jan 9, 2017

Manishearth commented Jan 9, 2017

eddyb commented Jan 9, 2017

bors commented Jan 9, 2017

bors commented Jan 9, 2017

nikomatsakis commented Jan 9, 2017

bors commented Jan 9, 2017

Manishearth commented Jan 9, 2017 •

edited by alexcrichton

Loading

Manishearth commented Jan 9, 2017

bors commented Jan 10, 2017

bors commented Jan 10, 2017

nikomatsakis commented Jan 10, 2017

Remove destructor-related restrictions from unions #38934

Remove destructor-related restrictions from unions #38934

Conversation

Manishearth commented Jan 8, 2017

rust-highfive commented Jan 8, 2017

eddyb commented Jan 8, 2017

bors commented Jan 8, 2017

eddyb commented Jan 9, 2017

Manishearth commented Jan 9, 2017

Manishearth commented Jan 9, 2017

petrochenkov commented Jan 9, 2017

Manishearth commented Jan 9, 2017

petrochenkov commented Jan 9, 2017 • edited Loading

Manishearth commented Jan 9, 2017

petrochenkov Jan 9, 2017

Choose a reason for hiding this comment

eddyb Jan 9, 2017

Choose a reason for hiding this comment

Manishearth Jan 9, 2017

Choose a reason for hiding this comment

petrochenkov commented Jan 9, 2017

Manishearth commented Jan 9, 2017

Manishearth commented Jan 9, 2017

eddyb commented Jan 9, 2017

bors commented Jan 9, 2017

bors commented Jan 9, 2017

nikomatsakis commented Jan 9, 2017

bors commented Jan 9, 2017

Manishearth commented Jan 9, 2017 • edited by alexcrichton Loading

Manishearth commented Jan 9, 2017

bors commented Jan 10, 2017

bors commented Jan 10, 2017

nikomatsakis commented Jan 10, 2017

petrochenkov commented Jan 9, 2017 •

edited

Loading

Manishearth commented Jan 9, 2017 •

edited by alexcrichton

Loading