diff options
7 files changed, 104 insertions, 107 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java index 88be43be0d..b4f3e69aa8 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java @@ -528,7 +528,6 @@ public final class EvalUtils { int step; if (stepObj == Runtime.NONE) { - // This case is excluded by the parser, but let's handle it for completeness. step = 1; } else if (stepObj instanceof Integer) { step = ((Integer) stepObj).intValue(); 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 ddea2970b7..b5350300d7 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 @@ -34,6 +34,7 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import javax.annotation.Nullable; /** * Recursive descent parser for LL(2) BUILD language. @@ -231,6 +232,32 @@ public class Parser { return Iterables.getOnlyElement(stmts); } + // stmt ::= simple_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) { + 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) { + list.add(parseIfStatement()); + } 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 { + parseSimpleStatement(list); + } + } + /** Parses an expression, possibly followed by newline tokens. */ @VisibleForTesting public static Expression parseExpression(ParserInputSource input, EventHandler eventHandler) { @@ -244,6 +271,30 @@ public class Parser { return result; } + private Expression parseExpression() { + return parseExpression(false); + } + + // Equivalent to 'testlist' rule in Python grammar. It can parse every kind of + // expression. In many cases, we need to use parseNonTupleExpression to avoid ambiguity: + // e.g. fct(x, y) vs fct((x, y)) + // + // Tuples can have a trailing comma only when insideParens is true. This prevents bugs + // where a one-element tuple is surprisingly created: + // e.g. foo = f(x), + private Expression parseExpression(boolean insideParens) { + int start = token.left; + Expression expression = parseNonTupleExpression(); + if (token.kind != TokenKind.COMMA) { + return expression; + } + + // It's a tuple + List<Expression> tuple = parseExprList(insideParens); + tuple.add(0, expression); // add the first expression to the front of the tuple + return setLocation(ListLiteral.makeTuple(tuple), start, Iterables.getLast(tuple)); + } + private void reportError(Location location, String message) { errorsCount++; // Limit the number of reported errors to avoid spamming output. @@ -398,12 +449,12 @@ public class Parser { } // Convenience wrapper method around ASTNode.setLocation - private <NODE extends ASTNode> NODE setLocation(NODE node, int startOffset, int endOffset) { + private <NodeT extends ASTNode> NodeT setLocation(NodeT node, int startOffset, int endOffset) { return ASTNode.setLocation(lexer.createLocation(startOffset, endOffset), node); } // Convenience method that uses end offset from the last node. - private <NODE extends ASTNode> NODE setLocation(NODE node, int startOffset, ASTNode lastNode) { + private <NodeT extends ASTNode> NodeT setLocation(NodeT node, int startOffset, ASTNode lastNode) { Preconditions.checkNotNull(lastNode, "can't extract end offset from a null node"); Preconditions.checkNotNull(lastNode.getLocation(), "lastNode doesn't have a location"); return setLocation(node, startOffset, lastNode.getLocation().getEndOffset()); @@ -671,7 +722,7 @@ public class Parser { expect(TokenKind.LBRACKET); if (token.kind == TokenKind.COLON) { - startExpr = setLocation(new Identifier("None"), token.left, token.right); + startExpr = null; } else { startExpr = parseExpression(); } @@ -682,8 +733,8 @@ public class Parser { return expr; } // This is a slice (or substring) - Expression endExpr = parseSliceArgument(new Identifier("None")); - Expression stepExpr = parseSliceArgument(new IntegerLiteral(1)); + Expression endExpr = parseSliceArgument(); + Expression stepExpr = parseSliceArgument(); Expression expr = setLocation( new SliceExpression(receiver, startExpr, endExpr, stepExpr), start, token.right); @@ -693,18 +744,9 @@ public class Parser { /** * Parses {@code [':' [expr]]} which can either be the end or the step argument of a slice - * operation. If no such expression is found, this method returns an argument that represents - * {@code defaultValue}. + * operation. If no such expression is found, this method returns null. */ - private Expression parseSliceArgument(Expression defaultValue) { - Expression explicitArg = getSliceEndOrStepExpression(); - if (explicitArg == null) { - return setLocation(defaultValue, token.left, token.right); - } - return explicitArg; - } - - private Expression getSliceEndOrStepExpression() { + private @Nullable Expression parseSliceArgument() { // There has to be a colon before any end or slice argument. // However, if the next token thereafter is another colon or a right bracket, no argument value // was specified. @@ -944,30 +986,6 @@ public class Parser { return new BinaryOperatorExpression(operator, expr, secondary); } - private Expression parseExpression() { - return parseExpression(false); - } - - // Equivalent to 'testlist' rule in Python grammar. It can parse every kind of - // expression. In many cases, we need to use parseNonTupleExpression to avoid ambiguity: - // e.g. fct(x, y) vs fct((x, y)) - // - // Tuples can have a trailing comma only when insideParens is true. This prevents bugs - // where a one-element tuple is surprisingly created: - // e.g. foo = f(x), - private Expression parseExpression(boolean insideParens) { - int start = token.left; - Expression expression = parseNonTupleExpression(); - if (token.kind != TokenKind.COMMA) { - return expression; - } - - // It's a tuple - List<Expression> tuple = parseExprList(insideParens); - tuple.add(0, expression); // add the first expression to the front of the tuple - return setLocation(ListLiteral.makeTuple(tuple), start, Iterables.getLast(tuple)); - } - // Equivalent to 'test' rule in Python grammar. private Expression parseNonTupleExpression() { int start = token.left; @@ -1324,31 +1342,6 @@ public class Parser { return list; } - // stmt ::= simple_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) { - 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) { - list.add(parseIfStatement()); - } 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 { - parseSimpleStatement(list); - } - } - // flow_stmt ::= BREAK | CONTINUE private FlowStatement parseFlowStatement(TokenKind kind) { int start = token.left; diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java index a2b6036e28..54c194a021 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java @@ -16,14 +16,15 @@ package com.google.devtools.build.lib.syntax; import com.google.devtools.build.lib.events.Location; import java.io.IOException; import java.util.List; +import javax.annotation.Nullable; /** Syntax node for a slice expression, e.g. obj[:len(obj):2]. */ public final class SliceExpression extends Expression { private final Expression object; - private final Expression start; - private final Expression end; - private final Expression step; + @Nullable private final Expression start; + @Nullable private final Expression end; + @Nullable private final Expression step; public SliceExpression(Expression object, Expression start, Expression end, Expression step) { this.object = object; @@ -36,43 +37,32 @@ public final class SliceExpression extends Expression { return object; } - public Expression getStart() { + public @Nullable Expression getStart() { return start; } - public Expression getEnd() { + public @Nullable Expression getEnd() { return end; } - public Expression getStep() { + public @Nullable Expression getStep() { return step; } @Override public void prettyPrint(Appendable buffer) throws IOException { - boolean startIsDefault = - (start instanceof Identifier) && ((Identifier) start).getName().equals("None"); - boolean endIsDefault = - (end instanceof Identifier) && ((Identifier) end).getName().equals("None"); - boolean stepIsDefault = - (step instanceof IntegerLiteral) && ((IntegerLiteral) step).getValue() == 1; - object.prettyPrint(buffer); buffer.append('['); - // Start and end are omitted if they are the literal identifier None, which is the default value - // inserted by the parser if no bound is given. Likewise, step is omitted if it is the literal - // integer 1. - // // The first separator colon is unconditional. The second separator appears only if step is // printed. - if (!startIsDefault) { + if (start != null) { start.prettyPrint(buffer); } buffer.append(':'); - if (!endIsDefault) { + if (end != null) { end.prettyPrint(buffer); } - if (!stepIsDefault) { + if (step != null) { buffer.append(':'); step.prettyPrint(buffer); } @@ -82,9 +72,9 @@ public final class SliceExpression extends Expression { @Override Object doEval(Environment env) throws EvalException, InterruptedException { Object objValue = object.eval(env); - Object startValue = start.eval(env); - Object endValue = end.eval(env); - Object stepValue = step.eval(env); + Object startValue = start == null ? Runtime.NONE : start.eval(env); + Object endValue = end == null ? Runtime.NONE : end.eval(env); + Object stepValue = step == null ? Runtime.NONE : step.eval(env); Location loc = getLocation(); if (objValue instanceof SkylarkList) { diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java index 22bc8b724b..9845a8ecb1 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java @@ -177,9 +177,15 @@ public class SyntaxTreeVisitor { public void visit(SliceExpression node) { visit(node.getObject()); - visit(node.getStart()); - visit(node.getEnd()); - visit(node.getStep()); + if (node.getStart() != null) { + visit(node.getStart()); + } + if (node.getEnd() != null) { + visit(node.getEnd()); + } + if (node.getStep() != null) { + visit(node.getStep()); + } } public void visit(@SuppressWarnings("unused") Comment node) {} diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java index 438c153ede..6553a3d88e 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java @@ -934,7 +934,7 @@ public class MethodLibraryTest extends EvaluationTestCase { public void testIndexOnFunction() throws Exception { new BothModesTest() .testIfErrorContains("type 'function' has no operator [](int)", "len[1]") - .testIfErrorContains("type 'function' has no operator [:](int, int, int)", "len[1:4]"); + .testIfErrorContains("type 'function' has no operator [:](int, int, NoneType)", "len[1:4]"); } @Test 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 f3fbe1e63f..50d6d88fe2 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 @@ -305,16 +305,16 @@ public class ParserTest extends EvaluationTestCase { @Test public void testSlice() throws Exception { - evalSlice("'0123'[:]", Runtime.NONE, Runtime.NONE, 1); - evalSlice("'0123'[1:]", 1, Runtime.NONE, 1); - evalSlice("'0123'[:3]", Runtime.NONE, 3, 1); - evalSlice("'0123'[::]", Runtime.NONE, Runtime.NONE, 1); - evalSlice("'0123'[1::]", 1, Runtime.NONE, 1); - evalSlice("'0123'[:3:]", Runtime.NONE, 3, 1); - evalSlice("'0123'[::-1]", Runtime.NONE, Runtime.NONE, -1); - evalSlice("'0123'[1:3:]", 1, 3, 1); - evalSlice("'0123'[1::-1]", 1, Runtime.NONE, -1); - evalSlice("'0123'[:3:-1]", Runtime.NONE, 3, -1); + evalSlice("'0123'[:]", "", "", ""); + evalSlice("'0123'[1:]", 1, "", ""); + evalSlice("'0123'[:3]", "", 3, ""); + evalSlice("'0123'[::]", "", "", ""); + evalSlice("'0123'[1::]", 1, "", ""); + evalSlice("'0123'[:3:]", "", 3, ""); + evalSlice("'0123'[::-1]", "", "", -1); + evalSlice("'0123'[1:3:]", 1, 3, ""); + evalSlice("'0123'[1::-1]", 1, "", -1); + evalSlice("'0123'[:3:-1]", "", 3, -1); evalSlice("'0123'[1:3:-1]", 1, 3, -1); Expression slice = parseExpression("'0123'[1:3:-1]"); @@ -325,9 +325,13 @@ public class ParserTest extends EvaluationTestCase { SliceExpression e = (SliceExpression) parseExpression(statement); // There is no way to evaluate the expression here, so we rely on string comparison. - assertThat(e.getStart().toString()).isEqualTo(expectedArgs[0].toString()); - assertThat(e.getEnd().toString()).isEqualTo(expectedArgs[1].toString()); - assertThat(e.getStep().toString()).isEqualTo(expectedArgs[2].toString()); + String start = e.getStart() == null ? "" : e.getStart().toString(); + String end = e.getEnd() == null ? "" : e.getEnd().toString(); + String step = e.getStep() == null ? "" : e.getStep().toString(); + + assertThat(start).isEqualTo(expectedArgs[0].toString()); + assertThat(end).isEqualTo(expectedArgs[1].toString()); + assertThat(step).isEqualTo(expectedArgs[2].toString()); } @Test @@ -469,7 +473,7 @@ public class ParserTest extends EvaluationTestCase { @Test public void testPrettyPrintFunctions() throws Exception { assertThat(parseFile("x[1:3]").toString()).isEqualTo("[x[1:3]\n]"); - assertThat(parseFile("x[1:3:1]").toString()).isEqualTo("[x[1:3]\n]"); + assertThat(parseFile("x[1:3:1]").toString()).isEqualTo("[x[1:3:1]\n]"); assertThat(parseFile("x[1:3:2]").toString()).isEqualTo("[x[1:3:2]\n]"); assertThat(parseFile("x[1::2]").toString()).isEqualTo("[x[1::2]\n]"); assertThat(parseFile("x[1:]").toString()).isEqualTo("[x[1:]\n]"); diff --git a/src/test/skylark/testdata/list_slices.sky b/src/test/skylark/testdata/list_slices.sky index 05d422621d..3ec2999cba 100644 --- a/src/test/skylark/testdata/list_slices.sky +++ b/src/test/skylark/testdata/list_slices.sky @@ -28,6 +28,11 @@ assert_eq([1, 2, 3, 4, 5][3:1:-1], [4, 3]) assert_eq([1, 2, 3, 4, 5][::-2], [5, 3, 1]) assert_eq([1, 2, 3, 4, 5][::-10], [5]) +# None +assert_eq([1, 2, 3][None:None:None], [1, 2, 3]) +assert_eq([1, 2, 3][None:None], [1, 2, 3]) +assert_eq([1, 2, 3][None:2:None], [1, 2]) + # Tuples assert_eq(()[1:2], ()) assert_eq(()[::1], ()) |