Parser: Make error recovery more conservative.

Reduce the number of error message after a parse error.
Recover only when we have confidence we are in good state.

--
MOS_MIGRATED_REVID=90147322

2 files changed, 46 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
index 297aa13922..7694ab70d3 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
@@ -250,8 +250,10 @@ class Parser {
     }
   }
 
-  // Consumes the current token.  If it is not of the specified (expected)
-  // kind, reports a syntax error.
+  /**
+   * Consumes the current token. If it is not of the specified (expected)
+   * kind, reports a syntax error.
+   */
   private boolean expect(TokenKind kind) {
     boolean expected = token.kind == kind;
     if (!expected) {
@@ -262,6 +264,15 @@ class Parser {
   }
 
   /**
+   * Same as expect, but stop the recovery mode if the token was expected.
+   */
+  private void expectAndRecover(TokenKind kind) {
+    if (expect(kind)) {
+      recoveryMode = false;
+    }
+  }
+
+  /**
    * Consume tokens past the first token that has a kind that is in the set of
    * teminatingTokens.
    * @param terminatingTokens
@@ -979,7 +990,12 @@ class Parser {
     List<Statement> list =  new ArrayList<>();
     while (token.kind != TokenKind.EOF) {
       if (token.kind == TokenKind.NEWLINE) {
-        expect(TokenKind.NEWLINE);
+        expectAndRecover(TokenKind.NEWLINE);
+      } else if (recoveryMode) {
+        // If there was a parse error, we want to recover here
+        // before starting a new top-level statement.
+        syncTo(STATEMENT_TERMINATOR_SET);
+        recoveryMode = false;
       } else {
         parseTopLevelStatement(list);
       }
@@ -1067,10 +1083,7 @@ class Parser {
       }
       parseSmallStatementOrPass(list);
     }
-    expect(TokenKind.NEWLINE);
-    // This is a safe place to recover: There is a new line at top-level
-    // and the parser is at the end of a statement.
-    recoveryMode = false;
+    expectAndRecover(TokenKind.NEWLINE);
   }
 
   //     small_stmt ::= assign_stmt
@@ -1259,7 +1272,7 @@ class Parser {
       while (token.kind != TokenKind.OUTDENT && token.kind != TokenKind.EOF) {
         parseStatement(list, false);
       }
-      expect(TokenKind.OUTDENT);
+      expectAndRecover(TokenKind.OUTDENT);
     } else {
       parseSimpleStatement(list);
     }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index 9157a09e9c..a786c10763 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -20,6 +20,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLiteral;
@@ -595,6 +596,29 @@ public class ParserTest extends AbstractParserTestCase {
   }
 
   @Test
+  public void testParserRecovery() throws Exception {
+    syntaxEvents.setFailFast(false);
+    List<Statement> statements = parseFileForSkylark(Joiner.on("\n").join(
+        "def foo():",
+        "  a = 2 not 4",  // parse error
+        "  b = [3, 4]",
+        "",
+        "d = 4 ada",  // parse error
+        "",
+        "def bar():",
+        "  a = [3, 4]",
+        "  b = 2 + + 5",  // parse error
+        ""));
+
+    assertThat(syntaxEvents.collector()).hasSize(4);
+    syntaxEvents.assertContainsEvent("syntax error at 'not': expected newline");
+    syntaxEvents.assertContainsEvent("syntax error at 'ada': expected newline");
+    syntaxEvents.assertContainsEvent("syntax error at '+': expected expression");
+    syntaxEvents.assertContainsEvent("contains syntax error(s)");
+    assertThat(statements).hasSize(3);
+  }
+
+  @Test
   public void testParserContainsErrorsIfSyntaxException() throws Exception {
     syntaxEvents.setFailFast(false);
     parseExpr("'foo' %%");
@@ -631,7 +655,7 @@ public class ParserTest extends AbstractParserTestCase {
       + "" + '\n'
     );
     syntaxEvents.assertContainsEvent("syntax error at 'error'");
-    assertThat(stmts).hasSize(2);
+    assertThat(stmts).hasSize(1);
   }
 
   @Test