Skip to content
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

specify that CoerceUnsized should ignore PhantomData fields #1234

Merged
merged 3 commits into from Sep 4, 2015

Conversation

Projects
None yet
7 participants
@Gankro
Copy link
Contributor

Gankro commented Aug 3, 2015

Otherwise you can't have a Smaht<T>(*const T, PhantomData<T>), which is problematic for drop check, which requires this annotation if Smaht owns the referrent (Box, Rc, Vec, etc..).

See rust-lang/rust#26905 for some discussion.

You could arguably ignore all zero-sized fields, but it's currently hard for the compiler to do so (size is largely a trans notion, to my knowledge). Also maybe you actually want a generic ZST field that has some meaning beyond PhantomData?

A potential workaround to this problem would be Smaht(*const u8, PhantomData<T>) but this is entering a fairly degenerate state for the semantics of raw pointers. Although we're already in a bit of a degenerate state in that all Smaht pointers desire some level of mutable access, but are forced to use *const to acquire the correct variance. We paper over that mess via Unique -- we could consider just embracing that and making Unique the only proper way to build Smaht pointers.

Of course Unique is incorrect for Rc, but I plan on adding a Shared equivalent for Rc anyway which could have similar magicks.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Aug 3, 2015

+1

Alternatively we could allow this for all ZSTs, and just implement for PhantomData for now. Special casing PhantomData is pretty much an implementation detail.

Could we approximate zero-sized == no non-zero-sized fields? Presumably we'd need to wait for the drop flag to disappear. Then just assert that we were right in trans.

@jroesch

This comment has been minimized.

Copy link
Member

jroesch commented Aug 3, 2015

👍

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Aug 3, 2015

I agree with @nrc that the text shouldn't be specific to PhantomData just because of the current implementation issues. But otherwise 👍 from me.

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Aug 3, 2015

I guess the question is whether unsizing two fields is forbidden for semantic or implementation (offsets fall over with multiple unsized fields) reasons -- if it's semantic then PhantomData is the only type we really know doesn't matter. If it's implementation then presumably all ZSTs should be fine.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 4, 2015

if we're going to patch in place, can we make a section at the end entitled something like "Updates since being accepted" and record the changes

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Aug 4, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 6, 2015

So, regarding the specialization to PhantomData, it's unclear to me why we even need to limit this to ZST. IIRC, we could permit coercion of any number of fields (whether ZST or not) within the smart pointer type. I presume that the current limitation is there to ease implementation (and be conservative).

@eddyb thoughts?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Aug 7, 2015

@nikomatsakis I would suggest limiting this as much as possible, otherwise it might allow writing transmute in safe code.
Not sure if this is what you're referring to, but removing the requirement that exactly one field coerces via unsizing would only require an impl<T, U> CoerceUnsized<PhantomData<U>> for PhantomData<T> (and more impls could be added for each ZST that needs it).
However, I don't think we should allow those 0-fields-unsizing coercions to actually happen in expressions (except that if the impl exists, it can't be guarded against it being used in coercions due to generics).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 7, 2015

@eddyb can you explain what you mean by "allow writing transmute in safe code"? (e.g., maybe give an example) Perhaps I have to refresh my memory of just which traits we're talking about here.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Aug 7, 2015

@nikomatsakis The transmute remark was about the worst case, where arbitrary types are converted without an actual coercion, e.g:

struct Foo<P, T>(Box<P>, T);
impl<P: Unsize<Q>, T, U> CoerceUnsized<Foo<Q, U>> for Foo<P, T> {}

Though I doubt you had that in mind.
Then there's odd usecases of ZSTs, such as those contravariant over a lifetime acquired from borrowing some resource (a "token" type).
But generic ZSTs are either PhantomData or have ZST fields, and in the latter case a CoerceUnsized impl for the ZST itself would be required anyway (and would not hold due to having no actual unsizing happening).
So whether you ignore all struct S<...>; or just PhantomData, it doesn't matter, only PhantomData can have that form.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 14, 2015

We discussed this today in the language subteam meeting. The conclusion was that while many of us believe we can generalize DST coercions further (read: @nikomatsakis does, at least), we should not let the perfect be the enemy of the good. Therefore, we should take this RFC, and consider further generalizing the rules another time.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 14, 2015

Hear ye, hear ye. This RFC is entering final comment period.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 14, 2015

Ah, @Gankro, can you amend the RFC to include as an "unresolved question" the extent to which coercion can be generalized to simply permit multiple fields to be coerced?

@Gankro

This comment has been minimized.

Copy link
Contributor Author

Gankro commented Aug 20, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 4, 2015

Huzzah. The lang subteam has decided to accept this RFC. Tracking issue rust-lang/rust#28239.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.