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

Review-Url: https://codereview.chromium.org/2981343002 .

pkg/analyzer/lib/src/generated/parser.dart

@@ -6911,6 +6911,12 @@ class Parser {
                 ['}', ']']);
             rightCurlyBracket = rightSquareBracket;
             rightSquareBracket = null;
+            // Skip over synthetic closer inserted by fasta
+            // since we've already reported an error
+            if (_currentToken.type == TokenType.CLOSE_CURLY_BRACKET &&
+                _currentToken.isSynthetic) {
+              _advance();
+            }
           } else {
             _reportErrorForCurrentToken(
                 ParserErrorCode.UNEXPECTED_TERMINATOR_FOR_PARAMETER_GROUP,
@@ -6927,6 +6933,12 @@ class Parser {
                 [']', '}']);
             rightSquareBracket = rightCurlyBracket;
             rightCurlyBracket = null;
+            // Skip over synthetic closer inserted by fasta
+            // since we've already reported an error
+            if (_currentToken.type == TokenType.CLOSE_SQUARE_BRACKET &&
+                _currentToken.isSynthetic) {
+              _advance();
+            }
           } else {
             _reportErrorForCurrentToken(
                 ParserErrorCode.UNEXPECTED_TERMINATOR_FOR_PARAMETER_GROUP,

pkg/analyzer/test/generated/parser_test.dart

@@ -4140,18 +4140,16 @@ main() {
     createParser('(a, b})');
     FormalParameterList list = parser.parseFormalParameterList();
     expectNotNullIfNoErrors(list);
-    listener.assertErrorsWithCodes(fe.Scanner.useFasta
-        ? [ScannerErrorCode.EXPECTED_TOKEN]
-        : [ParserErrorCode.UNEXPECTED_TERMINATOR_FOR_PARAMETER_GROUP]);
+    listener.assertErrorsWithCodes(
+        [ParserErrorCode.UNEXPECTED_TERMINATOR_FOR_PARAMETER_GROUP]);
   }
 
   void test_unexpectedTerminatorForParameterGroup_optional() {
     createParser('(a, b])');
     FormalParameterList list = parser.parseFormalParameterList();
     expectNotNullIfNoErrors(list);
-    listener.assertErrorsWithCodes(fe.Scanner.useFasta
-        ? [ScannerErrorCode.EXPECTED_TOKEN]
-        : [ParserErrorCode.UNEXPECTED_TERMINATOR_FOR_PARAMETER_GROUP]);
+    listener.assertErrorsWithCodes(
+        [ParserErrorCode.UNEXPECTED_TERMINATOR_FOR_PARAMETER_GROUP]);
   }
 
   void test_unexpectedToken_endOfFieldDeclarationStatement() {
@@ -4391,7 +4389,11 @@ void main() {
     FormalParameterList list = parser.parseFormalParameterList();
     expectNotNullIfNoErrors(list);
     listener.assertErrorsWithCodes(fe.Scanner.useFasta
-        ? [ScannerErrorCode.EXPECTED_TOKEN, ScannerErrorCode.EXPECTED_TOKEN]
+        ? [
+            // fasta scanner generates '(a, {b, c]})' where '}' is synthetic
+            ParserErrorCode.WRONG_TERMINATOR_FOR_PARAMETER_GROUP,
+            ScannerErrorCode.EXPECTED_TOKEN,
+          ]
         : [ParserErrorCode.WRONG_TERMINATOR_FOR_PARAMETER_GROUP]);
   }
 
@@ -4399,8 +4401,12 @@ void main() {
     createParser('(a, [b, c})');
     FormalParameterList list = parser.parseFormalParameterList();
     expectNotNullIfNoErrors(list);
-    listener.assertErrorsWithCodes(fe.Scanner.useFasta
-        ? [ScannerErrorCode.EXPECTED_TOKEN, ScannerErrorCode.EXPECTED_TOKEN]
+    listener.assertErrorsWithCodes(usingFastaScanner
+        ? [
+            // fasta scanner generates '(a, [b, c}])' where ']' is synthetic
+            ParserErrorCode.WRONG_TERMINATOR_FOR_PARAMETER_GROUP,
+            ScannerErrorCode.EXPECTED_TOKEN,
+          ]
         : [ParserErrorCode.WRONG_TERMINATOR_FOR_PARAMETER_GROUP]);
   }
 }

pkg/front_end/lib/src/fasta/scanner/array_based_scanner.dart

@@ -139,16 +139,17 @@ abstract class ArrayBasedScanner extends AbstractScanner {
   /**
    * Appends a token that begins an end group, represented by [type].
    * It handles the group end tokens '}', ')' and ']'. The tokens '>' and
-   * '>>' are handled separately bo [appendGt] and [appendGtGt].
+   * '>>' are handled separately by [appendGt] and [appendGtGt].
    */
   int appendEndGroup(TokenType type, int openKind) {
     assert(!identical(openKind, LT_TOKEN)); // openKind is < for > and >>
-    discardBeginGroupUntil(openKind);
-    appendPrecedenceToken(type);
-    Token close = tail;
-    if (groupingStack.isEmpty) {
+    if (!discardBeginGroupUntil(openKind)) {
+      // No begin group found. Just continue.
+      appendPrecedenceToken(type);
       return advance();
     }
+    appendPrecedenceToken(type);
+    Token close = tail;
     BeginToken begin = groupingStack.head;
     if (!identical(begin.kind, openKind)) {
       assert(begin.kind == STRING_INTERPOLATION_TOKEN &&
@@ -166,21 +167,52 @@ abstract class ArrayBasedScanner extends AbstractScanner {
   }
 
   /**
-   * Discards begin group tokens until a match with [openKind] is found.
-   * This recovers nicely from from a situation like "{[}".
+   * If a begin group token matches [openKind],
+   * then discard begin group tokens up to that match and return `true`,
+   * otherwise return `false`.
+   * This recovers nicely from from situations like "{[}" and "{foo());}",
+   * but not "foo(() {bar());});
    */
-  void discardBeginGroupUntil(int openKind) {
-    while (!groupingStack.isEmpty) {
+  bool discardBeginGroupUntil(int openKind) {
+    Link<BeginToken> originalStack = groupingStack;
+
+    bool first = true;
+    do {
       // Don't report unmatched errors for <; it is also the less-than operator.
       discardOpenLt();
-      if (groupingStack.isEmpty) return;
+      if (groupingStack.isEmpty) break; // recover
       BeginToken begin = groupingStack.head;
-      if (openKind == begin.kind) return;
-      if (openKind == OPEN_CURLY_BRACKET_TOKEN &&
-          begin.kind == STRING_INTERPOLATION_TOKEN) return;
-      unmatchedBeginGroup(begin);
+      if (openKind == begin.kind ||
+          (openKind == OPEN_CURLY_BRACKET_TOKEN &&
+              begin.kind == STRING_INTERPOLATION_TOKEN)) {
+        if (first) {
+          // If the expected opener has been found on the first pass
+          // then no recovery necessary.
+          return true;
+        }
+        break; // recover
+      }
+      first = false;
       groupingStack = groupingStack.tail;
+    } while (!groupingStack.isEmpty);
+
+    // If the stack does not have any opener of the given type,
+    // then return without discarding anything.
+    // This recovers nicely from from situations like "{foo());}".
+    if (groupingStack.isEmpty) {
+      groupingStack = originalStack;
+      return false;
     }
+
+    // Insert synthetic closers and report errors for any unbalanced openers.
+    // This recovers nicely from from situations like "{[}".
+    while (!identical(originalStack, groupingStack)) {
+      // Don't report unmatched errors for <; it is also the less-than operator.
+      if (!identical(groupingStack.head.kind, LT_TOKEN))
+        unmatchedBeginGroup(originalStack.head);
+      originalStack = originalStack.tail;
+    }
+    return true;
   }
 
   /**

pkg/front_end/test/scanner_test.dart

@@ -779,17 +779,34 @@ abstract class ScannerTestBase {
   }
 
   void test_mismatched_closer() {
-    // When openers and closers are mismatched, analyzer favors considering the
-    // closer to be mismatched, which means that `(])` parses as a pair of
-    // matched parentheses with an unmatched closing bracket between them.
+    // Normally when openers and closers are mismatched
+    // fasta favors considering the opener to be mismatched,
+    // and inserts synthetic closers as needed.
+    // In this particular case, fasta cannot find an opener for ']'
+    // and thus marks ']' as an error and moves on.
     ErrorListener listener = new ErrorListener();
     BeginToken openParen = scanWithListener('(])', listener);
+    var closeBracket = openParen.next;
+    var closeParen = closeBracket.next;
+    expect(closeParen.next.type, TokenType.EOF);
+    expect(openParen.endToken, same(closeParen));
+    listener.assertNoErrors();
+  }
+
+  void test_mismatched_closer2() {
+    // When openers and closers are mismatched, analyzer favors considering the
+    // closer to be mismatched, which means that `[(])` parses as a pair of
+    // matched parenthesis with an unmatched open bracket before them
+    // and an unmatched closing bracket between them.
+    ErrorListener listener = new ErrorListener();
+    BeginToken openBracket = scanWithListener('[(])', listener);
+    BeginToken openParen = openBracket.next;
     if (usingFasta) {
-      // When openers and closers are mismatched,
+      // When openers and closers are mismatched
       // fasta favors considering the opener to be mismatched,
       // and inserts synthetic closers as needed.
-      // `(])` is parsed as `()])` where the first `)` is synthetic
-      // and the trailing `])` are unmatched.
+      // `[(])` is parsed as `[()])` where the first `)` is synthetic
+      // and the trailing `)` is unmatched.
       var closeParen = openParen.next;
       expect(closeParen.isSynthetic, isTrue);
       var closeBracket = closeParen.next;
@@ -797,14 +814,16 @@ abstract class ScannerTestBase {
       var closeParen2 = closeBracket.next;
       expect(closeParen2.isSynthetic, isFalse);
       expect(closeParen2.next.type, TokenType.EOF);
+      expect(openBracket.endToken, same(closeBracket));
       expect(openParen.endToken, same(closeParen));
       listener.assertErrors([
-        new TestError(0, ScannerErrorCode.EXPECTED_TOKEN, [')']),
+        new TestError(1, ScannerErrorCode.EXPECTED_TOKEN, [')']),
       ]);
     } else {
       var closeBracket = openParen.next;
       var closeParen = closeBracket.next;
       expect(closeParen.next.type, TokenType.EOF);
+      expect(openBracket.endToken, isNull);
       expect(openParen.endToken, same(closeParen));
       listener.assertNoErrors();
     }

tests/compiler/dart2js/error_token_test.dart

@@ -12,7 +12,8 @@ import "package:compiler/src/diagnostics/messages.dart";
 import 'package:expect/expect.dart';
 import 'memory_compiler.dart';
 
-Future runTest(String code, {MessageKind error}) async {
+Future runTest(String code,
+    {MessageKind error, int expectedWarningCount: 0}) async {
   DiagnosticCollector diagnostics = new DiagnosticCollector();
   OutputCollector output = new OutputCollector();
   await runCompiler(
@@ -22,10 +23,9 @@ Future runTest(String code, {MessageKind error}) async {
       outputProvider: output);
 
   Expect.equals(error != null ? 1 : 0, diagnostics.errors.length);
-  if (error != null) {
+  if (error != null)
     Expect.equals(error, diagnostics.errors.first.message.kind);
-  }
-  Expect.equals(0, diagnostics.warnings.length);
+  Expect.equals(expectedWarningCount, diagnostics.warnings.length);
   Expect.equals(0, diagnostics.hints.length);
   Expect.equals(0, diagnostics.infos.length);
 }
@@ -34,23 +34,25 @@ void main() {
   asyncTest(() async {
     await runTest(
         '''
-main() {}
+main() {Foo.bar();}
 class Foo {
 	static void bar() {
 		baz());
 	}
 }
 ''',
-        error: MessageKind.UNMATCHED_TOKEN);
+        error: MessageKind.MISSING_TOKEN_AFTER_THIS,
+        expectedWarningCount: 1);
 
     await runTest(
         '''
-main() {}
+main() {new C(v);}
 class C {
   C(v) {
     throw '');
   }
 }''',
-        error: MessageKind.UNMATCHED_TOKEN);
+        error: MessageKind.MISSING_TOKEN_AFTER_THIS,
+        expectedWarningCount: 1);
   });
 }