Implement object-safety for arbitrary_self_types #50173
Conversation
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 |
|
It looks like, in order to do the |
mikeyhew
force-pushed
the
arbitrary-self-types-object-safety
branch
from
7905214
to
a234899
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 |
|
@mikeyhew My suggestion lately has been to do the check that a The problems I've been thinking of arise when the (BTW I think |
|
Hmm, I remember that I really wanted to revisit the way that the "coerce unsized" check works in general. I need to go refresh my memory I guess. PS thanks a ton @mikeyhew for starting in on this! Sorry I've not had time for feedback yet. |
|
Ping from triage, @nikomatsakis — will you have time to check on this soon? |
shepmaster
added
S-waiting-on-author
mikeyhew
force-pushed
the
arbitrary-self-types-object-safety
branch
from
a234899
to
ede12aa
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 |
|
@nikomatsakis / @rust-lang/compiler: It looks like this PR needs your input to continue. |
|
@TimNN, @nikomatsakis and I have been talking about this in Discord. It looks like this will be easier to implement the CoerceUnsized check once some trait system refactoring has landed — specifically, once I'm also working on refactoring the |
TimNN
added
S-blocked
|
Thanks for the update @mikeyhew. I've marked this PR is blocked, is there an issue or PR we can follow to see when this PR gets unblocked? |
|
@TimNN there isn't one. #50753 is sort of a blocker in that it would be nice to do that before this, but it's not strictly necessary. I'll talk to @nikomatsakis on Discord soon, so we can figure out what specifically this PR is blocked on, and maybe open an issue or PR for that. |
pietroalbini
added
S-blocked
TimNN
added
A-allocators
TimNN
added
A-allocators
mikeyhew
force-pushed
the
arbitrary-self-types-object-safety
branch
from
ede12aa
to
24b563a
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 |
|
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 |
test includes `Rc`, `Arc,` `PinMut`, and `PinBox` as receiver types. To be converted to a run-pass test once dynamic dispatch is implemented.
|
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 |
|
Does anyone know what's causing this Travis error? |
src/libcore/marker.rs
Outdated
| @@ -135,6 +135,7 @@ pub trait Unsize<T: ?Sized> { | |||
| // Empty. | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
That's a holdover from when CoerceSized was there and should be removed
Compiler-builtins and clippy submodules are changed in this pr, so you have to update |
|
Oh that's weird, I definitely didn't mean to update any submodules. I'll change them back. Thanks @bjorn3! |
|
@mikeyhew did you have submodules inited during rebase? They will keep their old checked out commit when checking out to a commit which updates a submodule to a different commit. I always run |
|
@bjorn3 do you think that when I did |
|
@mikeyhew yes. This has happened to me several times :) |
| let target_self_ty: Ty<'tcx> = self.mk_ty_param( | ||
| ::std::u32::MAX, | ||
| Name::intern("RustaceansAreAwesome").as_interned_str(), | ||
| ); |
There was a problem hiding this comment.
Oh, yes, I forgot about this =)
There was a problem hiding this comment.
So — this is obviously a horrible hack — but is it one that users will ever notice? The code below just invokes predicate_must_hold, which won't itself generate an error, so perhaps not? The only way I could imagine would be through overflow maybe.
|
(Note that we had planned — as part of the work on chalk — to make it possible to express the query in question cleanly; I'd be ok leaving this as a FIXME.) |
|
Good point, then I will add a FIXME comment above that code |
|
@mikeyhew @nikomatsakis ping on this -- I'm wondering if we have an ETA to get this over the finish line? |
|
It's very close. @eddyb helped me with the codegen parts, so that's actually implemented now in a separate branch. We simplified it a bit — for now, we're only going to allow receivers whose ABI is the same as a pointer, which basically means it's limited to (newtypes of)* pointers, with the only extra fields being zero-sized like What's left is adding checks in src/librustc_typeck/coherence/builtin.rs to enforce that. Once that's done, I'll open a new PR with everything, and it will just need to be reviewed. EDIT: @aturon asked for an ETA. I'm thinking by Monday I'll have the new PR open... but we'll see 😄 |
|
@mikeyhew thanks for the update! Our hope is that we can stabilize using (but not defining) arbitrary self type pointers in one of the next few release cycles. That will mean that any pointer that's can be used as a self pointer that's defined in |
|
@withoutboats that plan sounds good. We'll have to figure out a way to disallow creation of receiver types on stable then, because right now they just have to implement |
|
@mikeyhew what do you mean by this?
using how? |
|
This currently requires use std::rc::Rc;
trait Trait {
fn foo(self: Rc<Self>);
}However, we should still require struct MyBox<T: ?Sized>(Box<T>);
impl<T: ?Sized> Deref for MyCustomPtr<T> {
fn deref(&self) -> &T {
&*self.0
}
}
trait Trait {
fn foo(self: MyBox<Self>);
}In order to have the above distinction, we will need some way mark types like |
|
We could use Another option would be an attribute on the struct definition for I think the most robust way to do it would be to use a new, unstable trait with trait Receiver: Deref {}
impl<T: ?Sized> Receiver for Rc<T> {}
impl<P: Receiver> Receiver for Pin<P> {}Then for a type to be used as a receiver, it just needs to implement I know it seems like a lot of traits, with |
|
☔ The latest upstream changes (presumably #53877) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing in favour of #54383 |
…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
…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
r? @nikomatsakis
cc @withoutboats @arielb1 @eddyb
This is a work-in-progress PR that I am opening so that I can ask people questions as I implement it. Currently, I've added a single run-pass test that shows the ultimate goal of this PR in its most general form.Custom method receivers, that is, anything except for
self,&self,&mut selfandself: Box<Self>, are currently considered non-object-safe. This PR will relax that restriction, and will allow any receiver that can be coerced from its fat version (containing the vtable) to its thin version (without a vtable). To check this, we require the following trait obligation to be satisfied:where
Receiveris the type of the method'sselfargument, and theReceiver<Self=U>syntax meansReceiverbut with all instances ofSelfreplaced withU.