Improve parsing of function expressions.

Improve parsing of function expressions.

Fixes #29937
Fixes #29978
Fixes #29979

R=danrubel@google.com

Committed: https://github.com/dart-lang/sdk/commit/99d637f5810fe6c5cae58af474c31f35864ebbc6

[ahe, 2017-07-06 15:01:31]

ahe@google.com changed reviewers:
+ danrubel@google.com, johnniwinther@google.com

[danrubel, 2017-07-06 15:43:34]

LGTM with comments for you to address as you see fit.

https://codereview.chromium.org/2968093003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/fasta/summary_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/fasta/summary_builder.dart:374: void
endFunctionExpression(Token beginToken, Token token) {
@override ?

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2501: void
endFunctionExpression(Token beginToken, Token token) {
rename token to endToken? Or is it tokenAfterExpression?

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/listener.dart:259: void
endNamedFunctionExpression(Token endToken) {
dartdoc? indicate substructures?

[ahe, 2017-07-06 19:54:20]

Thank you, Dan!

https://codereview.chromium.org/2968093003/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/fasta/summary_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/fasta/summary_builder.dart:374: void
endFunctionExpression(Token beginToken, Token token) {
On 2017/07/06 15:43:34, danrubel wrote:
> @override ?

Done.

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2501: void
endFunctionExpression(Token beginToken, Token token) {
On 2017/07/06 15:43:34, danrubel wrote:
> rename token to endToken? Or is it tokenAfterExpression?

Now that I think about it, I think I've been rather consistent about not calling
it endToken. It's always "token" for the current position in the token stream,
and if there's more than one token, the other tokens get a name.

I need to think more about this.

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/listener.dart:259: void
endNamedFunctionExpression(Token endToken) {
On 2017/07/06 15:43:34, danrubel wrote:
> dartdoc? indicate substructures?

Done.

[danrubel, 2017-07-06 19:58:42]

LGTM

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2501: void
endFunctionExpression(Token beginToken, Token token) {
On 2017/07/06 19:54:18, ahe wrote:
> On 2017/07/06 15:43:34, danrubel wrote:
> > rename token to endToken? Or is it tokenAfterExpression?
> 
> Now that I think about it, I think I've been rather consistent about not
calling
> it endToken. It's always "token" for the current position in the token stream,
> and if there's more than one token, the other tokens get a name.

Naming the current token 'token' sounds good to me.

[Paul Berry, 2017-07-06 20:06:56]

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

[Paul Berry, 2017-07-06 20:06:57]

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2501: void
endFunctionExpression(Token beginToken, Token token) {
On 2017/07/06 19:58:42, danrubel wrote:
> On 2017/07/06 19:54:18, ahe wrote:
> > On 2017/07/06 15:43:34, danrubel wrote:
> > > rename token to endToken? Or is it tokenAfterExpression?
> > 
> > Now that I think about it, I think I've been rather consistent about not
> calling
> > it endToken. It's always "token" for the current position in the token
stream,
> > and if there's more than one token, the other tokens get a name.
> 
> Naming the current token 'token' sounds good to me.

Not a blocking issue, just food for thought: I have been thinking of the
parser's "current token" as an implementation detail of the parser that
shouldn't be exposed to its clients.  E.g. if the language contains an ambiguous
construct whose interpretation depends on the tokens that follow, the client
shouldn't care whether the parser addresses the ambiguity by peeking ahead, then
calling the callback, then consuming additional tokens, or by consuming enough
tokens to resolve the ambiguity, then calling the callback.  All the client
cares about is what callbacks are called and what their parameters are.

So I'd personally favor using the name "endToken" in all cases.

However, I'm already used to interpreting "token" as "endToken" when writing
parser clients, so I am ok with whatever you decide.

[ahe, 2017-07-06 20:58:53]

Thank you for chiming in, Paul!

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2968093003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2501: void
endFunctionExpression(Token beginToken, Token token) {
On 2017/07/06 20:06:57, Paul Berry wrote:
> On 2017/07/06 19:58:42, danrubel wrote:
> > On 2017/07/06 19:54:18, ahe wrote:
> > > On 2017/07/06 15:43:34, danrubel wrote:
> > > > rename token to endToken? Or is it tokenAfterExpression?
> > > 
> > > Now that I think about it, I think I've been rather consistent about not
> > calling
> > > it endToken. It's always "token" for the current position in the token
> stream,
> > > and if there's more than one token, the other tokens get a name.
> > 
> > Naming the current token 'token' sounds good to me.
> 
> Not a blocking issue, just food for thought: I have been thinking of the
> parser's "current token" as an implementation detail of the parser that
> shouldn't be exposed to its clients.  E.g. if the language contains an
ambiguous
> construct whose interpretation depends on the tokens that follow, the client
> shouldn't care whether the parser addresses the ambiguity by peeking ahead,
then
> calling the callback, then consuming additional tokens, or by consuming enough
> tokens to resolve the ambiguity, then calling the callback.  All the client
> cares about is what callbacks are called and what their parameters are.

That's actually what I do for type variables. And a really good point.

> So I'd personally favor using the name "endToken" in all cases.
> 
> However, I'm already used to interpreting "token" as "endToken" when writing
> parser clients, so I am ok with whatever you decide.

But, the slightly more terse "token" is my favorite so far, simply for being
shorter.

[ahe, 2017-07-11 08:36:06]

Description was changed from

==========
Improve parsing of function expressions.

Fixes #29937
Fixes #29978
Fixes #29979
==========

to

==========
Improve parsing of function expressions.

Fixes #29937
Fixes #29978
Fixes #29979

R=danrubel@google.com

Committed:
https://github.com/dart-lang/sdk/commit/99d637f5810fe6c5cae58af474c31f35864ebbc6
==========

[ahe, 2017-07-11 08:36:07]

Committed patchset #5 (id:80001) manually as
99d637f5810fe6c5cae58af474c31f35864ebbc6 (presubmit successful).