Parse method names that include type variables.

Parse method names that include type variables.

As a first step to allow factory methods to specify type variables, this
allows the parser to accept statements similar to the form:

  class A {
    factory B<T>.create() { }
  }

http://code.google.com/p/dart/issues/detail?id=417

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

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

[zundel, 2011-12-01 23:45:54]

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartScannerParserContext.java
(right):

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScannerParserContext.java:148:
State.RollbackToken token = oldState.rollbackTokens.pop();
Found a bug in this logic when rolling back >>> parsed as the closing braces for
type arguments - the old for loop unrolled the state in the wrong order.

http://codereview.chromium.org/8774024/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java
(right):

http://codereview.chromium.org/8774024/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:347:
public void disabledBadFactoryTypeArgs1() {
I wrote these tests but was not able to finish the AST changes and resolver code
in time for when the corelib team wanted to start using the new syntax so I left
them in disabled.

[zundel, 2011-12-01 23:47:32]

committed TBR r2004

I am still working on the validation and codegen part of this work.

[mmendez, 2011-12-02 13:20:13]

LGTM - just take a look at the nits.

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (left):

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:896: while
(peek(k) == Token.PERIOD && peek(k + 1) == Token.IDENTIFIER) {
This construct was removed, just double checking that you intended this to be
the case.

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:965: if
(peek(0).equals(Token.LPAREN)) {
Is this code reachable?  Haven't you already checked that
peek(0).equals(Token.PERIOD) above?

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:973: if
(peek(0).equals(Token.PERIOD) && peek(1).equals(Token.IDENTIFIER) &&
peek(2).equals(Token.LPAREN)) {
Nit: long line

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:980:
parseTypeArgumentsOpt();
Nit: with this code flow you could allow x.y<...>.z<...>.  This should be okay
since you are trying to determine how the source should be parsed and the
subsequent code should validate.

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:1094:
parseTypeArgumentsOpt();
Nit: ws

http://codereview.chromium.org/8774024/diff/1/compiler/javatests/com/google/d...
File compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java
(right):

http://codereview.chromium.org/8774024/diff/1/compiler/javatests/com/google/d...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:347:
public void disabledBadFactoryTypeArgs1() {
ok
On 2011/12/01 23:45:54, zundel wrote:
> I wrote these tests but was not able to finish the AST changes and resolver
code
> in time for when the corelib team wanted to start using the new syntax so I
left
> them in disabled.

[zundel, 2011-12-02 13:46:02]

I've updated a few changes since the commit to address Miguel's concerns, and
added 2 more resolver tests to show that the type variables will resolve when
inside one of these
factory methods.

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (left):

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:896: while
(peek(k) == Token.PERIOD && peek(k + 1) == Token.IDENTIFIER) {
On 2011/12/02 13:20:13, mmendez wrote:
> This construct was removed, just double checking that you intended this to be
> the case.

It is too simplistic now, with the introduction of type arguments. I wrapped it
into looksLikeMethodOrAccessorDefinition()

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:965: if
(peek(0).equals(Token.LPAREN)) {
On 2011/12/02 13:20:13, mmendez wrote:
> Is this code reachable?  Haven't you already checked that
> peek(0).equals(Token.PERIOD) above?

Removed

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:973: if
(peek(0).equals(Token.PERIOD) && peek(1).equals(Token.IDENTIFIER) &&
peek(2).equals(Token.LPAREN)) {
On 2011/12/02 13:20:13, mmendez wrote:
> Nit: long line

Done.

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartParser.java:980:
parseTypeArgumentsOpt();
On 2011/12/02 13:20:13, mmendez wrote:
> Nit: with this code flow you could allow x.y<...>.z<...>.  This should be okay
> since you are trying to determine how the source should be parsed and the
> subsequent code should validate.

I think it would accept x.y.<...><...>.z  but regardless, that's wrong. 
Updated.

[codefu, 2011-12-02 13:51:21]

LGTM

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/parser/DartScannerParserContext.java
(right):

http://codereview.chromium.org/8774024/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/parser/DartScannerParserContext.java:148:
State.RollbackToken token = oldState.rollbackTokens.pop();
On 2011/12/01 23:45:54, zundel wrote:
> Found a bug in this logic when rolling back >>> parsed as the closing braces
for
> type arguments - the old for loop unrolled the state in the wrong order.

Thanks for catching this!

http://codereview.chromium.org/8774024/diff/2002/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/parser/DartParser.java (right):

http://codereview.chromium.org/8774024/diff/2002/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:964: // Case 2, a
constructor of the form library.class.<typearguments?>.id
There's no more case 1 prior to this comment.

http://codereview.chromium.org/8774024/diff/2002/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/parser/DartParser.java:967:
parseTypeArgumentsOpt();
ws