Take 2: Implement object-safety and dynamic dispatch for arbitrary_self_types #54383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
mikeyhew
force-pushed
the
custom-receivers-object-safety
branch
from
9e50ace
to
474f7f2
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I see a few problems with
I'd prefer something like this, instead: trait CoerceUnsizedDispatchable<T: ?Sized>: CoerceUnsized<T> {}Making the requirements (also including some other slight renames): ConcreteReceiver = (typeof self) where Self: Sized;
DynReceiver = (typeof self)[dyn Trait/Self];
ConcreteReceiver: CoerceUnsizedDispatchable<DynReceiver> |
|
I don't really understand why you would say this is not a "sizing" coercion. It seems like exactly what it is to me. Like any coercion, the type changes and so does the in-memory representation (in contrast to a subtyping relationship which -- at least in rustc -- implies that the type changes but not the in-memory representation). In this case, we are coercing a You are absolutely correct that the final type ( Obviously, this all a bikeshed, so I'm willing to go with whatever we settle on. The name |
|
We discussed this on Discord and we reached some agreement that the current state is not as clear-cut as we'd like it to be eventually (e.g. we can't currently make The name we settled on was Also, I suggested that that these additional checks are performed (for object safety): layout_of(ConcreteReceiver).abi == Scalar(Pointer)
layout_of(DynReceiver).abi == ScalarPair(Pointer, Pointer)This way, we don't die in |
|
☔ The latest upstream changes (presumably #54389) made this pull request unmergeable. Please resolve the merge conflicts. |
mikeyhew
force-pushed
the
custom-receivers-object-safety
branch
from
474f7f2
to
d1a31d0
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@eddyb @nikomatsakis so what trait do you want, and what would example |
src/libcore/ops/unsize.rs
Outdated
| /// - and similarly for &mut T, *const T, *mut T, Box<T>, Rc<T>, Arc<T> | ||
| #[unstable(feature = "coerce_sized", issue = "0")] | ||
| #[cfg_attr(not(stage0), lang = "coerce_sized")] | ||
| pub trait CoerceSized<T> where T: CoerceUnsized<Self> { |
There was a problem hiding this comment.
So, we need to rename this, per @eddyb's suggestion
| continue; | ||
| // descend through newtype wrappers until `op` is a builtin pointer to | ||
| // `dyn Trait`, e.g. `*const dyn Trait`, `&mut dyn Trait` | ||
| 'descend_newtypes: while !op.layout.ty.is_unsafe_ptr() |
There was a problem hiding this comment.
can we encapsulate this into a helper fn? this say hacky loop appears twice, no?
There was a problem hiding this comment.
Sure, if you think it will make the code more readable. I worry it might make it less so, but I'll see what I can do and you can review it
There was a problem hiding this comment.
Now that I'm rebasing on #54183, which has conflicts here, I'm starting to realize that this code isn't very easy to understand. So I'm going to try to do the refactor you suggested and add some comments to explain what's going on, since it's very hacky. Essentially we're taking a newtyped pointer, removing all the newtype wrappers to get a *mut dyn Trait, &dyn Trait or similar, taking away the vtable so it's just a thin pointer, but telling the compiler its type is still *mut dyn Trait because it understands that as a special case somewhere else according to @eddyb
I think the name we settled on was pub trait CoerceSized<T> where T: CoerceUnsized<Self> {..}
impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceSized<Rc<T>> for Rc<U> {}Then we might have: pub trait DispatchFromDyn<T> { }
impl<T: ?Sized + Unsize<U>, U: ?Sized> DispatchFromDyn<Rc<U>> for Rc<T> {}Does that sound about right, @eddyb? I'm not sure what the implications are of removing the |
|
And of course @eddyb mentioned adding these "layout checks" for object safety: |
src/librustc/traits/object_safety.rs
Outdated
| /// | ||
| /// forall (U: ?Sized) { | ||
| /// if (Self: Unsize<U>) { | ||
| /// Receiver[Self => U]: CoerceSized<Receiver> |
There was a problem hiding this comment.
Receiver: DynDispatchFrom<Receiver[Self => U]>
No, because I ended up reimplementing all of the |
|
All of @nikomatsakis' comments seem good, I'll take a look at the PR after all the changes have been made. |
|
☔ The latest upstream changes (presumably #54660) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
added
S-waiting-on-author
mikeyhew
force-pushed
the
custom-receivers-object-safety
branch
from
d1a31d0
to
166fc50
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
OK, I'm guessing CI will fail on error-index.md again. If it does, hopefully someone can tell me how to run that test locally, so I don't have to work off CI. I'm getting a failing LLVM assertion on one of the thin-lto tests, but I'm guessing it's a stage 1 thing because I had it before and it didn't show up in CI |
mikeyhew
commented
Oct 4, 2018
Oh, I just remembered this. Will add them later, don't let me forget |
mikeyhew
commented
Oct 4, 2018
| @@ -100,30 +100,6 @@ error[E0624]: associated constant `A` is private | |||
| LL | C::A; //~ ERROR associated constant `A` is private | |||
| | ^^^^ | |||
|
|
|||
There was a problem hiding this comment.
Someone ought to take a look at this, I'm not sure why some errors are going away but see the commit message on 6db1879
There was a problem hiding this comment.
seems plausible, I'm not too worried about this
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
tests were failing because I didn't wrap code snippets like in backticks. fixed that now, so hopefully tests will pass on travis
Rename `CoerceSized` to `DispatchFromDyn`, and reverse the direction so that, for example, you write
```
impl<T: Unsize<U>, U> DispatchFromDyn<*const U> for *const T {}
```
instead of
```
impl<T: Unsize<U>, U> DispatchFromDyn<*const T> for *const U {}
```
this way the trait is really just a subset of `CoerceUnsized`.
The checks in object_safety.rs are updated for the new trait, and some documentation and method names in there are updated for the new trait name — e.g. `receiver_is_coercible` is now called `receiver_is_dispatchable`. Since the trait now works in the opposite direction, some code had to updated here for that too.
I did not update the error messages for invalid `CoerceSized` (now `DispatchFromDyn`) implementations, except to find/replace `CoerceSized` with `DispatchFromDyn`. Will ask for suggestions in the PR thread.
If object-safety checks succeed for a receiver type, make sure the receiver’s abi is a) a Scalar, when Self = () b) a ScalarPair, when Self = dyn Trait
Added to `DispatchFromDyn` and `CoerceUnsized` error messages
also updated the doc on `receiver_is_dispatchable` to reflect current state of the implementation
…candidates I don't really understand what it's for, but see the comment here: rust-lang#50173 (comment) where arielb1 said > Does this check do anything these days? I think `$0: Trait` is always considered ambiguous and nikomatsakis agreed we may be able to get rid of it
disallow `#[repr(C)] and `#[repr(packed)]` on structs implementing DispatchFromDyn because they will change the ABI from Scalar/ScalarPair to Aggregrate, resulting in an ICE during object-safety checks or codegen
Done. |
mikeyhew
force-pushed
the
custom-receivers-object-safety
branch
from
1b36350
to
192e7c4
Compare
|
This is ready to go. |
|
@nikomatsakis can you r+ this? |
|
@bors r=nikomatsakis (based on the approval below this comment) |
|
📌 Commit 192e7c4 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 3, 2018
…omatsakis Take 2: Implement object-safety and dynamic dispatch for arbitrary_self_types This replaces #50173. Over the months that that PR was open, we made a lot of changes to the way this was going to be implemented, and the long, meandering comment thread and commit history would have been confusing to people reading it in the future. So I decided to package everything up with new, straighforward commits and open a new PR. Here are the main points. Please read the commit messages for details. - To simplify codegen, we only support receivers that have the ABI of a pointer. That means they are builtin pointer types, or newtypes thereof. - We introduce a new trait: `DispatchFromDyn<T>`, similar to `CoerceUnsized<T>`. `DispatchFromDyn` has extra requirements that `CoerceUnsized` does not: when you implement `DispatchFromDyn` for a struct, there cannot be any extra fields besides the field being coerced and `PhantomData` fields. This ensures that the struct's ABI is the same as a pointer. - For a method's receiver (e.g. `self: Rc<Self>`) to be object-safe, it needs to have the following property: - let `DynReceiver` be the receiver when `Self = dyn Trait` - let `ConcreteReceiver` be the receiver when `Self = T`, where `T` is some unknown `Sized` type that implements `Trait`, and is the erased type of the trait object. - `ConcreteReceiver` must implement `DispatchFromDyn<DynReceiver>` In the case of `Rc<Self>`, this requires `Rc<T>: DispatchFromDyn<Rc<dyn Trait>>` These rules are explained more thoroughly in the doc comment on `receiver_is_dispatchable` in object_safety.rs. r? @nikomatsakis and @eddyb cc @arielb1 @cramertj @withoutboats Special thanks to @nikomatsakis for getting me un-stuck when implementing the object-safety checks, and @eddyb for helping with the codegen parts. EDIT 2018-11-01: updated because CoerceSized has been replaced with DispatchFromDyn
|
☀️ Test successful - status-appveyor, status-travis |
15 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This replaces #50173. Over the months that that PR was open, we made a lot of changes to the way this was going to be implemented, and the long, meandering comment thread and commit history would have been confusing to people reading it in the future. So I decided to package everything up with new, straighforward commits and open a new PR.
Here are the main points. Please read the commit messages for details.
DispatchFromDyn<T>, similar toCoerceUnsized<T>.DispatchFromDynhas extra requirements thatCoerceUnsizeddoes not: when you implementDispatchFromDynfor a struct, there cannot be any extra fields besides the field being coerced andPhantomDatafields. This ensures that the struct's ABI is the same as a pointer.self: Rc<Self>) to be object-safe, it needs to have the following property:DynReceiverbe the receiver whenSelf = dyn TraitConcreteReceiverbe the receiver whenSelf = T, whereTis some unknownSizedtype that implementsTrait, and is the erased type of the trait object.ConcreteReceivermust implementDispatchFromDyn<DynReceiver>In the case of
Rc<Self>, this requiresRc<T>: DispatchFromDyn<Rc<dyn Trait>>These rules are explained more thoroughly in the doc comment on
receiver_is_dispatchablein object_safety.rs.r? @nikomatsakis and @eddyb
cc @arielb1 @cramertj @withoutboats
Special thanks to @nikomatsakis for getting me un-stuck when implementing the object-safety checks, and @eddyb for helping with the codegen parts.
EDIT 2018-11-01: updated because CoerceSized has been replaced with DispatchFromDyn