gracefully recover from extraneous top level modifiers

pkg/front_end/lib/src/fasta/parser/parser.dart

Index: pkg/front_end/lib/src/fasta/parser/parser.dart
diff --git a/pkg/front_end/lib/src/fasta/parser/parser.dart b/pkg/front_end/lib/src/fasta/parser/parser.dart
index f903a8b597cdd4b9bd9acff7d4c602147693a562..40ff7c60b353d11fa189e38bbefa8e185aab2f94 100644
--- a/pkg/front_end/lib/src/fasta/parser/parser.dart
+++ b/pkg/front_end/lib/src/fasta/parser/parser.dart
@@ -270,22 +270,60 @@ class Parser {
       return parseScript(token);
     }
     token = parseMetadataStar(token);
-    String value = token.stringValue;
-    if (identical(value, 'abstract')) {
-      if (optional('class', token.next)) {
-        return parseClassOrNamedMixinApplication(token);
-      }
-      reportRecoverableErrorWithToken(token, fasta.templateExtraneousModifier);
+    if (token.isTopLevelKeyword) {
+      return parseTopLevelKeywordDeclaration(null, token);
+    }
+    Token start = token;
+    Link<Token> modifiers = const Link<Token>();

[ahe, 2017/09/01 14:42:52]

Perhaps we can take advantage of the next pointer in the tokens themselves? That
is, not create a Link?

[danrubel, 2017/09/01 15:13:55]

I was following what you do in parseTopLevelMember so that we would not check
modifier tokens more than once. That said, I like your idea about using the next
token pointer rather than creating a separate Link object. I'll take another
crack at it.

+    while (token.isModifier) {
+      modifiers = modifiers.prepend(token);
       token = token.next;
-      value = token.stringValue;
     }
+    if (token.isTopLevelKeyword) {
+      Token abstractToken;
+      while (modifiers.isNotEmpty) {
+        Token modifierToken = modifiers.head;
+        if (optional('abstract', modifierToken) &&
+            optional('class', token) &&
+            abstractToken == null) {
+          abstractToken = modifierToken;
+        } else {
+          // TODO(danrubel): Report more specific recoverable error message
+          // for `const class` to better help the user.
+          // See ParserErrorCode CONST_CLASS
+
+          // Report an error for each extraneous modifier
+          reportRecoverableErrorWithToken(
+              modifierToken, fasta.templateExtraneousModifier);
+        }
+        modifiers = modifiers.tail;
+      }
+      return parseTopLevelKeywordDeclaration(abstractToken, token);
+    } else if (token.isIdentifier || token.keyword != null) {
+      return parseTopLevelMember(token, start, modifiers);
+    } else if (modifiers.isNotEmpty && modifiers.head.type.isBuiltIn) {
+      // Handle the edge case where a modifier is being used as an identifier
+      return parseTopLevelMember(start);
+    }
+    // Ignore any preceding modifiers and just report the unexpected token
+    reportRecoverableErrorWithToken(token, fasta.templateExpectedDeclaration);
+    listener.handleInvalidTopLevelDeclaration(token);
+    return token.next;
+  }
+
+  Token parseTopLevelKeywordDeclaration(Token abstractToken, Token token) {
+    final String value = token.stringValue;
     if (identical(value, 'class')) {
-      return parseClassOrNamedMixinApplication(token);
+      return parseClassOrNamedMixinApplication(abstractToken, token);
     } else if (identical(value, 'enum')) {
       return parseEnum(token);
-    } else if (identical(value, 'typedef') &&
-        (token.next.isIdentifier || optional("void", token.next))) {
-      return parseTypedef(token);
+    } else if (identical(value, 'typedef')) {
+      Token next = token.next;
+      if (next.isIdentifier || optional("void", next)) {
+        return parseTypedef(token);
+      } else {
+        return parseTopLevelMember(token);
+      }
     } else if (identical(value, 'library')) {
       return parseLibraryName(token);
     } else if (identical(value, 'import')) {
@@ -294,13 +332,9 @@ class Parser {
       return parseExport(token);
     } else if (identical(value, 'part')) {
       return parsePartOrPartOf(token);
-    } else if (token.type == TokenType.IDENTIFIER || token.keyword != null) {
-      return parseTopLevelMember(token);
-    } else {
-      reportRecoverableErrorWithToken(token, fasta.templateExpectedDeclaration);
-      listener.handleInvalidTopLevelDeclaration(token);
-      return token.next;
     }
+
+    throw "Internal error: Unhandled top level keyword '$value'.";
   }
 
   /// library qualified ';'
@@ -827,11 +861,11 @@ class Parser {
     return token;
   }
 
-  Token parseClassOrNamedMixinApplication(Token token) {
+  Token parseClassOrNamedMixinApplication(Token abstractToken, Token token) {
     listener.beginClassOrNamedMixinApplication(token);
-    Token begin = token;
-    if (optional('abstract', token)) {
-      token = parseModifier(token);
+    Token begin = abstractToken ?? token;
+    if (abstractToken != null) {
+      token = parseModifier(abstractToken);
       listener.handleModifiers(1);
     } else {
       listener.handleModifiers(0);
@@ -1575,11 +1609,12 @@ class Parser {
     return token;
   }
 
-  Token parseTopLevelMember(Token token) {
-    Token start = token;
+  Token parseTopLevelMember(Token token,
+      [Token start, Link<Token> identifiers = const Link<Token>()]) {

[ahe, 2017/09/01 14:42:52]

I try to avoid optional arguments in the parser, as I'm not sure they have great
performance.

+    start ??= token;
     listener.beginTopLevelMember(token);
 
-    Link<Token> identifiers = findMemberName(token);
+    identifiers = findMemberName(token, identifiers);
     if (identifiers.isEmpty) {
       return reportUnrecoverableErrorWithToken(
               start, fasta.templateExpectedDeclaration)
@@ -1777,7 +1812,8 @@ class Parser {
   /// results in
   ///     ['(', '*', 'operator']
   ///
-  Link<Token> findMemberName(Token token) {
+  Link<Token> findMemberName(Token token,
+      [Link<Token> identifiers = const Link<Token>()]) {
     // TODO(ahe): This method is rather broken for examples like this:
     //
     //     get<T>(){}
@@ -1785,7 +1821,6 @@ class Parser {
     // In addition, the loop below will include things that can't be
     // identifiers. This may be desirable (for error recovery), or
     // not. Regardless, this method probably needs an overhaul.
-    Link<Token> identifiers = const Link<Token>();
 
     // `true` if 'get' has been seen.
     bool isGetter = false;