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

Take 2: Implement object-safety and dynamic dispatch for arbitrary_self_types #54383

Merged
merged 19 commits into from Nov 3, 2018

Conversation

Projects
None yet
8 participants
@mikeyhew
Contributor

mikeyhew commented Sep 20, 2018

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 20, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:46:58]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:47:16] warning: `[T]` cannot be resolved, ignoring it...
[00:47:16]   --> libcore/ops/unsize.rs:89:9
[00:47:16]    |
[00:47:16] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:47:16]    |
[00:47:16]    = note: #[warn(intra_doc_link_resolution_failure)] on by default
[00:47:16]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:47:16] 
[00:47:16] 
[00:47:16] warning: `[T]` cannot be resolved, ignoring it...
[00:47:16]   --> libcore/ops/unsize.rs:89:9
[00:47:16]    |
[00:47:16] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:47:16]    |
[00:47:16]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:47:16] 
[00:47:37]     Checking compiler_builtins v0.0.0 (file:///checkout/src/rustc/compiler_builtins_shim)
---
[00:47:40]     Checking panic_abort v0.0.0 (file:///checkout/src/libpanic_abort)
[00:47:42] warning: `[T]` cannot be resolved, ignoring it...
[00:47:42]   --> /checkout/src/libcore/ops/unsize.rs:89:9
[00:47:42]    |
[00:47:42] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:47:42]    |
[00:47:42]    = note: #[warn(intra_doc_link_resolution_failure)] on by default
[00:47:42]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:47:42] 
---
[00:47:51] 
[00:47:51] warning: `[T]` cannot be resolved, ignoring it...
[00:47:51]   --> /checkout/src/libcore/ops/unsize.rs:89:9
[00:47:51]    |
[00:47:51] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:47:51]    |
[00:47:51]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:47:51] 
[00:47:51] warning: `[Weak::upgrade]` cannot be resolved, ignoring it...
---
[00:57:48] ....................................................................................................
[00:57:51] .......................................................i............................................
[00:57:54] ....................................................................................................
[00:57:57] ....................................................................................................
[00:58:00] ..iiiiiiiii.........................................................................................
[00:58:06] ....................................................................................................
[00:58:10] .....................................................................................i..............
[00:58:12] ....................................................................................................
[00:58:16] ........................................i.i..ii.....................................................
---
[01:03:36] .......................................................................................i............
[01:03:39] ....................................................................................................
[01:03:43] ....................................................................................................
[01:03:45] ....................................................................................................
[01:03:48] ..i.ii.ii.ii.............................i..........................................................
[01:03:48] test result: ok. 6714 passed; 0 failed; 88 ignored; 0 measured; 0 filtered out
[01:03:48] 
[01:03:48]  finished in 421.992
[01:03:48] travis_fold:end:test_ui_nll
---
travis_time:start:test_run-pass
Check compiletest suite=run-pass mode=run-pass (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:03:48] 
[01:03:48] running 551 tests
[01:04:03] ........F.................................i.........................................................
[01:04:30] ....................................................................................................
[01:04:40] ....................................................................................................
[01:04:58] ..................i.................................................................................
[01:05:05] ...................................................
[01:05:05] ...................................................
[01:05:05] failures:
[01:05:05] 
[01:05:05] ---- [run-pass] run-pass/arbitrary_self_types_stdlib_pointers.rs stdout ----
[01:05:05] 
[01:05:05] error: compilation failed!
[01:05:05] status: exit code: 1
[01:05:05] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/arbitrary_self_types_stdlib_pointers.rs" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/arbitrary_self_types_stdlib_pointers/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/arbitrary_self_types_stdlib_pointers/auxiliary"
[01:05:05] ------------------------------------------
[01:05:05] 
[01:05:05] ------------------------------------------
[01:05:05] stderr:
[01:05:05] stderr:
[01:05:05] ------------------------------------------
[01:05:05] error[E0282]: type annotations needed
[01:05:05]   --> /checkout/src/test/run-pass/arbitrary_self_types_stdlib_pointers.rs:54:19
[01:05:05]    |
[01:05:05] 54 |     let pin_box = Box::new(4i64).into() as Pin<Box<dyn Trait>>;
[01:05:05]    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type for `T`
[01:05:05]    = note: type must be known at this point
[01:05:05] 
[01:05:05] error: aborting due to previous error
[01:05:05] 
[01:05:05] 
[01:05:05] For more information about this error, try `rustc --explain E0282`.
[01:05:05] 
[01:05:05] ------------------------------------------
[01:05:05] 
[01:05:05] thread '[run-pass] run-pass/arbitrary_self_types_stdlib_pointers.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3205:9
[01:05:05] 
[01:05:05] 
[01:05:05] failures:
[01:05:05]     [run-pass] run-pass/arbitrary_self_types_stdlib_pointers.rs
[01:05:05]     [run-pass] run-pass/arbitrary_self_types_stdlib_pointers.rs
[01:05:05] 
[01:05:05] test result: FAILED. 548 passed; 1 failed; 2 ignored; 0 measured; 0 filtered out
[01:05:05] 
[01:05:05] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:496:22
[01:05:05] 
[01:05:05] 
[01:05:05] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/run-pass" "--build-base" "/check

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:2270ea6e
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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 @TimNN. (Feature Requests)

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 20, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:40:48]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:41:01] warning: `[T]` cannot be resolved, ignoring it...
[00:41:01]   --> libcore/ops/unsize.rs:89:9
[00:41:01]    |
[00:41:01] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:41:01]    |
[00:41:01]    = note: #[warn(intra_doc_link_resolution_failure)] on by default
[00:41:01]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:41:01] 
[00:41:01] 
[00:41:01] warning: `[T]` cannot be resolved, ignoring it...
[00:41:01]   --> libcore/ops/unsize.rs:89:9
[00:41:01]    |
[00:41:01] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:41:01]    |
[00:41:01]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:41:01] 
[00:41:19]     Checking compiler_builtins v0.0.0 (file:///checkout/src/rustc/compiler_builtins_shim)
---
[00:41:21]     Checking alloc_jemalloc v0.0.0 (file:///checkout/src/liballoc_jemalloc)
[00:41:23] warning: `[T]` cannot be resolved, ignoring it...
[00:41:23]   --> /checkout/src/libcore/ops/unsize.rs:89:9
[00:41:23]    |
[00:41:23] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:41:23]    |
[00:41:23]    = note: #[warn(intra_doc_link_resolution_failure)] on by default
[00:41:23]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:41:23] 
---
[00:41:29] 
[00:41:29] warning: `[T]` cannot be resolved, ignoring it...
[00:41:29]   --> /checkout/src/libcore/ops/unsize.rs:89:9
[00:41:29]    |
[00:41:29] 89 | /// - &[T] is CoerceSized<&[T; N]> for any N
[00:41:29]    |
[00:41:29]    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:41:29] 
[00:41:29] warning: `[Weak::upgrade]` cannot be resolved, ignoring it...
---
[00:49:23] ....................................................................................................
[00:49:26] ......................................................i.............................................
[00:49:28] ....................................................................................................
[00:49:31] ....................................................................................................
[00:49:33] ..iiiiiiiii.........................................................................................
[00:49:39] ....................................................................................................
[00:49:41] .....................................................................................i..............
[00:49:44] ....................................................................................................
[00:49:46] ........................................i.i..ii.....................................................
---
[01:16:30]   |
[01:16:30] 3 | use std::marker::PhantomData;
[01:16:30]   |
[01:16:30] 
[01:16:30] error[E0378]: the trait `CoerceSized` may only be implemented for structs containing the field being coerced, `PhantomData` fields, and nothing else
[01:16:30]   --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:5599:1
[01:16:30]    |
[01:16:30] 16 | / impl<T, U> CoerceSized<Wrapper<T>> for Wrapper<U>
[01:16:30] 17 | | where
[01:16:30] 18 | |     T: CoerceUnsized<U>,
[01:16:30] 19 | |     U: CoerceSized<T>,
[01:16:30] 20 | | {}
[01:16:30]    |
[01:16:30]    |
[01:16:30]    = note: extra field `_phantom` of type `[type error]` is not allowed
[01:16:30] thread '/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0378 (line 5585)' panicked at 'couldn't compile the test', librustdoc/test.rs:333:13
[01:16:30] 
[01:16:30] 
[01:16:30] failures:
---
[01:16:30] 
[01:16:30] 
[01:16:30] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:16:30] Build completed unsuccessfully in 0:34:23
[01:16:30] make: *** [check] Erro5381060 .
2984884 ./obj/build
2365004 ./obj/build/x86_64-unknown-linux-gnu
1192992 ./.git
1067680 ./src
---
162932 ./.git/modules/src/tools/lldb/objects/pack
151412 ./src/tools/clang
149112 ./src/llvm-emscripten/test
148628 ./obj/build/bootstrap/debug/incremental
142256 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86 printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0655edaa
travis_time:start:0655edaa
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0729ee22
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 20, 2018

I see a few problems with CoerceSized:

  1. the name - CoerceUnsized means "coerce, via the unsizing coercion (applied on a field)"; there is no "sizing" coercion, and even if it it made sense, it'd be "re-sized" or something
  2. the conceptualizing, of "getting back the sized type", doesn't really work in current Rust - I believe what we want here is "existential unpacking", involving skolem forms or something like that - but that's much longer term, we don't have the compiler technology in rustc for soundly doing it
  3. the parameter order compared to CoerceUnsized doesn't seem to serve a specific purpose

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>
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 20, 2018

@eddyb

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 Rc<U> type to a Rc<S> type
- where U is the unsized type we start with
- and S is the (hidden) sized type that underlies it

You are absolutely correct that the final type (Rc<S>) is not a type that we know statically, but I don't know why that makes it less of a coercion.

Obviously, this all a bikeshed, so I'm willing to go with whatever we settle on. The name CoerceUnsizedDispatchable doesn't exactly "roll off the tongue though".

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 20, 2018

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 Rc<T> out of Rc<dyn Trait>, so codegen has to get the internal *mut RcBox<dyn Trait> from Rc and dispatch on that).

The name we settled on was DynDispatchFrom (as in, dispatch on concrete from dynamic), although I just thought of DispatchFromDyn, which might be more intuitive.

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 codegen / miri, and we instead, detect it much earlier (this is "temporary", until we can do something closer to existential unpacking).

@bors

This comment has been minimized.

Contributor

bors commented Sep 21, 2018

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 21, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:54:26] ....................................................................................................
[00:54:28] ......................................................i.............................................
[00:54:31] ....................................................................................................
[00:54:34] ....................................................................................................
[00:54:37] ..iiiiiiiii.........................................................................................
[00:54:43] ....................................................................................................
[00:54:46] ......................................................................................i.............
[00:54:49] ....................................................................................................
[00:54:52] .........................................i.i..ii....................................................
---
[01:27:28] 
[01:27:28] failures:
[01:27:28] 
[01:27:28] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0378 (line 5565) stdout ----
[01:27:28] error[E0405]: cannot find trait `CoerceUnsized` in this scope
[01:27:28]   --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:5574:28
[01:27:28]    |
[01:27:28] 11 | impl<T: ?Sized, U: ?Sized> CoerceUnsized<Ptr<U>> for Ptr<T>
[01:27:28]    |                            ^^^^^^^^^^^^^ did you mean `CoerceSized`?
[01:27:28]    |
[01:27:28]    |
[01:27:28] 3  | use std::ops::CoerceUnsized;
[01:27:28] 
[01:27:28] thread '/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0378 (line 5565)' panicked at 'couldn't compile the test', librustdoc/test.rs:333:13
[01:27:28] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:27:28] 
[01:27:28] 
[01:27:28] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0378 (line 5585) stdout ----
[01:27:28] error[E0412]: cannot find type `PhantomData` in this scope
[01:27:28]  --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:5591:15
[01:27:28]   |
[01:27:28] 8 |     _phantom: PhantomData<()>,
[01
travis_time:end:04993b6f:start=1537510713754336308,finish=1537515962628450007,duration=5248874113699

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
---
travis_time:end:1a9d4020:start=1537515964439982332,finish=1537515964443983046,duration=4000714
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2caf179e
$ ln -s . checkout && for CORE in obj/cores/core.

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 @TimNN. (Feature Requests)

@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Sep 26, 2018

@eddyb @nikomatsakis so what trait do you want, and what would example impls look like for Rc<T> and Pin<P>?

@nikomatsakis

Argh, I appear to keep failing to finish a complete review. Here are a few in-process comments, all tiny.

/// - 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> {

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 27, 2018

Contributor

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()

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 27, 2018

Contributor

can we encapsulate this into a helper fn? this say hacky loop appears twice, no?

This comment has been minimized.

@mikeyhew

mikeyhew Oct 4, 2018

Contributor

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

This comment has been minimized.

@mikeyhew

mikeyhew Oct 28, 2018

Contributor

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

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 27, 2018

@mikeyhew

so what trait do you want, and what would example impls look like for Rc<T> and Pin<P>?

I think the name we settled on was DispatchFromDyn. I think this implies that we swap the order of the arguments, and I also believe @eddyb said something about removing the T: CoerceUnsized<U> requirement. So if the current definition is:

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 CoerceUnsized requirement, though. Are there any?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 27, 2018

And of course @eddyb mentioned adding these "layout checks" for object safety:

layout_of(ConcreteReceiver).abi == Scalar(Pointer)
layout_of(DynReceiver).abi == ScalarPair(Pointer, Pointer)
Show resolved Hide resolved src/librustc/traits/object_safety.rs Outdated
///
/// forall (U: ?Sized) {
/// if (Self: Unsize<U>) {
/// Receiver[Self => U]: CoerceSized<Receiver>

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 27, 2018

Contributor
Receiver: DynDispatchFrom<Receiver[Self => U]>
@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Sep 27, 2018

I'm not sure what the implications are of removing the CoerceUnsized requirement, though. Are there any?

No, because I ended up reimplementing all of the CoerceUnsized checks for CoerceSized here: https://github.com/rust-lang/rust/pull/54383/files#diff-aaae39ee387dc6a0aea969d3799e2bfa

@eddyb

This comment has been minimized.

Member

eddyb commented Sep 28, 2018

All of @nikomatsakis' comments seem good, I'll take a look at the PR after all the changes have been made.

@bors

This comment has been minimized.

Contributor

bors commented Sep 29, 2018

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 4, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:49:17] ...............................................................................................i.... 2200/4549
[00:49:21] .................................................................................................... 2300/4549
[00:49:25] .................................................................................................... 2400/4549
[00:49:28] .................................................................................................... 2500/4549
[00:49:32] .......iiiiiiiii.................................................................................... 2600/4549
[00:49:38] .................................................................................................... 2800/4549
[00:49:42] .................................................................................................... 2900/4549
[00:49:45] ..........................i......................................................................... 3000/4549
[00:49:47] ......................................................................................i.i..ii....... 3100/4549
---
travis_time:start:test_run-pass
Check compiletest suite=run-pass mode=run-pass (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:50:34] 
[00:50:34] running 2874 tests
[00:50:45] ...........F........................................................................................ 100/2874
[00:51:03] .................................................................................................... 300/2874
[00:51:13] .................................................................................................... 400/2874
[00:51:21] .................................................................................................... 500/2874
[00:51:33] .................................................................................................... 600/2874
---
[00:55:30] .................................................................................................... 2500/2874
[00:55:56] .................................................................................................... 2600/2874
[00:56:05] .................................................................................................... 2700/2874
[00:56:13] .................................................................................................... 2800/2874
56:23] To update references, rerun the tests and pass the `--bless` flag
[00:56:23] To only update this specific test, also pass `--test-args arbitrary_self_types_pointers_and_wrappers.rs`
[00:56:23] error: 1 errors occurred comparing output.
[00:56:23] status: exit code: 0
[00:56:23] status: exit code: 0
[00:56:23] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/arbitrary_self_types_pointers_and_wrappers.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/arbitrary_self_types_pointers_and_wrappers/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/arbitrary_self_types_pointers_and_wrappers/auxiliary"
[00:56:23] ------------------------------------------
[00:56:23] 
[00:56:23] ------------------------------------------
[00:56:23] stderr:
[00:56:23] stderr:
[00:56:23] ------------------------------------------
[00:56:23] {"message":"unused import: `fmt::Debug`","code":{"code":"unused_imports","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/arbitrary_self_types_pointers_and_wrappers.rs","byte_start":645,"byte_end":655,"line_start":16,"line_end":16,"column_start":5,"column_end":15,"is_primary":true,"text":[{"text":"    fmt::Debug,","highlight_start":5,"highlight_end":15}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unused/bootstrap-3ivyub3ic2113/s-f5eakr5kqq-ffhwli-8vudgacockuw
134192 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
131276 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release/deps
130808 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu
130804 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release

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 @TimNN. (Feature Requests)

@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Oct 4, 2018

OK, CoerceSized is now called DispatchFromDyn. @eddyb I guess it's your turn to review.

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

This comment has been minimized.

Contributor

mikeyhew commented Oct 4, 2018

And of course @eddyb mentioned adding these "layout checks" for object safety:

layout_of(ConcreteReceiver).abi == Scalar(Pointer)
layout_of(DynReceiver).abi == ScalarPair(Pointer, Pointer)

Oh, I just remembered this. Will add them later, don't let me forget

@@ -100,30 +100,6 @@ error[E0624]: associated constant `A` is private
LL | C::A; //~ ERROR associated constant `A` is private
| ^^^^

This comment has been minimized.

@mikeyhew

mikeyhew Oct 4, 2018

Contributor

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Oct 8, 2018

Contributor

seems plausible, I'm not too worried about this

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 4, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:45:56] .................................................................................................i.. 2200/4552
[00:46:01] .................................................................................................... 2300/4552
[00:46:04] .................................................................................................... 2400/4552
[00:46:07] .................................................................................................... 2500/4552
[00:46:11] .........iiiiiiiii.................................................................................. 2600/4552
[00:46:16] .................................................................................................... 2800/4552
[00:46:20] .................................................................................................... 2900/4552
[00:46:23] ............................i....................................................................... 3000/4552
[00:46:25] ........................................................................................i.i..ii..... 3100/4552
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:58:09] 
[00:58:09] running 107 tests
[00:58:12] i..ii...iii....i...i.........i..iii...........i.....i....ii...i.i.ii..............i...ii..ii.i....ii 100/107
[00:58:12] test result: ok. 77 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out
[00:58:12] 
[00:58:12]  finished in 3.193
[00:58:12] travis_fold:end:test_codegen
---
[01:10:39] .................................................................................................... 1500/2166
[01:10:45] .................................................................................................... 1600/2166
[01:10:52] .................................................................................................... 1700/2166
[01:11:00] .................................................................................................... 1800/2166
[01:11:07] ..................F................................................................................. 1900/2166
[01:11:25] ..................................................................................i................. 2100/2166
[01:11:30] .....................................i............................
[01:11:30] failures:
[01:11:30] 
[01:11:30] 
[01:11:30] ---- ops/unsize.rs - ops::unsize::DispatchFromDyn (line 86) stdout ----
[01:11:30] error[E0405]: cannot find trait `DispatchFromDyn` in this scope
[01:11:30]  --> ops/unsize.rs:87:28
[01:11:30]   |
[01:11:30] 4 | impl<T: ?Sized, U: ?Sized> DispatchFromDyn<Rc<U>> for Rc<T>
[01:11:30] help: possible candidate is found in another module, you can import it into scope
[01:11:30]   |
[01:11:30]   |
[01:11:30] 3 | use std::ops::DispatchFromDyn;
[01:11:30] 
[01:11:30] error[E0412]: cannot find type `Rc` in this scope
[01:11:30]  --> ops/unsize.rs:87:44
[01:11:30]   |
[01:11:30]   |
[01:11:30] 4 | impl<T: ?Sized, U: ?Sized> DispatchFromDyn<Rc<U>> for Rc<T>
[01:11:30]   |                                            ^^ not found in this scope
[01:11:30]   |
[01:11:30] 3 | use std::rc::Rc;
[01:11:30]   |
[01:11:30] 
[01:11:30] 
[01:11:30] error[E0412]: cannot find type `Rc` in this scope
[01:11:30]  --> ops/unsize.rs:87:55
[01:11:30]   |
[01:11:30] 4 | impl<T: ?Sized, U: ?Sized> DispatchFromDyn<Rc<U>> for Rc<T>
[01:11:30]   |                                                       ^^ not found in this scope
[01:11:30]   |
[01:11:30] 3 | use std::rc::Rc;
[01:11:30]   |
[01:11:30] 
[01:11:30] 
[01:11:30] error[E0405]: cannot find trait `Unsize` in this scope
[01:11:30]  --> ops/unsize.rs:89:8
[01:11:30]   |
[01:11:30] 6 |     T: Unsize<U>,
[01:11:30] help: possible candidate is found in another module, you can import it into scope
[01:11:30]   |
[01:11:30] 3 | use std::marker::Unsize;
[01:11:30]   |
[01:11:30]   |
[01:11:30] 
[01:11:30] thread 'ops/unsize.rs - ops::unsize::DispatchFromDyn (line 86)' panicked at 'couldn't compile the test', librustdoc/test.rs:333:13
[01:11:30] 
[01:11:30] 
[01:11:30] failures:
[01:11:30] failures:
[01:11:30]     ops/unsize.rs - ops::unsize::DispatchFromDyn (line 86)
[01:11:30] test result: FAILED. 2162 passed; 1 failed; 3 ignored; 0 measured; 0 filtered out
[01:11:30] 
[01:11:30] error: test failed, to rerun pass '--doc'
[01:11:30] 
[01:11:30] 
[01:11:30] 
[01:11:30] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[01:11:30] 
[01:11:30] 
[01:11:30] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:11:30] Build completed unsuccessfully in 0:29:55
[01:11:30] Build completed unsuccessfully in 0:29:55
[01:11:30] Makefile:58: recipe for target 'check' failed
[01:11:30] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:05be68ce
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:05816295:start=1538643468589890657,finish=1538643468594941308,duration=5050651
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:080e3e78
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\

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 @TimNN. (Feature Requests)

@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Oct 4, 2018

This llvm assertion keeps failing when I run ./x.py test, but doesn't seem to be failing on Travis. Are llvm assertions not enabled in CI?

---- [codegen] codegen/lto-removes-invokes.rs stdout ----

error: compilation failed!
status: signal: 6
command: "/users/mahewson/rust1/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/users/mahewson/rust1/src/test/codegen/lto-removes-invokes.rs" "--target=x86_64-unknown-linux-gnu" "--out-dir" "/users/mahewson/rust1/build/x86_64-unknown-linux-gnu/test/codegen/lto-removes-invokes" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/users/mahewson/rust1/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-C" "lto" "-C" "panic=abort" "-O" "-L" "/users/mahewson/rust1/build/x86_64-unknown-linux-gnu/test/codegen/lto-removes-invokes/auxiliary" "--emit=llvm-ir"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
warning: function is marked #[no_mangle], but not exported
  --> /users/mahewson/rust1/src/test/codegen/lto-removes-invokes.rs:20:1
   |
20 |   fn foo() {
   |   ^
   |   |
   |  _help: try making it public: `pub`
   | |
21 | |     let _a = Box::new(3);
22 | |     bar();
23 | | // CHECK-LABEL: foo
24 | | // CHECK: call {{.*}} void @bar
25 | | }
   | |_^
   |
   = note: #[warn(private_no_mangle_fns)] on by default

warning: function is marked #[no_mangle], but not exported
  --> /users/mahewson/rust1/src/test/codegen/lto-removes-invokes.rs:29:1
   |
29 |   fn bar() {
   |   ^
   |   |
   |  _help: try making it public: `pub`
   | |
30 | |     println!("hello!");
31 | | }
   | |_^

rustc: /users/mahewson/rust1/src/llvm/lib/IR/Value.cpp:256: void llvm::Value::setNameImpl(const llvm::Twine&): Assertion `!getType()->isVoidTy() && "Cannot assign a name to void values!"' failed.

------------------------------------------

see the last line of the error message. @eddyb I wonder if this has anything to do with the codegen code I wrote

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Oct 5, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:48:39] ................................................................................................i... 2200/4551
[00:48:43] .................................................................................................... 2300/4551
[00:48:46] .................................................................................................... 2400/4551
[00:48:50] .................................................................................................... 2500/4551
[00:48:53] ........iiiiiiiii................................................................................... 2600/4551
[00:48:59] .................................................................................................... 2800/4551
[00:49:03] .................................................................................................... 2900/4551
[00:49:05] ...........................i........................................................................ 3000/4551
[00:49:08] .......................................................................................i.i..ii...... 3100/4551
---
[01:27:54] 
[01:27:54] failures:
[01:27:54] 
[01:27:54] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0378 (line 5580) stdout ----
[01:27:54] error[E0412]: cannot find type `PhantomData` in this scope
[01:27:54]  --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:5586:15
[01:27:54]   |
[01:27:54] 8 |     _phantom: PhantomData<()>,
[01:27:54] help: possible candidate is found in another module, you can import it into scope
[01:27:54]   |
[01:27:54] 3 | use std::marker::PhantomData;
[01:27:54]   |
[01:27:54]   |
[01:27:54] 
[01:27:54] error[E0378]: the trait `DispatchFromDyn` may only be implemented for structs containing the field being coerced, `PhantomData` fields, and nothing else
[01:27:54]   --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:5589:1
[01:27:54]    |
[01:27:54] 11 | / impl<T, U> DispatchFromDyn<Wrapper<U>> for Wrapper<T>
[01:27:54] 12 | | where
[01:27:54] 13 | |     T: DispatchFromDyn<U>,
[01:27:54] 14 | | {}
[01:27:54]    |
[01:27:54]    |
[01:27:54]    = note: extra field `_phantom` of type `[type error]` is not allowed
[01:27:54] thread '/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0378 (line 5580)' panicked at 'couldn't compile the test', librustdoc/test.rs:333:13
[01:27:54] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:27:54] 
[01:27:54] 
---
[01:27:54] 
[01:27:54] 
[01:27:54] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:27:54] Build completed unsuccessfully in 0:43:45
[01:27:54] Makefile:58: recipe for target 'check' failed
[01:27:54] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1c170ff0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:1dbd56d0:start=1538730462552182258,finish=1538730462683663231,duration=131480973
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:012a87f0
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:08e65a34
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Oct 7, 2018

And of course @eddyb mentioned adding these "layout checks" for object safety:

layout_of(ConcreteReceiver).abi == Scalar(Pointer)
layout_of(DynReceiver).abi == ScalarPair(Pointer, Pointer)

Oh, I just remembered this. Will add them later, don't let me forget

It turns out this is... a bit complicated. During object safety checks, we don't know the concrete type. I can fudge it with Receiver[Self => ()], which is well-formed for all of the receiver types that we have in std right now, but you can imagine cases where it wouldn't be, if Receiver has bounds on Self. I don't know how bad it is to compute the layout of a type that isn't well formed — maybe it's fine.

The dynamic receiver doesn't have that issue, but it does take a lot of code to create the dyn Trait type, so I guess the question is whether it's worth it just to ICE during object-safety checks instead of during codegen.

mikeyhew added some commits Sep 20, 2018

Add CoerceSized trait and lang item
This trait is more-or-less the reverse of CoerceUnsized, and will be
used for object-safety checks. Receiver types like `Rc` will have to
implement `CoerceSized` so that methods that use `Rc<Self>` as the
receiver will be considered object-safe.
Implement the object-safety checks for arbitrary_self_types: part 1
For a trait method to be considered object-safe, the receiver type must
satisfy certain properties: first, we need to be able to get the vtable
to so we can look up the method, and second, we need to convert the
receiver from the version where `Self=dyn Trait`, to the version where
`Self=T`, `T` being some unknown, `Sized` type that implements `Trait`.

To check that the receiver satisfies those properties, we use the
following query:

forall (U) {
if (Self: Unsize<U>) {
Receiver[Self => U]: CoerceSized<Receiver>
}
}

where `Receiver` is the receiver type of the method (e.g. `Rc<Self>`),
and `Receiver[Self => U]` is the receiver type where `Self = U`, e.g.
`Rc<U>`.

forall queries like this aren’t implemented in the trait system yet, so
for now we are using a bit of a hack — see the code for explanation.
Implement object-safety for arbitrary_self_types: part 2
For now, all of the receivers that we care about are just a newtyped
pointer — i.e. `Box<Self>`, `Rc<Self>`, `Pin<Box<Self>>`, `Pin<&mut
Self>`. This is much simpler to implement in codeine than the more
general case, because the ABI is the same as a pointer. So we add some
checks in typeck/coherence/builtin.rs to make sure that implementors of
CoerceSized are just newtyped pointers. In this commit, we also
implement the codegen bits.
Add CoerceSized impls throughout libstd
This will make receiver types like `Rc<Self>` and `Pin<&mut Self>`
object-safe.
update tests that have changed output
 I’m not sure why these tests have different output now, but they do.
In all cases, the error message that is missing looks like this: “the
trait bound `dyn Trait: Trait` is not satisfied”

My guess is that the error message is going away because object-safety
now involves trait solving, and these extra error messages are no
longer leaking out.
fix docs on trait
tests were failing because I didn't wrap code snippets like  in backticks. fixed that now, so hopefully tests will pass on travis
Replace CoerceSized trait with DispatchFromDyn
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.
Add layout sanity checks in object safety
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
Put backticks around field names, types and paths in error messages
Added to `DispatchFromDyn` and `CoerceUnsized` error messages
add `U: Trait` to the param env during DispatchFromDyn check
also updated the doc on `receiver_is_dispatchable` to reflect current state of the implementation
Remove this check for object-safety during selection of trait object …
…candidates

I don't really understand what it's for, but see the comment here:
#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
Add a check for reprs that could change the ABI
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
@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Nov 1, 2018

@kennytm

Could you update the PR description e.g. s/CoerceSized/DispatchFromDyn/g?

Done.

@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Nov 1, 2018

This is ready to go.

@mikeyhew

This comment has been minimized.

Contributor

mikeyhew commented Nov 2, 2018

@nikomatsakis can you r+ this?

@cramertj

This comment has been minimized.

Member

cramertj commented Nov 3, 2018

@bors r=nikomatsakis

(based on the approval below this comment)

@bors

This comment has been minimized.

Contributor

bors commented Nov 3, 2018

📌 Commit 192e7c4 has been approved by nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Nov 3, 2018

⌛️ Testing commit 192e7c4 with merge 3fc70e8...

bors added a commit that referenced this pull request Nov 3, 2018

Auto merge of #54383 - mikeyhew:custom-receivers-object-safety, r=nik…
…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
@bors

This comment has been minimized.

Contributor

bors commented Nov 3, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 3fc70e8 to master...

@bors bors merged commit 192e7c4 into rust-lang:master Nov 3, 2018

2 checks passed

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

@mikeyhew mikeyhew referenced this pull request Nov 3, 2018

Open

Tracking issue for `arbitrary_self_types` #44874

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment