improve fasta closing brace recovery

improve fasta closing brace recovery

Currently, fasta synthetically closes open braces until it finds
a match for the current closing brace. This works most of the time,
but provides less than optimal recovery in some common cases.
For example, given the following

class { foo()){print(a);} var a = 'hello'; }

the current brace recovery method fails to find any match for the
second closing parenthesis and synthetically closes the class
causing the rest of the file to be parsed as outside the class.

With this CL, fasta still synthetically closes open braces when it finds
a match for the current closing brace, but if there is no match, then
it just skips over the extra closer and continues. This approach dramatically
improves recovery in many cases where there is an extra closing parenthesis
or extra closing square bracket. In the example above, fasta parses
everything after the second closing parenthesis inside the class.

R=paulberry@google.com

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

[danrubel, 2017-07-20 02:30:29]

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

[Paul Berry, 2017-07-20 16:01:02]

lgtm assuming the while loop is changed

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

https://codereview.chromium.org/2981343002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/array_based_scanner.dart:148:
appendPrecedenceToken(type);
Not a blocking issue for this CL, but would it perhaps be better for error
recovery to just skip the unmatched end token?  That way the token stream passed
to the parser would always have matched openers/closers regardless of user
input.

https://codereview.chromium.org/2981343002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/array_based_scanner.dart:209: while
(originalStack != groupingStack) {
Link.operator== does a deep equality comparison, which is unnecessary here. 
Use:

while (!identical(originalStack, groupingStack) {

[danrubel, 2017-07-21 14:23:51]

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

https://codereview.chromium.org/2981343002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/array_based_scanner.dart:148:
appendPrecedenceToken(type);
On 2017/07/20 16:01:02, Paul Berry wrote:
> Not a blocking issue for this CL, but would it perhaps be better for error
> recovery to just skip the unmatched end token?  That way the token stream
passed
> to the parser would always have matched openers/closers regardless of user
> input.

I have this (maybe unfounded) fear that analysis, and more specifically code
completion, might do really strange things if the token stream does not contain
all of the original text.

For example, given the following where "^" is the cursor location...

String foo = 'hello';
main() { foo))))).^ }

... then I fear that code completion would offer code completions at the cursor
location when I'm not sure that it should.

That said, dropping the unmatched end tokens would make parsing easier and is
definitely something to discuss.

https://codereview.chromium.org/2981343002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/array_based_scanner.dart:209: while
(originalStack != groupingStack) {
On 2017/07/20 16:01:02, Paul Berry wrote:
> Link.operator== does a deep equality comparison, which is unnecessary here. 
> Use:
> 
> while (!identical(originalStack, groupingStack) {

Good catch! Fixed.

[Paul Berry, 2017-07-21 17:13:45]

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

https://codereview.chromium.org/2981343002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/array_based_scanner.dart:148:
appendPrecedenceToken(type);
On 2017/07/21 14:23:50, danrubel wrote:
> On 2017/07/20 16:01:02, Paul Berry wrote:
> > Not a blocking issue for this CL, but would it perhaps be better for error
> > recovery to just skip the unmatched end token?  That way the token stream
> passed
> > to the parser would always have matched openers/closers regardless of user
> > input.
> 
> I have this (maybe unfounded) fear that analysis, and more specifically code
> completion, might do really strange things if the token stream does not
contain
> all of the original text.
> 
> For example, given the following where "^" is the cursor location...
> 
> String foo = 'hello';
> main() { foo))))).^ }
> 
> ... then I fear that code completion would offer code completions at the
cursor
> location when I'm not sure that it should.

That's a good point.  I hadn't thought of that.

> 
> That said, dropping the unmatched end tokens would make parsing easier and is
> definitely something to discuss.

I'm currently leaning toward the idea we discussed this morning (insert a
synthetic opener so that balanced openers/closers are always delivered to the
parser).  But of course that's an issue for another CL, and I'm open to changing
my opinion based on what you find out :)

[danrubel, 2017-07-26 14:37:26]

Description was changed from

==========
improve fasta closing brace recovery

Currently, fasta synthetically closes open braces until it finds
a match for the current closing brace. This works most of the time,
but provides less than optimal recovery in some common cases.
For example, given the following

class { foo()){print(a);} var a = 'hello'; }

the current brace recovery method fails to find any match for the
second closing parenthesis and synthetically closes the class
causing the rest of the file to be parsed as outside the class.

With this CL, fasta still synthetically closes open braces when it finds
a match for the current closing brace, but if there is no match, then
it just skips over the extra closer and continues. This approach dramatically
improves recovery in many cases where there is an extra closing parenthesis
or extra closing square bracket. In the example above, fasta parses
everything after the second closing parenthesis inside the class.
==========

to

==========
improve fasta closing brace recovery

Currently, fasta synthetically closes open braces until it finds
a match for the current closing brace. This works most of the time,
but provides less than optimal recovery in some common cases.
For example, given the following

class { foo()){print(a);} var a = 'hello'; }

the current brace recovery method fails to find any match for the
second closing parenthesis and synthetically closes the class
causing the rest of the file to be parsed as outside the class.

With this CL, fasta still synthetically closes open braces when it finds
a match for the current closing brace, but if there is no match, then
it just skips over the extra closer and continues. This approach dramatically
improves recovery in many cases where there is an extra closing parenthesis
or extra closing square bracket. In the example above, fasta parses
everything after the second closing parenthesis inside the class.

R=paulberry@google.com

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

[danrubel, 2017-07-26 14:37:28]

Committed patchset #4 (id:60001) manually as
0b93c279e7a2d81ef9945e1d2bfb976638211a8e (presubmit successful).