Various semantic checks on formal parameters.

Various semantic checks on formal parameters.

R=danrubel@google.com

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

[ahe, 2017-06-08 20:25:37]

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

[ahe, 2017-06-08 20:25:38]

Dan, I know you want to get to know the parser. Take your time with this, and we
can also do a Hangouts and go through the code together.

[ahe, 2017-06-08 20:28:40]

FYI

https://codereview.chromium.org/2926953004/diff/1/pkg/front_end/messages.yaml
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2926953004/diff/1/pkg/front_end/messages.yaml...
pkg/front_end/messages.yaml:412: dart2jsCode: FASTA_IGNORED
This makes dart2js ignore this message. Dart2js already implements all these
semantic checks, but at later phases.

[danrubel, 2017-06-09 17:08:52]

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

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

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/parser.dart:728:
listener.handleNoName(token);
Will nameToken be null in this code path and will that cause problems later on
in this method?

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:525: if (count == 1) {
It feels like these if / else if blocks should be pulled out into a separate
popListOrCountEntries method.

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:528: formals = new
List<FormalParameterBuilder>.from(last);
Will `last` ever have zero entries, or more than count entries, and if so is
that a problem?

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/messages....
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/messages....
pkg/front_end/messages.yaml:416: dart2jsCode: FASTA_IGNORED
If fasta recognizes these errors, then should they be reported by fasta and
removed or not reported by dart2js ?

[ahe, 2017-06-09 18:53:31]

Thank you, Dan!

I think we agree that some of this could be cleaned up.

I'm already getting a lot of ideas, but I'd like to get back to that later.

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

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/parser.dart:728:
listener.handleNoName(token);
On 2017/06/09 17:08:52, danrubel wrote:
> Will nameToken be null in this code path and will that cause problems later on
> in this method?

It is null in this code path, because there's no name. This is due to the new
generalized function type syntax:

    int Function(int)

The second int is a type, not a name. But you can write:

    int Function(int x)

Will that cause problems later on in this method? No, I think we have that
covered, and it is well tested by now. However, this has caused a lot of crashes
in Fasta.

The original code Florian wrote was simpler, but less resilient to errors. I
would like to iterate more on this code.

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:525: if (count == 1) {
On 2017/06/09 17:08:52, danrubel wrote:
> It feels like these if / else if blocks should be pulled out into a separate
> popListOrCountEntries method.

I think this code needs some TLC :-)

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:528: formals = new
List<FormalParameterBuilder>.from(last);
On 2017/06/09 17:08:52, danrubel wrote:
> Will `last` ever have zero entries, or more than count entries, and if so is
> that a problem?

This is a problem with how the parser generates events. When parsing this:

foo(x, [y, z])

The parser will generate events like this:

formal x
formal y
formal z
optional formals 2
formals 2

At the last event "formals 2", the stack will look like this (top of stack is
first):

[FormalParameterBuilder(y), FormalParameterBuilder(z)]
FormalParameterBuilder(x)

In this case:

foo(x, y)

It will generate these events:

formal x
formal y
formals 2

And the stack will look like this:

FormalParameterBuilder(y)
FormalParameterBuilder(x)

The parser isn't providing any information to distinguish these two situations,
so I have to look at the top of the stack (last).

The actual number of formal parameters is precisely:

last is List ? last.length + count - 1 : count

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/messages....
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2926953004/diff/20001/pkg/front_end/messages....
pkg/front_end/messages.yaml:416: dart2jsCode: FASTA_IGNORED
On 2017/06/09 17:08:52, danrubel wrote:
> If fasta recognizes these errors, then should they be reported by fasta and
> removed or not reported by dart2js ?

The code "FASTA_IGNORED" doesn't mean that Fasta ignores the error.

What is dart2jsCode for? It's a code that we look up in MessageKind in
pkg/compiler/lib/src/diagnostics/messages.dart, and this way dart2js can
continue reporting the same errors that it has always done.

But sometimes we want to send a special signal to dart2js, for example, "please
ignore this error" (because it's already implemented in a different location in
dart2js).

But we have to be careful that what we write in this field so it doesn't
conflict with anything in MessageKind. So I figured, let's prefix all the
special codes with FASTA_.

I think this would also work, and be less confusing:

  dart2jsCode: "*ignored*"

and:

  dart2jsCode: "*fatal*"

WDYT?

So why not modify dart2js so that it doesn't double report errors that the
parser has already found? Well, the less changes I have to make to that part of
dart2js, the faster I can make progress on Fasta which should eventually replace
those parts of dart2js anyways.

Why not implement the check in BodyBuilder? Well, then I would also have to
implement it in AstBuilder. The more errors I can detect in the parser, the less
duplication between BodyBuilder and AstBuilder. This was early feedback from
Siggi and Paul: perhaps we should catch more errors in the parser.

[ahe, 2017-06-10 08:10:12]

Description was changed from

==========
Various semantic checks on formal parameters.
==========

to

==========
Various semantic checks on formal parameters.

R=danrubel@google.com

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

[ahe, 2017-06-10 08:10:14]

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