Start adding support for mixin application syntax. We now parse the typedef variant of mixin applic…

Start adding support for mixin application syntax. We now parse the typedef variant of mixin applications like:

   typedef C = S with M;

but the construct is still unhandled.

R=ahe@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=17129

[ahe, 2013-01-15 15:35:04]

I think you need to add a test case. Otherwise, I really like this change.

As for testing, I suggest adding a new method to
tests/compiler/dart2js/unparser_test.dart.

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:264: if
(identical(token.next.next.kind, EQ_TOKEN)) {
if (optional('=', token.next.next)) {

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:289: //
TODO(kasperl): The endToken is one too far ahead.
That is fine. As long as you don't store it.

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:244: final Token
beginToken;
typedefKeyword?

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:245: final Token
endToken;
This is not necessary.

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:262: Token
getEndToken() => endToken;
mixins.getEndToken();

[kasperl, 2013-01-15 15:58:18]

Thanks for the review, Peter! I'll add an unparser test tomorrow morning and let
you know when this change is ready for another peek.

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:244: final Token
beginToken;
On 2013/01/15 15:35:04, ahe wrote:
> typedefKeyword?

The mixin application itself doesn't start with typedef. It starts with the
superclass reference.

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:262: Token
getEndToken() => endToken;
On 2013/01/15 15:35:04, ahe wrote:
> mixins.getEndToken();

Unfortunately, the node list produced from parseTypeList doesn't get a good
endToken. Any suggestion on how to make it get a reasonable endToken? I tried a
few simple things, but the token I have available after parsing the last type
using parseType isn't exactly what I need (again one too far ahead).

[ahe, 2013-01-15 16:03:39]

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:262: Token
getEndToken() => endToken;
On 2013/01/15 15:58:18, kasperl wrote:
> On 2013/01/15 15:35:04, ahe wrote:
> > mixins.getEndToken();
> 
> Unfortunately, the node list produced from parseTypeList doesn't get a good
> endToken. Any suggestion on how to make it get a reasonable endToken? I tried
a
> few simple things, but the token I have available after parsing the last type
> using parseType isn't exactly what I need (again one too far ahead).

I think NodeList.getEndToken() should do what you want.

[kasperl, 2013-01-16 07:00:03]

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/tree/nodes.dart (right):

https://codereview.chromium.org/11878043/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/nodes.dart:262: Token
getEndToken() => endToken;
On 2013/01/15 16:03:40, ahe wrote:
> On 2013/01/15 15:58:18, kasperl wrote:
> > On 2013/01/15 15:35:04, ahe wrote:
> > > mixins.getEndToken();
> > 
> > Unfortunately, the node list produced from parseTypeList doesn't get a good
> > endToken. Any suggestion on how to make it get a reasonable endToken? I
tried
> a
> > few simple things, but the token I have available after parsing the last
type
> > using parseType isn't exactly what I need (again one too far ahead).
> 
> I think NodeList.getEndToken() should do what you want.

Thanks. That looks exactly right.

[kasperl, 2013-01-16 07:14:43]

On 2013/01/15 15:35:04, ahe wrote:
> I think you need to add a test case. Otherwise, I really like this change.
> 
> As for testing, I suggest adding a new method to
> tests/compiler/dart2js/unparser_test.dart.

In it's current state, this is hard to test since I reject the syntax in the
listener. Next step is to add an element for a named mixin application and
reject it in the resolver and at that point, it seems easy to test this with the
unparser.

[ahe, 2013-01-16 07:42:20]

On 2013/01/16 07:14:43, kasperl wrote:
> In it's current state, this is hard to test since I reject the syntax in the
> listener. Next step is to add an element for a named mixin application and
> reject it in the resolver and at that point, it seems easy to test this with
the
> unparser.

Could you change the listener? I really don't like adding untested.

[kasperl, 2013-01-16 08:14:19]

On 2013/01/16 07:42:20, ahe wrote:
> On 2013/01/16 07:14:43, kasperl wrote:
> > In it's current state, this is hard to test since I reject the syntax in the
> > listener. Next step is to add an element for a named mixin application and
> > reject it in the resolver and at that point, it seems easy to test this with
> the
> > unparser.
> 
> Could you change the listener? I really don't like adding untested.

It is certainly doable and it would be the logical next step, so I'd be happy to
add some sort of outline of an element that can be pretty printed and rejected
by the resolver.

[ahe, 2013-01-16 08:18:03]

https://codereview.chromium.org/11878043/diff/9001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/scanner/listener.dart (right):

https://codereview.chromium.org/11878043/diff/9001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/listener.dart:836:
recoverableError('unsupported mixin application in typedef',
You can (and should) implement this method in NodeListener to create an AST
node. This should then be pretty printed and tested in unparser.

[kasperl, 2013-01-16 09:27:06]

Thanks for the hint about node listener. I've added a few tests and fixed a few
issues that were uncovered.

[kasperl, 2013-01-16 09:29:24]

PTAL.

[ahe, 2013-01-16 09:37:57]

LGTM!

https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):

https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:268: bool
isFunctionTypeAlias = true;
Try using peekAfterType.

https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:269: if
(identical(token.next.kind, IDENTIFIER_TOKEN)) {
token.next.isIdentifier() (it's complicated)

https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:271: if
(identical(peek.kind, LT_TOKEN)) {
optional('<', peek)

[kasperl, 2013-01-16 09:44:31]

On 2013/01/16 09:37:57, ahe wrote:
> LGTM!
> 
>
https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
> File sdk/lib/_internal/compiler/implementation/scanner/parser.dart (right):
> 
>
https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
> sdk/lib/_internal/compiler/implementation/scanner/parser.dart:268: bool
> isFunctionTypeAlias = true;
> Try using peekAfterType.
> 
>
https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
> sdk/lib/_internal/compiler/implementation/scanner/parser.dart:269: if
> (identical(token.next.kind, IDENTIFIER_TOKEN)) {
> token.next.isIdentifier() (it's complicated)
> 
>
https://codereview.chromium.org/11878043/diff/15001/sdk/lib/_internal/compile...
> sdk/lib/_internal/compiler/implementation/scanner/parser.dart:271: if
> (identical(peek.kind, LT_TOKEN)) {
> optional('<', peek)

I originally didn't want to use peekAfterType because it allows too much
(qualified name). It is simpler though and I'm not sure the error message that
it leads to is actually worse than the alternative so I've uploaded another
patch.

[ahe, 2013-01-16 09:57:10]

LGTM!