Skip to content
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

Changing the type of self in methods doesn't work #27941

Closed
jonas-schievink opened this issue Aug 22, 2015 · 19 comments
Closed

Changing the type of self in methods doesn't work #27941

jonas-schievink opened this issue Aug 22, 2015 · 19 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@jonas-schievink
Copy link
Contributor

The book states:

Methods take a special first parameter, of which there are three variants: self, &self, and &mut self.

The reference uses typed self in a few places, but doesn't explain the exact rules.

The compiler also prints redundant and misleading diagnostics:

struct S;

impl S {
    fn f(self: Vec<Self>) {}
}

gives:

<anon>:6:10: 6:28 error: mismatched self type: expected `S`: expected struct `S`, found struct `collections::vec::Vec` [E0211]
<anon>:6     fn f(self: Vec<Self>) {}
                  ^~~~~~~~~~~~~~~~~~

Since replacing Vec<Self> with Box<Self> makes the code compile, "expected struct S" isn't quite correct.

@eefriedman
Copy link
Contributor

Explicitly specifying the type of self using the syntax self: Box<Self> is sort of vestigial. The feature of allowing Box<Self> as the type of self, along with the syntax for explicitly writing out the type of self, sort of slipped into 1.0 without anyone really considering the issue. The other syntax is strongly preferred.

The explicit type syntax will probably be deprecated at some point.

@steveklabnik
Copy link
Member

/cc @rust-lang/lang , what do you think? Should we explain this more, or leave it as is?

@eddyb
Copy link
Member

eddyb commented Aug 25, 2015

@eefriedman On the contrary, self: Foo<Self> is part of UFCS, and should be implemented at some point.
I discussed the necessary changes to the way we store inherent impls with @arielb1 some time ago, though I can't recall who was asking in the first place.

One constraint we could put on it is that Foo<Self>: Deref<Target=Self>, which is easy to check and would allow self: Rc<Self> - although, if we want to use this for traits, as well, it would have to be smarter and only allow pointers.

Foo<Self> can probably be tested for object-safe compatibility inside a trait definition by replacing Self with the trait and checking Foo<Trait>: CoerceUnsized<Foo<Self>>.
Only one leaf field can be coerced (and it's a pointer), the others are forced to have the same representation (same type, or PhantomData<T> -> PhantomData<U>).

That implies you could have, e.g. self: Triplet<String, Rc<Self>, bool> in a trait and be object-safe, with virtual calls passing that entire ADT to the virtual method, modulo the vtable pointer (though keeping it doesn't hurt anyone, either, especially if the argument is passed with indirection).

@arielb1
Copy link
Contributor

arielb1 commented Aug 25, 2015

@eddyb

compatibility with the trait? I would prefer to store explicit Self explicitly and use the normal check_method code.

If we want decent performance, requiring the method to appear in the autoderef loop would be nice (so do the autoderef loop in a probe, gather all methods, then sort them by priority and find the first matching one).

@eddyb
Copy link
Member

eddyb commented Aug 25, 2015

@arielb1 Wording was unclear, I edited that bit.
I was talking about a custom self type, like self: Rc<Self> in a method inside a trait definition, that can still be treated as object-safe, given that we can prove it is pointer-like (and can thus be passed to a virtual method).
CoerceUnsize is currently only implemented on pointers and pointers with data around them, so it fits the bill.

@steveklabnik steveklabnik added A-lang and removed A-docs labels Nov 4, 2015
@steveklabnik
Copy link
Member

untagging docs and tagging with lang until @rust-lang/lang tells me what they want here

@steveklabnik
Copy link
Member

Triage: over a year later, nothing has happened with more exotic Selfs. While it's strictly true that Box<Self> will also work, making the diagnostic technically incorrect, this seems very minor. In addition, the book isn't going to cover this until it's made more general.

So: closing this ticket, in my mind, involves fixing the diagnostic to say "or Box".

@steveklabnik steveklabnik added A-diagnostics Area: Messages for errors, warnings, and lints and removed A-lang labels Nov 29, 2016
@nikomatsakis
Copy link
Contributor

(FWIW, I think we really want, at minimum, *mut Self and so forth to work. Not having support for this is quite frustrating in Rayon, at least if I try to be careful about my types in unsafe code.)

@steveklabnik steveklabnik removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 1, 2017
@Mark-Simulacrum
Copy link
Member

Nominating for lang team discussion. I assume this needs an RFC? Or perhaps there already was one, just not yet implemented.

@Mark-Simulacrum Mark-Simulacrum changed the title Typed self is badly documented (and has bad diagnostics) Changing the type of self in methods doesn't work Jun 1, 2017
@nikomatsakis
Copy link
Contributor

I think an RFC would be a good idea, just so we spend some time thinking carefully about the cases. In particular, it's important to work out how this interacts with trait objects.

@nikomatsakis
Copy link
Contributor

Some notes from the @rust-lang/lang meeting:

  • We probably want to support "any type that derefs (transitively) to Self".
  • A good example use case is self: Rc<Self>.
  • Have to work out how to find the vtable for trait objects.
  • This doesn't seem to fall under the roadmap / ergonomic initiative, in terms of prioritization.

If someone is interested in pursuing it, we'd be happy to work with them (feel free to ping me), but it's not the highest priority at the moment.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 22, 2017
@jimmycuadra
Copy link
Contributor

I think this issue has just gained some fire for prioritization with the recent introduction of generators and async/await.

See https://github.com/alexcrichton/futures-await/tree/e09c87c7aa796bf6e5b6d8d45d6cba2078d12210#borrowing for a motivating example for self: Rc<Self>.

@aturon
Copy link
Member

aturon commented Sep 26, 2017

@nikomatsakis @arielb1 Can we consider this for the impl period?

@arielb1
Copy link
Contributor

arielb1 commented Sep 26, 2017

@aturon

I suspect this needs an actual RFC. However, with the newly-fixed method dispatch, I don't expect any obstacles to implementing this.

@arielb1
Copy link
Contributor

arielb1 commented Sep 26, 2017

I mean, for implementing it in method dispatch. The interesting part that actually needs the RFC is the compiler being able to downcast the Rc<Trait> to an Rc<SomeSpecificType>.

With the current ABI, this is basically done in the compiler-generated impl of <Trait as Trait>::method, which means it needs to be "uniform" and not be specific to the specific type.

Maybe have the compiler do a reverse unsized coercion? That's it, we can have this object safety rule:

foo: for<LTS> fn(ε, ...χ) method
χ does not refer to Self
(used to be `ε` is of the form &Self, &mut Self, Box<Self>)
Γ, Self: Sized ⊢ for<LTS> ε: CoerceUnsized<ε[Self ← dyn Self<Assoc1=Self::Assoc1,...>]>
    \-- this is in the trait param env, where we know that ε: Trait
------------
Γ ⊢ foo object-safe

Actually, I'm not sure that would work, because you might have the unsized coercion go through a different Unsize impl, say using an associated type (e.g. the example from #44861). We ICE on that (in trans), so maybe there's good room to lock things up somewhat. Or maybe just add an additional trait:

pub trait CoerceSized<Target> {
    type SizeSource: ?Sized;
    type SizeTarget: Unsize<Self::SizeSource>;
}

and have the bound be

Γ, Self: Sized ⊢ for<LTS> {
  ε: CoerceSized<
    ε[Self ← dyn Trait<Assoc1=Self::Assoc1,...>], 
    SizeSource=dyn Trait<Assoc1=Self::Assoc1,...>, 
    SizeTarget=Self
  >
}

I believe we can then implement <Trait as Trait>::method "as follows":

(sized_abstract, vtable) = coerce_sized self
fn_ptr = vtable[INDEX]
ret = fn_ptr(sized_abstract, ..χ)

Or maybe there's another solution I'm missing.

@aturon
Copy link
Member

aturon commented Oct 19, 2017

Lang team meeting: an RFC is needed prior to stabilization, but we're happy to accept experimentatal implementation in the meantime.

bors added a commit that referenced this issue Nov 12, 2017
Implement arbitrary_self_types

r? @arielb1
cc @nikomatsakis

Partial implementation of #44874.  Supports trait and struct methods with arbitrary self types, as long as the type derefs (transitively) to `Self`. Doesn't support raw-pointer `self` yet.

Methods with non-standard self types (i.e. anything other than `&self, &mut self, and Box<Self>`) are not object safe, because dynamic dispatch hasn't been implemented for them yet.

I believe this is also a (partial) fix for #27941.
@kevincox
Copy link
Contributor

kevincox commented Mar 7, 2018

@arielb1 Is there any progress on an object-safe implementation of this?

(sized_abstract, vtable) = coerce_sized self
fn_ptr = vtable[INDEX]
ret = fn_ptr(sized_abstract, ..χ)

I'm not sure I fully understand the syntax here but it seems like you also want to pass self to fn_ptr. This way the function can access the full self (the containing Rc as well as the contained value). I'm also not sure why the CoerceSized trait is required to be honest. I would imagine something like this:

trait MyTrait {
    fn foo(self: &Rc<Self>, arg: String);
}

struct Thing;

impl MyTrait for Thing {
    fn foo(self: &Rc<Self>, arg: String) {
        unimplemented!()
    }
}

Would create a vtable with Fn(&Rc<MyTrait>, &Thing, String). The &Self parameter is required because the user will want access to Rc<MyThing> as if it was a Rc<Self> but that relies on Rc returning the same type as it did last time you called .deref() which I don't think can be guaranteed. Passing a &Self parameter gives the user a reference with the correct type but now the signature in the trait and impl look different which is likely confusing.

So a "correct" impl signature would look something like this, but it is kinda ugly.

impl MyTrait for Thing {
    fn foo(self: &Rc<MyTrait>, inner_self: &Self, arg: String) {
        unimplemented!()
    }
}

Also I have been really sloppy about throwing & around for the types, I guess just adding a &Self parameter and leaving the "actual" self parameter should work if it is self: &Foo<Self> but it won't be as easy if it is a value or &mut parameter as you won't be allowed to have that aliasing.

@arielb1
Copy link
Contributor

arielb1 commented Mar 12, 2018

@kevincox

Not really. Someone needs to work on that.

@Centril
Copy link
Contributor

Centril commented Feb 18, 2019

The diagnostic seems improved so I'm closing in favor of #44874.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests