Remove dialect distinction from the parser.

RELNOTES: None.
PiperOrigin-RevId: 166058718

5 files changed, 95 insertions, 169 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 03c50d3a3b..56e6233905 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
@@ -14,8 +14,6 @@
 
 package com.google.devtools.build.lib.syntax;
 
-import static com.google.devtools.build.lib.syntax.Parser.Dialect.SKYLARK;
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
@@ -108,15 +106,6 @@ public class Parser {
           TokenKind.RPAREN,
           TokenKind.SEMI);
 
-  private static final EnumSet<TokenKind> BLOCK_STARTING_SET =
-      EnumSet.of(
-          TokenKind.CLASS,
-          TokenKind.DEF,
-          TokenKind.ELSE,
-          TokenKind.FOR,
-          TokenKind.IF,
-          TokenKind.TRY);
-
   private static final EnumSet<TokenKind> EXPR_TERMINATOR_SET =
       EnumSet.of(
           TokenKind.COLON,
@@ -130,14 +119,6 @@ public class Parser {
           TokenKind.RPAREN,
           TokenKind.SLASH);
 
-  /**
-   * Keywords that are forbidden in both Skylark and BUILD parsing modes.
-   *
-   * <p>(Mapping: token -> human-readable string description)
-   */
-  private static final ImmutableMap<TokenKind, String> ILLEGAL_BLOCK_KEYWORDS =
-      ImmutableMap.of(TokenKind.CLASS, "Class definition", TokenKind.TRY, "Try statement");
-
   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
@@ -147,7 +128,6 @@ public class Parser {
   private final Lexer lexer;
   private final EventHandler eventHandler;
   private final List<Comment> comments;
-  private final Dialect dialect;
 
   private static final Map<TokenKind, Operator> binaryOperators =
       new ImmutableMap.Builder<TokenKind, Operator>()
@@ -198,10 +178,9 @@ public class Parser {
   private int errorsCount;
   private boolean recoveryMode;  // stop reporting errors until next statement
 
-  private Parser(Lexer lexer, EventHandler eventHandler, Dialect dialect) {
+  private Parser(Lexer lexer, EventHandler eventHandler) {
     this.lexer = lexer;
     this.eventHandler = eventHandler;
-    this.dialect = dialect;
     this.tokens = lexer.getTokens().iterator();
     this.comments = new ArrayList<>();
     nextToken();
@@ -229,13 +208,15 @@ public class Parser {
   public static ParseResult parseFile(
       ParserInputSource input, EventHandler eventHandler, Dialect dialect) {
     Lexer lexer = new Lexer(input, eventHandler);
-    Parser parser = new Parser(lexer, eventHandler, dialect);
+    Parser parser = new Parser(lexer, eventHandler);
     List<Statement> statements = parser.parseFileInput();
+    boolean errors = parser.errorsCount > 0 || lexer.containsErrors();
+    // TODO(laurentlb): Remove dialect argument.
+    if (dialect == Dialect.BUILD) {
+      errors |= !ValidationEnvironment.checkBuildSyntax(statements, eventHandler);
+    }
     return new ParseResult(
-        statements,
-        parser.comments,
-        locationFromStatements(lexer, statements),
-        parser.errorsCount > 0 || lexer.containsErrors());
+        statements, parser.comments, locationFromStatements(lexer, statements), errors);
   }
 
   /**
@@ -245,10 +226,9 @@ public class Parser {
    * function definitions, for statements, etc., are allowed.
    */
   public static List<Statement> parseStatements(
-      ParserInputSource input, EventHandler eventHandler,
-      ParsingLevel parsingLevel, Dialect dialect) {
+      ParserInputSource input, EventHandler eventHandler, ParsingLevel parsingLevel) {
     Lexer lexer = new Lexer(input, eventHandler);
-    Parser parser = new Parser(lexer, eventHandler, dialect);
+    Parser parser = new Parser(lexer, eventHandler);
     List<Statement> result = new ArrayList<>();
     parser.parseStatement(result, parsingLevel);
     while (parser.token.kind == TokenKind.NEWLINE) {
@@ -264,18 +244,15 @@ public class Parser {
    * @throws IllegalArgumentException if the number of parsed statements was not exactly one
    */
   public static Statement parseStatement(
-      ParserInputSource input, EventHandler eventHandler,
-      ParsingLevel parsingLevel, Dialect dialect) {
-    List<Statement> stmts = parseStatements(
-        input, eventHandler, parsingLevel, dialect);
+      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. */
-  public static Expression parseExpression(
-      ParserInputSource input, EventHandler eventHandler, Dialect dialect) {
+  public static Expression parseExpression(ParserInputSource input, EventHandler eventHandler) {
     Lexer lexer = new Lexer(input, eventHandler);
-    Parser parser = new Parser(lexer, eventHandler, dialect);
+    Parser parser = new Parser(lexer, eventHandler);
     Expression result = parser.parseExpression();
     while (parser.token.kind == TokenKind.NEWLINE) {
       parser.nextToken();
@@ -364,10 +341,24 @@ public class Parser {
 
   // Keywords that exist in Python and that we don't parse.
   private static final EnumSet<TokenKind> FORBIDDEN_KEYWORDS =
-      EnumSet.of(TokenKind.AS, TokenKind.ASSERT,
-          TokenKind.DEL, TokenKind.EXCEPT, TokenKind.FINALLY, TokenKind.FROM, TokenKind.GLOBAL,
-          TokenKind.IMPORT, TokenKind.IS, TokenKind.LAMBDA, TokenKind.NONLOCAL, TokenKind.RAISE,
-          TokenKind.TRY, TokenKind.WITH, TokenKind.WHILE, TokenKind.YIELD);
+      EnumSet.of(
+          TokenKind.AS,
+          TokenKind.ASSERT,
+          TokenKind.CLASS,
+          TokenKind.DEL,
+          TokenKind.EXCEPT,
+          TokenKind.FINALLY,
+          TokenKind.FROM,
+          TokenKind.GLOBAL,
+          TokenKind.IMPORT,
+          TokenKind.IS,
+          TokenKind.LAMBDA,
+          TokenKind.NONLOCAL,
+          TokenKind.RAISE,
+          TokenKind.TRY,
+          TokenKind.WITH,
+          TokenKind.WHILE,
+          TokenKind.YIELD);
 
   private void checkForbiddenKeywords(Token token) {
     if (!FORBIDDEN_KEYWORDS.contains(token.kind)) {
@@ -450,22 +441,12 @@ public class Parser {
     final int start = token.left;
     // parse **expr
     if (token.kind == TokenKind.STAR_STAR) {
-      if (dialect != SKYLARK) {
-        reportError(
-            lexer.createLocation(token.left, token.right),
-            "**kwargs arguments are not allowed in BUILD files");
-      }
       nextToken();
       Expression expr = parseNonTupleExpression();
       return setLocation(new Argument.StarStar(expr), start, expr);
     }
     // parse *expr
     if (token.kind == TokenKind.STAR) {
-      if (dialect != SKYLARK) {
-        reportError(
-            lexer.createLocation(token.left, token.right),
-            "*args arguments are not allowed in BUILD files");
-      }
       nextToken();
       Expression expr = parseNonTupleExpression();
       return setLocation(new Argument.Star(expr), start, expr);
@@ -1364,61 +1345,24 @@ public class Parser {
     return list;
   }
 
-  // skipSuite does not check that the code is syntactically correct, it
-  // just skips based on indentation levels.
-  private void skipSuite() {
-    if (token.kind == TokenKind.NEWLINE) {
-      expect(TokenKind.NEWLINE);
-      if (token.kind != TokenKind.INDENT) {
-        reportError(lexer.createLocation(token.left, token.right),
-                    "expected an indented block");
-        return;
-      }
-      expect(TokenKind.INDENT);
-
-      // Don't try to parse all the Python syntax, just skip the block
-      // until the corresponding outdent token.
-      int depth = 1;
-      while (depth > 0) {
-        // Because of the way the lexer works, this should never happen
-        Preconditions.checkState(token.kind != TokenKind.EOF);
-
-        if (token.kind == TokenKind.INDENT) {
-          depth++;
-        }
-        if (token.kind == TokenKind.OUTDENT) {
-          depth--;
-        }
-        nextToken();
-      }
-
-    } else {
-      // the block ends at the newline token
-      // e.g.  if x == 3: print "three"
-      syncTo(STATEMENT_TERMINATOR_SET);
-    }
-  }
-
   // stmt ::= simple_stmt
   //        | compound_stmt
   private void parseStatement(List<Statement> list, ParsingLevel parsingLevel) {
-    if (token.kind == TokenKind.DEF && dialect == SKYLARK) {
+    if (token.kind == TokenKind.DEF) {
       if (parsingLevel == ParsingLevel.LOCAL_LEVEL) {
         reportError(lexer.createLocation(token.left, token.right),
             "nested functions are not allowed. Move the function to top-level");
       }
       parseFunctionDefStatement(list);
-    } else if (token.kind == TokenKind.IF && dialect == SKYLARK) {
+    } else if (token.kind == TokenKind.IF) {
       list.add(parseIfStatement());
-    } else if (token.kind == TokenKind.FOR && dialect == SKYLARK) {
+    } else if (token.kind == TokenKind.FOR) {
       if (parsingLevel == ParsingLevel.TOP_LEVEL) {
         reportError(
             lexer.createLocation(token.left, token.right),
             "for loops are not allowed on top-level. Put it into a function");
       }
       parseForStatement(list);
-    } else if (BLOCK_STARTING_SET.contains(token.kind)) {
-      skipBlock();
     } else {
       parseSimpleStatement(list);
     }
@@ -1456,28 +1400,6 @@ public class Parser {
     return setLocation(new ReturnStatement(expression), start, expression);
   }
 
-  // block ::= ('if' | 'for' | 'class' | 'try' | 'def') expr ':' suite
-  private void skipBlock() {
-    int start = token.left;
-    Token blockToken = token;
-    syncTo(EnumSet.of(TokenKind.COLON, TokenKind.EOF)); // skip over expression or name
-    if (blockToken.kind == TokenKind.ELSE) {
-      reportError(
-          lexer.createLocation(blockToken.left, blockToken.right),
-          "syntax error at 'else': not allowed here.");
-    } else {
-      String msg =
-          ILLEGAL_BLOCK_KEYWORDS.containsKey(blockToken.kind)
-              ? String.format("%ss are not supported.", ILLEGAL_BLOCK_KEYWORDS.get(blockToken.kind))
-              : "This is not supported in BUILD files. Move the block to a .bzl file and load it";
-      reportError(
-          lexer.createLocation(start, token.right),
-          String.format("syntax error at '%s': %s", blockToken, msg));
-    }
-    expect(TokenKind.COLON);
-    skipSuite();
-  }
-
   // create a comment node
   private void makeComment(Token token) {
     comments.add(setLocation(new Comment((String) token.value), token.left, token.right));
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 6f5eb9c401..da63ca2e19 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
@@ -289,4 +289,48 @@ public final class ValidationEnvironment extends SyntaxTreeVisitor {
   private void closeBlock() {
     block = Preconditions.checkNotNull(block.parent);
   }
+
+  /**
+   * Checks that the AST is using the restricted syntax.
+   *
+   * <p>Restricted syntax is used by Bazel BUILD files. It forbids function definitions, *args, and
+   * **kwargs. This creates a better separation between code and data.
+   */
+  public static boolean checkBuildSyntax(
+      List<Statement> statements, final EventHandler eventHandler) {
+    // Wrap the boolean inside an array so that the inner class can modify it.
+    final boolean[] success = new boolean[] {true};
+    // TODO(laurentlb): Merge with the visitor above when possible (i.e. when BUILD files use it).
+    SyntaxTreeVisitor checker =
+        new SyntaxTreeVisitor() {
+          @Override
+          public void visit(FuncallExpression node) {
+            for (Argument.Passed arg : node.getArguments()) {
+              if (arg.isStarStar()) {
+                eventHandler.handle(
+                    Event.error(
+                        node.getLocation(), "**kwargs arguments are not allowed in BUILD files"));
+                success[0] = false;
+              } else if (arg.isStar()) {
+                eventHandler.handle(
+                    Event.error(
+                        node.getLocation(), "*args arguments are not allowed in BUILD files"));
+                success[0] = false;
+              }
+            }
+          }
+
+          @Override
+          public void visit(FunctionDefStatement node) {
+            eventHandler.handle(
+                Event.error(
+                    node.getLocation(),
+                    "syntax error at 'def': This is not supported in BUILD files. "
+                        + "Move the block to a .bzl file and load it"));
+            success[0] = false;
+          }
+        };
+    checker.visitAll(statements);
+    return success[0];
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 35b1e3797b..516a20eb8e 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -1314,16 +1314,17 @@ public class SkylarkIntegrationTest extends BuildViewTestCase {
         "test/skylark/macro.bzl",
         "x = 5");
 
-    scratch.file("test/skylark/BUILD",
+    scratch.file(
+        "test/skylark/BUILD",
         "load('//test/skylark:macro.bzl', 'x')",
-        "if 0: pass", // syntax error
+        "pass", // syntax error
         "print(1 / (5 - x)"); // division by 0
 
     // Make sure that evaluation continues and load() succeeds, despite a syntax
     // error in the file.
     // We can get the division by 0 only if x was correctly loaded.
     getConfiguredTarget("//test/skylark:a");
-    assertContainsEvent("syntax error at 'if'");
+    assertContainsEvent("syntax error");
     assertContainsEvent("integer division by zero");
   }
 
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 784b0a7926..94cdb30105 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
@@ -914,22 +914,6 @@ public class ParserTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testFunctionDefinitionErrorRecovery() throws Exception {
-    // Parser skips over entire function definitions, and reports a meaningful
-    // error.
-    setFailFast(false);
-    List<Statement> stmts = parseFile(
-        "x = 1;\n"
-        + "def foo(x, y, **z):\n"
-        + "  # a comment\n"
-        + "  x = 2\n"
-        + "  foo(bar)\n"
-        + "  return z\n"
-        + "x = 3");
-    assertThat(stmts).hasSize(2);
-  }
-
-  @Test
   public void testDefSingleLine() throws Exception {
     List<Statement> statements = parseFileForSkylark(
         "def foo(): x = 1; y = 2\n");
@@ -955,19 +939,6 @@ public class ParserTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testSkipIfBlockFail() throws Exception {
-    // Do not parse 'if' blocks, when parsePython is not set
-    setFailFast(false);
-    List<Statement> stmts = parseFile(
-        "x = 1;",
-        "if x == 1:",
-        "  x = 2",
-        "x = 3;\n");
-    assertThat(stmts).hasSize(2);
-    assertContainsError("This is not supported in BUILD files");
-  }
-
-  @Test
   public void testForLoopMultipleVariables() throws Exception {
     List<Statement> stmts1 = parseFile("[ i for i, j, k in [(1, 2, 3)] ]\n");
     assertThat(stmts1).hasSize(1);
@@ -1402,7 +1373,7 @@ public class ParserTest extends EvaluationTestCase {
         "def func(a):",
         // no if
         "  else: return a");
-    assertContainsError("syntax error at 'else': not allowed here.");
+    assertContainsError("syntax error at 'else': expected expression");
   }
 
   @Test
@@ -1413,42 +1384,36 @@ public class ParserTest extends EvaluationTestCase {
         "  for i in range(a):",
         "    print(i)",
         "  else: return a");
-    assertContainsError("syntax error at 'else': not allowed here.");
+    assertContainsError("syntax error at 'else': expected expression");
   }
 
   @Test
   public void testTryStatementInBuild() throws Exception {
     setFailFast(false);
     parseFile("try: pass");
-    assertContainsError("syntax error at 'try': Try statements are not supported.");
-  }
-
-  @Test
-  public void testTryStatementInSkylark() throws Exception {
-    setFailFast(false);
-    parseFileForSkylark("try: pass");
-    assertContainsError("syntax error at 'try': Try statements are not supported.");
+    assertContainsError("'try' not supported, all exceptions are fatal");
   }
 
   @Test
   public void testClassDefinitionInBuild() throws Exception {
     setFailFast(false);
-    parseFile("class test(object): pass");
-    assertContainsError("syntax error at 'class': Class definitions are not supported.");
+    parseFileWithComments("class test(object): pass");
+    assertContainsError("keyword 'class' not supported");
   }
 
   @Test
   public void testClassDefinitionInSkylark() throws Exception {
     setFailFast(false);
     parseFileForSkylark("class test(object): pass");
-    assertContainsError("syntax error at 'class': Class definitions are not supported.");
+    assertContainsError("keyword 'class' not supported");
   }
 
   @Test
   public void testDefInBuild() throws Exception {
     setFailFast(false);
-    parseFile("def func(): pass");
+    BuildFileAST result = parseFileWithComments("def func(): pass");
     assertContainsError("syntax error at 'def': This is not supported in BUILD files. "
         + "Move the block to a .bzl file and load it");
+    assertThat(result.containsErrors()).isTrue();
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index ceeffe74eb..4d52f6f51c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -159,30 +159,24 @@ public class EvaluationTestCase {
 
   /** Parses a statement, possibly followed by newlines. */
   protected Statement parseStatement(Parser.ParsingLevel parsingLevel, String... input) {
-    return Parser.parseStatement(
-        makeParserInputSource(input), getEventHandler(),
-        parsingLevel, Parser.Dialect.SKYLARK);
+    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, Parser.Dialect.SKYLARK);
+        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, Parser.Dialect.SKYLARK);
+        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(),
-        Parser.Dialect.SKYLARK);
+    return Parser.parseExpression(makeParserInputSource(input), getEventHandler());
   }
 
   public EvaluationTestCase update(String varname, Object value) throws Exception {