New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Union initialization and Drop #2514

Merged
merged 12 commits into from Oct 17, 2018

Conversation

@RalfJung
Member

RalfJung commented Aug 3, 2018

This RFC realizes the second part of https://internals.rust-lang.org/t/pre-rfc-unions-drop-types-and-manuallydrop/8025: Changing unions to no longer allow fields with drop glue, and otherwise describing what we need to settle before unions can be fully stabilized. Unfortunately this got somewhat more complicated than I thought.

As usual I had trouble separating things between the guide-level and the reference-level explanation; I hope this makes sense!

Rendered
Tracking issue

@scottmcm scottmcm added the T-lang label Aug 3, 2018

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Aug 3, 2018

Contributor

LGTM.
It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.

This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.

Contributor

petrochenkov commented Aug 3, 2018

LGTM.
It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.

This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.

@dlight

This comment has been minimized.

Show comment
Hide comment
@dlight

dlight Aug 4, 2018

The RFC summary should probably link to something that explains what a drop glue is.

(The Drop chapter of the Rust book doesn't contain the word "glue")

dlight commented Aug 4, 2018

The RFC summary should probably link to something that explains what a drop glue is.

(The Drop chapter of the Rust book doesn't contain the word "glue")

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Aug 6, 2018

Member

It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.

What would be a good example?

This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.

Yes. (I avoided calling them "Drop fields" because a field can need drop glue even if it does not impl Drop -- e.g. if it is a struct and one of its fields has impl Drop.)

The RFC summary should probably link to something that explains what a drop glue is.

I added a brief explanation.

Member

RalfJung commented Aug 6, 2018

It would be nice to see some practical code ported from Drop fields to ManuallyDrop though to estimate impact on ergonomics and readability.

What would be a good example?

This RFC doesn't seem to prevent reintroducing Drop fields in uncertain future if the need arises, so we don't lose anything by adopting it.

Yes. (I avoided calling them "Drop fields" because a field can need drop glue even if it does not impl Drop -- e.g. if it is a struct and one of its fields has impl Drop.)

The RFC summary should probably link to something that explains what a drop glue is.

I added a brief explanation.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Aug 9, 2018

Member

@rfcbot merge

Based on discussion in the lang team and with Ralf, I think this is ready to merge. Fields in unions that impl Drop have never been part of stable Rust, so this isn't a breaking change, and this fixes various concerns and painful corner cases that kept coming up.

Member

joshtriplett commented Aug 9, 2018

@rfcbot merge

Based on discussion in the lang team and with Ralf, I think this is ready to merge. Fields in unions that impl Drop have never been part of stable Rust, so this isn't a breaking change, and this fixes various concerns and painful corner cases that kept coming up.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Aug 9, 2018

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

Concerns:

  • pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box resolved by #2514 (comment)

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.

rfcbot commented Aug 9, 2018

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

Concerns:

  • pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box resolved by #2514 (comment)

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.

@rkruppe rkruppe referenced this pull request Sep 5, 2018

Open

Representation of unions #13

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Sep 27, 2018

Member

@rfcbot concern pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box

I've been swamped, but this is an area I sort of care about and I want to read this

Member

pnkfelix commented Sep 27, 2018

@rfcbot concern pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box

I've been swamped, but this is an area I sort of care about and I want to read this

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Oct 4, 2018

Member

@rfcbot resolve pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box

read it, looks good.

Member

pnkfelix commented Oct 4, 2018

@rfcbot resolve pnkfelix-wants-a-chance-to-read-this-before-he-checks-his-box

read it, looks good.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Oct 4, 2018

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

rfcbot commented Oct 4, 2018

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

@crlf0710

This comment has been minimized.

Show comment
Hide comment
@crlf0710

crlf0710 Oct 13, 2018

Sorry it's a bit late, but in prior art section: Just want to point out that actually C++ does have destructors for unions. (See https://en.cppreference.com/w/cpp/language/union). Maybe a small edit to the text will be good.

crlf0710 commented Oct 13, 2018

Sorry it's a bit late, but in prior art section: Just want to point out that actually C++ does have destructors for unions. (See https://en.cppreference.com/w/cpp/language/union). Maybe a small edit to the text will be good.

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 13, 2018

Member

@crlf0710 Thanks for pointing that out! I fixed the text. Do you agree it is correct now?

Member

RalfJung commented Oct 13, 2018

@crlf0710 Thanks for pointing that out! I fixed the text. Do you agree it is correct now?

@crlf0710

This comment has been minimized.

Show comment
Hide comment
@crlf0710

crlf0710 commented Oct 13, 2018

@RalfJung Looks good!

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Oct 13, 2018

Contributor

Would this auto trait be an accurate expression of the “has no drop glue” concept?

pub auto trait NoDropGlue {}
impl<T> !NoDropGlue for T where T: Drop {}

If it is and we make it a lang item (in core::marker), generic unions could be extended to allow fields that are not necessarily Copy:

union Foo<T: NoDropGlue> {
    f: T,
}
Contributor

SimonSapin commented Oct 13, 2018

Would this auto trait be an accurate expression of the “has no drop glue” concept?

pub auto trait NoDropGlue {}
impl<T> !NoDropGlue for T where T: Drop {}

If it is and we make it a lang item (in core::marker), generic unions could be extended to allow fields that are not necessarily Copy:

union Foo<T: NoDropGlue> {
    f: T,
}
@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 13, 2018

Member

I think it would begin to express that concept if it worked, but AFAIK those kind of negative bounds don't work...at least I recall that was the result of someone checking last time.

We'd however also need

impl<T> NoDropGlue for ManuallyDrop<T>

and then what about unions themselves? Do they get auto traits the same way everything else does, or are auto traits just never implemented for them? Either way we'd want NoDropGlue implemented for all unions

Member

RalfJung commented Oct 13, 2018

I think it would begin to express that concept if it worked, but AFAIK those kind of negative bounds don't work...at least I recall that was the result of someone checking last time.

We'd however also need

impl<T> NoDropGlue for ManuallyDrop<T>

and then what about unions themselves? Do they get auto traits the same way everything else does, or are auto traits just never implemented for them? Either way we'd want NoDropGlue implemented for all unions

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Oct 13, 2018

Contributor

Good points. So leaving aside the auto-trait definition and assuming it can be implemented with ~compiler magic~ instead, would it be desirable for this trait to exist to enable such generic unions?

Contributor

SimonSapin commented Oct 13, 2018

Good points. So leaving aside the auto-trait definition and assuming it can be implemented with ~compiler magic~ instead, would it be desirable for this trait to exist to enable such generic unions?

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Oct 13, 2018

Member

I would think so, yes. Copy has always been a bad approximation. IIRC either @eddyb or @nikomatsakis had other uses for such a trait as well.

However, introducing such a trait is firmly out of scope for this PR.

Member

RalfJung commented Oct 13, 2018

I would think so, yes. Copy has always been a bad approximation. IIRC either @eddyb or @nikomatsakis had other uses for such a trait as well.

However, introducing such a trait is firmly out of scope for this PR.

@crlf0710

This comment has been minimized.

Show comment
Hide comment
@crlf0710

crlf0710 Oct 13, 2018

i think the “has no drop glue” approximately corresponds to std::mem::needs_drop which is implemented using a intrinsic. As the documentation says it's conservative, so... maybe it will be a matter of making it accurate and const fn... Maybe it's also a good idea to lift it to a trait(that's a common problem for all const fns, i think).

crlf0710 commented Oct 13, 2018

i think the “has no drop glue” approximately corresponds to std::mem::needs_drop which is implemented using a intrinsic. As the documentation says it's conservative, so... maybe it will be a matter of making it accurate and const fn... Maybe it's also a good idea to lift it to a trait(that's a common problem for all const fns, i think).

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Oct 14, 2018

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

rfcbot commented Oct 14, 2018

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

@Centril Centril merged commit e5276df into rust-lang:master Oct 17, 2018

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Oct 17, 2018

Contributor

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#55149

Contributor

Centril commented Oct 17, 2018

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#55149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment