Arbitrary self types v2: avoid dyn, generic ICE

[adetaylor]

Bug #57276 (and several duplicates) are an ICE which occurs when a generic type is used as a receiver which implements both Deref and DispatchFromDyn.

This change proposes to detect this error condition and turn it into a regular error rather than an ICE. Future changes could relax things here. But meanwhile, as this same code path currently produces an ICE, this seems to be strictly better, and it seems defensible to inform the user that their excessively generic type is not dyn-safe.

This is somewhat related to the stabilization of arbitrary self types in PR #135881, tracked in #44874.

Even if we don't want to simply go ahead and make a legitimate error in this case, I'm hoping this PR will provoke discussion of a better solution. We could repeat similar layout checks in receiver_is_dispatchable but I'm not sure why that would be better.

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
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

[adetaylor]

r? @wesleywiser

[compiler-errors]

I don't think this is the right solution.

The DispatchFromDyn trait should be marked with #[rustc_do_not_implement_via_object], since dyn trait object types do not "inherit" DispatchFromDyn just because they're unsized from a type that is DispatchFromDyn. This attribute is used for special "structural" traits such as Unsize and Sized, which act differently from other traits in that they aren't really implied via a dyn impl.

[adetaylor]

@compiler-errors Do you literally mean adding that attribute to the trait, like this?

diff --git a/library/core/src/ops/unsize.rs b/library/core/src/ops/unsize.rs
index d2a07197f6f..e7a4b7905bd 100644
--- a/library/core/src/ops/unsize.rs
+++ b/library/core/src/ops/unsize.rs
@@ -116,6 +116,7 @@ impl<T: ?Sized + Unsize<U>, U: ?Sized> CoerceUnsized<*const U> for *const T {}
 /// [^1]: Formerly known as *object safety*.
 #[unstable(feature = "dispatch_from_dyn", issue = "none")]
 #[lang = "dispatch_from_dyn"]
+#[rustc_do_not_implement_via_object]
 pub trait DispatchFromDyn<T> {
     // Empty.
 }

Unfortunately that doesn't solve the ICE.

[compiler-errors]

Hm, right. I understand the nature of the ICE better now.

Still, the error recovery here seems a bit dissatisfying... Especially because T: DispatchFromDyn<T> is (almost¹) never true, so it feels like these situations are trivially unreachable. Emitting a "layout is too generic" error feels a bit too raw, and even using layout computation machinery at all during method receiver checks, feels like it's exposing some internal machinery that users shouldn't need to be concerned with.

Footnotes

1. dyn Trait: Unsize<dyn Trait> for an arbitrary Trait, but that's not useful or relevant here. ↩

[adetaylor]

Emitting a "layout is too generic" error feels a bit too raw

I agree, but I'll also openly admit that I don't understand all this stuff (remotely) well enough to do any better.

, and even using layout computation machinery at all during method receiver checks, feels like it's exposing some internal machinery that users shouldn't need to be concerned with.

I'd push back gently on that. The docs for DispatchFromDyn say:

the conversion is hard-wired into the compiler.

I think as a user of DispatchFromDyn that means I've already decided to deal with internal machinery, and getting layout errors would not be surprising to me.

Maybe another way to approach this would be to try to specify the extra bullet point to add to those docs.

[compiler-errors]

Here's another ICE that isn't fixed by this PR afaict:

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

use std::ops::Deref;
use std::ops::DispatchFromDyn;

trait Trait<T: Deref<Target = Self>>
where
    for<'a> &'a T: DispatchFromDyn<&'a T>,
{
    fn foo(self: &T) -> Box<dyn Trait<T>>;
}

error: internal compiler error: receiver &ReLateParam(DefId(0:8 ~ test[0374]::Trait::foo), BrNamed(DefId(0:10 ~ test[0374]::Trait::foo::'_), '_)) T/#1 when `Self = (dyn Trait<T> + 'static)` should have a ScalarPair ABI; found Ok(Scalar(Initialized { value: Pointer(AddressSpace(0)), valid_range: 1..=18446744073709551615 }))
  --> /home/mgx/test.rs:11:5
   |
11 |     fn foo(self: &T) -> Box<dyn Trait<T>>;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: delayed at compiler/rustc_trait_selection/src/traits/dyn_compatibility.rs:555:23 - disabled backtrace
  --> /home/mgx/test.rs:11:5
   |
11 |     fn foo(self: &T) -> Box<dyn Trait<T>>;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I think in this case, generalizing this fix to say something like "the layout is not ScalarPair" is even worse than saying "the layout is too generic".

I feel like philosophically, the underlying problem here is that all of the work that we go through with substituting Self in the receiver checks (e.g. to ensure that when we're calling the Self via a wide pointer when Self is substituted with dyn Trait) is going to be moot if we hide Self behind a parameter that generically derefs into something that is valid. Notice how that for<'a> &'a T: DispatchFromDyn<&'a T> where clause that the current object-safety receiver validation code requires doesn't really make a lot of sense...

Now, I'm not totally sure how or if it's possible to reformulate this check to be less weird -- as a side-note, I'm kinda surprised that the trait is dyn compatible at all. Given a trait of trait Trait<T: Deref<Target = Self>> {}, unsizing some concrete type into dyn Trait<...> will always make that T: Deref<Target = Self> bound unsatisfied since Self is now a totally different (dyn) type.

I think as a user of DispatchFromDyn that means I've already decided to deal with internal machinery, and getting layout errors would not be surprising to me.

I'm gonna go out on a limb and say that maybe you're more experienced than a random user with this, so of course you're gonna be unsurprised by a layout error.

On the contrary, I feel like the whole reason we're using traits like DispatchFromDyn to expose and enforce these receiver requirements is so that we can do this generically and in the "native language" of typeck which is the type system layer, not with things like layout computation, and the fact that these layout checks start to fail as soon as we get interesting with where clauses suggests that they've reached their limits.

[compiler-errors]

Closing in favor of #136520