Implement parsing support for generic functions

Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored. Proper
handling of the parameters will be added subsequently.

The feature must be enabled with --generic-method-syntax.

Compiling all of dart2js consumes about 2.2% more tokens with
this change, due to the necessary lookahead to distinguish
generic functions from generic types.

===== with this change =========

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3628934  <<<<<
Classes parsed:          2457
  Tokens consumed:       977999
Functions parsed:        49607
  Tokens consumed:       2430638

===== without this change ======

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3547765 <<<<<
Classes parsed:          2457
  Tokens consumed:       942536
Functions parsed:        49607
  Tokens consumed:       2371357



BUG=25869
R=asiva@google.com

Committed: https://github.com/dart-lang/sdk/commit/19af8524710507ff29ec077ce3e1e67e5a7e8f30

[hausner, 2016-08-19 15:52:59]

Description was changed from

==========
Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored.

The feature must be enabled with --generic-method-syntax

BUG=25869
==========

to

==========
Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored. Proper
handling of the parameters will be added subsequently.

The feature must be enabled with --generic-method-syntax.

BUG=25869
==========

[hausner, 2016-08-19 15:53:00]

hausner@google.com changed reviewers:
+ asiva@google.com

[hausner, 2016-08-19 16:29:46]

Description was changed from

==========
Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored. Proper
handling of the parameters will be added subsequently.

The feature must be enabled with --generic-method-syntax.

BUG=25869
==========

to

==========
Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored. Proper
handling of the parameters will be added subsequently.

The feature must be enabled with --generic-method-syntax.

Compiling all of dart2js consumes about 2.2% more tokens with
this change, due to the necessary lookahead to distinguish
generic functions from generic types.

===== with this change =========

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3628934  <<<<<
Classes parsed:          2457
  Tokens consumed:       977999
Functions parsed:        49607
  Tokens consumed:       2430638

===== without this change ======

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3547765 <<<<<
Classes parsed:          2457
  Tokens consumed:       942536
Functions parsed:        49607
  Tokens consumed:       2371357



BUG=25869
==========

[siva, 2016-08-19 20:30:59]

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:143: TokenPosition _saved_pos;
Normally we have been using _ as a suffix not prefix for private instance
variables, any reason for changing that.

DISALLOW_COPY_AND_ASSIGN(Parser::TokenPosScope);

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2105: ReportError(type_param_pos, "error in type
parameters");
This error seems to be printed unconditionally even when
'FLAG_generic_method_syntax is not on', the old code did not have this error?

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3791: }
Ditto comment about error being unconditional and we don't seem to complain
about this for the !FLAG_generic_method_syntax case.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5720: }
Ditto comment about the error being unconditional (FLAG_generic_method_syntax)

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7853: }
Ditto comment about the error being unconditional (FLAG_generic_method_syntax)

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8031: bool Parser::TryParseTypeParameters() {
seems to be always called after a CurrentToken() == Token::kLT check,
can we just turn this 'if' check into an assertion
ASSERT(CurrentToken() == Token::kLT);

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8060: }
This will return true for something like
<<>,<>> or <> or <<>> or <<>A,>
is that expected?

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8097: if (CurrentToken() == Token::kLT) {
Ditto comment about turning this 'if' into an ASSERT

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8110: if (ct == Token::kPERIOD) {
ct was kIDENT based on the check above, how will (ct == kPERIOD) ever be true.

Probably needs:
 if (LookahreadToken(1) == Token::kPERIOD) {
    ConsumeToken();
 }

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8123: return true;
This will return true for something like
<<>,<>> or <> or <<>> or <<>A,>
is that expected?

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8331: // Skip over 'patch' for top-level function
declarations in patch sources.
I thought we removed support for 'patch' going forward.

[hausner, 2016-08-19 21:40:05]

PTAL. Thank you.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:143: TokenPosition _saved_pos;
On 2016/08/19 20:30:58, siva wrote:
> Normally we have been using _ as a suffix not prefix for private instance
> variables, any reason for changing that.
> 
> DISALLOW_COPY_AND_ASSIGN(Parser::TokenPosScope);

Done. Accedentally mixed dart and c++ convention.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:2105: ReportError(type_param_pos, "error in type
parameters");
On 2016/08/19 20:30:58, siva wrote:
> This error seems to be printed unconditionally even when
> 'FLAG_generic_method_syntax is not on', the old code did not have this error?

The old code would have printed some other syntax error message. In practice,
this should actually not fail since the IsParameterPart() call above already
checks whether the type parameters look syntactically correct.

I'll reverse the two error messages so that the "not supported" error is always
printed when the flag is not enabled.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3791: }
On 2016/08/19 20:30:58, siva wrote:
> Ditto comment about error being unconditional and we don't seem to complain
> about this for the !FLAG_generic_method_syntax case.

The error will be printed when the function is actually compiled. If the
function is not used, we silently accept the syntax if it's correct. Is that too
permissive? Maybe. I'll add the error eagerly.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5720: }
On 2016/08/19 20:30:58, siva wrote:
> Ditto comment about the error being unconditional (FLAG_generic_method_syntax)

Done.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:7853: }
On 2016/08/19 20:30:58, siva wrote:
> Ditto comment about the error being unconditional (FLAG_generic_method_syntax)

Done.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8031: bool Parser::TryParseTypeParameters() {
On 2016/08/19 20:30:58, siva wrote:
> seems to be always called after a CurrentToken() == Token::kLT check,
> can we just turn this 'if' check into an assertion
> ASSERT(CurrentToken() == Token::kLT);

Yes, that makes sense.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8060: }
On 2016/08/19 20:30:58, siva wrote:
> This will return true for something like
> <<>,<>> or <> or <<>> or <<>A,>
> is that expected?

Yes, that is expected. This function is an approximation to parsing type
parameters. It checks that the next few tokens could be type parameters. If
there is a syntax error, it will be caught by the code that actually parses and
processes the type parameters. (This code does not yet exist for functions, only
for classes.)

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8097: if (CurrentToken() == Token::kLT) {
On 2016/08/19 20:30:58, siva wrote:
> Ditto comment about turning this 'if' into an ASSERT

Done.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8110: if (ct == Token::kPERIOD) {
On 2016/08/19 20:30:58, siva wrote:
> ct was kIDENT based on the check above, how will (ct == kPERIOD) ever be true.
> 
> Probably needs:
>  if (LookahreadToken(1) == Token::kPERIOD) {
>     ConsumeToken();
>  }
Nice catch, thank you.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8123: return true;
On 2016/08/19 20:30:58, siva wrote:
> This will return true for something like
> <<>,<>> or <> or <<>> or <<>A,>
> is that expected?

Yes, for the same reason as explained above. ParseTypeArguments() will catch
those errors.

This function is mainly a way to peek ahead so the parser can determine what the
next tokens most likely are.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8331: // Skip over 'patch' for top-level function
declarations in patch sources.
On 2016/08/19 20:30:58, siva wrote:
> I thought we removed support for 'patch' going forward.

Yes. This probably reappeared because I copied this function before the patch
removal CL, and the merge didn't catch it later.

Removed.

[siva, 2016-08-19 23:08:14]

lgtm

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:51: DEFINE_FLAG(bool, generic_method_syntax, false,
"Enbable generic functions.");
s/Enbable/Enable/

[hausner, 2016-08-19 23:12:01]

Thank you.

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2260693002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:8060: }
On 2016/08/19 21:40:04, hausner wrote:
> On 2016/08/19 20:30:58, siva wrote:
> > This will return true for something like
> > <<>,<>> or <> or <<>> or <<>A,>
> > is that expected?
> 
> Yes, that is expected. This function is an approximation to parsing type
> parameters. It checks that the next few tokens could be type parameters. If
> there is a syntax error, it will be caught by the code that actually parses
and
> processes the type parameters. (This code does not yet exist for functions,
only
> for classes.)

One small correction: <>> (or anything else with too many closing brackets) will
return false. The nesting level would end up being -1, which is caught in line
8057.

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:51: DEFINE_FLAG(bool, generic_method_syntax, false,
"Enbable generic functions.");
On 2016/08/19 23:08:13, siva wrote:
> s/Enbable/Enable/

Done. I was clearly babbling.

[hausner, 2016-08-19 23:21:04]

Description was changed from

==========
Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored. Proper
handling of the parameters will be added subsequently.

The feature must be enabled with --generic-method-syntax.

Compiling all of dart2js consumes about 2.2% more tokens with
this change, due to the necessary lookahead to distinguish
generic functions from generic types.

===== with this change =========

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3628934  <<<<<
Classes parsed:          2457
  Tokens consumed:       977999
Functions parsed:        49607
  Tokens consumed:       2430638

===== without this change ======

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3547765 <<<<<
Classes parsed:          2457
  Tokens consumed:       942536
Functions parsed:        49607
  Tokens consumed:       2371357



BUG=25869
==========

to

==========
Implement parsing support for generic functions

Type parameters and arguments are parsed but ignored. Proper
handling of the parameters will be added subsequently.

The feature must be enabled with --generic-method-syntax.

Compiling all of dart2js consumes about 2.2% more tokens with
this change, due to the necessary lookahead to distinguish
generic functions from generic types.

===== with this change =========

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3628934  <<<<<
Classes parsed:          2457
  Tokens consumed:       977999
Functions parsed:        49607
  Tokens consumed:       2430638

===== without this change ======

Number of tokens:        1056691
Source length:           5850143 characters
Number of source tokens: 936226

Total tokens consumed:   3547765 <<<<<
Classes parsed:          2457
  Tokens consumed:       942536
Functions parsed:        49607
  Tokens consumed:       2371357



BUG=25869
R=asiva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/19af8524710507ff29ec077ce3e1e67e5a7e8f30
==========

[hausner, 2016-08-19 23:21:07]

Committed patchset #3 (id:40001) manually as
19af8524710507ff29ec077ce3e1e67e5a7e8f30 (presubmit successful).

[Leaf, 2016-08-20 01:15:39]

On 2016/08/19 23:21:07, hausner wrote:
> Committed patchset #3 (id:40001) manually as
> 19af8524710507ff29ec077ce3e1e67e5a7e8f30 (presubmit successful).

Great to see, thanks!

[regis, 2016-08-24 14:14:01]

regis@google.com changed reviewers:
+ regis@google.com

[regis, 2016-08-24 14:14:02]

Looks great!

DBC

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:3783: ReportError("generic type arguments not supported.");
Being picky, but instead of "generic type arguments", either "generic function"
or "type parameters" would be more accurate.

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11838: ReportError("generic type arguments not
supported.");
I would drop "generic".
Or maybe "type arguments of generic function not supported".
Or simply "generic function not supported".

https://codereview.chromium.org/2260693002/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11938: ReportError("generic type arguments not
supported.");
ditto