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 6edf73744f7b88..b25f89be19837d 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 @@ -70,17 +70,6 @@ public ParseResult(List statements, List 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 augmentedAssignmentMethods = new ImmutableMap.Builder() .put(TokenKind.PLUS_EQUALS, Operator.PLUS) @@ -237,6 +224,7 @@ public static List 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 stmts = parseStatements(input, eventHandler, parsingLevel); @@ -244,6 +232,7 @@ public static Statement parseStatement( } /** 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 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 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 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 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 list) { expect(TokenKind.IN); Expression collection = parseExpression(); expect(TokenKind.COLON); - enterLoop(); - try { - List block = parseSuite(); - Statement stmt = new ForStatement(new LValue(loopVar), collection, block); - list.add(setLocation(stmt, start, token.right)); - } finally { - exitLoop(); - } + List 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 list) { int start = token.left; expect(TokenKind.DEF); @@ -1340,7 +1308,9 @@ private List parseSuite() { } // stmt ::= simple_stmt - // | compound_stmt + // | def_stmt + // | for_stmt + // | if_stmt private void parseStatement(List list, ParsingLevel parsingLevel) { if (token.kind == TokenKind.DEF) { if (parsingLevel == ParsingLevel.LOCAL_LEVEL) { @@ -1362,16 +1332,11 @@ private void parseStatement(List 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--; - } } 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 da63ca2e191cc9..95a1c840210b32 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 @@ -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); } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java index 49f5dbb2930af9..51ae1543df345d 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java +++ b/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 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 fc0780e68b7527..8ff4bf14017d14 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 @@ -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());