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

DST custom coercions. #982

Merged
merged 5 commits into from Jun 3, 2015

Conversation

@nrc
Copy link
Member

nrc commented Mar 16, 2015

Custom coercions allow smart pointers to fully participate in the DST system.
In particular, they allow practical use of Rc<T> and Arc<T> where T is unsized.

This RFC subsumes part of RFC 401 coercions.

Rendered

[edited to add rendered link]

@nrc nrc self-assigned this Mar 17, 2015

* If the impl is for a built-in pointer type, we check nothing, otherwise...
* The compiler checks that the `Self` type is a struct or tuple struct and that
the `Target` type is a simple substitution of type parameters from the `Self`

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 18, 2015

Contributor

I don't quite know what "simple substitution of type parameters" means... but I think we want to check that both of them are applications of the same base type constructor. e.g. Foo<X> and Foo<Y> are ok, but not Foo<X> and Bar<Y>.

This comment has been minimized.

Copy link
@nrc

nrc Mar 18, 2015

Author Member

I meant that there exists some Ts (where Ts = a sequence of T) and some Xs such that Target = [Ts/Xs]Self and Self is not in Xs. Which I think subsumes your requirement, but also precludes some pathalogical cases such as Target = Foo<T1, T2>, Self = Foo<X, X>.

* The compiler checks each field in the `Self` type against the corresponding field
in the `Target` type. Assuming `Fs` is the type of a field in `Self` and `Ft` is
the type of the corresponding field in `Target`, then either `Ft <: Fs` or
`Fs: CoerceUnsized<Ft>` (note that this includes built-in coercions).

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 18, 2015

Contributor

How does this include builtin coercions? There are some coercions, at least, that are not represented by the CoerceUnsized trait, right? I guess that's primarily the coercions around fn pointers and in particular unsafe fn items.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 18, 2015

Contributor

Sorry, in particular the safe to unsafe coercion, as well as the coercion from fn item to fn pointer. The latter doesn't matter too much in this context right now, but it might if we ever get a syntax for naming the type of a fn item (typeof foo, perhaps).

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 18, 2015

Contributor

That said we could probably rewrite this rule as "if the type of the source is coercible to the type of the target", which encompasses a CoerceUnsized search.

This comment has been minimized.

Copy link
@nrc

nrc Mar 18, 2015

Author Member

I had assumed we wouldn't coerce other than using the one nested coercion since that might add complications to the compiler's representation. Also the motivation here is to support strictly DST coercions, not all coercions. I guess if we want to coerce Rc<Fn> to Rc<unsafe Fn> we need this. I'd be happier to add it later though and only if needed.

# Drawbacks

Not as flexible as the previous proposal. Can't handle pointer-like types like
`Option<Box<T>>`.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 18, 2015

Contributor

I believe it can handle Option<Box<T>> in the new design.

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 18, 2015

Member

I agree, but the rest of the RFC assumes only struct types.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 18, 2015

I read over the revised version -- it seems pretty good to me, I left a few comments.


```
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<Rc<U>> for Rc<T> {}
impl<T: ?Sized+CoerceUnsized<U>, U: ?Sized> NonZero<U> for NonZero<T> {}

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 18, 2015

Member

Is this supposed to be implementing CoerceUnsized<NonZero<U>>?

This comment has been minimized.

Copy link
@nrc

nrc Mar 18, 2015

Author Member

yes

@@ -0,0 +1,182 @@
- Feature Name: dst-coercions

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 18, 2015

Member

Isn't this supposed to be a valid identifier, i.e. s/-/_/?

Unsize<[i32]>`. Note that the existence of an `Unsize` impl does not signify a
coercion can itself can take place, it represents an internal part of the
coercion mechanism (it corresponds with `coerce_inner` from RFC 401). The trait
is defined as:

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 18, 2015

Member

I don't think this design can prevent writing functions generic over unsizing coercions - is that even desired?

This comment has been minimized.

Copy link
@nrc

nrc Mar 18, 2015

Author Member

You should use T: CoerceUnsized<U> for a bound and then I think it is possible. Using Unsize as a bound only really makes sense in the CoerceUnsized impls.

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 19, 2015

Member

An Unsize bound makes the CoerceUnsized impls applicable, which enables the coercions.


We add `AdjustCustom` to the `AutoAdjustment` enum as a placeholder for coercions
due to a `CoerceUnsized` bound. I don't think we need the `UnsizeKind` enum at
all now, since all checking is postponed until trans or relies on traits and impls.

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 18, 2015

Member

Yes! I've done this in a local branch and now there's only unsizing coercion creation (typeck) and unsizing information generation (trans) that cares about the specific kind of unsizing being performed.
The later is even O(1), it ignores structure nesting completely.
This is how it looks like (I hope to replace root_target by the resulting pointer type):

// In the case of `&Struct<[T; N]>` -> `&Struct<[T]>`, the types are:
// * leaf_source: `[T; N]`
// * leaf_target: `[T]`
// * root_target: `Struct<[T]>`
pub struct AutoUnsize<'tcx> {
    leaf_source: Ty<'tcx>,
    leaf_target: Ty<'tcx>,
    root_target: Ty<'tcx>
}
impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a> CoerceUnsized<&'a U> for Box<T> {}
impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a> CoerceUnsized<&mut 'a U> for Box<T> {}
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<*const U> for Box<T> {}
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<*mut U> for Box<T> {}

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 26, 2015

Member

I forgot to mention, all of these (but the first one) are unsafe as they convert a Box<T> value into a reference/pointer that doesn't own the value, making it trivial to implement mem::forget.
None of the Box<T> -> ref/ptr to T coercions are currently implemented so not having them isn't a breaking change.

impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<*mut U> for Box<T> {}
impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a, 'b: 'a> CoerceUnsized<&'a U> for &mut 'b U {}
impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a> CoerceUnsized<&mut 'a U> for &mut 'a U {}

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 26, 2015

Member

&'a mut T not &mut 'a T. Also, why is this one so strict, surely shorte- oh, I see!
This is supposed to be used inside a structure, where &'a mut T is invariant.

Which makes me wonder, how can I enforce that?
Does subtyping handle &'a mut T being invariant and &'a T contravariant wrt 'a?

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 26, 2015

Member

Actually, come to think of it, this is by-value so it should be fine to have both &mut T -> &U and &mut T -> &mut U capable of shortening the lifetime.

This comment has been minimized.

Copy link
@nrc

nrc Jun 3, 2015

Author Member

I think this is true. I'll probably leave the RFC as is, since that reflects the implementation. But it does feel we could make this more flexible.

cc @nikomatsakis

impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a> CoerceUnsized<*const U> for &mut 'a U {}
impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a> CoerceUnsized<*mut U> for &mut 'a U {}
impl<T: ?Sized+Unsize<U>, U: ?Sized, 'a, 'b> CoerceUnsized<&'a U> for &'b U {}

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 26, 2015

Member

Missing 'b: 'a here. Also, all the Self types should contain T not U.

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 27, 2015

Member

And lifetime params come before type params. I had to try to compile it to actually find that out...


```
impl<T: ?Sized+Unsize<U>, U: ?Sized> CoerceUnsized<Rc<U>> for Rc<T> {}
impl<T: ?Sized+CoerceUnsized<U>, U: ?Sized> CoerceUnsized<NonZero<U>> for NonZero<T> {}

This comment has been minimized.

Copy link
@eddyb

eddyb Mar 27, 2015

Member

This shouldn't make T and U unsized, NonZero wraps scalars (they also both need a Zeroable bound).

@nrc nrc added the T-lang label May 15, 2015
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 21, 2015

@nrc did you plan to incorporate @eddyb's feedback here?

@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented May 21, 2015

I do, I had forgotten this was not merged yet.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented May 26, 2015

Hear ye, hear ye. This RFC is now in final comment period until June 2nd.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jun 2, 2015

This RFC has been accepted. It is not yet merged to allow @nrc to incorporate final comments reflecting the current design. (It has been partially implemented in the meantime.)

-- Language design subteam

nrc added 5 commits Mar 16, 2015
Custom coercions allow smart pointers to fully participate in the DST system.
In particular, they allow practical use of `Rc<T>` and `Arc<T>` where `T` is unsized.

This RFC subsumes part of [RFC 401 coercions](https://github.com/rust-lang/rfcs/blob/master/text/0401-coercions.md).
@nrc nrc force-pushed the nrc:dst-coercions branch from 433245c to 4b99006 Jun 3, 2015
@nrc

This comment has been minimized.

Copy link
Member Author

nrc commented Jun 3, 2015

@nikomatsakis updated

@nikomatsakis nikomatsakis merged commit 4b99006 into rust-lang:master Jun 3, 2015
nikomatsakis added a commit that referenced this pull request Jun 3, 2015
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jun 3, 2015

Merged. Tracking issue is rust-lang/rust#18598

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