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

Generic Associated Types Parsing & Name Resolution #45904

Merged
merged 19 commits into from Dec 2, 2017

Conversation

Projects
None yet
7 participants
@sunjay
Member

sunjay commented Nov 10, 2017

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

Tracking Issue: #44265

Notes For Reviewers

  • 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

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 12, 2017

Contributor

鈽旓笍 The latest upstream changes (presumably #45870) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Nov 12, 2017

鈽旓笍 The latest upstream changes (presumably #45870) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

I pushed some examples of how we could write "successful parse" tests.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 14, 2017

Contributor

(In my example test I went a bit overboard testing the generics parsing; since that's re-using something existing, it's not really needed.)

Contributor

nikomatsakis commented Nov 14, 2017

(In my example test I went a bit overboard testing the generics parsing; since that's re-using something existing, it's not really needed.)

@@ -4437,6 +4438,38 @@ impl<'a> Parser<'a> {
})
}
/// Parses the following grammar:
/// TraitItemAssocTy = Ident ["<"...">"] [":" [TyParamBounds]] ["where" ...] ["=" Ty]
fn parse_trait_item_assoc_ty(&mut self, preceding_attrs: Vec<Attribute>)

This comment has been minimized.

@sunjay

sunjay Nov 19, 2017

Member

@nikomatsakis Is there a reason why parse_trait_item_assoc_ty doesn't consume the semicolon? I think it really could given that it's only used in one place and it wouldn't make any sense to parse all of this without the semicolon at the end. The only invocation of this function immediately expects a semicolon right after it.

(added this question again because GitHub removed it on the latest diff)

@sunjay

sunjay Nov 19, 2017

Member

@nikomatsakis Is there a reason why parse_trait_item_assoc_ty doesn't consume the semicolon? I think it really could given that it's only used in one place and it wouldn't make any sense to parse all of this without the semicolon at the end. The only invocation of this function immediately expects a semicolon right after it.

(added this question again because GitHub removed it on the latest diff)

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 19, 2017

Contributor

It doesn't make a big difference either way. A lot of times it's good to leave separators and terminators out from functions like this, since it makes them more re-usable, but I think in this case the ; logically "belongs" to the trait item, so it would make sense for this function to consume it.

@nikomatsakis

nikomatsakis Nov 19, 2017

Contributor

It doesn't make a big difference either way. A lot of times it's good to leave separators and terminators out from functions like this, since it makes them more re-usable, but I think in this case the ; logically "belongs" to the trait item, so it would make sense for this function to consume it.

This comment has been minimized.

@sunjay

sunjay Nov 26, 2017

Member

In the latest code, the semicolon is consumed.

@sunjay

sunjay Nov 26, 2017

Member

In the latest code, the semicolon is consumed.

} else if self.is_const_item() {
// This parses the grammar:
// ImplItemConst = "const" Ident ":" Ty "=" Expr ";"

This comment has been minimized.

@sunjay

sunjay Nov 19, 2017

Member

This comment isn't relevant to what I'm adding, but I added it because @nikomatsakis mentioned that it's good to document the grammar in places like this

@sunjay

sunjay Nov 19, 2017

Member

This comment isn't relevant to what I'm adding, but I added it because @nikomatsakis mentioned that it's good to document the grammar in places like this

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 21, 2017

Contributor

鈽旓笍 The latest upstream changes (presumably #45771) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Nov 21, 2017

鈽旓笍 The latest upstream changes (presumably #45771) made this pull request unmergeable. Please resolve the merge conflicts.

@sunjay sunjay changed the title from Generic Associated Types Parsing to Generic Associated Types Parsing & Name Resolution Nov 22, 2017

@sunjay

This comment has been minimized.

Show comment
Hide comment
@sunjay

sunjay Nov 22, 2017

Member

The name resolution PR (#46146) has been merged into this PR. Once I fix the tests, this should be ready to go!

Member

sunjay commented Nov 22, 2017

The name resolution PR (#46146) has been merged into this PR. Once I fix the tests, this should be ready to go!

@sunjay

This comment has been minimized.

Show comment
Hide comment
@sunjay

sunjay Nov 22, 2017

Member

@nikomatsakis Extensions to the parser that will be made soon but not as part of this PR:

  • type Quux<'a> = <T as Foo>::Bar<'a, 'static>;
  • fn foo<T: for<'a> StreamingIterator<Item<'a>=&'a [i32]>>(iter: T) { /* ... */ }
Member

sunjay commented Nov 22, 2017

@nikomatsakis Extensions to the parser that will be made soon but not as part of this PR:

  • type Quux<'a> = <T as Foo>::Bar<'a, 'static>;
  • fn foo<T: for<'a> StreamingIterator<Item<'a>=&'a [i32]>>(iter: T) { /* ... */ }
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 23, 2017

Contributor

鈽旓笍 The latest upstream changes (presumably #46051) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors commented Nov 23, 2017

鈽旓笍 The latest upstream changes (presumably #46051) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov petrochenkov referenced this pull request Nov 23, 2017

Merged

trait alias infrastructure #45047

7 of 10 tasks complete
type Bar<'a, 'b, T: Debug, U,>: Deref<Target = T> + Into<U>;
type Bar<'a, 'b, T: Debug, U,> where T: Deref<Target = U>, U: Into<T>;
type Bar<'a, 'b, T: Debug, U,>: Deref<Target = T> + Into<U>
where T: Deref<Target = U>, U: Into<T>;

This comment has been minimized.

@sunjay

sunjay Nov 26, 2017

Member

@nikomatsakis I extended this with quite a few cases. Let me know if there is anything important that I missed. (It's probably not totally exhaustive, but I believe I have covered the major cases.)

Note: This code is pretty much nonsense as far as its meaning goes, but that's okay because it's just really meant to test parsing.

@sunjay

sunjay Nov 26, 2017

Member

@nikomatsakis I extended this with quite a few cases. Let me know if there is anything important that I missed. (It's probably not totally exhaustive, but I believe I have covered the major cases.)

Note: This code is pretty much nonsense as far as its meaning goes, but that's okay because it's just really meant to test parsing.

@petrochenkov petrochenkov removed their assignment Nov 26, 2017

@nikomatsakis

Looks good!

impl Foo for Bar {
type Assoc = usize;
type Assoc2<T> = Vec<T>;

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2017

Contributor

Maybe also add type Assoc3<T> where T: Iterator = Vec<T>?

@nikomatsakis

nikomatsakis Nov 29, 2017

Contributor

Maybe also add type Assoc3<T> where T: Iterator = Vec<T>?

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2017

Contributor

But it's not super important.

@nikomatsakis

nikomatsakis Nov 29, 2017

Contributor

But it's not super important.

This comment has been minimized.

@sunjay

sunjay Nov 29, 2017

Member

It's added below this comment. Please r+ once again. :)

@sunjay

sunjay Nov 29, 2017

Member

It's added below this comment. Please r+ once again. :)

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Nov 29, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 29, 2017

Contributor

馃搶 Commit 11613a0 has been approved by nikomatsakis

Contributor

bors commented Nov 29, 2017

馃搶 Commit 11613a0 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis
Contributor

nikomatsakis commented Nov 30, 2017

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 30, 2017

Contributor

@bors r-

Contributor

arielb1 commented Nov 30, 2017

@bors r-

@sunjay

This comment has been minimized.

Show comment
Hide comment
@sunjay

sunjay Dec 1, 2017

Member

Rebased. Should be mergeable again.

Member

sunjay commented Dec 1, 2017

Rebased. Should be mergeable again.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Dec 1, 2017

Contributor

@bors r+

Contributor

nikomatsakis commented Dec 1, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 1, 2017

Contributor

馃搶 Commit 9d5592b has been approved by nikomatsakis

Contributor

bors commented Dec 1, 2017

馃搶 Commit 9d5592b has been approved by nikomatsakis

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 2, 2017

Contributor

鈱涳笍 Testing commit 9d5592b with merge ddaebe9...

Contributor

bors commented Dec 2, 2017

鈱涳笍 Testing commit 9d5592b with merge ddaebe9...

bors added a commit that referenced this pull request 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
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 2, 2017

Contributor

鈽锔 Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing ddaebe9 to master...

Contributor

bors commented Dec 2, 2017

鈽锔 Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing ddaebe9 to master...

@bors bors merged commit 9d5592b into rust-lang:master Dec 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@sunjay sunjay deleted the sunjay:gat-parser branch Dec 2, 2017

@sunjay

This comment has been minimized.

Show comment
Hide comment
@sunjay

sunjay Dec 2, 2017

Member

馃帀 馃帀 馃帀 馃帀 馃帀

Member

sunjay commented Dec 2, 2017

馃帀 馃帀 馃帀 馃帀 馃帀

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