DispatchFromDyn: require additional checks

[dingxiangfei2009]

The new checks are we check the pair of constituent types for same shapes structurally, and demand capability for dispatch for ADTs as usual; and we demand source generic parameter to be unsized of another for sanity.

Fix #148727

cc @theemathas

[rustbot]

r? @WaffleLapkin

rustbot has assigned @WaffleLapkin.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[dingxiangfei2009]

@theemathas What do you think? Can we break DispatchFromDyn further?

[theemathas]

The documentation for DispatchFromDyn also needs to be changed.

[dingxiangfei2009]

which is not possible

I am not sure. We have an option to make it possible. The question is, would it be a bad idea?

In any case, let me add a stop-gap to disallow this case.

[jackh726]

r? types

[dingxiangfei2009]

Discussion under t-lang: #t-lang > Should we support dyn dispatch on `&Adt<_>`?

I think we are quite convinced that &Adt<_> as receiver is not dyn-compatible.

[Darksonn]

Mentioning references here seems awfully specific. What about Box<Ptr<U>> or similar?

[dingxiangfei2009]

I am in the middle of updating the documentation on this trait.

Box<Ptr<U>> falls in the second case of the new documentation. We would foremost require Ptr<T>: DispatchFromDyn<Ptr<U>> among the other requirements.

[theemathas]

Why does this text not show up in the test error output?

[dingxiangfei2009]

I forgot about a #[note] attribute on the error type. Now it should work.

[theemathas]

This code doesn't compile with this PR. Should it?

#![feature(dispatch_from_dyn, arbitrary_self_types, unsize)]

use std::marker::{PhantomData, Unsize};
use std::ops::{DispatchFromDyn, Receiver};

pub struct Inner<T: ?Sized>(T);
#[repr(transparent)]
pub struct Outer<T: ?Sized>(PhantomData<T>, Inner<T>);

fn wrap<T: ?Sized>(x: &Inner<T>) -> &Outer<T> {
    // SAFETY: We're using repr(transparent)
    unsafe { &*(x as *const Inner<T> as *const Outer<T>) }
}

impl<'a, T: Unsize<U> + ?Sized, U: ?Sized> DispatchFromDyn<&'a Outer<U>> for &'a Outer<T> {}
impl<T: ?Sized> Receiver for Outer<T> {
    type Target = T;
}

pub trait Trait {
    fn method(self: &Outer<Self>);
}

struct Thing(i32);
impl Trait for Thing {
    fn method(self: &Outer<Self>) {
        println!("{}", self.1.0.0);
    }
}

fn main() {
    let inner: &Inner<dyn Trait> = &Inner(Thing(1));
    let x: &Outer<dyn Trait> = wrap(inner);
    x.method();
}

error: the trait `DispatchFromDyn` does not allow dispatch through references
  --> src\main.rs:15:1
   |
15 | impl<'a, T: Unsize<U> + ?Sized, U: ?Sized> DispatchFromDyn<&'a Outer<U>> for &'a Outer<T> {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `foo` (bin "foo") due to 1 previous error

[dingxiangfei2009]

To answer the comment I have jotted down the reason that we do not want to compile the example in the trait documentation.

[theemathas]

@dingxiangfei2009 Your comment in the code unfortunately does not answer my latest comment in this PR. My comment does not involve a double pointer indirection.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[dingxiangfei2009]

Quote @theemathas

@dingxiangfei2009 Your comment in the code unfortunately does not answer my latest comment in this PR. My comment does not involve a double pointer indirection.

I don't think my text articulates my thoughts right. &Outer<T> -> &Outer<U> is not a double pointer indirection, true, and the point is that there are too many "indirection"s.

Maybe I should reword it. The pre-requisite of a valid DispatchFromDyn relation from a type A to a type B is that

• A and B are of the same shape, sharing the same concrete type constructor, excluding aliases and generics
• A and B are both generic over two distinct type parameter T and U respectively
• T: Unsize<U>
• Most importantly, the "downcast" of a value of B back to A is supported. Here we can give an exhaustive list of supported "downcasts" and we can add more as supported types grow.
  • A := &T from B := &U, or A := &mut T from B := &mut U
    • This works today by dispatching the call to the corresponding method after transparently discarding the metadata
  • A := Adt<T> from B := Adt<U> when T: DispatchFromDyn<U> and Adt<_> is reasonable
    • This works today by delegating the dispatch to the only field in A after transparently discarding the metadata

The last bullet point will answer the important question...

Should it?

... which is no, because we don't know how to "downcast" &Outer<U> back to &Outer<T> because that metadata is actually behind one layer of "indirection." The argument against &mut Outer<U> back to &mut Outer<T> case should run like this. The "downcast" mutation would need to leave the address unchanged and, in turn, necessitate mutation of the data behind this &mut borrow. The transformation is no longer transparent and actually would change type of the place behind that borrow. I don't see a way to support it in Rust.

The A := &T case, on the other hand, has no indirection because the metadata is directly attached to the &U pointer. So this "downcast" is simple for us to implement.

I hope that this would be a better answer to the earlier question. I am not sure how much this content could go into the documentation. Please advise.

[Darksonn]

This impl should be illegal:

impl<'a, T: Unsize<U> + ?Sized, U: ?Sized> DispatchFromDyn<&'a Outer<U>> for &'a Outer<T> {}

It compiles because it falls into the

EITHER Self and T are either both references or both raw pointers; in either case, with the same mutability.

case in the built-in check. But the reference / raw pointer types are supposed to only have the impls found in the stdlib. If they can have more, there's going to be all sorts of problems. We need to reject all downstream impls of DispatchFromDyn for references and raw pointers.

Probably DispatchFromDyn should be "anti-fundamental" so that impl DispatchFromDyn<_> for Fundamental<LocalType> is rejected in general, which would reject the above impl. It's similar to what has been discussed elsewhere as being needed for DerefMut in #145608 and CoerceUnsize in #148899. cc @lcnr