Cleanup in the parser

- Move break/continue check from ValidationEnvironment to the Parser
- Remove some differences between BUILD / Skylark parsing mode
- Fix location off-by-one error in the break/continue tokens
- Remove duplicated error message ('for loops are not allowed on top-level')

--
MOS_MIGRATED_REVID=137259929

4 files changed, 36 insertions, 66 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
index e498b9a54b..15f1ee25f2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
@@ -58,11 +58,7 @@ public final class FlowStatement extends Statement {
   }
 
   @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    if (!env.isInsideLoop()) {
-      throw new EvalException(getLocation(), kind.name + " statement must be inside a for loop");
-    }
-  }
+  void validate(ValidationEnvironment env) throws EvalException {}
 
   @Override
   public String toString() {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index b641be321f..97ac5061c4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -30,16 +30,14 @@ import com.google.devtools.build.lib.syntax.compiler.LoopLabels;
 import com.google.devtools.build.lib.syntax.compiler.Variable.InternalVariable;
 import com.google.devtools.build.lib.syntax.compiler.VariableScope;
 import com.google.devtools.build.lib.util.Preconditions;
-
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.implementation.bytecode.ByteCodeAppender;
 import net.bytebuddy.implementation.bytecode.Duplication;
 import net.bytebuddy.implementation.bytecode.constant.IntegerConstant;
 
-import java.util.ArrayList;
-import java.util.Iterator;
-import java.util.List;
-
 /**
  * Syntax node for a for loop statement.
  */
@@ -111,21 +109,12 @@ public final class ForStatement extends Statement {
 
   @Override
   void validate(ValidationEnvironment env) throws EvalException {
-    if (env.isTopLevel()) {
-      throw new EvalException(getLocation(), "'For' is not allowed as a top level statement");
-    }
-    env.enterLoop();
-
-    try {
-      // TODO(bazel-team): validate variable. Maybe make it temporarily readonly.
-      collection.validate(env);
-      variable.validate(env, getLocation());
+    // TODO(bazel-team): validate variable. Maybe make it temporarily readonly.
+    collection.validate(env);
+    variable.validate(env, getLocation());
 
-      for (Statement stmt : block) {
-        stmt.validate(env);
-      }
-    } finally {
-      env.exitLoop(getLocation());
+    for (Statement stmt : block) {
+      stmt.validate(env);
     }
   }
 
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 41010629bc..8b28b3ef47 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
@@ -135,6 +135,7 @@ public class Parser {
 
   private Token token; // current lookahead token
   private Token pushedToken = null; // used to implement LL(2)
+  private int loopCount; // break/continue keywords can be used only inside a loop
 
   private static final boolean DEBUGGING = false;
 
@@ -1171,8 +1172,7 @@ public class Parser {
     int start = token.left;
     if (token.kind == TokenKind.RETURN) {
       return parseReturnStatement();
-    } else if ((parsingMode == SKYLARK)
-        && (token.kind == TokenKind.BREAK || token.kind == TokenKind.CONTINUE)) {
+    } else if (token.kind == TokenKind.BREAK || token.kind == TokenKind.CONTINUE) {
       return parseFlowStatement(token.kind);
     }
     Expression expression = parseExpression();
@@ -1232,9 +1232,14 @@ public class Parser {
     expect(TokenKind.IN);
     Expression collection = parseExpression();
     expect(TokenKind.COLON);
-    List<Statement> block = parseSuite();
-    Statement stmt = new ForStatement(loopVar, collection, block);
-    list.add(setLocation(stmt, start, token.right));
+    enterLoop();
+    try {
+      List<Statement> block = parseSuite();
+      Statement stmt = new ForStatement(loopVar, collection, block);
+      list.add(setLocation(stmt, start, token.right));
+    } finally {
+      exitLoop();
+    }
   }
 
   // def foo(bar1, bar2):
@@ -1401,10 +1406,16 @@ public class Parser {
   // flow_stmt ::= break_stmt | continue_stmt
   private FlowStatement parseFlowStatement(TokenKind kind) {
     int start = token.left;
+    int end = token.right;
     expect(kind);
+    if (loopCount == 0) {
+      reportError(
+          lexer.createLocation(start, end),
+          kind.getPrettyName() + " statement must be inside a for loop");
+    }
     FlowStatement.Kind flowKind =
         kind == TokenKind.BREAK ? FlowStatement.Kind.BREAK : FlowStatement.Kind.CONTINUE;
-    return setLocation(new FlowStatement(flowKind), start, token.right);
+    return setLocation(new FlowStatement(flowKind), start, end);
   }
 
   // return_stmt ::= RETURN [expr]
@@ -1429,7 +1440,7 @@ public class Parser {
     int start = token.left;
     Token blockToken = token;
     syncTo(EnumSet.of(TokenKind.COLON, TokenKind.EOF)); // skip over expression or name
-    if (blockToken.kind == TokenKind.ELSE && parsingMode == SKYLARK) {
+    if (blockToken.kind == TokenKind.ELSE) {
       reportError(
           lexer.createLocation(blockToken.left, blockToken.right),
           "syntax error at 'else': not allowed here.");
@@ -1450,4 +1461,13 @@ public class Parser {
   private void makeComment(Token token) {
     comments.add(setLocation(new Comment((String) token.value), token.left, token.right));
   }
+
+  private void enterLoop() {
+    loopCount++;
+  }
+
+  private void exitLoop() {
+    Preconditions.checkState(loopCount > 0);
+    loopCount--;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index 095b46f243..c1a94c5084 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -46,12 +46,6 @@ public final class ValidationEnvironment {
   private final Stack<Set<String>> futureReadOnlyVariables = new Stack<>();
 
   /**
-   * Tracks the number of nested for loops that contain the statement that is currently being
-   * validated
-   */
-  private int loopCount = 0;
-
-  /**
    * Create a ValidationEnvironment for a given global Environment.
    */
   public ValidationEnvironment(Environment env) {
@@ -169,33 +163,4 @@ public final class ValidationEnvironment {
       return false;
     }
   }
-
-  /**
-   * Returns whether the current statement is inside a for loop (either in this environment or one
-   * of its parents)
-   *
-   * @return True if the current statement is inside a for loop
-   */
-  public boolean isInsideLoop() {
-    return (loopCount > 0);
-  }
-
-  /**
-   * Signals that the block of a for loop was entered
-   */
-  public void enterLoop()   {
-    ++loopCount;
-  }
-
-  /**
-   * Signals that the block of a for loop was left
-   *
-   * @param location The current location
-   * @throws EvalException If there was no corresponding call to
-   *         {@code ValidationEnvironment#enterLoop}
-   */
-  public void exitLoop(Location location) throws EvalException {
-    Preconditions.checkState(loopCount > 0);
-    --loopCount;
-  }
 }