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

Allow Self to appear in the where clause of trait impls #1647

Merged
merged 2 commits into from
Jan 6, 2017

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Jun 13, 2016

@durka
Copy link
Contributor

durka commented Jun 13, 2016

Slightly related: #1588

This seems like a good idea. The only backcompat thing I could come up with is this, and it already doesn't compile, so no issue:

trait Foo {
    fn foo(&self);
}

trait Bar {}

struct Baz;
struct Quux;

impl Foo for Baz {
    fn foo(&self) {
        impl Bar for Quux where Self: Sized {}
            // before this RFC: erroneous attempt to use outer `Self` (i.e. `Baz`)
            // after this RFC: `Self` here refers to `Quux`
    }
}

fn main() {}

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jun 14, 2016
@Ericson2314
Copy link
Contributor

Ericson2314 commented Jun 14, 2016

IIUC, the associated type thing is weird in that <Self as Trait>::Assoc: Bound will sometimes fail, but then <Self as Trait>::Assoc isn't bound to begin with.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 14, 2016

Not really any different than having T: Trait, <T as Trait>::Assoc: Bound where T: Trait doesn't hold.

@Ericson2314
Copy link
Contributor

Say a different impl with a different associated type succeeds?

@Ericson2314
Copy link
Contributor

If <SelfWrittenOut as Trait>::Assoc: Bound is already allowed, then nevermind.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 17, 2016

<ExplicitSelfType as Trait>::Assoc: Bound is already allowed. I could probably sum this up more easily in that the goal is to have Self in impl Trait for T where Self: IsUsedHere to mean the same thing as fn method_in_trait_body() where Self: IsUsedHere in effectively every way

}

impl<T, U, V> MyTrait for SomeStruct<T, U, V> where
SomeOtherStruct<T, U, V>: SomeBound,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you could change this from SomeOtherStruct<T, U, V> to <Self as MyTrait>::MyType, that would make clear the current verbosity is not quite as bad :).

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jun 18, 2016

@sgrif ah, sorry for the noise then. I think the goal is clear enough, but I commented on one line you might consider changing then to reflect the status-quo as you just clarified it to be.


All that straightened out, I am definitely for this!

@aturon aturon self-assigned this Jun 23, 2016
@nikomatsakis
Copy link
Contributor

In general we already allow Self inside impls, iirc, so I am a bit surprised that it doesn't work in where clauses. Seems like an oversight or bug to me, to be honest.

When it comes to associated types, though, I am a bit dubious about this paragraph:

Accessing associated types will have the same result as copying the body of the associated type into the place where it's being used. That is to say that it will assume that all constraints hold, and evaluate to what the type would have been in that case. Ideally one should never have to write ::SomeType, but in practice it will likely be required to remove issues with recursive evaluation.

This seems to suggest some kind of distinct pathway for handling associated types in this location. I am not too keen on that, I would prefer for this to fall out from normalization (and indeed I think it very well might, though there may be some issues with resolving recursive references -- we could probably do better in this respect than we do in general).

@withoutboats
Copy link
Contributor

@rfcbot fcp merge

(I hope I'm doing this right!)

I agree with @nikomatsakis that this seems like a bug. The 'type header' of an impl block (its where clauses, etc) should be considered a part of that impl block, and the Self type should be understood there as the receiving type.

@withoutboats
Copy link
Contributor

withoutboats commented Nov 2, 2016

@rfcbot concern "Include type parameters"

I think this should also be accepted:

trait Foo<T> { }

struct Bar;

impl Foo<Self> for Bar { }
// equivalent to:
impl Foo<Bar> for Bar { }

And this:

trait Foo { }

struct Bar<T>(Box<T>);

impl Foo for Bar<Self> { }
// equivalent to
impl Foo for Bar<Bar> { }

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 2, 2016

EDIT: @withoutboats was faster at typing, but I'll still write this.

Technically, Self can be used in the "whole impl" except for the impl type itself, this includes the trait and bounds on impl parameters:

trait Tr<T> {}

impl<T: Tr<Self>> Tr<Self> for u8 {}

The only question is "How confusing this may be?", especially if the impl lives in another impl with its own Self.
The impl syntax is arbitrary, if it looked like impl<T> u8 as Trait<Self> where T: Tr<Self>, probably it would look more intuitive, because Self is used after the perceived point of "type introduction".

@petrochenkov
Copy link
Contributor

@withoutboats

impl Foo for Bar<Self> { }
// equivalent to
impl Foo for Bar<Bar<Self>> { }
// equivalent to
impl Foo for Bar<Bar<Bar<Self>>> { }
// equivalent to
impl Foo for Bar<Bar<Bar<Bar<Self>>>> { }
// equivalent to
impl Foo for Bar<Bar<Bar<Bar<Bar<Self>>>>> { }
...

@withoutboats
Copy link
Contributor

withoutboats commented Nov 2, 2016

@petrochenkov of course 😅 it should fail with some recursive type definition error, and not a "use of Self outside of an impl or trait" error

@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 2, 2016

fail with some recursive type definition error, and not a "use of Self outside of an impl or trait" error

That... sounds very reasonable.
EDIT: I only hope impl Self {} won't create a black hole.

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 2, 2016

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 4, 2016

Wait, I am confused. The RFC concerns using Self in where-clauses, right? Do we intend to permit impl Trait<Self> for u8? (Do we already?)

@withoutboats
Copy link
Contributor

@nikomatsakis I would like to see the RFC amended to support that.

@petrochenkov
Copy link
Contributor

For reference, trait items, unlike impl items, already permit Self everywhere and not only inside of the block.
This, for example, compiles:

trait Tr<T: Tr<Self> = Self> where Self: Copy {}

@nikomatsakis
Copy link
Contributor

@withoutboats huh, really? Interesting. I guess it seems intuitive enough what it means, even if I find it a bit surprising. Presumably Self would also be "in scope" for bounds, right?

e.g.,

impl<T: Foo<Self>> Bar for u8 

@withoutboats
Copy link
Contributor

withoutboats commented Nov 8, 2016

@nikomatsakis Yea. I agree that it seems weird to actually do, but we have a history of Self not always working for no particular reason, which IMO teaches users that Rust has complicated and arbitrary rules. :-\ I'd like to see the system be consistent between impls and traits - in any type position in any impl or trait, Self is the receiving type.

@nikomatsakis
Copy link
Contributor

@withoutboats yeah, seems ok. I can get behind it.

@withoutboats
Copy link
Contributor

@rfcbot resolved "Include type parameters"

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 10, 2016

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Dec 10, 2016
@briansmith
Copy link

briansmith commented Dec 30, 2016

@sgrif Could you please write a patch to the Rust Reference for this change? Perhaps it would be an empty patch since the Rust Reference already says “All traits define an implicit type parameter Self that refers to ʻthe type that is implementing this interface,ʻ" which supports the conclusion above that this is fixing a bug.

@strega-nil
Copy link

Yay!

hadronized pushed a commit to hadronized/rfcs that referenced this pull request Feb 10, 2017
@Centril Centril added A-syntax Syntax related proposals & ideas A-traits Trait system related proposals & ideas labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-syntax Syntax related proposals & ideas A-traits Trait system related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.