Skip to content

Update object safety to match impl around self as receiver #1248

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update object safety to match impl around self as receiver #1248

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
805ac05
Update object safety to match impl around self as receiver
upsuper Aug 13, 2022
ac40b15
tweak
upsuper Aug 13, 2022
3408890
remove reference to unstable feature
upsuper Aug 14, 2022
bc9a036
fix another mention of unstable
upsuper Aug 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/items/traits.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,22 +75,25 @@ Object safe traits can be the base trait of a [trait object]. A trait is
* Not have any type parameters (although lifetime parameters are allowed),
* Be a [method] that does not use `Self` except in the type of the receiver.
* Have a receiver with one of the following types:
* `Self` (i.e. `self`)
* However, it is currently not possible to dispatch it on a trait object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@upsuper upsuper Aug 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@upsuper upsuper Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@upsuper upsuper Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@upsuper upsuper Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* `&Self` (i.e. `&self`)
* `&mut Self` (i.e `&mut self`)
* [`Box<Self>`]
* [`Rc<Self>`]
* [`Arc<Self>`]
* [`Pin<P>`] where `P` is one of the types above
* Does not have a `where Self: Sized` bound (receiver type of `Self` (i.e. `self`) implies this).
* Does not have a `where Self: Sized` bound
* Explicitly non-dispatchable functions require:
* Have a `where Self: Sized` bound (receiver type of `Self` (i.e. `self`) implies this).
* Have a `where Self: Sized` bound

```rust
# use std::rc::Rc;
# use std::sync::Arc;
# use std::pin::Pin;
// Examples of object safe methods.
trait TraitMethods {
fn by_value(self: Self);
fn by_ref(self: &Self) {}
fn by_ref_mut(self: &mut Self) {}
fn by_box(self: Box<Self>) {}
Expand All @@ -101,7 +104,9 @@ trait TraitMethods {
fn nested_pin(self: Pin<Arc<Self>>) {}
}
# struct S;
# impl TraitMethods for S {}
# impl TraitMethods for S {
# fn by_value(self) {}
# }
# let t: Box<dyn TraitMethods> = Box::new(S);
```

Expand All @@ -110,6 +115,8 @@ trait TraitMethods {
trait NonDispatchable {
// Non-methods cannot be dispatched.
fn foo() where Self: Sized {}
// Self as receiver can't be dispatched on a trait object.
fn by_value(self);
// Self type isn't known until runtime.
fn returns(&self) -> Self where Self: Sized;
// `other` may be a different concrete type of the receiver.
Expand All @@ -120,12 +127,14 @@ trait NonDispatchable {

struct S;
impl NonDispatchable for S {
fn by_value(self) {}
fn returns(&self) -> Self where Self: Sized { S }
}
let obj: Box<dyn NonDispatchable> = Box::new(S);
obj.returns(); // ERROR: cannot call with Self return
obj.param(S); // ERROR: cannot call with Self parameter
obj.typed(1); // ERROR: cannot call with generic type
obj.by_value(); // ERROR: cannot call with Self as receiver
obj.returns(); // ERROR: cannot call with Self return
obj.param(S); // ERROR: cannot call with Self parameter
obj.typed(1); // ERROR: cannot call with generic type
```

```rust,compile_fail
Expand Down