new handleInvalidTopLevelDeclaration event in fasta parser

new handleInvalidTopLevelDeclaration event in fasta parser

This adds a new event for better fasta parser recovery
when the parser detects an invalid character at the top level.

R=ahe@google.com

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

[danrubel, 2017-07-21 18:38:25]

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

[Paul Berry, 2017-08-03 18:01:10]

https://codereview.chromium.org/2985703002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):

https://codereview.chromium.org/2985703002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_fasta_listener.dart:34: if (!_stack.isEmpty
&& _stack.last != event) {
This seems sloppy.  If we have to do a check like this, it means there are some
situations where endTopLevelDeclaration (or handleInvalidTopLevelDeclaration)
might occur inside a "compilation unit", and some situations where it might
occur outside of a "compilation unit".  Which means that in order to do proper
error recovery, all clients of the parser are going to have to be able to handle
both situations.  That seems dangerous because there are a lot of parser
clients.

What are the circumstances in which the stack will be empty vs. have a
compilation unit at the top?  Is it possible to change things so that all inputs
are handled the same way, so that there will either always be a "compilation
unit" at the top of the stack, or the stack will always be empty?  If we can do
that it seems like it would make it easier to write correct clients.

[danrubel, 2017-08-07 12:34:28]

Comment addressed. PTAL.

https://codereview.chromium.org/2985703002/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):

https://codereview.chromium.org/2985703002/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/parser_fasta_listener.dart:34: if (!_stack.isEmpty
&& _stack.last != event) {
On 2017/08/03 18:01:09, Paul Berry wrote:
> This seems sloppy.  If we have to do a check like this, it means there are
some
> situations where endTopLevelDeclaration (or handleInvalidTopLevelDeclaration)
> might occur inside a "compilation unit", and some situations where it might
> occur outside of a "compilation unit".  Which means that in order to do proper
> error recovery, all clients of the parser are going to have to be able to
handle
> both situations.  That seems dangerous because there are a lot of parser
> clients.
> 
> What are the circumstances in which the stack will be empty vs. have a
> compilation unit at the top?  Is it possible to change things so that all
inputs
> are handled the same way, so that there will either always be a "compilation
> unit" at the top of the stack, or the stack will always be empty?  If we can
do
> that it seems like it would make it easier to write correct clients.

The tests call into the parser at different levels. If all tests called
parseUnit, then I could use the `_in` method everywhere and not need
`_inOrEmpty`. For example, when the tests call parseUnit, then the
`endTopLevelDeclaration` event below could call `_in('CompilationUnit')`, but
when parseTopLevelDeclaration is called, then the stack should be empty. Any
parser client that calls `parseUnit` should not have to worry about this, but
those that call into the parser using a different "parse" method will have to be
cognizant of the higher level events not being generated.

I've updated the tests to specify when there is an "enclosing event" that is
expected but not generated because a lower level parse method was called. This
change eliminates the need for this method.

[ahe, 2017-08-07 13:31:00]

I don't like adding state to the parser. I think you don't need to do so if you
think of it this way: an invalid top-level declaration also counts as a
top-level declaration.

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (left):

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:409: void
endMetadataStar(int count, bool forParameter) {
Why remove this method?

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

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/parser.dart:235: int
_topLevelDeclarationCount = 0;
Why is this a field now?

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

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/unhandled_listener.dart:39: void
handleInvalidTopLevelDeclaration(Token beginToken, Token endToken) {
We're supposed to get rid of this class, so I don't think you should add new
features here.

Features that are shared between stack-based listeners belong in
stack_listener.dart.

[danrubel, 2017-08-07 15:06:53]

On 2017/08/03 18:01:10, Paul Berry wrote:
>
https://codereview.chromium.org/2985703002/diff/1/pkg/analyzer/test/generated...
> File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):
> 
>
https://codereview.chromium.org/2985703002/diff/1/pkg/analyzer/test/generated...
> pkg/analyzer/test/generated/parser_fasta_listener.dart:34: if (!_stack.isEmpty
> && _stack.last != event) {
> 
> What are the circumstances in which the stack will be empty vs. have a
> compilation unit at the top?

I've extracted the listener enclosing event assertion from this CL into
https://codereview.chromium.org/2993833002/

[Paul Berry, 2017-08-07 15:36:17]

Ok, since my concern is now addressed by a different CL, I'll carry on the
conversation about it there, and I'll defer to Peter's opinion about this CL.

[danrubel, 2017-08-14 16:43:12]

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (left):

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:409: void
endMetadataStar(int count, bool forParameter) {
On 2017/08/07 13:31:00, ahe wrote:
> Why remove this method?

The superclass has an identical method, so I removed it here.

Based on your suggestion and comments in the superclass, I will push the method
out of the superclass and into the various subclasses.
See https://codereview.chromium.org/2998933002/

[danrubel, 2017-08-15 20:13:33]

Comments addressed. Code refactored per discussion. PTAL

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

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/parser.dart:235: int
_topLevelDeclarationCount = 0;
On 2017/08/07 13:31:00, ahe wrote:
> Why is this a field now?

Per our discussion, I've reverted this so that invalid top level declarations
are included in the count. Each listener will have to decide how invalid
declarations are to be handled.

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

https://codereview.chromium.org/2985703002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/unhandled_listener.dart:39: void
handleInvalidTopLevelDeclaration(Token beginToken, Token endToken) {
On 2017/08/07 13:31:00, ahe wrote:
> We're supposed to get rid of this class, so I don't think you should add new
> features here.
> 
> Features that are shared between stack-based listeners belong in
> stack_listener.dart.

Yeah. The class name and the class dartdoc should have given me a clue.
Refactored to be handled elsewhere.

[ahe, 2017-08-16 13:22:45]

lgtm

https://codereview.chromium.org/2985703002/diff/60001/pkg/analyzer/lib/src/fa...
File pkg/analyzer/lib/src/fasta/ast_builder.dart (right):

https://codereview.chromium.org/2985703002/diff/60001/pkg/analyzer/lib/src/fa...
pkg/analyzer/lib/src/fasta/ast_builder.dart:1160: // representing the invalid
declaration to better support code completion,
Long line! P0!!! ;-)

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

https://codereview.chromium.org/2985703002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/listener.dart:718: /// and is followed by
[endTopLevelDeclaration].
You got this documentation stuff pretty well under control. Nice!

[danrubel, 2017-08-16 13:34:30]

https://codereview.chromium.org/2985703002/diff/60001/pkg/analyzer/lib/src/fa...
File pkg/analyzer/lib/src/fasta/ast_builder.dart (right):

https://codereview.chromium.org/2985703002/diff/60001/pkg/analyzer/lib/src/fa...
pkg/analyzer/lib/src/fasta/ast_builder.dart:1160: // representing the invalid
declaration to better support code completion,
On 2017/08/16 13:22:45, ahe wrote:
> Long line! P0!!! ;-)

Fixed. :-)

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

https://codereview.chromium.org/2985703002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/parser/listener.dart:718: /// and is followed by
[endTopLevelDeclaration].
On 2017/08/16 13:22:45, ahe wrote:
> You got this documentation stuff pretty well under control. Nice!

:-)

[danrubel, 2017-08-16 13:36:29]

Description was changed from

==========
new handleInvalidTopLevelDeclaration event in fasta parser

This adds a new event for better fasta parser recovery
when the parser detects an invalid character at the top level.
==========

to

==========
new handleInvalidTopLevelDeclaration event in fasta parser

This adds a new event for better fasta parser recovery
when the parser detects an invalid character at the top level.

R=ahe@google.com

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

[danrubel, 2017-08-16 13:36:31]

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