Implement `CoerceUnsized` for `{Cell, RefCell, UnsafeCell}` #35627

Merged
merged 1 commit into from Aug 23, 2016

Conversation

Projects
None yet
6 participants
@apasel422
Member

apasel422 commented Aug 12, 2016

These impls are analogous to the one for NonZero. It's occasionally useful to be able to coerce the cell types when they're being used inside another abstraction. See Manishearth/rust-gc#17 for an example.

r? @eddyb

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 12, 2016

Member

LGTM. cc @rust-lang/lang Do we want such impls? While the feature is unstable, they would be usable in coercions from stable code.

Member

eddyb commented Aug 12, 2016

LGTM. cc @rust-lang/lang Do we want such impls? While the feature is unstable, they would be usable in coercions from stable code.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 12, 2016

Member

Would it be possible to add a few tests to the repo as well ensuring that we don't regress this functionality?

Member

alexcrichton commented Aug 12, 2016

Would it be possible to add a few tests to the repo as well ensuring that we don't regress this functionality?

@apasel422

This comment has been minimized.

Show comment
Hide comment
Member

apasel422 commented Aug 13, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 13, 2016

Member

Thanks! I think though that libcore doesn't actually have tests run, so could they be moved to libcoretest?

Member

alexcrichton commented Aug 13, 2016

Thanks! I think though that libcore doesn't actually have tests run, so could they be moved to libcoretest?

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 Aug 13, 2016

Member

@alexcrichton They're not actually tests -- they'll cause compilation to fail if we regress the functionality. This is similar to other assertions that types are covariant or Send/Sync.

Member

apasel422 commented Aug 13, 2016

@alexcrichton They're not actually tests -- they'll cause compilation to fail if we regress the functionality. This is similar to other assertions that types are covariant or Send/Sync.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 14, 2016

Member

Oh gah, right! Carry on!

Member

alexcrichton commented Aug 14, 2016

Oh gah, right! Carry on!

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Aug 14, 2016

Member

Usually, I'd be cautious about impls that let us skirt stability, but the DST coercions have been around for ages and I don't think there are problems with them, but we could do with more testing, so maybe this would help that?

Member

nrc commented Aug 14, 2016

Usually, I'd be cautious about impls that let us skirt stability, but the DST coercions have been around for ages and I don't think there are problems with them, but we could do with more testing, so maybe this would help that?

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 15, 2016

Contributor

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

Contributor

bors commented Aug 15, 2016

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

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 18, 2016

Contributor

I don't know that these impls make sense for Cell -- cell relies on being able to copy memory in and out, and coercing to an unsized value means we can't do that, right?

RefCell and UnsafeCell make more sense to me and I think I am in favor.

Contributor

nikomatsakis commented Aug 18, 2016

I don't know that these impls make sense for Cell -- cell relies on being able to copy memory in and out, and coercing to an unsized value means we can't do that, right?

RefCell and UnsafeCell make more sense to me and I think I am in favor.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 18, 2016

Contributor

I guess you could copy things in and out if you permitted dynamically sized stack frames (as I sort of want to permit now, in order to generally loosen the rules on DST). But no reason to jump the gun there. :)

Contributor

nikomatsakis commented Aug 18, 2016

I guess you could copy things in and out if you permitted dynamically sized stack frames (as I sort of want to permit now, in order to generally loosen the rules on DST). But no reason to jump the gun there. :)

@apasel422

This comment has been minimized.

Show comment
Hide comment
@apasel422

apasel422 Aug 18, 2016

Member

@nikomatsakis See the link in my original post for an example of where this is useful for Cell. This impl means that if you have Cell<Shared<T>> where T: Send, you can coerce it to Cell<Shared<Send>>.

Member

apasel422 commented Aug 18, 2016

@nikomatsakis See the link in my original post for an example of where this is useful for Cell. This impl means that if you have Cell<Shared<T>> where T: Send, you can coerce it to Cell<Shared<Send>>.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Aug 18, 2016

Member

@nikomatsakis Cell<Unsized> does indeed not make sense - but that's not what's being discussed here - that works automatically as long as you have T: ?Sized, which RefCell and UnsafeCell already do (RefCell<Trait> has been working for many versions now).

What's being added here is the ability to coerce {Unsafe,Ref,}Cell<Ptr<T>> to {Unsafe,Ref,}Cell<Ptr<Unsized>> where T: Unsize<Unsized>.

More concretely, Cell<&T> would coerce to Cell<&Trait> and that is useful.

Member

eddyb commented Aug 18, 2016

@nikomatsakis Cell<Unsized> does indeed not make sense - but that's not what's being discussed here - that works automatically as long as you have T: ?Sized, which RefCell and UnsafeCell already do (RefCell<Trait> has been working for many versions now).

What's being added here is the ability to coerce {Unsafe,Ref,}Cell<Ptr<T>> to {Unsafe,Ref,}Cell<Ptr<Unsized>> where T: Unsize<Unsized>.

More concretely, Cell<&T> would coerce to Cell<&Trait> and that is useful.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Aug 18, 2016

Contributor

Ah, right, thanks for the correction. =)

In that case, I have no objection. It is sort of nifty that this works. Seems like it'd be a variance error, but since there's no aliasing...

Contributor

nikomatsakis commented Aug 18, 2016

Ah, right, thanks for the correction. =)

In that case, I have no objection. It is sort of nifty that this works. Seems like it'd be a variance error, but since there's no aliasing...

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Aug 23, 2016

Member

Discussed during @rust-lang/libs triage discussion was also that this is good to merge, thanks @apasel422!

@bors: r+

Member

alexcrichton commented Aug 23, 2016

Discussed during @rust-lang/libs triage discussion was also that this is good to merge, thanks @apasel422!

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2016

Contributor

📌 Commit 1fd791a has been approved by alexcrichton

Contributor

bors commented Aug 23, 2016

📌 Commit 1fd791a has been approved by alexcrichton

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Aug 23, 2016

Contributor

⌛️ Testing commit 1fd791a with merge 43204ff...

Contributor

bors commented Aug 23, 2016

⌛️ Testing commit 1fd791a with merge 43204ff...

bors added a commit that referenced this pull request Aug 23, 2016

Auto merge of #35627 - apasel422:coerce-cell, r=alexcrichton
Implement `CoerceUnsized` for `{Cell, RefCell, UnsafeCell}`

These impls are analogous to the one for `NonZero`. It's occasionally useful to be able to coerce the cell types when they're being used inside another abstraction. See Manishearth/rust-gc#17 for an example.

r? @eddyb

@bors bors merged commit 1fd791a into rust-lang:master Aug 23, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@apasel422 apasel422 deleted the apasel422:coerce-cell branch Aug 23, 2016

@apasel422 apasel422 added the relnotes label Aug 30, 2016

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