add handleInvalidMember to improve recovery

* add handleInvalidMember event
* update NodeListener to track invalidMemberCount
* update ClassMethodModifierContext to trigger handleInvalidMember
  event, reducing number of superflous errors.

Also, address comment
https://dart-review.googlesource.com/c/sdk/+/16040/1/pkg/analyzer/lib/src/fasta/ast_builder.dart#1580
Change-Id: I4c630faeee7731e2f85bbfaf0a6790bf6f4382b3
Reviewed-on: https://dart-review.googlesource.com/16100
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Dan Rubel <danrubel@google.com>

pkg/analyzer/lib/src/fasta/ast_builder.dart

@@ -1585,11 +1585,6 @@ class AstBuilder extends ScopeListener {
   @override
   void beginClassDeclaration(Token beginToken, Token name) {
     assert(classDeclaration == null);
-    // TODO(brianwilkerson): Enable this assert.
-    // Parser.parseClassOrNamedMixinApplication will pass in a non-identifier if
-    // `class` is at the end of the file.
-    // (See partial_code class_declaration keyword_eof)
-//    assert(name.isIdentifier);
   }
 
   @override
@@ -2073,6 +2068,12 @@ class AstBuilder extends ScopeListener {
     }
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    debugEvent("InvalidMember");
+    pop(); // metadata star
+  }
+
   @override
   void endMember() {
     debugEvent("Member");

pkg/analyzer/test/generated/parser_fasta_listener.dart

@@ -1118,6 +1118,12 @@ class ForwardingTestListener extends ForwardingListener {
     listener.handleRecoverImport(semicolon);
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    expectIn('Member');
+    super.handleInvalidMember(endToken);
+  }
+
   @override
   void handleInvalidTopLevelDeclaration(Token endToken) {
     expectIn('CompilationUnit');

pkg/analyzer/test/generated/parser_test.dart

@@ -2320,25 +2320,12 @@ abstract class ErrorParserTestMixin implements AbstractParserTestCase {
 
   void test_classInClass_abstract() {
     parseCompilationUnit(
-        "class C { abstract class B {} }",
-        usingFastaParser
-            ? [
-                ParserErrorCode.ABSTRACT_CLASS_MEMBER,
-                ParserErrorCode.CLASS_IN_CLASS,
-                ParserErrorCode.MISSING_FUNCTION_PARAMETERS,
-              ]
-            : [ParserErrorCode.CLASS_IN_CLASS]);
+        "class C { abstract class B {} }", [ParserErrorCode.CLASS_IN_CLASS]);
   }
 
   void test_classInClass_nonAbstract() {
     parseCompilationUnit(
-        "class C { class B {} }",
-        usingFastaParser
-            ? [
-                ParserErrorCode.MISSING_FUNCTION_PARAMETERS,
-                ParserErrorCode.CLASS_IN_CLASS,
-              ]
-            : [ParserErrorCode.CLASS_IN_CLASS]);
+        "class C { class B {} }", [ParserErrorCode.CLASS_IN_CLASS]);
   }
 
   void test_classTypeAlias_abstractAfterEq() {
@@ -2654,29 +2641,13 @@ abstract class ErrorParserTestMixin implements AbstractParserTestCase {
   }
 
   void test_enumInClass() {
-    parseCompilationUnit(
-        r'''
+    parseCompilationUnit(r'''
 class Foo {
   enum Bar {
     Bar1, Bar2, Bar3
   }
 }
-''',
-        usingFastaParser
-            ? [
-                ParserErrorCode.ENUM_IN_CLASS,
-                ParserErrorCode.MISSING_IDENTIFIER,
-                ParserErrorCode.MISSING_IDENTIFIER,
-                ParserErrorCode.MISSING_FUNCTION_PARAMETERS,
-                ParserErrorCode.UNEXPECTED_TOKEN,
-                ParserErrorCode.UNEXPECTED_TOKEN,
-                ParserErrorCode.EXPECTED_TOKEN,
-                ParserErrorCode.EXPECTED_TOKEN,
-                ParserErrorCode.EXPECTED_TOKEN,
-                ParserErrorCode.EXPECTED_TOKEN,
-                ParserErrorCode.EXPECTED_TOKEN
-              ]
-            : [ParserErrorCode.ENUM_IN_CLASS]);
+''', [ParserErrorCode.ENUM_IN_CLASS]);
   }
 
   void test_equalityCannotBeEqualityOperand_eq_eq() {

pkg/analyzer/tool/summary/mini_ast.dart

@@ -320,6 +320,12 @@ class MiniAstBuilder extends StackListener {
     }
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    debugEvent("InvalidMember");
+    pop(); // metadata star
+  }
+
   @override
   void endMember() {
     debugEvent("Member");

pkg/compiler/lib/src/parser/node_listener.dart

@@ -19,6 +19,7 @@ import 'package:front_end/src/fasta/parser.dart' as fasta show Assert;
 
 class NodeListener extends ElementListener {
   int invalidTopLevelDeclarationCount = 0;
+  int invalidMemberCount = 0;
 
   NodeListener(ScannerOptions scannerOptions, DiagnosticReporter reporter,
       CompilationUnitElement element)
@@ -231,7 +232,9 @@ class NodeListener extends ElementListener {
 
   @override
   void endClassBody(int memberCount, Token beginToken, Token endToken) {
-    pushNode(makeNodeList(memberCount, beginToken, endToken, null));
+    pushNode(makeNodeList(
+        memberCount - invalidMemberCount, beginToken, endToken, null));
+    invalidMemberCount = 0;
   }
 
   @override
@@ -716,6 +719,12 @@ class NodeListener extends ElementListener {
     pushNode(null);
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    popNode(); // Discard metadata
+    ++invalidMemberCount;
+  }
+
   @override
   void endMember() {
     // TODO(sigmund): consider moving metadata into each declaration

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

@@ -1012,6 +1012,11 @@ class ForwardingListener implements Listener {
     listener?.handleInvalidFunctionBody(token);
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    listener?.handleInvalidMember(endToken);
+  }
+
   @override
   void handleInvalidTypeReference(Token token) {
     listener?.handleInvalidTypeReference(token);

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

@@ -615,6 +615,12 @@ class Listener {
 
   void beginMember(Token token) {}
 
+  /// Handle an invalid member declaration. Substructures:
+  /// - metadata
+  void handleInvalidMember(Token endToken) {
+    logEvent("InvalidMember");
+  }
+
   /// This event is added for convenience. Normally, one should override
   /// [endMethod] or [endFields] instead.
   void endMember() {

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

@@ -399,6 +399,11 @@ class ClassMethodModifierContext {
   Token externalToken;
   Token staticToken;
 
+  /// If recovery finds an invalid class member declaration
+  /// (e.g. an enum declared inside a class),
+  /// then this is set to the last token in the invalid declaration.
+  Token endInvalidMemberToken;
+
   ClassMethodModifierContext(this.parser);
 
   Token parseRecovery(Token token, Token externalToken, Token staticToken,
@@ -416,35 +421,75 @@ class ClassMethodModifierContext {
     while (token != afterModifiers) {
       String value = token.stringValue;
       if (identical(value, 'abstract')) {
-        parser.reportRecoverableError(token, fasta.messageAbstractClassMember);
+        token = parseAbstractRecovery(token);
       } else if (identical(value, 'class')) {
-        parser.reportRecoverableError(token, fasta.messageClassInClass);
-      } else if (identical(value, 'enum')) {
-        parser.reportRecoverableError(token, fasta.messageEnumInClass);
+        token = parseClassRecovery(token);
       } else if (identical(value, 'const')) {
         parseConstRecovery(token);
+        token = token.next;
       } else if (identical(value, 'covariant')) {
         parseCovariantRecovery(token);
+        token = token.next;
+      } else if (identical(value, 'enum')) {
+        token = parseEnumRecovery(token);
       } else if (identical(value, 'external')) {
         parseExternalRecovery(token);
+        token = token.next;
       } else if (identical(value, 'static')) {
         parseStaticRecovery(token);
+        token = token.next;
       } else if (identical(value, 'typedef')) {
         parser.reportRecoverableError(token, fasta.messageTypedefInClass);
+        token = token.next;
       } else if (identical(value, 'var')) {
         parseVarRecovery(token);
+        token = token.next;
       } else if (token.isModifier) {
         parser.reportRecoverableErrorWithToken(
             token, fasta.templateExtraneousModifier);
+        token = token.next;
       } else {
         parser.reportRecoverableErrorWithToken(
             token, fasta.templateUnexpectedToken);
         // We found something that doesn't look like a modifier,
         // so skip the rest of the tokens.
-        token = afterModifiers;
+        token = afterModifiers.next;
         break;
       }
+      if (endInvalidMemberToken != null) {
+        return afterModifiers;
+      }
+    }
+    return token;
+  }
+
+  Token parseAbstractRecovery(Token token) {
+    assert(optional('abstract', token));
+    if (optional('class', token.next)) {
+      return parseClassRecovery(token.next);
+    }
+    parser.reportRecoverableError(token, fasta.messageAbstractClassMember);
+    return token.next;
+  }
+
+  Token parseClassRecovery(Token token) {
+    assert(optional('class', token));
+    parser.reportRecoverableError(token, fasta.messageClassInClass);
+    token = token.next;
+    // If the declaration appears to be a valid class declaration
+    // then skip the entire declaration so that we only generate the one
+    // error (above) rather than a plethora of unhelpful errors.
+    if (token.isIdentifier) {
+      endInvalidMemberToken = token;
+      // skip class name
       token = token.next;
+      // TODO(danrubel): consider parsing (skipping) the class header
+      // with a recovery listener so that no events are generated
+      if (optional('{', token) && token.endGroup != null) {
+        // skip class body
+        endInvalidMemberToken = token.endGroup;
+        token = endInvalidMemberToken.next;
+      }
     }
     return token;
   }
@@ -480,6 +525,28 @@ class ClassMethodModifierContext {
     }
   }
 
+  Token parseEnumRecovery(Token token) {
+    assert(optional('enum', token));
+    parser.reportRecoverableError(token, fasta.messageEnumInClass);
+    token = token.next;
+    // If the declaration appears to be a valid enum declaration
+    // then skip the entire declaration so that we only generate the one
+    // error (above) rather than a plethora of unhelpful errors.
+    if (token.isIdentifier) {
+      endInvalidMemberToken = token;
+      // skip enum name
+      token = token.next;
+      if (optional('{', token) && token.endGroup != null) {
+        // TODO(danrubel): Consider replacing this `skip enum` functionality
+        // with something that can parse and resolve the declaration
+        // even though it is in a class context
+        endInvalidMemberToken = token.endGroup;
+        token = token.next;
+      }
+    }
+    return token;
+  }
+
   void parseExternalRecovery(Token token) {
     assert(optional('external', token));
     if (externalToken != null) {

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

@@ -3116,7 +3116,6 @@ class Parser {
   Token parseMethod(Token token, Token afterModifiers, Token type,
       Token getOrSet, Token name) {
     Token start = token;
-    listener.beginMethod(start, name);
 
     Token externalModifier;
     Token staticModifier;
@@ -3159,14 +3158,25 @@ class Parser {
             final context = new ClassMethodModifierContext(this);
             token = context.parseRecovery(token, externalModifier,
                 staticModifier, getOrSet, afterModifiers);
+
+            // If the modifiers form a partial top level directive or declaration
+            // and we have found the start of a new top level declaration
+            // then return to parse that new declaration.
+            if (context.endInvalidMemberToken != null) {
+              listener.handleInvalidMember(context.endInvalidMemberToken);
+              return context.endInvalidMemberToken.next;
+            }
+
             externalModifier = context.externalToken;
             staticModifier = context.staticToken;
             modifierCount = context.modifierCount;
           }
         }
       }
+      listener.beginMethod(start, name);
       listener.handleModifiers(modifierCount);
     } else {
+      listener.beginMethod(start, name);
       listener.handleModifiers(0);
     }
 

pkg/front_end/lib/src/fasta/source/diet_listener.dart

@@ -523,6 +523,12 @@ class DietListener extends StackListener {
         token, metadata, isTopLevel);
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    debugEvent("InvalidMember");
+    pop(); // metadata star
+  }
+
   @override
   void endMember() {
     debugEvent("Member");

pkg/front_end/lib/src/fasta/source/outline_builder.dart

@@ -970,6 +970,12 @@ class OutlineBuilder extends UnhandledListener {
     // This is a constructor initializer and it's ignored for now.
   }
 
+  @override
+  void handleInvalidMember(Token endToken) {
+    debugEvent("InvalidMember");
+    pop(); // metadata star
+  }
+
   @override
   void endMember() {
     debugEvent("Member");