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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃敩 Tracking issue for generic associated types (GAT) #44265

Open
withoutboats opened this Issue Sep 2, 2017 · 30 comments

Comments

@withoutboats
Contributor

withoutboats commented Sep 2, 2017

This is a tracking issue for generic associated types (rust-lang/rfcs#1598)

TODO:

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 20, 2017

Here is a kind of implementation plan I will endeavor to keep updated.

  • Step one: add support into the AST and pretty-printing
    • Probably the first step is to start parsing the new forms and to add support for them to the AST.
    • See this comment for more detailed thoughts here.
    • We should be able to write some parsing-only tests and also test the pretty-printer hir
    • When we get to HIR lowering, we can error out if any GAT are present
    • We can also do the feature gate then
  • More to come

@nikomatsakis nikomatsakis changed the title from Tracking issue for generic associated types to Tracking issue for generic associated types (GAT) Sep 21, 2017

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 21, 2017

Let me start by writing about the AST in more detail. First let's discuss how it works today:

A type Foo: Bar [= Baz]; item in a trait definition is defined by this AST variant. That includes the bounds (Bar) and the (optional) default value Baz. The name is defined in the TraitItem struct.

A type Foo = Bar; item in a trait impl is defined by this AST variant -- that only includes the type Bar, because the Foo etc is defined in the ImplItem struct.

Methods are an interesting case because they can already be made generic. Those generic parameters are declared in the field Generics of the MethodSig structure. This is an instance of the Generics struct.

My take is that the best thing to do would be to "lift" Generics out from methods into the TraitItem (and ImplItem) so that it applies equally to all forms of trait and impl items. For now, we will not support generic constants, I guess, but honestly they probably just fall out from the work we are doing in any case, so they would be a small extension. I think the work will go better if we just plan for them now.

Perhaps a decent first PR would be to just do that change, keeping all other existing functionality the same. That is, we would more Generics into TraitItem (and ImplItem) and out of MethodSig. We would supply an empty Generics for non-methods. We would work through the existing code, plumbing the generics along as needed to make it work.

@sunjay

This comment has been minimized.

Member

sunjay commented Sep 21, 2017

@nikomatsakis Cool! Thank you so much! I started experimenting with this last night and I'm proud to say that I found the same places you pointed to in your comment about the AST. 馃槃 (Given that this was my first time in rustc, I count that as an accomplishment!)

I didn't think to lift generics into TraitItem. My approach was to put Generics into TraitItemKind::Type since that's where the type declaration is stored already. Your approach makes sense too so I'll work on implementing that. Since I am still completely new to this codebase, I'm interested to know what the pitfalls of my approach would be if it were used over the one you suggested. Could you give me some insight into your thought process? 馃槂

Here's the change I would have made:

pub enum TraitItemKind {
    // Generics aren't supported here yet
    Const(P<Ty>, Option<P<Expr>>),
    // `Generics` is already a field in `MethodSig`
    Method(MethodSig, Option<P<Block>>),
    // Added `Generics` here:
    Type(Generics, TyParamBounds, Option<P<Ty>>),
    Macro(Mac),
}

Edit: Answer by nikomatsakis on Gitter

regarding the pitfalls of putting them in type
I think that could work too
the reason that I was reluctant to do that
is that we really want to do the same things (in theory, at least) for methods and types
and -- as I said -- in principle I see no reason we couldn't do the same things for constants
I think if you just move the Generics to the type variant
that would probably work ok, but if you look about, right now we often have to do "one thing for types/consts, one thing for methods" precisely because they are different
so I suspect the code will just get more uniform
I'm not really sure how it will go to be honest =) -- it might be a pain
but a lot of times having things be more generic than they have to be isn't so bad, because you can insert span_bug! calls in the impossible cases for now (and later we come aoround and patch them up)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 26, 2017

All right! Next step is to extend the parser. Here are a few tips. Let's start with trait items.

This routine parses trait items. We want to extend the case that handles associated types to also parse things like type Foo<....> = ...; (also maybe where-clauses). (The <...> is the "generics" that we just added to the AST.)

Currently it is using parse_ty_param, which basically parses something like T: Foo etc. We'll have to stop doing that, because the grammar for associated type declarations no longer matches that for type parameters. So we'll probably want to add something like parse_trait_item_assoc_ty. This can start as a kind of clone of parse_ty_param(), but then we'll want to modify it to invoke parse_generics() right about here. That routine will parse a generics declaration (<...>) if one is present, otherwise it just returns an empty generics. Then we want to add a call to parse where clauses right here -- you can model that on the call that occurs when parsing methods, note that the result is stored into the generics that we parsed earlier.

Once we've done that, we should be able to add some parsing tests. I would do that by making a directory like src/test/run-pass/rfc1598-generic-associated-types/ and adding files that you expect to successfully parse in there. Right now they won't work right, but that doesn't matter. Just add an empty main function. Then we can also add examples that should not parse into src/test/ui/rfc1598-generic-associated-types/ (see COMPILER_TESTS.md for directions on adding UI tests).

Something else -- we need to feature gate this work at this point, to avoid people using this stuff in stable builds. There are some instructions for adding a feature-gate here on forge (see the final section). We should add a visit_trait_item and visit_impl_item to the visitor in feature_gate.rs; if that item is not a method, but it has a non-empty generics, we can invoke gate_feature_post (example).

bors added a commit that referenced this issue Sep 27, 2017

Auto merge of #44766 - sunjay:lift_generics, r=nikomatsakis
Move Generics from MethodSig to TraitItem and ImplItem

As part of `rust-impl-period/WG-compiler-traits`, we want to "lift" `Generics` from `MethodSig` into `TraitItem` and `ImplItem`. This is in preparation for adding associated type generics. (#44265 (comment))

Currently this change is only made in the AST. In the future, it may also impact the HIR. (Still discussing)

To understand this PR, it's probably best to start from the changes to `ast.rs` and then work your way to the other files to understand the far reaching effects of this change.

r? @nikomatsakis

bors added a commit that referenced this issue Oct 23, 2017

Auto merge of #44766 - sunjay:lift_generics, r=nikomatsakis
Move Generics from MethodSig to TraitItem and ImplItem

As part of `rust-impl-period/WG-compiler-traits`, we want to "lift" `Generics` from `MethodSig` into `TraitItem` and `ImplItem`. This is in preparation for adding associated type generics. (#44265 (comment))

Currently this change is only made in the AST. In the future, it may also impact the HIR. (Still discussing)

To understand this PR, it's probably best to start from the changes to `ast.rs` and then work your way to the other files to understand the far reaching effects of this change.

r? @nikomatsakis

bors added a commit that referenced this issue Oct 24, 2017

Auto merge of #44766 - sunjay:lift_generics, r=nikomatsakis
Move Generics from MethodSig to TraitItem and ImplItem

As part of `rust-impl-period/WG-compiler-traits`, we want to "lift" `Generics` from `MethodSig` into `TraitItem` and `ImplItem`. This is in preparation for adding associated type generics. (#44265 (comment))

Currently this change is only made in the AST. In the future, it may also impact the HIR. (Still discussing)

To understand this PR, it's probably best to start from the changes to `ast.rs` and then work your way to the other files to understand the far reaching effects of this change.

r? @nikomatsakis
@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 14, 2017

To setup name resolution, I think all we have to do is to get the proper "ribs" in place (the name resolution stuff organizes the sets of names that are in scope into ribs; each rib represents one binding level). e.g. for an impl:

impl<A,B> Foo<B> for Vec<A> {
   fn bar<T,U>(x: ...) { 
       for y in ... {
       }
   }
}

we would have the following ribs:

- <A,B> (from the impl)
   - <T,U> (from the `bar` method's generics)
      - `x` (from the parameter list)
          - `y` (from the let)

In general, modeling things on how methods work is not a bad idea. We might also do a bit of "future proofing" here, I suppose.

Here is the code that brings a method's type parameters into scope (this is for a method defined in a trait):

let type_parameters =
HasTypeParameters(&trait_item.generics,
MethodRibKind(!sig.decl.has_self()));

Whereas for a type defined in a trait, we are hardcoded to add an empty type parameter rib (NoTypeParameters):

TraitItemKind::Type(..) => {
this.with_type_parameter_rib(NoTypeParameters, |this| {
visit::walk_trait_item(this, trait_item)
});
}

Now that generics are in place on every trait/impl item, I think we probably want to remove the handling for type and extract the method handling so that it occurs at a higher level. For the items (e.g., const) where there are no generics, then the newly introduced rib ought to be empty and hence harmless (I hope).

Other points of interest:

You get the idea.

@petrochenkov -- sound about right?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 18, 2017

@nikomatsakis

sound about right?

Everything looks correct.

bors added a commit that referenced this issue Dec 2, 2017

Auto merge of #45904 - sunjay:gat-parser, r=nikomatsakis
Generic Associated Types Parsing & Name Resolution

Hi!
This PR adds parsing for generic associated types! 馃帀 馃帀 馃帀

Tracking Issue: #44265

## Notes For Reviewers
* [x] I still need to add the stdout and stderr files to my ui tests. It takes me a *long* time to compile the compiler locally, so I'm going to add this as soon as possible in the next day or so.
* [ ] My current ui tests aren't very good or very thorough. I'm reusing the `parse_generics` and `parse_where_clause` methods from elsewhere in the parser, so my changes work without being particularly complex. I'm not sure if I should duplicate all of the generics test cases for generic associated types. It might actually be appropriate to duplicate everything here, since we don't want to rely on an implementation detail in case it changes in the future. If you think so too, I'll adapt all of the generics test cases into the generic associated types test cases.
* [ ] There is still more work required to make the run-pass tests pass here. In particular, we need to make the following errors disappear:
```
error[E0110]: lifetime parameters are not allowed on this type
  --> ./src/test/run-pass/rfc1598-generic-associated-types/streaming_iterator.rs:23:41
   |
23 |     bar: <T as StreamingIterator>::Item<'static>,
   |                                         ^^^^^^^ lifetime parameter not allowed on this type
```
```
error[E0261]: use of undeclared lifetime name `'a`
  --> ./src/test/run-pass/rfc1598-generic-associated-types/iterable.rs:15:47
   |
15 |     type Iter<'a>: Iterator<Item = Self::Item<'a>>;
   |                                               ^^ undeclared lifetime
```
There is a FIXME comment in streaming_iterator. If you uncomment that line, you get the following:
```
error: expected one of `!`, `+`, `,`, `::`, or `>`, found `=`
  --> ./src/test/run-pass/rfc1598-generic-associated-types/streaming_iterator.rs:29:45
   |
29 | fn foo<T: for<'a> StreamingIterator<Item<'a>=&'a [i32]>>(iter: T) { /* ... */ }
   |                                             ^ expected one of `!`, `+`, `,`, `::`, or `>` here
```

r? @nikomatsakis
@rickyhan

This comment has been minimized.

rickyhan commented Mar 23, 2018

Is anyone still working on this? It seems to me that this has a long way to go. GAT is my most desired RFC. If not, I'd love to contribute some tests...

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Mar 23, 2018

@rickyhan so @Centril and @gavento were talking on the WG-traits gitter about divvying up the test work, but I don't know if any progress has been made. Maybe they can chime in. I think a PR would be welcome. =)

@sinkuu sinkuu referenced this issue Apr 21, 2018

Closed

ICE with GATs #50115

bors added a commit that referenced this issue Apr 23, 2018

Auto merge of #49423 - gavento:gavento-dev, r=nikomatsakis
Extend tests for RFC1598 (GAT)

More GAT tests, namely some usage for `Iterable` and `StreamingIterator`, shadowing (lifetimes and type params), `Collection<T>` and `CollectionFamily` from [the series](http://smallcultfollowing.com/babysteps/blog/2016/11/03/associated-type-constructors-part-2-family-traits/) with default associated types. Tracking issue: #44265

r? @nikomatsakis

Wrong GAT argument numbers / kinds and default values are next.

alexcrichton added a commit to alexcrichton/rust that referenced this issue May 10, 2018

Rollup merge of #49423 - gavento:gavento-dev, r=nikomatsakis
Extend tests for RFC1598 (GAT)

More GAT tests, namely some usage for `Iterable` and `StreamingIterator`, shadowing (lifetimes and type params), `Collection<T>` and `CollectionFamily` from [the series](http://smallcultfollowing.com/babysteps/blog/2016/11/03/associated-type-constructors-part-2-family-traits/) with default associated types. Tracking issue: rust-lang#44265

r? @nikomatsakis

Wrong GAT argument numbers / kinds and default values are next.

alexcrichton added a commit to alexcrichton/rust that referenced this issue May 10, 2018

Rollup merge of rust-lang#49423 - gavento:gavento-dev, r=nikomatsakis
Extend tests for RFC1598 (GAT)

More GAT tests, namely some usage for `Iterable` and `StreamingIterator`, shadowing (lifetimes and type params), `Collection<T>` and `CollectionFamily` from [the series](http://smallcultfollowing.com/babysteps/blog/2016/11/03/associated-type-constructors-part-2-family-traits/) with default associated types. Tracking issue: rust-lang#44265

r? @nikomatsakis

Wrong GAT argument numbers / kinds and default values are next.
@quadrupleslap

This comment has been minimized.

quadrupleslap commented May 21, 2018

Sorry if I'm being stupid, but is this kind of thing going to be legal with GATs?

trait Sequencer {
    type Wrap<A>;
    fn chain<A, B, F>(Self::Wrap<A>, F) -> Self::Wrap<B>
        where F: FnOnce(A) -> Self::Wrap<B>;
    fn wrap<A>(A) -> Self::Wrap<A>;
}
@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented May 27, 2018

What's the status of this? I know that chalk recently gained gat support. Is that intended to land in rust soon?

@baloo

This comment has been minimized.

baloo commented May 29, 2018

@mark-i-m I gave it a shot last week. From my understanding, the syntax parser is there (although missing tests). But the "implementation" is not written yet. (see #44265 (comment) for more details)

@Boscop

This comment has been minimized.

Boscop commented Jun 2, 2018

@quadrupleslap AIUI, that will be possible later, but at first, GATs will only support lifetime parameters..

@Centril

This comment has been minimized.

Contributor

Centril commented Jun 2, 2018

@Boscop the RFC specifies that type parameters will also be supported.

@scalexm

This comment has been minimized.

Member

scalexm commented Jun 4, 2018

Does someone know the exact status of the implementation of the syntax in rustc? It seems mostly there, but higher ranked bounds generate ICEs:
http://play.rust-lang.org/?gist=a48959858ed5dd432c2396feae5c3cc1&version=nightly&mode=debug

I鈥檇 at least need the whole syntax to be implemented in order to advance on the chalkification work. If anybody is still working on the syntax, please let me know, or else I might go and fix it myself :)

@rphmeier rphmeier referenced this issue Jun 6, 2018

Merged

Make substrate generic #169

7 of 7 tasks complete
@LukasKalbertodt

This comment has been minimized.

Contributor

LukasKalbertodt commented Jun 9, 2018

To me it looks like communication is the major problem slowing down the implementation of GATs. There seem to be at least a few people interested in working on this, but no one really knows the exact implementation status and who is working on this already. What do you think about an Discord (or IRC?) channel just to talk about implementing GATs? I think that would certainly help.

Also, I could certainly contribute a few work days in the following weeks to work on this (but I don't really know anything about this part of the compiler yet).

@LukasKalbertodt

This comment has been minimized.

Contributor

LukasKalbertodt commented Jun 12, 2018

So I asked for a dedicated channel on Discord. But instead of getting a channel, I learned a few things. I also searched for everything related to GATs. So I'll try to summarize all information regarding this feature; maybe it's useful to some people. But I don't know anything, so take this with a grain of salt! Also: please tell me if some information is incorrect so that I can fix it.

Summary of implementation efforts regarding GATs

Since then there weren't any UI tests added. And I can't find any other PRs directly related to GATs.

However, the point (c) from above (the trait system) is being worked on "in secret". As far as I understand it, the plan is to migrate to the new chalk-based trait solver soon and not make GATs work on the old system. Integration of the new trait solver is tracking by the "Chalkification" tracking issue. There have been quite a few PRs related to chalk and chalkification. Notably, there is a chalk PR called "Finish implementing GATs" (merged 2018-05-24). So it seems like the core system for GATs is already in place.

That said, the "GATs" in chalk are a prototype implementation and using it in rustc is not just a use chalk;. As @scalexm told me: "There seems to be quite a lot [to do]".


For more information and to help out, it's probably useful to take a look into the #wg-traits Discord channel and the traits WG tracking issue.

@LukasKalbertodt

This comment has been minimized.

Contributor

LukasKalbertodt commented Jun 13, 2018

So @nikomatsakis just created the #wg-traits-gat channel on the rust-lang discord server (join here). It would be great if we could get everyone who wants to help in there. Plus a few people who know about the status of this feature (in particular, what still needs to be done and where we could help). That should make communication easier and faster, especially for people like me who are not yet deeply involved/part of the traits WG :)

@alexreg

This comment has been minimized.

Contributor

alexreg commented Aug 14, 2018

Is there any update on this recently? Are we waiting for Chalk integration into the compiler? (Perhaps there's a separate issue for that.)

@shepmaster

This comment has been minimized.

Member

shepmaster commented Aug 14, 2018

Perhaps there's a separate issue for that.

#48049

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment