skylark/syntax: Move flow statement check to the validation pass.

Mutiple other cleanups in the parser, update code documentation.

RELNOTES: None.
PiperOrigin-RevId: 167501136

src/main/java/com/google/devtools/build/lib/syntax/Parser.java

@@ -70,17 +70,6 @@ public ParseResult(List<Statement> statements, List<Comment> comments, Location
     }
   }
 
-  /** Used to select whether the parser rejects features that are prohibited for BUILD files. */
-  // TODO(brandjon): Instead of using an enum to control what features are allowed, factor these
-  // restrictions into a separate visitor that can be outside the core Skylark parser. This will
-  // reduce parser complexity and help keep Bazel-specific knowledge out of the interpreter.
-  public enum Dialect {
-    /** Used for BUILD files. */
-    BUILD,
-    /** Used for .bzl and other Skylark files. This allows all language features. */
-    SKYLARK,
-  }
-
   /** Used to select what constructs are allowed based on whether we're at the top level. */
   public enum ParsingLevel {
     TOP_LEVEL,
@@ -121,7 +110,6 @@ public enum ParsingLevel {
 
   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;
 
@@ -150,7 +138,6 @@ public enum ParsingLevel {
           .put(TokenKind.STAR, Operator.MULT)
           .build();
 
-  // TODO(bazel-team): add support for |=
   private static final Map<TokenKind, Operator> augmentedAssignmentMethods =
       new ImmutableMap.Builder<TokenKind, Operator>()
           .put(TokenKind.PLUS_EQUALS, Operator.PLUS)
@@ -237,13 +224,15 @@ public static List<Statement> parseStatements(
    *
    * @throws IllegalArgumentException if the number of parsed statements was not exactly one
    */
+  @VisibleForTesting
   public static Statement parseStatement(
       ParserInputSource input, EventHandler eventHandler, ParsingLevel parsingLevel) {
     List<Statement> stmts = parseStatements(input, eventHandler, parsingLevel);
     return Iterables.getOnlyElement(stmts);
   }
 
   /** Parses an expression, possibly followed by newline tokens. */
+  @VisibleForTesting
   public static Expression parseExpression(ParserInputSource input, EventHandler eventHandler) {
     Lexer lexer = new Lexer(input, eventHandler);
     Parser parser = new Parser(lexer, eventHandler);
@@ -408,14 +397,9 @@ private Identifier makeErrorExpression(int start, int end) {
     return setLocation(new Identifier("$error$"), start, end);
   }
 
-  // Convenience wrapper around ASTNode.setLocation that returns the node.
-  private <NODE extends ASTNode> NODE setLocation(NODE node, Location location) {
-    return ASTNode.setLocation(location, node);
-  }
-
-  // Another convenience wrapper method around ASTNode.setLocation
+  // Convenience wrapper method around ASTNode.setLocation
   private <NODE extends ASTNode> NODE setLocation(NODE node, int startOffset, int endOffset) {
-    return setLocation(node, lexer.createLocation(startOffset, endOffset));
+    return ASTNode.setLocation(lexer.createLocation(startOffset, endOffset), node);
   }
 
   // Convenience method that uses end offset from the last node.
@@ -427,10 +411,8 @@ private <NODE extends ASTNode> NODE setLocation(NODE node, int startOffset, ASTN
 
   // arg ::= IDENTIFIER '=' nontupleexpr
   //       | expr
-  //       | *args       (only in Skylark mode)
-  //       | **kwargs    (only in Skylark mode)
-  // To keep BUILD files declarative and easy to process, *args and **kwargs
-  // arguments are allowed only in Skylark mode.
+  //       | *args
+  //       | **kwargs
   private Argument.Passed parseFuncallArgument() {
     final int start = token.left;
     // parse **expr
@@ -789,7 +771,7 @@ private Expression parseComprehensionSuffix(
   // list_maker ::= '[' ']'
   //               |'[' expr ']'
   //               |'[' expr expr_list ']'
-  //               |'[' expr ('FOR' loop_variables 'IN' expr)+ ']'
+  //               |'[' expr comprehension_suffix ']'
   private Expression parseListMaker() {
     int start = token.left;
     expect(TokenKind.LBRACKET);
@@ -848,7 +830,7 @@ private Expression parseListMaker() {
 
   // dict_expression ::= '{' '}'
   //                    |'{' dict_entry_list '}'
-  //                    |'{' dict_entry 'FOR' loop_variables 'IN' expr '}'
+  //                    |'{' dict_entry comprehension_suffix '}'
   private Expression parseDictExpression() {
     int start = token.left;
     expect(TokenKind.LBRACE);
@@ -1156,21 +1138,12 @@ private void parseSimpleStatement(List<Statement> list) {
 
   //     small_stmt ::= assign_stmt
   //                  | expr
-  //                  | RETURN expr
+  //                  | return_stmt
   //                  | flow_stmt
   //     assign_stmt ::= expr ('=' | augassign) expr
-  //     augassign ::= ('+=' | '-=' | '*=' | '/=' | '%=')
+  //     augassign ::= ('+=' | '-=' | '*=' | '/=' | '%=' | '//=' )
   // Note that these are in Python, but not implemented here (at least for now):
-  // '&=' | '|=' | '^=' |'<<=' | '>>=' | '**=' | '//='
-  // Semantic difference from Python:
-  // In Skylark, x += y is simple syntactic sugar for x = x + y.
-  // In Python, x += y is more or less equivalent to x = x + y, but if a method is defined
-  // on x.__iadd__(y), then it takes precedence, and in the case of lists it side-effects
-  // the original list (it doesn't do that on tuples); if no such method is defined it falls back
-  // to the x.__add__(y) method that backs x + y. In Skylark, we don't support this side-effect.
-  // Note also that there is a special casing to translate 'ident[key] = value'
-  // to 'ident = ident + {key: value}'. This is needed to support the pure version of Python-like
-  // dictionary assignment syntax.
+  // '&=' | '|=' | '^=' |'<<=' | '>>=' | '**='
   private Statement parseSmallStatement() {
     int start = token.left;
     if (token.kind == TokenKind.RETURN) {
@@ -1236,17 +1209,12 @@ private void parseForStatement(List<Statement> list) {
     expect(TokenKind.IN);
     Expression collection = parseExpression();
     expect(TokenKind.COLON);
-    enterLoop();
-    try {
-      List<Statement> block = parseSuite();
-      Statement stmt = new ForStatement(new LValue(loopVar), collection, block);
-      list.add(setLocation(stmt, start, token.right));
-    } finally {
-      exitLoop();
-    }
+    List<Statement> block = parseSuite();
+    Statement stmt = new ForStatement(new LValue(loopVar), collection, block);
+    list.add(setLocation(stmt, start, token.right));
   }
 
-  // def foo(bar1, bar2):
+  // def_stmt ::= DEF IDENTIFIER '(' arguments ')' ':' suite
   private void parseFunctionDefStatement(List<Statement> list) {
     int start = token.left;
     expect(TokenKind.DEF);
@@ -1340,7 +1308,9 @@ private List<Statement> parseSuite() {
   }
 
   // stmt ::= simple_stmt
-  //        | compound_stmt
+  //        | def_stmt
+  //        | for_stmt
+  //        | if_stmt
   private void parseStatement(List<Statement> list, ParsingLevel parsingLevel) {
     if (token.kind == TokenKind.DEF) {
       if (parsingLevel == ParsingLevel.LOCAL_LEVEL) {
@@ -1362,16 +1332,11 @@ private void parseStatement(List<Statement> list, ParsingLevel parsingLevel) {
     }
   }
 
-  // flow_stmt ::= break_stmt | continue_stmt
+  // flow_stmt ::= BREAK | CONTINUE
   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, end);
@@ -1395,13 +1360,4 @@ private ReturnStatement parseReturnStatement() {
   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--;
-  }
 }

src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java

@@ -58,6 +58,7 @@ private static class ValidationException extends RuntimeException {
 
   private final SkylarkSemanticsOptions semantics;
   private Block block;
+  private int loopCount;
 
   /** Create a ValidationEnvironment for a given global Environment. */
   ValidationEnvironment(Environment env) {
@@ -106,7 +107,24 @@ public void visit(LValue node) {
   public void visit(ReturnStatement node) {
     if (isTopLevel()) {
       throw new ValidationException(
-          node.getLocation(), "Return statements must be inside a function");
+          node.getLocation(), "return statements must be inside a function");
+    }
+    super.visit(node);
+  }
+
+  @Override
+  public void visit(ForStatement node) {
+    loopCount++;
+    super.visit(node);
+    Preconditions.checkState(loopCount > 0);
+    loopCount--;
+  }
+
+  @Override
+  public void visit(FlowStatement node) {
+    if (loopCount <= 0) {
+      throw new ValidationException(
+          node.getLocation(), node.getKind().getName() + " statement must be inside a for loop");
     }
     super.visit(node);
   }

src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java

@@ -45,7 +45,7 @@ public void testAugmentedAssignmentWithMultipleLValues() {
 
   @Test
   public void testReturnOutsideFunction() throws Exception {
-    checkError("Return statements must be inside a function", "return 2\n");
+    checkError("return statements must be inside a function", "return 2\n");
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java

@@ -162,18 +162,6 @@ protected Statement parseStatement(Parser.ParsingLevel parsingLevel, String... i
     return Parser.parseStatement(makeParserInputSource(input), getEventHandler(), parsingLevel);
   }
 
-  /** Parses a top-level statement, possibly followed by newlines. */
-  protected Statement parseTopLevelStatement(String... input) {
-    return Parser.parseStatement(
-        makeParserInputSource(input), getEventHandler(), Parser.ParsingLevel.TOP_LEVEL);
-  }
-
-  /** Parses a local statement, possibly followed by newlines. */
-  protected Statement parseLocalLevelStatement(String... input) {
-    return Parser.parseStatement(
-        makeParserInputSource(input), getEventHandler(), Parser.ParsingLevel.LOCAL_LEVEL);
-  }
-
   /** Parses an expression, possibly followed by newlines. */
   protected Expression parseExpression(String... input) {
     return Parser.parseExpression(makeParserInputSource(input), getEventHandler());