Update object safety to match impl around self as receiver

[upsuper]

This fixes #1247.

[Havvy]

Please remove the part about unstable features.

Also, change the description so that it just says "fixes #1247". The prefix means it won't autoclose the issue when this is merged.

[upsuper]

Also, change the description so that it just says "fixes #1247". The prefix means it won't autoclose the issue when this is merged.

I don't think that's true:

[Havvy]

Missed one unstable reference.

(And I blame Github not showing a visual indicator on that. I remember it showing a dashed underline in the past.)

[Havvy]

The other reference to the unstable feature.

[upsuper]

Oops, fixed in bc9a036

[upsuper]

(And I blame Github not showing a visual indicator on that. I remember it showing a dashed underline in the past.)

It does show a dashed underline for me, and the line disappear when I hover it to show the tooltip.

[Havvy]

This would make it explicitly non-dispatchable currently though, so having a receiver of Self would be explicitly non-dispatchable and placed into the other list, yes? It made sense when the unstable feature comment was here, but without it, it doesn't.

[upsuper]

That was exactly the reason that I referenced the unstable feature.

You can't place it into the other list, because Self as receiver doesn't make the method explicitly non-dispatchable, as that would fail to explain why fn b(self) -> Self; doesn't compile.

There is basically no simple way otherwise to explain the current situation, and I think the code basically said self as receiver is considered object safe: https://github.com/rust-lang/rust/blob/d70c0ecfae6dd9cb00ffae9ad806c47256c9ef15/compiler/rustc_trait_selection/src/traits/object_safety.rs#L456-L460

[upsuper]

Also if you see this example: https://godbolt.org/z/TPz4WEWh5

It is pretty clear that fn a(self); is treated as dispatchable, and it has an item in the vtable.

For future reference the code and output is the following:

#![feature(ptr_metadata, bench_black_box)]

use std::hint::black_box;
use std::ptr::DynMetadata;

trait Foo {
    fn a(self);
    fn b(&self) {}
}

struct S;
impl Foo for S {
    fn a(self) { black_box(self); }
}

pub fn x() -> DynMetadata<dyn Foo> {
    let a: Box<dyn Foo> = Box::new(S);
    (a.as_ref() as *const dyn Foo).to_raw_parts().1
}

core::ptr::drop_in_place<example::S>:
        ret

example::Foo::a{{vtable.shim}}:
        ret

example::Foo::b:
        ret

<example::S as example::Foo>::a:
        ret

example::x:
        lea     rax, [rip + .L__unnamed_1]
        ret

.L__unnamed_1:
        .quad   core::ptr::drop_in_place<example::S>
        .asciz  "\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
        .quad   example::Foo::a{{vtable.shim}}
        .quad   example::Foo::b

[Havvy]

Alright. This is too weird for me to reason about at 2am. I'll think about it tomorrow unless ehuss wants to step in.

[ehuss]

Just to follow up on this: I also don't think it looks correct to include this in this list of dispatchable functions. And I believe that there is an implied self: Sized when implementing, which is already covered in the points below.

I think it would be good to clear up the parentheticals, though, as they seemed a little confusing to me. Perhaps remove them and place the elaboration inside a note block? Maybe something like:

Note: Because receivers of type Self imply the Self: Sized bound, those methods are object-safe, but non-dispatchable.

[upsuper]

And I believe that there is an implied self: Sized when implementing, which is already covered in the points below.

All I'm arguing here is that this statement doesn't match the current behavior and can't explain the examples in the issue linked.

[ehuss]

Ah, I see. Sorry, trying to catch up. This is a part of the compiler I am unfamiliar with.

So it looks like there are more complex rules that are not covered here. It looks like Self can be a receiver (for a non-dispatchable method). But there are several other restrictions that would cover what was raised in the issue:

• Self cannot be used in any parameter
• Self cannot be used anywhere in a return type
• The where clause cannot reference Self (currently a future-incompatibility lint). Although this strangely allows Copy and Clone. That needs more investigation. contains_illegal_self_type_reference has a long description.

I think removing the parentheticals, and trying to incorporate the additional restrictions might be the way to go. For the future-incompat thing, I would lean towards just documenting the intended behavior, with a footnote pointing to the tracking issue.

[upsuper]

I think removing the parentheticals, and trying to incorporate the additional restrictions might be the way to go.

That would be much more complicated to document than simply saying it's dispatchable but you just can't call it through trait object right now. You would efficiently need to duplicate all the rules again.

It's especially the case that vtable actually includes entry for such method indicating the compiler does consider it dispatchable.

[upsuper]

For the future-incompat thing, I would lean towards just documenting the intended behavior, with a footnote pointing to the tracking issue.

I believe the intented behavior is that it's dispatchable but just not fully supported yet.

[upsuper]

@ehuss @Havvy Any other thought?

[traviscross]

We reviewed this PR in the rustdocs call, and we've asked for review of this by the types team in:

• Review of Reference change regarding receivers and object safety rust#127251

[rustbot]

☔ The latest upstream changes (possibly bf115a4) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

There is a PR at #1455 where we may be updating this instead.