Skip to content

Commit

Permalink
Fix end offset of expressions in Skylark parser
Browse files Browse the repository at this point in the history 
This CL doesn't fix all of the problems. The end location of blocks
after an if, elif, else and for is still wrong. (I added TODOs for them)
The latter is not easy to fix: One might think that one could just
set the end location of a block to the end location of the last
statement. However, if the last statement is a "pass" statement,
this is not preserved in the AST, in particular, there's no location
for it. In the future, we probably want to preserve "pass" statements.

RELNOTES: none
PiperOrigin-RevId: 170028466
  • Loading branch information
@fanzier @katre
fanzier authored and katre committed Sep 26, 2017
1 parent d16a067 commit aa8540d
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 26 deletions.
46 changes: 28 additions & 18 deletions src/main/java/com/google/devtools/build/lib/syntax/Parser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ private Expression parseSelectorSuffix(int start, Expression receiver) {
expect(TokenKind.DOT);
if (token.kind == TokenKind.IDENTIFIER) {
Identifier ident = parseIdent();
return setLocation(new DotExpression(receiver, ident), start, token.right);
return setLocation(new DotExpression(receiver, ident), start, ident);
} else {
syntaxError(token, "expected identifier after dot");
int end = syncTo(EXPR_TERMINATOR_SET);
Expand Down Expand Up @@ -677,15 +677,18 @@ private Expression parseSubstringSuffix(int start, Expression receiver) {
}
// This is an index/key access
if (token.kind == TokenKind.RBRACKET) {
Expression expr = setLocation(new IndexExpression(receiver, startExpr), start, token.right);
expect(TokenKind.RBRACKET);
return setLocation(new IndexExpression(receiver, startExpr), start, token.right);
return expr;
}
// This is a slice (or substring)
Expression endExpr = parseSliceArgument(new Identifier("None"));
Expression stepExpr = parseSliceArgument(new IntegerLiteral(1));
Expression expr =
setLocation(
new SliceExpression(receiver, startExpr, endExpr, stepExpr), start, token.right);
expect(TokenKind.RBRACKET);
return setLocation(new SliceExpression(receiver, startExpr, endExpr, stepExpr),
start, token.right);
return expr;
}

/**
Expand Down Expand Up @@ -735,14 +738,16 @@ private Expression parseForLoopVariables() {
}
tuple.add(parsePrimaryWithSuffix());
}
return setLocation(ListLiteral.makeTuple(tuple), start, token.right);
return setLocation(ListLiteral.makeTuple(tuple), start, Iterables.getLast(tuple));
}

// comprehension_suffix ::= 'FOR' loop_variables 'IN' expr comprehension_suffix
// | 'IF' expr comprehension_suffix
// | ']'
private Expression parseComprehensionSuffix(
AbstractComprehension.AbstractBuilder comprehensionBuilder, TokenKind closingBracket) {
AbstractComprehension.AbstractBuilder comprehensionBuilder,
TokenKind closingBracket,
int comprehensionStartOffset) {
while (true) {
if (token.kind == TokenKind.FOR) {
nextToken();
Expand All @@ -758,12 +763,14 @@ private Expression parseComprehensionSuffix(
// [x for x in li if (1, 2)] # ok
comprehensionBuilder.addIf(parseNonTupleExpression(0));
} else if (token.kind == closingBracket) {
Expression expr = comprehensionBuilder.build();
setLocation(expr, comprehensionStartOffset, token.right);
nextToken();
return comprehensionBuilder.build();
return expr;
} else {
syntaxError(token, "expected '" + closingBracket.getPrettyName() + "', 'for' or 'if'");
syncPast(LIST_TERMINATOR_SET);
return makeErrorExpression(token.left, token.right);
return makeErrorExpression(comprehensionStartOffset, token.right);
}
}
}
Expand Down Expand Up @@ -794,11 +801,10 @@ private Expression parseListMaker() {
}
case FOR:
{ // list comprehension
Expression result =
parseComprehensionSuffix(
new ListComprehension.Builder().setOutputExpression(expression),
TokenKind.RBRACKET);
return setLocation(result, start, token.right);
return parseComprehensionSuffix(
new ListComprehension.Builder().setOutputExpression(expression),
TokenKind.RBRACKET,
start);
}
case COMMA:
{
Expand Down Expand Up @@ -843,12 +849,12 @@ private Expression parseDictExpression() {
DictionaryEntryLiteral entry = parseDictEntry();
if (token.kind == TokenKind.FOR) {
// Dict comprehension
Expression result = parseComprehensionSuffix(
return parseComprehensionSuffix(
new DictComprehension.Builder()
.setKeyExpression(entry.getKey())
.setValueExpression(entry.getValue()),
TokenKind.RBRACE);
return setLocation(result, start, token.right);
TokenKind.RBRACE,
start);
}
List<DictionaryEntryLiteral> entries = new ArrayList<>();
entries.add(entry);
Expand Down Expand Up @@ -959,7 +965,7 @@ private Expression parseExpression(boolean insideParens) {
// 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, token.right);
return setLocation(ListLiteral.makeTuple(tuple), start, Iterables.getLast(tuple));
}

// Equivalent to 'test' rule in Python grammar.
Expand Down Expand Up @@ -1000,7 +1006,7 @@ private Expression parseNotExpression(int prec) {
Expression expression = parseNonTupleExpression(prec + 1);
UnaryOperatorExpression notExpression =
new UnaryOperatorExpression(UnaryOperator.NOT, expression);
return setLocation(notExpression, start, token.right);
return setLocation(notExpression, start, expression);
}

// file_input ::= ('\n' | stmt)* EOF
Expand Down Expand Up @@ -1187,6 +1193,7 @@ private IfStatement parseIfStatement() {
} else {
elseBlock = ImmutableList.of();
}
// TODO(skylark-team): the end offset should be the *previous* token, not the current one.
return setLocation(new IfStatement(thenBlocks, elseBlock), start, token.right);
}

Expand All @@ -1198,6 +1205,7 @@ private ConditionalStatements parseConditionalStatements(TokenKind tokenKind) {
expect(TokenKind.COLON);
List<Statement> thenBlock = parseSuite();
ConditionalStatements stmt = new ConditionalStatements(expr, thenBlock);
// TODO(skylark-team): the end offset should be the *previous* token, not the current one.
return setLocation(stmt, start, token.right);
}

Expand All @@ -1211,6 +1219,7 @@ private void parseForStatement(List<Statement> list) {
expect(TokenKind.COLON);
List<Statement> block = parseSuite();
Statement stmt = new ForStatement(new LValue(loopVar), collection, block);
// TODO(skylark-team): the end offset should be the *previous* token, not the current one.
list.add(setLocation(stmt, start, token.right));
}

Expand All @@ -1227,6 +1236,7 @@ private void parseFunctionDefStatement(List<Statement> list) {
expect(TokenKind.COLON);
List<Statement> block = parseSuite();
FunctionDefStatement stmt = new FunctionDefStatement(ident, params, signature, block);
// TODO(skylark-team): the end offset should be the *previous* token, not the current one.
list.add(setLocation(stmt, start, token.right));
}

Expand Down
55 changes: 47 additions & 8 deletions src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLiteral;
import com.google.devtools.build.lib.syntax.Parser.ParsingLevel;
import com.google.devtools.build.lib.syntax.SkylarkImports.SkylarkImportSyntaxException;
import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -488,8 +489,8 @@ public void testFuncallLocation() {
@Test
public void testListPositions() throws Exception {
String expr = "[0,f(1),2]";
assertExpressionLocationCorrect(expr);
ListLiteral list = (ListLiteral) parseExpression(expr);
assertThat(getText(expr, list)).isEqualTo("[0,f(1),2]");
assertThat(getText(expr, getElem(list, 0))).isEqualTo("0");
assertThat(getText(expr, getElem(list, 1))).isEqualTo("f(1)");
assertThat(getText(expr, getElem(list, 2))).isEqualTo("2");
Expand All @@ -498,21 +499,59 @@ public void testListPositions() throws Exception {
@Test
public void testDictPositions() throws Exception {
String expr = "{1:2,2:f(1),3:4}";
assertExpressionLocationCorrect(expr);
DictionaryLiteral list = (DictionaryLiteral) parseExpression(expr);
assertThat(getText(expr, list)).isEqualTo("{1:2,2:f(1),3:4}");
assertThat(getText(expr, getElem(list, 0))).isEqualTo("1:2");
assertThat(getText(expr, getElem(list, 1))).isEqualTo("2:f(1)");
assertThat(getText(expr, getElem(list, 2))).isEqualTo("3:4");
}

@Test
public void testArgumentPositions() throws Exception {
String stmt = "f(0,g(1,2),2)";
FuncallExpression f = (FuncallExpression) parseExpression(stmt);
assertThat(getText(stmt, f)).isEqualTo(stmt);
assertThat(getText(stmt, getArg(f, 0))).isEqualTo("0");
assertThat(getText(stmt, getArg(f, 1))).isEqualTo("g(1,2)");
assertThat(getText(stmt, getArg(f, 2))).isEqualTo("2");
String expr = "f(0,g(1,2),2)";
assertExpressionLocationCorrect(expr);
FuncallExpression f = (FuncallExpression) parseExpression(expr);
assertThat(getText(expr, getArg(f, 0))).isEqualTo("0");
assertThat(getText(expr, getArg(f, 1))).isEqualTo("g(1,2)");
assertThat(getText(expr, getArg(f, 2))).isEqualTo("2");
}

@Test
public void testSuffixPosition() throws Exception {
assertExpressionLocationCorrect("'a'.len");
assertExpressionLocationCorrect("'a'[0]");
assertExpressionLocationCorrect("'a'[0:1]");
}

@Test
public void testTuplePosition() throws Exception {
String input = "for a,b in []: pass";
ForStatement stmt = (ForStatement) parseStatement(ParsingLevel.LOCAL_LEVEL, input);
assertThat(getText(input, stmt.getVariable())).isEqualTo("a,b");
input = "for (a,b) in []: pass";
stmt = (ForStatement) parseStatement(ParsingLevel.LOCAL_LEVEL, input);
assertThat(getText(input, stmt.getVariable())).isEqualTo("(a,b)");
assertExpressionLocationCorrect("a, b");
assertExpressionLocationCorrect("(a, b)");
}

@Test
public void testComprehensionPosition() throws Exception {
assertExpressionLocationCorrect("[[] for x in []]");
assertExpressionLocationCorrect("{1: [] for x in []}");
}

@Test
public void testUnaryOperationPosition() throws Exception {
assertExpressionLocationCorrect("not True");
}

private void assertExpressionLocationCorrect(String exprStr) {
Expression expr = parseExpression(exprStr);
assertThat(getText(exprStr, expr)).isEqualTo(exprStr);
// Also try it with another token at the end (newline), which broke the location in the past.
expr = parseExpression(exprStr + "\n");
assertThat(getText(exprStr, expr)).isEqualTo(exprStr);
}

@Test
Expand Down

0 comments on commit aa8540d

Please sign in to comment.