Add more mixin tests and start rejecting illegal syntax.

Add more mixin tests and start rejecting illegal syntax.


R=johnniwinther@google.com
BUG=

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

[Johnni Winther, 2013-01-21 11:15:22]

lgtm

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

https://codereview.chromium.org/11953012/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:282: Token
parseMixinApplication(Token token, bool isTypedef) {
Make isTypedef a named parameter. Helps reading call-sites.

[kasperl, 2013-01-21 11:16:30]

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

https://codereview.chromium.org/11953012/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:282: Token
parseMixinApplication(Token token, bool isTypedef) {
On 2013/01/21 11:15:22, Johnni Winther wrote:
> Make isTypedef a named parameter. Helps reading call-sites.

Only problem is that I'd prefer not to make it optional :-)

[ahe, 2013-01-21 12:38:53]

DBC

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

https://codereview.chromium.org/11953012/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:287:
listener.handleModifiers(0);
This is a hack. You should let endMixinApplication know that there are no
modifiers instead.

Also, I have generally tried to let the parser accept as much as possible, and
let later phases report errors. This gives better error recovery.

[kasperl, 2013-01-21 12:46:16]

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

https://codereview.chromium.org/11953012/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:287:
listener.handleModifiers(0);
On 2013/01/21 12:38:53, ahe wrote:
> This is a hack. You should let endMixinApplication know that there are no
> modifiers instead.
> 
> Also, I have generally tried to let the parser accept as much as possible, and
> let later phases report errors. This gives better error recovery.

Ah, okay. In that case, it's a hack used in at least one other part of the
parser not written by me and without any TODOs or indications that it's a hack.

[ahe, 2013-01-21 13:03:10]

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

https://codereview.chromium.org/11953012/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:287:
listener.handleModifiers(0);
On 2013/01/21 12:46:16, kasperl wrote:
> On 2013/01/21 12:38:53, ahe wrote:
> > This is a hack. You should let endMixinApplication know that there are no
> > modifiers instead.
> > 
> > Also, I have generally tried to let the parser accept as much as possible,
and
> > let later phases report errors. This gives better error recovery.
> 
> Ah, okay. In that case, it's a hack used in at least one other part of the
> parser not written by me and without any TODOs or indications that it's a
hack.

Yes. I'm not sure I realized it the time, but it became painfully clear that
parseFunctionExpression is a hack when I looked at how to address
http://dartbug.com/6708.

parseFunctionBody and parseLiteralListOrMap do something similar, and it always
causes problems.

This call in parseLiteralListOrMap:

  listener.handleLiteralList(0, token, constKeyword, token);

causes problems for unparsing.

This call inparseFunctionBody:

  listener.endFunctionBody(0, null, token);

Means that the listener has to guess what to do based on weird argument
combinations:

  void endFunctionBody(int count, Token beginToken, Token endToken) {
    if (count == 0 && beginToken == null) {
      pushNode(new EmptyStatement(endToken));
    } else {
      pushNode(new Block(makeNodeList(count, beginToken, endToken, null)));
    }
  }

It seemed like a good idea at the time, but it wasn't :-(

[kasperl, 2013-01-21 13:04:46]

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

https://codereview.chromium.org/11953012/diff/3001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/scanner/parser.dart:287:
listener.handleModifiers(0);
On 2013/01/21 13:03:10, ahe wrote:
> On 2013/01/21 12:46:16, kasperl wrote:
> > On 2013/01/21 12:38:53, ahe wrote:
> > > This is a hack. You should let endMixinApplication know that there are no
> > > modifiers instead.
> > > 
> > > Also, I have generally tried to let the parser accept as much as possible,
> and
> > > let later phases report errors. This gives better error recovery.
> > 
> > Ah, okay. In that case, it's a hack used in at least one other part of the
> > parser not written by me and without any TODOs or indications that it's a
> hack.
> 
> Yes. I'm not sure I realized it the time, but it became painfully clear that
> parseFunctionExpression is a hack when I looked at how to address
> http://dartbug.com/6708.
> 
> parseFunctionBody and parseLiteralListOrMap do something similar, and it
always
> causes problems.
> 
> This call in parseLiteralListOrMap:
> 
>   listener.handleLiteralList(0, token, constKeyword, token);
> 
> causes problems for unparsing.
> 
> This call inparseFunctionBody:
> 
>   listener.endFunctionBody(0, null, token);
> 
> Means that the listener has to guess what to do based on weird argument
> combinations:
> 
>   void endFunctionBody(int count, Token beginToken, Token endToken) {
>     if (count == 0 && beginToken == null) {
>       pushNode(new EmptyStatement(endToken));
>     } else {
>       pushNode(new Block(makeNodeList(count, beginToken, endToken, null)));
>     }
>   }
> 
> It seemed like a good idea at the time, but it wasn't :-(

Yeah, that is pretty nasty. I'll try to address it by always allowing modifiers
and weed them out (report them) later during resolution.