Skylark: SlicingExpression: do not create new nodes for optional expressions
RELNOTES: None.
PiperOrigin-RevId: 185353994
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 88be43b..b4f3e69 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 @@
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 ddea297..b535030 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.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 @@
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 @@
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 @@
}
// 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 @@
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 @@
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 @@
/**
* 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 @@
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 @@
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 a2b6036..54c194a 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 @@
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 @@
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 @@
@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 22bc8b7..9845a8e 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 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) {}
