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

librustc: Implement associated types behind a feature gate. #16377

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@pcwalton
Copy link
Contributor

pcwalton commented Aug 9, 2014

This is waiting on an RFC, but this basic functionality should be
straightforward. The implementation essentially desugars during type
collection and AST type conversion time into the parameter scheme we
have now.

r? @nikomatsakis

(addendum by pnkfelix: the feature gate's name is associated_types)

@brendanzab

This comment has been minimized.

Copy link
Member

brendanzab commented Aug 13, 2014

Awesome!

Would it be that much harder to add associated statics?

@ptal

This comment has been minimized.

Copy link

ptal commented Aug 13, 2014

This is great! I have tried to use it but faced some bugs.

And finally just a remark, it is a bit cumbersome to prefix types that are declared in the trait with the current trait type, but maybe if it has some implications for the type-checking. If not, it could be cool to have the same shortcut for inherited associated types if there is no ambiguity.

All my tries has been done with this version of the compiler. It's not specified in the code snippets but I used #![feature(associated_types)].

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Aug 13, 2014

Yes, associated statics will be a lot of work and this patch is already 5+ KLOC.

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Aug 13, 2014

And finally just a remark, it is a bit cumbersome to prefix types that are declared in the trait with the current trait type, but maybe if it has some implications for the type-checking. If not, it could be cool to have the same shortcut for inherited associated types if there is no ambiguity.

There's no reason for it other than the fact that this is what the compiler already had support for. This patch is already huge so I'd like to implement that in a followup.

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Aug 13, 2014

Internal compiler bug: http://is.gd/xiHkUe

Good catch. That should be an error (because you didn't have a generic type parameter), but instead it ICE'd.

You won't be able to do what you want until both this patch and #16432 land.

@brendanzab

This comment has been minimized.

Copy link
Member

brendanzab commented Aug 15, 2014

I guess associated statics are easier to add to APIs in a backwards compatible way than associated types. This is still awesome nonetheless - I am super excited about using this in gfx-rs and cgmath-rs.

@nmsmith

This comment has been minimized.

Copy link

nmsmith commented Aug 20, 2014

This is awesome, I've been looking forward to associated types. I've also found need for associated statics/constants, so I hope to see that implemented in the future.

@pcwalton pcwalton force-pushed the pcwalton:associated-items branch 3 times, most recently from 987e30b to e797049 Sep 9, 2014

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Sep 11, 2014

Updated with the syntax from the RFC. re-r? @nikomatsakis

@bluss

This comment has been minimized.

Copy link
Contributor

bluss commented Sep 11, 2014

Very exciting stuff. I'm testing the PR at yesterday's version (commit e797049 )

Here are some bug reports already:

Inheriting has a bug:

pub trait Dimension {
   type IndexType;
}

// error: wrong number of type arguments: expected 1, found 0
// the error indicates the Dimension trait
pub trait RemoveAxis : Dimension { .. }

And <X as Trait>::InnerType can't be used in where clauses.

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Sep 11, 2014

I'd like to do the where clause stuff in a followup since this patch is getting pretty big. (Also it may depend on trait reform.)

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Sep 11, 2014

If you forget the associated type in an impl block, you get an ICE. Example:

#![feature(associated_types)]

trait Borrow {
    type Owned;

    fn borrow<'a>(&'a Borrow::Owned) -> &'a Self;
}

impl Borrow for int {
    //type Owned = int;

    fn borrow(_: &int) -> &int {
        unimplemented!();
    }
}

fn main() {}

Output:

ai.rs:9:6: 9:12 error: internal compiler error: ImplCtxt::associated_type_binding(): didn't find associated type
ai.rs:9 impl Borrow for int {

Version: This PR on top of 1dc3195

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 15, 2014

Reviewing latest push now (FYI)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 15, 2014

OK, I more-or-less read through the PR and it all seems to make sense. I'm doing a local build to do some more local testing.

It's clear that the PR implements only a subset of what is described in the RFC (e.g., I don't believe that notation like T::U works, where T is a type; the scoping of associated types in a trait is different; you can't use associated types outside of generic fns; etc.). We should probably carefully enumerate the functionality of the RFC and plan out how we are going to bring it all in (and when!).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 15, 2014

On the ride home, it occurred to me that some of the tests seemed to be in error. For example, I don't think it is intended that one can write (@aturon can confirm):

trait Foo {
    type T;
    fn get() -> Foo::T;
}

The problem is that Foo::T is basically saying "infer the self type" and there is no context with which to infer in a fn signature. Instead one ought to write T, Self::T or (maximally explicit) <Self as Foo>::T.

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Sep 15, 2014

@nikomatsakis Yes, that's right: the RFC does not give any special treatment to the trait name in the body of the trait, so Foo::T is ambiguous there and elsewhere.

That said, the full resolution/matching rules still need to be laid out; you and I should sit down and write that up as an RFC soon.

(Interesting sidenote: under a possible HKT design, you could allow Foo::T as a kind of UFCS-style reference to a type, giving you back a type constructor. Of course that can be added later, if desired.)

@pcwalton pcwalton force-pushed the pcwalton:associated-items branch 3 times, most recently from 60a88a0 to 13c5acb Sep 16, 2014

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Sep 16, 2014

@nikomatsakis Ambiguity (and, relatedly, scoping) rules addressed. re-r?

librustc: Implement associated types behind a feature gate.
The implementation essentially desugars during type collection and AST
type conversion time into the parameter scheme we have now. Only fully
qualified names--e.g. `<T as Foo>::Bar`--are supported.

@pcwalton pcwalton force-pushed the pcwalton:associated-items branch from 13c5acb to 78a8418 Sep 17, 2014

@pcwalton

This comment has been minimized.

Copy link
Owner Author

pcwalton commented on 78a8418 Sep 17, 2014

r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented on 78a8418 Sep 17, 2014

saw approval from nikomatsakis
at pcwalton@78a8418

This comment has been minimized.

Copy link
Contributor

bors replied Sep 17, 2014

merging pcwalton/rust/associated-items = 78a8418 into auto

This comment has been minimized.

Copy link
Contributor

bors replied Sep 17, 2014

pcwalton/rust/associated-items = 78a8418 merged ok, testing candidate = 9508faa

This comment has been minimized.

Copy link
Contributor

bors replied Sep 18, 2014

fast-forwarding master to auto = 9508faa

bors added a commit that referenced this pull request Sep 17, 2014

auto merge of #16377 : pcwalton/rust/associated-items, r=nikomatsakis
This is waiting on an RFC, but this basic functionality should be
straightforward. The implementation essentially desugars during type
collection and AST type conversion time into the parameter scheme we
have now.

r? @nikomatsakis

@bors bors closed this Sep 18, 2014

@bgamari

This comment has been minimized.

Copy link
Contributor

bgamari commented Sep 18, 2014

@pcwalton I guess this doesn't cover bounds on associated types?

@pcwalton pcwalton deleted the pcwalton:associated-items branch Sep 18, 2014

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Sep 18, 2014

@bgamari No, it doesn't. I believe @nikomatsakis is working on generalized where clauses, which should make that more straightforward to implement.

@tedhorst

This comment has been minimized.

Copy link
Contributor

tedhorst commented Sep 18, 2014

Did you mean to revert jemalloc?

@pcwalton

This comment has been minimized.

Copy link
Contributor Author

pcwalton commented Sep 18, 2014

No, I'll fix that.

@alexcrichton alexcrichton referenced this pull request Oct 7, 2014

Closed

Implement associated items #17307

23 of 26 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.