Parse optional and named parameters.

Parse optional and named parameters.

I'm not a big fan of the currentParameterKind stack, this information is
almost accessible from parseFormalParameter(), but we don't pass the type
to the listener.

R=ahe@google.com, paulberry@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/4500bb09203159a25eb7ff43296979344befb5c4

[ahe, 2017-02-22 09:25:16]

lgtm

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:509:
currentParameterKind.addLast(ParameterKind.POSITIONAL);
I suggest that you push this on the stack instead of [currentParameterKind].

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:525:
parameterDefaultValueExpression = pop();
I would recommend against setting state in the listener as it can interfere with
error recovery. For example, try parsing this:

f([void f([x=1]) = null]) {
}

If you use pkg/front_end/lib/src/fasta/parser/bin/parser.dart, you can see all
the events generated:

MetadataStar
Modifiers
NoType
Identifier: f
NoTypeVariables
MetadataStar
Modifiers
VoidKeyword
Identifier: f
NoTypeVariables
MetadataStar
Modifiers
NoType
Identifier: x
ValuedFormalParameter
FormalParameter
OptionalFormalParameters
FormalParameters
FunctionTypedFormalParameter
ValuedFormalParameter
FormalParameter
OptionalFormalParameters
FormalParameters
AsyncModifier
TopLevelMethod
TopLevelDeclaration
CompilationUnit

Notice how ValuedFormalParameter is repeated. However, I think you're safe as
FormalParameter always follows ValuedFormalParameter. I just think it might be
easier if you put theses values on the stack, and I think all that's required is
an extra event in parseFormalParameter, for example, just before
endFormalParameter is called:

} else {
  listener.handleFormalParameterWithoutValue(token);
}

[scheglov, 2017-02-22 17:18:15]

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:509:
currentParameterKind.addLast(ParameterKind.POSITIONAL);
On 2017/02/22 09:25:15, ahe wrote:
> I suggest that you push this on the stack instead of [currentParameterKind].

How can I access it later?
After parsing the first parameter, the top of the stack will always be a
parameter.

What do you think about passing FormalParameterType into endFormalParameter() ?
This would require moving FormalParameterType into listener.dart, or importing
FormalParameterType into listener.dart from parser.dart.

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:525:
parameterDefaultValueExpression = pop();
On 2017/02/22 09:25:15, ahe wrote:
> I would recommend against setting state in the listener as it can interfere
with
> error recovery. For example, try parsing this:
> 
> f([void f([x=1]) = null]) {
> }
> 
> If you use pkg/front_end/lib/src/fasta/parser/bin/parser.dart, you can see all
> the events generated:
> 
> MetadataStar
> Modifiers
> NoType
> Identifier: f
> NoTypeVariables
> MetadataStar
> Modifiers
> VoidKeyword
> Identifier: f
> NoTypeVariables
> MetadataStar
> Modifiers
> NoType
> Identifier: x
> ValuedFormalParameter
> FormalParameter
> OptionalFormalParameters
> FormalParameters
> FunctionTypedFormalParameter
> ValuedFormalParameter
> FormalParameter
> OptionalFormalParameters
> FormalParameters
> AsyncModifier
> TopLevelMethod
> TopLevelDeclaration
> CompilationUnit
> 
> Notice how ValuedFormalParameter is repeated. However, I think you're safe as
> FormalParameter always follows ValuedFormalParameter. I just think it might be
> easier if you put theses values on the stack, and I think all that's required
is
> an extra event in parseFormalParameter, for example, just before
> endFormalParameter is called:
> 
> } else {
>   listener.handleFormalParameterWithoutValue(token);
> }

OK, I will add handleFormalParameterWithoutValue().

[ahe, 2017-02-22 17:30:23]

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709783003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:509:
currentParameterKind.addLast(ParameterKind.POSITIONAL);
On 2017/02/22 17:18:14, scheglov wrote:
> On 2017/02/22 09:25:15, ahe wrote:
> > I suggest that you push this on the stack instead of [currentParameterKind].
> 
> How can I access it later?
> After parsing the first parameter, the top of the stack will always be a
> parameter.
> 
> What do you think about passing FormalParameterType into endFormalParameter()
?
> This would require moving FormalParameterType into listener.dart, or importing
> FormalParameterType into listener.dart from parser.dart.

That sounds like a good idea. We need to take advantage of having the ability to
rapidly change this API.

But we should make sure that it doesn't harm performance. Let's see what we
figure out with CL 2711463005.

[scheglov, 2017-02-22 19:36:44]

PTAL

There is no difference in speed of running parser tests, with and without the
change it takes 43-44 seconds.

dart pkg/testing/bin/testing.dart --config=pkg/front_end/test/fasta/testing.json
parser

[Paul Berry, 2017-02-22 20:50:05]

lgtm assuming you run dartfmt.  My other comments are at your discretion.

https://codereview.chromium.org/2709783003/diff/20001/pkg/compiler/lib/src/pa...
File pkg/compiler/lib/src/parser/node_listener.dart (right):

https://codereview.chromium.org/2709783003/diff/20001/pkg/compiler/lib/src/pa...
pkg/compiler/lib/src/parser/node_listener.dart:728: void
handleFormalParameterWithoutValue(Token token) {
Nit: run dartfmt

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:510:
push(_ParameterDefaultValue.nullValue);
Personally I would prefer to use the enum NullValue for this (see
pkg/front_end/lib/src/fasta/source/stack_listener.dart).  You'll have to add a
new value to the enum (e.g. NullValue.parameterDefault).

The pop() method automatically converts NullValue enums to `null`, so if you do
this, then line 526 changes to simply "if (defaultValue != null)"

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1174: } else
Nit: run dartfmt

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1217: static final
nullValue = new _ParameterDefaultValue(null, null);
Nit: make this const.

(Or delete it if you take my advice above about using NullValue).

[ahe, 2017-02-23 14:54:49]

lgtm

Please don't run dartfmt on body_builder.dart.

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:510:
push(_ParameterDefaultValue.nullValue);
On 2017/02/22 20:50:04, Paul Berry wrote:
> Personally I would prefer to use the enum NullValue for this (see
> pkg/front_end/lib/src/fasta/source/stack_listener.dart).  You'll have to add a
> new value to the enum (e.g. NullValue.parameterDefault).
> 
> The pop() method automatically converts NullValue enums to `null`, so if you
do
> this, then line 526 changes to simply "if (defaultValue != null)"

I agree. Don't hesitate to add values to that enum as you need them, its only
there to help us debug listeners.

[scheglov, 2017-02-23 17:55:10]

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709783003/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:510:
push(_ParameterDefaultValue.nullValue);
On 2017/02/22 20:50:04, Paul Berry wrote:
> Personally I would prefer to use the enum NullValue for this (see
> pkg/front_end/lib/src/fasta/source/stack_listener.dart).  You'll have to add a
> new value to the enum (e.g. NullValue.parameterDefault).
> 
> The pop() method automatically converts NullValue enums to `null`, so if you
do
> this, then line 526 changes to simply "if (defaultValue != null)"

Done.

[scheglov, 2017-02-23 17:55:22]

Description was changed from

==========
Parse optional and named parameters.

I'm not a big fan of the currentParameterKind stack, this information is
almost accessible from parseFormalParameter(), but we don't pass the type
to the listener.

R=ahe@google.com, paulberry@google.com
BUG=
==========

to

==========
Parse optional and named parameters.

I'm not a big fan of the currentParameterKind stack, this information is
almost accessible from parseFormalParameter(), but we don't pass the type
to the listener.

R=ahe@google.com, paulberry@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/4500bb09203159a25eb7ff43296979344befb5c4
==========

[scheglov, 2017-02-23 17:55:24]

Committed patchset #2 (id:20001) manually as
4500bb09203159a25eb7ff43296979344befb5c4 (presubmit successful).