bazel syntax: fine-grained syntax locations
This change improves the precision with which the locations
of source tokens are recorded in the syntax tree. Prior to
this change, every Node held a single LexerLocation object
that recorded the start and end offsets of the node, plus
a reference to the shared LineNumberTable (LNT), that maps
these offsets to Locations. This had a cost of one reference
and one LexerLocation object per node.
This change causes every Node to record the offsets only of
its salient tokens, plus a reference to the LNT. For example,
in the expression "1 + 2", the only salient token is the plus
operator; the start and end offsets can be computed inductively
by delegating to x.getStartLocation and y.getEndLocation.
Similarly, in f(x), the salient tokens are '(' and ')'.
This has a cost of 1 word plus approximately 1 int per Node.
Consequently, we can record the exact position of operators
that fail, and do so using less memory than before.
Now, when an expression such as 'f().g() + 1' fails,
the location in the error message will refer to the '+'
operator or one of the two '(' tokens. Before, all
three errors would be wrongly reported at the same place:
f, since it is the start of all three subexpressions.
Overview:
- Every Node has a reference to the LNT, set immediately
after construction. (Morally it is part of the constructor
but it's fussy to set it that way.)
- Every node defines getStartOffset and getEndOffset,
typically by delegating to its left and right subtrees.
- Node end offsets are exclusive again. CL 170723732 was a mistake:
half-open intervals are mathematically simpler.
A client that wants to subtract one may do that.
But there are none.
- Comprehension.{For,If} are now true Nodes.
- StarlarkFile's extent is now (correctly) the entire file,
not just the range from the first statement to the last.
- The parser provides offsets of salient tokens to the Node constructors.
- IntegerLiteral now retains the raw token text in addition to the value.
- Token is gone. Its four fields are now embedded in the Lexer.
- Eval uses the following token positions in run-time error messages:
x+y f(x) x[i] x.y x[i:j] k: v
^ ^ ^ ^ ^ ^
- Location is final. LexerLocation and LineAndColumn are gone.
- Misparsed source represented as an Identifier now has the text of the
source instead of "$error$". This is more faithful and causes
the offsets to be correct.
- The offsets of the orig Identifier in load("module", local="orig")
coincide with the text 'orig', sans quotation marks.
Benchmark: saves about 65MB (1% of live RAM) retained by the
Usual Benchmark, a deps query.
RELNOTES: N/A
PiperOrigin-RevId: 305803031
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 2c4d394..79846fd 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -937,8 +937,7 @@
&& arg.getName().equals("name")
&& arg.getValue() instanceof StringLiteral) {
generatorNameByLocation.put(
- // TODO(adonovan): use lparen location
- call.getStartLocation(), ((StringLiteral) arg.getValue()).getValue());
+ call.getLparenLocation(), ((StringLiteral) arg.getValue()).getValue());
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Argument.java b/src/main/java/com/google/devtools/build/lib/syntax/Argument.java
index cc54b58..37d2738 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Argument.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Argument.java
@@ -24,7 +24,7 @@
*/
public abstract class Argument extends Node {
- private final Expression value;
+ protected final Expression value;
Argument(Expression value) {
this.value = Preconditions.checkNotNull(value);
@@ -34,6 +34,11 @@
return value;
}
+ @Override
+ public int getEndOffset() {
+ return value.getEndOffset();
+ }
+
/** Return the name of this argument's parameter, or null if it is not a Keyword argument. */
@Nullable
public String getName() {
@@ -45,6 +50,11 @@
Positional(Expression value) {
super(value);
}
+
+ @Override
+ public int getStartOffset() {
+ return value.getStartOffset();
+ }
}
/** Syntax node for a keyword argument, {@code f(id=expr)}. */
@@ -53,34 +63,55 @@
// Unlike in Python, keyword arguments in Bazel BUILD files
// are about 10x more numerous than positional arguments.
- final Identifier identifier;
+ final Identifier id;
- Keyword(Identifier identifier, Expression value) {
+ Keyword(Identifier id, Expression value) {
super(value);
- this.identifier = identifier;
+ this.id = id;
}
public Identifier getIdentifier() {
- return identifier;
+ return id;
}
@Override
public String getName() {
- return identifier.getName();
+ return id.getName();
+ }
+
+ @Override
+ public int getStartOffset() {
+ return id.getStartOffset();
}
}
/** Syntax node for an argument of the form {@code f(*expr)}. */
public static final class Star extends Argument {
- Star(Expression value) {
+ private final int starOffset;
+
+ Star(int starOffset, Expression value) {
super(value);
+ this.starOffset = starOffset;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return starOffset;
}
}
/** Syntax node for an argument of the form {@code f(**expr)}. */
public static final class StarStar extends Argument {
- StarStar(Expression value) {
+ private final int starStarOffset;
+
+ StarStar(int starStarOffset, Expression value) {
super(value);
+ this.starStarOffset = starStarOffset;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return starStarOffset;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
index d750ac7..2b56c7c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
@@ -23,7 +23,8 @@
public final class AssignmentStatement extends Statement {
private final Expression lhs; // = IDENTIFIER | DOT | INDEX | LIST_EXPR
- @Nullable private final TokenKind op;
+ @Nullable private final TokenKind op; // TODO(adonovan): make this mandatory even when '='.
+ private final int opOffset;
private final Expression rhs;
/**
@@ -32,9 +33,10 @@
* {@code (e, ...)}, where x, i, and e are arbitrary expressions. For an augmented assignment, the
* list and tuple forms are disallowed.
*/
- AssignmentStatement(Expression lhs, @Nullable TokenKind op, Expression rhs) {
+ AssignmentStatement(Expression lhs, @Nullable TokenKind op, int opOffset, Expression rhs) {
this.lhs = lhs;
this.op = op;
+ this.opOffset = opOffset;
this.rhs = rhs;
}
@@ -49,6 +51,21 @@
return op;
}
+ /** Returns the location of the assignment operator. */
+ public Location getOperatorLocation() {
+ return lnt.getLocation(opOffset);
+ }
+
+ @Override
+ public int getStartOffset() {
+ return lhs.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rhs.getEndOffset();
+ }
+
/** Reports whether this is an augmented assignment ({@code getOperator() != null}). */
public boolean isAugmented() {
return op != null;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BUILD b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
index c301fbb..9833bde 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
@@ -66,7 +66,6 @@
"Statement.java",
"StringLiteral.java",
"SyntaxError.java",
- "Token.java",
"TokenKind.java",
"UnaryOperatorExpression.java",
"ValidationEnvironment.java",
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index fef702d..aa0de06 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -20,6 +20,7 @@
private final Expression x;
private final TokenKind op; // one of 'operators'
+ private final int opOffset;
private final Expression y;
/** operators is the set of valid binary operators. */
@@ -43,9 +44,10 @@
TokenKind.PIPE,
TokenKind.STAR);
- BinaryOperatorExpression(Expression x, TokenKind op, Expression y) {
+ BinaryOperatorExpression(Expression x, TokenKind op, int opOffset, Expression y) {
this.x = x;
this.op = op;
+ this.opOffset = opOffset;
this.y = y;
}
@@ -59,12 +61,26 @@
return op;
}
+ public Location getOperatorLocation() {
+ return lnt.getLocation(opOffset);
+ }
+
/** Returns the right operand. */
public Expression getY() {
return y;
}
@Override
+ public int getStartOffset() {
+ return x.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return y.getEndOffset();
+ }
+
+ @Override
public String toString() {
// This omits the parentheses for brevity, but is not correct in general due to operator
// precedence rules.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/CallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/CallExpression.java
index 699bbff..64af972 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/CallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/CallExpression.java
@@ -21,12 +21,21 @@
public final class CallExpression extends Expression {
private final Expression function;
+ private final Location lparenLocation;
private final ImmutableList<Argument> arguments;
+ private final int rparenOffset;
+
private final int numPositionalArgs;
- CallExpression(Expression function, ImmutableList<Argument> arguments) {
+ CallExpression(
+ Expression function,
+ Location lparenLocation,
+ ImmutableList<Argument> arguments,
+ int rparenOffset) {
this.function = Preconditions.checkNotNull(function);
+ this.lparenLocation = lparenLocation;
this.arguments = arguments;
+ this.rparenOffset = rparenOffset;
int n = 0;
for (Argument arg : arguments) {
@@ -53,6 +62,25 @@
}
@Override
+ public int getStartOffset() {
+ return function.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rparenOffset + 1;
+ }
+
+ public Location getLparenLocation() {
+ // Unlike all other getXXXLocation methods, this one returns a reference to
+ // a previously materialized Location. getLparenLocation is unique among
+ // locations because the tree-walking evaluator needs it frequently even
+ // in the absence of errors. When we switch to a compiled representation
+ // we can dispense with this optimization.
+ return lparenLocation;
+ }
+
+ @Override
public String toString() {
StringBuilder buf = new StringBuilder();
buf.append(function);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Comment.java b/src/main/java/com/google/devtools/build/lib/syntax/Comment.java
index 161c7a4..ee7d6a7 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Comment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Comment.java
@@ -17,14 +17,26 @@
/** Syntax node for comments. */
public final class Comment extends Node {
- protected final String value;
+ private final int offset;
+ private final String text;
- public Comment(String value) {
- this.value = value;
+ Comment(int offset, String text) {
+ this.offset = offset;
+ this.text = text;
}
- public String getValue() {
- return value;
+ public String getText() {
+ return text;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return offset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return offset + text.length();
}
@Override
@@ -34,6 +46,6 @@
@Override
public String toString() {
- return value;
+ return text;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java
index 771196e..4549bbc 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Comprehension.java
@@ -37,14 +37,16 @@
public final class Comprehension extends Expression {
/** For or If */
- public abstract static class Clause {}
+ public abstract static class Clause extends Node {}
/** A for clause in a comprehension, e.g. "for a in b" in the example above. */
public static final class For extends Clause {
+ private final int forOffset;
private final Expression vars;
private final Expression iterable;
- For(Expression vars, Expression iterable) {
+ For(int forOffset, Expression vars, Expression iterable) {
+ this.forOffset = forOffset;
this.vars = vars;
this.iterable = iterable;
}
@@ -56,29 +58,70 @@
public Expression getIterable() {
return iterable;
}
+
+ @Override
+ public int getStartOffset() {
+ return forOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return iterable.getEndOffset();
+ }
+
+ @Override
+ public void accept(NodeVisitor visitor) {
+ visitor.visit(this);
+ }
}
/** A if clause in a comprehension, e.g. "if c" in the example above. */
public static final class If extends Clause {
+ private final int ifOffset;
private final Expression condition;
- If(Expression condition) {
+ If(int ifOffset, Expression condition) {
+ this.ifOffset = ifOffset;
this.condition = condition;
}
public Expression getCondition() {
return condition;
}
+
+ @Override
+ public int getStartOffset() {
+ return ifOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return condition.getEndOffset();
+ }
+
+ @Override
+ public void accept(NodeVisitor visitor) {
+ visitor.visit(this);
+ }
}
private final boolean isDict; // {k: v for vars in iterable}
+ private final int lbracketOffset;
private final Node body; // Expression or DictExpression.Entry
private final ImmutableList<Clause> clauses;
+ private final int rbracketOffset;
- Comprehension(boolean isDict, Node body, ImmutableList<Clause> clauses) {
+ Comprehension(
+ boolean isDict,
+ int lbracketOffset,
+ Node body,
+ ImmutableList<Clause> clauses,
+ int rbracketOffset) {
this.isDict = isDict;
+ this.lbracketOffset = lbracketOffset;
this.body = body;
this.clauses = clauses;
+ this.rbracketOffset = rbracketOffset;
}
public boolean isDict() {
@@ -98,6 +141,16 @@
}
@Override
+ public int getStartOffset() {
+ return lbracketOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rbracketOffset + 1;
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
index ffeb9ea..8085d55 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
@@ -13,25 +13,40 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
-
-/** Syntax node for an if/else expression. */
+/** Syntax node for an expression of the form {@code t if cond else f}. */
public final class ConditionalExpression extends Expression {
- // Python conditional expressions: $thenCase if $condition else $elseCase
- // https://docs.python.org/3.5/reference/expressions.html#conditional-expressions
- private final Expression thenCase;
- private final Expression condition;
- private final Expression elseCase;
+ private final Expression t;
+ private final Expression cond;
+ private final Expression f;
- public Expression getThenCase() { return thenCase; }
- public Expression getCondition() { return condition; }
- public Expression getElseCase() { return elseCase; }
+ public Expression getThenCase() {
+ return t;
+ }
+
+ public Expression getCondition() {
+ return cond;
+ }
+
+ public Expression getElseCase() {
+ return f;
+ }
/** Constructor for a conditional expression */
- ConditionalExpression(Expression thenCase, Expression condition, Expression elseCase) {
- this.thenCase = thenCase;
- this.condition = condition;
- this.elseCase = elseCase;
+ ConditionalExpression(Expression t, Expression cond, Expression f) {
+ this.t = t;
+ this.cond = cond;
+ this.f = f;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return t.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return f.getEndOffset();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/DefStatement.java
index 1fe77f5..d128c0e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DefStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DefStatement.java
@@ -19,20 +19,23 @@
/** Syntax node for a 'def' statement, which defines a function. */
public final class DefStatement extends Statement {
+ private final int defOffset;
private final Identifier identifier;
private final FunctionSignature signature;
- private final ImmutableList<Statement> statements;
+ private final ImmutableList<Statement> body; // non-empty if well formed
private final ImmutableList<Parameter> parameters;
DefStatement(
+ int defOffset,
Identifier identifier,
ImmutableList<Parameter> parameters,
FunctionSignature signature,
- ImmutableList<Statement> statements) {
+ ImmutableList<Statement> body) {
+ this.defOffset = defOffset;
this.identifier = identifier;
this.parameters = Preconditions.checkNotNull(parameters);
- this.signature = signature;
- this.statements = Preconditions.checkNotNull(statements);
+ this.signature = Preconditions.checkNotNull(signature);
+ this.body = Preconditions.checkNotNull(body);
}
@Override
@@ -48,8 +51,9 @@
return identifier;
}
+ // TODO(adonovan): rename to getBody.
public ImmutableList<Statement> getStatements() {
- return statements;
+ return body;
}
public ImmutableList<Parameter> getParameters() {
@@ -61,6 +65,18 @@
}
@Override
+ public int getStartOffset() {
+ return defOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return body.isEmpty()
+ ? identifier.getEndOffset() // wrong, but tree is ill formed
+ : body.get(body.size() - 1).getEndOffset();
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java
index e0b1fa3..c144d4a7 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictExpression.java
@@ -23,10 +23,12 @@
public static final class Entry extends Node {
private final Expression key;
+ private final int colonOffset;
private final Expression value;
- Entry(Expression key, Expression value) {
+ Entry(Expression key, int colonOffset, Expression value) {
this.key = key;
+ this.colonOffset = colonOffset;
this.value = value;
}
@@ -39,15 +41,43 @@
}
@Override
+ public int getStartOffset() {
+ return key.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return value.getEndOffset();
+ }
+
+ public Location getColonLocation() {
+ return lnt.getLocation(colonOffset);
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
}
+ private final int lbraceOffset;
private final ImmutableList<Entry> entries;
+ private final int rbraceOffset;
- DictExpression(List<Entry> entries) {
+ DictExpression(int lbraceOffset, List<Entry> entries, int rbraceOffset) {
+ this.lbraceOffset = lbraceOffset;
this.entries = ImmutableList.copyOf(entries);
+ this.rbraceOffset = rbraceOffset;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return lbraceOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rbraceOffset + 1;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index 102f1c3..65b2048 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -18,11 +18,12 @@
public final class DotExpression extends Expression {
private final Expression object;
-
+ private final int dotOffset;
private final Identifier field;
- DotExpression(Expression object, Identifier field) {
+ DotExpression(Expression object, int dotOffset, Identifier field) {
this.object = object;
+ this.dotOffset = dotOffset;
this.field = field;
}
@@ -35,6 +36,20 @@
}
@Override
+ public int getStartOffset() {
+ return object.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return field.getEndOffset();
+ }
+
+ public Location getDotLocation() {
+ return lnt.getLocation(dotOffset);
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index d979b9b..fd245e8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -81,8 +81,7 @@
try {
assign(fr, node.getLHS(), rvalue);
} catch (EvalException ex) {
- // TODO(adonovan): use location of = operator.
- throw ex.ensureLocation(node.getStartLocation());
+ throw ex.ensureLocation(node.getOperatorLocation());
}
}
}
@@ -113,7 +112,7 @@
}
}
} catch (EvalException ex) {
- throw ex.ensureLocation(node.getLHS().getStartLocation());
+ throw ex.ensureLocation(node.getStartLocation());
} finally {
EvalUtils.removeIterator(o);
}
@@ -173,7 +172,7 @@
StarlarkThread.Extension module = fr.thread.getExtension(moduleName);
if (module == null) {
throw new EvalException(
- node.getImport().getStartLocation(),
+ node.getStartLocation(),
String.format(
"file '%s' was not correctly loaded. "
+ "Make sure the 'load' statement appears in the global scope in your file",
@@ -220,7 +219,7 @@
private static TokenKind exec(StarlarkThread.Frame fr, Statement st)
throws EvalException, InterruptedException {
if (fr.dbg != null) {
- Location loc = st.getStartLocation();
+ Location loc = st.getStartLocation(); // not very precise
fr.setLocation(loc);
fr.dbg.before(fr.thread, loc); // location is now redundant since it's in the thread
}
@@ -385,19 +384,18 @@
try {
EvalUtils.setIndex(object, key, z);
} catch (EvalException ex) {
- Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
- throw ex.ensureLocation(loc);
+ throw ex.ensureLocation(stmt.getOperatorLocation());
}
} else if (lhs instanceof ListExpression) {
// TODO(adonovan): make this a static error.
- Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
- throw new EvalException(loc, "cannot perform augmented assignment on a list literal");
+ throw new EvalException(
+ stmt.getOperatorLocation(), "cannot perform augmented assignment on a list literal");
} else {
// Not possible for validated ASTs.
- Location loc = stmt.getStartLocation(); // TODO(adonovan): use operator location
- throw new EvalException(loc, "cannot perform augmented assignment on '" + lhs + "'");
+ throw new EvalException(
+ stmt.getOperatorLocation(), "cannot perform augmented assignment on '" + lhs + "'");
}
}
@@ -445,8 +443,7 @@
return EvalUtils.binaryOp(
binop.getOperator(), x, y, fr.thread.getSemantics(), fr.thread.mutability());
} catch (EvalException ex) {
- // TODO(adonovan): use operator location
- ex.ensureLocation(binop.getStartLocation());
+ ex.ensureLocation(binop.getOperatorLocation());
throw ex;
}
}
@@ -473,13 +470,11 @@
try {
dict.put(k, v, (Location) null);
} catch (EvalException ex) {
- // TODO(adonovan): use colon location
- throw ex.ensureLocation(entry.getKey().getStartLocation());
+ throw ex.ensureLocation(entry.getColonLocation());
}
if (dict.size() == before) {
- // TODO(adonovan): use colon location
throw new EvalException(
- entry.getKey().getStartLocation(),
+ entry.getColonLocation(),
"Duplicated key " + Starlark.repr(k) + " when creating dictionary");
}
}
@@ -498,7 +493,7 @@
}
return result;
} catch (EvalException ex) {
- throw ex.ensureLocation(dot.getStartLocation()); // TODO(adonovan): use dot token
+ throw ex.ensureLocation(dot.getDotLocation());
}
}
@@ -582,7 +577,7 @@
}
}
- Location loc = call.getStartLocation(); // TODO(adonovan): use call lparen
+ Location loc = call.getLparenLocation(); // (Location is prematerialized)
fr.setLocation(loc);
try {
return Starlark.fastcall(fr.thread, fn, positional, named);
@@ -657,8 +652,7 @@
try {
return EvalUtils.index(fr.thread.mutability(), fr.thread.getSemantics(), object, key);
} catch (EvalException ex) {
- // TODO(adonovan): use location of lbracket token
- throw ex.ensureLocation(index.getStartLocation());
+ throw ex.ensureLocation(index.getLbracketLocation());
}
}
@@ -688,8 +682,7 @@
try {
return Starlark.slice(fr.thread.mutability(), x, start, stop, step);
} catch (EvalException ex) {
- // TODO(adonovan): use lbracket location
- throw ex.ensureLocation(slice.getStartLocation());
+ throw ex.ensureLocation(slice.getLbracketLocation());
}
}
@@ -754,8 +747,7 @@
execClauses(index + 1);
}
} catch (EvalException ex) {
- // TODO(adonovan): use location of 'for' token
- throw ex.ensureLocation(comp.getStartLocation());
+ throw ex.ensureLocation(forClause.getStartLocation());
} finally {
EvalUtils.removeIterator(iterable);
}
@@ -778,8 +770,7 @@
try {
dict.put(k, v, (Location) null);
} catch (EvalException ex) {
- // TODO(adonovan): use colon location
- throw ex.ensureLocation(comp.getStartLocation());
+ throw ex.ensureLocation(body.getColonLocation());
}
} else {
list.add(eval(fr, ((Expression) comp.getBody())));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
index 90edb7c..ee64493 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalExceptionWithStackTrace.java
@@ -18,9 +18,8 @@
import java.util.Deque;
import java.util.LinkedList;
-/**
- * EvalException with a stack trace.
- */
+/** EvalException with a stack trace. */
+// TODO(adonovan): get rid of this. Every EvalException should record the stack.
public class EvalExceptionWithStackTrace extends EvalException {
private StackFrame mostRecentElement;
@@ -45,11 +44,17 @@
*/
private static Location extractLocation(Exception original, Node culprit) {
if (culprit != null) {
- return culprit.getStartLocation();
+ return nodeLocation(culprit);
}
return original instanceof EvalException ? ((EvalException) original).getLocation() : null;
}
+ private static Location nodeLocation(Node node) {
+ return node instanceof CallExpression
+ ? ((CallExpression) node).getLparenLocation()
+ : node.getStartLocation();
+ }
+
/**
* Returns the "real" cause of this exception.
*
@@ -61,8 +66,8 @@
}
/** Adds an entry for the given {@code Node} to the stack trace. */
- public void registerNode(Node node) {
- addStackFrame(node.toString().trim(), node.getStartLocation());
+ void registerNode(Node node) {
+ addStackFrame(node.toString().trim(), nodeLocation(node));
}
/**
@@ -109,12 +114,24 @@
/** Adds a line for the given frame. */
private void addStackFrame(String label, Location location, boolean canPrint) {
// TODO(bazel-team): This check was originally created to weed out duplicates in case the same
- // node is added twice, but it's not clear if that is still a possibility. In any case, it would
- // be better to eliminate the check and not create unwanted duplicates in the first place.
+ // node is added twice, but it's not clear if that is still a possibility.
//
- // The check is problematic because it suppresses tracebacks in the REPL, where line numbers
- // can be reset within a single session.
- if (mostRecentElement != null && location.equals(mostRecentElement.getLocation())) {
+ // [I suspect the real reason it was added is not because of duplicate nodes,
+ // but because the stack corresponds to the stack of expressions in the tree-walking
+ // evaluator's recursion, which often includes several subexpressions within
+ // the same line, e.g. f().g()+1. If the stack had one entry per function call,
+ // like StarlarkThread.CallStack, there would be no problem.
+ // This was revealed when we started recording operator positions precisely,
+ // causing the f(), .g(), and + operations in the example above to have different
+ // locations within the same line. --adonovan]
+ //
+ // In any case, it would be better to eliminate the check and not create unwanted duplicates in
+ // the first place.
+ // The check is problematic because it suppresses tracebacks in the REPL,
+ // where line numbers can be reset within a single session.
+ if (mostRecentElement != null
+ && location.file().equals(mostRecentElement.getLocation().file())
+ && location.line() == mostRecentElement.getLocation().line()) {
return;
}
mostRecentElement = new StackFrame(label, location, mostRecentElement, canPrint);
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 8ba6bb1..8250178 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
@@ -782,7 +782,7 @@
expr.getStartLocation(),
FunctionSignature.NOARGS,
/*defaultValues=*/ Tuple.empty(),
- ImmutableList.<Statement>of(new ReturnStatement(expr)),
+ ImmutableList.<Statement>of(ReturnStatement.make(expr)),
module);
return Starlark.fastcall(thread, fn, NOARGS, NOARGS);
@@ -816,7 +816,7 @@
stmts =
ImmutableList.<Statement>builder()
.addAll(stmts.subList(0, n - 1))
- .add(new ReturnStatement(expr))
+ .add(ReturnStatement.make(expr))
.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
index 1ff2b21..8e54ff0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
@@ -14,8 +14,7 @@
package com.google.devtools.build.lib.syntax;
-
-/** Syntax node for a function call statement. Used for build rules. */
+/** Syntax node for a statement consisting of an expression evaluated for effect. */
public final class ExpressionStatement extends Statement {
private final Expression expression;
@@ -34,6 +33,16 @@
}
@Override
+ public int getStartOffset() {
+ return expression.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return expression.getEndOffset();
+ }
+
+ @Override
public Kind kind() {
return Kind.EXPRESSION;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
index b24a672..1637934 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
@@ -18,10 +18,12 @@
public final class FlowStatement extends Statement {
private final TokenKind kind; // BREAK | CONTINUE | PASS
+ private final int offset;
/** @param kind The label of the statement (break, continue, or pass) */
- FlowStatement(TokenKind kind) {
+ FlowStatement(TokenKind kind, int offset) {
this.kind = kind;
+ this.offset = offset;
}
public TokenKind getKind() {
@@ -34,6 +36,16 @@
}
@Override
+ public int getStartOffset() {
+ return offset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return offset + kind.toString().length();
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index 5b43f75..9b3fdb9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -20,17 +20,20 @@
/** Syntax node for a for loop statement. */
public final class ForStatement extends Statement {
+ private final int forOffset;
private final Expression lhs;
private final Expression collection;
- private final ImmutableList<Statement> block;
+ private final ImmutableList<Statement> block; // non-empty if well formed
/** Constructs a for loop statement. */
- ForStatement(Expression lhs, Expression collection, List<Statement> block) {
+ ForStatement(int forOffset, Expression lhs, Expression collection, List<Statement> block) {
+ this.forOffset = forOffset;
this.lhs = Preconditions.checkNotNull(lhs);
this.collection = Preconditions.checkNotNull(collection);
this.block = ImmutableList.copyOf(block);
}
+ // TODO(adonovan): rename to getVars.
public Expression getLHS() {
return lhs;
}
@@ -47,6 +50,18 @@
}
@Override
+ public int getStartOffset() {
+ return forOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return block.isEmpty()
+ ? collection.getEndOffset() // wrong, but tree is ill formed
+ : block.get(block.size() - 1).getEndOffset();
+ }
+
+ @Override
public String toString() {
return "for " + lhs + " in " + collection + ": ...\n";
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index 76af45b..70af64e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -27,16 +27,30 @@
public final class Identifier extends Expression {
private final String name;
+ private final int nameOffset;
+
// The scope of the variable. The value is set when the AST has been analysed by
// ValidationEnvironment.
@Nullable private ValidationEnvironment.Scope scope;
- Identifier(String name) {
+ Identifier(String name, int nameOffset) {
this.name = name;
+ this.nameOffset = nameOffset;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return nameOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return nameOffset + name.length();
}
/**
- * Returns the name of the Identifier.
+ * Returns the name of the Identifier. If there were parse errors, misparsed regions may be
+ * represented as an Identifier for which {@code !isValid(getName())}.
*/
public String getName() {
return name;
@@ -65,32 +79,19 @@
return Kind.IDENTIFIER;
}
- /** @return The {@link Identifier} of the provided name. */
- static Identifier of(String name) {
- return new Identifier(name);
- }
-
- /** Returns true if the string is a syntactically valid identifier. */
+ /** Reports whether the string is a valid identifier. */
public static boolean isValid(String name) {
- // TODO(laurentlb): Handle Unicode characters.
- if (name.isEmpty()) {
- return false;
- }
+ // Keep consistent with Lexer.scanIdentifier.
for (int i = 0; i < name.length(); i++) {
char c = name.charAt(i);
- if ((c >= 'a' && c <= 'z')
- || (c >= 'A' && c <= 'Z')
- || (c >= '0' && c <= '9')
- || (c == '_')) {
- continue;
+ if (!(('a' <= c && c <= 'z')
+ || ('A' <= c && c <= 'Z')
+ || (i > 0 && '0' <= c && c <= '9')
+ || (c == '_'))) {
+ return false;
}
- return false;
}
- if (name.charAt(0) >= '0' && name.charAt(0) <= '9') {
- return false;
- }
-
- return true;
+ return !name.isEmpty();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
index 378bfbd..5769057 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
@@ -21,12 +21,15 @@
public final class IfStatement extends Statement {
private final TokenKind token; // IF or ELIF
+ private final int ifOffset;
private final Expression condition;
+ // These blocks may be non-null but empty after a misparse:
private final ImmutableList<Statement> thenBlock; // non-empty
@Nullable ImmutableList<Statement> elseBlock; // non-empty if non-null; set after construction
- IfStatement(TokenKind token, Expression condition, List<Statement> thenBlock) {
+ IfStatement(TokenKind token, int ifOffset, Expression condition, List<Statement> thenBlock) {
this.token = token;
+ this.ifOffset = ifOffset;
this.condition = condition;
this.thenBlock = ImmutableList.copyOf(thenBlock);
}
@@ -59,6 +62,19 @@
}
@Override
+ public int getStartOffset() {
+ return ifOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ List<Statement> body = elseBlock != null ? elseBlock : thenBlock;
+ return body.isEmpty()
+ ? condition.getEndOffset() // wrong, but tree is ill formed
+ : body.get(body.size() - 1).getEndOffset();
+ }
+
+ @Override
public String toString() {
return String.format("if %s: ...\n", condition);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
index e3517d6..c5c9c8c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
@@ -22,12 +22,15 @@
public final class IndexExpression extends Expression {
private final Expression object;
-
+ private final int lbracketOffset;
private final Expression key;
+ private final int rbracketOffset;
- IndexExpression(Expression object, Expression key) {
+ IndexExpression(Expression object, int lbracketOffset, Expression key, int rbracketOffset) {
this.object = object;
+ this.lbracketOffset = lbracketOffset;
this.key = key;
+ this.rbracketOffset = rbracketOffset;
}
public Expression getObject() {
@@ -39,6 +42,20 @@
}
@Override
+ public int getStartOffset() {
+ return object.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rbracketOffset + 1;
+ }
+
+ public Location getLbracketLocation() {
+ return lnt.getLocation(lbracketOffset);
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
index e909536..d5ae4ec 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
@@ -16,9 +16,13 @@
/** Syntax node for an integer literal. */
public final class IntegerLiteral extends Expression {
+ private final String raw;
+ private final int tokenOffset;
private final int value;
- IntegerLiteral(int value) {
+ IntegerLiteral(String raw, int tokenOffset, int value) {
+ this.raw = raw;
+ this.tokenOffset = tokenOffset;
this.value = value;
}
@@ -26,6 +30,21 @@
return value;
}
+ /** Returns the raw source text of the literal. */
+ public String getRaw() {
+ return raw;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return tokenOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return tokenOffset + raw.length();
+ }
+
@Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java b/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java
index 34d78d3..9ec6be3 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Lexer.java
@@ -16,8 +16,6 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
@@ -27,6 +25,47 @@
/** A scanner for Starlark. */
final class Lexer {
+ // --- These fields are accessed directly by the parser: ---
+
+ // Mapping from file offsets to Locations.
+ final LineNumberTable lnt;
+
+ // Information about current token. Updated by nextToken.
+ // raw and value are defined only for STRING, INT, IDENTIFIER, and COMMENT.
+ // TODO(adonovan): rename s/xyz/tokenXyz/
+ TokenKind kind;
+ int left; // start offset TODO(adonovan): rename to start, end
+ int right; // end offset
+ String raw; // source text of token
+ Object value; // String or Integer value of token
+
+ // --- end of parser-visible fields ---
+
+ private final List<SyntaxError> errors;
+
+ // Input buffer and position
+ private final char[] buffer;
+ private int pos;
+
+ private final FileOptions options;
+
+ // The stack of enclosing indentation levels in spaces.
+ // The first (outermost) element is always zero.
+ private final Stack<Integer> indentStack = new Stack<>();
+
+ private final List<Comment> comments;
+
+ // The number of unclosed open-parens ("(", '{', '[') at the current point in
+ // the stream. Whitespace is handled differently when this is nonzero.
+ private int openParenStackDepth = 0;
+
+ // True after a NEWLINE token. In other words, we are outside an
+ // expression and we have to check the indentation.
+ private boolean checkIndentation;
+
+ // Number of saved INDENT (>0) or OUTDENT (<0) tokens detected but not yet returned.
+ private int dents;
+
// Characters that can come immediately prior to an '=' character to generate
// a different token
private static final ImmutableMap<Character, TokenKind> EQUAL_TOKENS =
@@ -45,43 +84,8 @@
.put('|', TokenKind.PIPE_EQUALS)
.build();
- // Input buffer and position
- private final char[] buffer;
- private int pos;
-
- private final FileOptions options;
-
- private final LineNumberTable lnt; // maps offsets to Locations
-
- // The stack of enclosing indentation levels; always contains '0' at the
- // bottom.
- private final Stack<Integer> indentStack = new Stack<>();
-
- /**
- * Token to return. This token is mutated in-place. Its kind is set to
- * null to indicate the intermediate state, where the new token has not
- * been scanned yet.
- */
- private final Token token;
-
- private final List<Comment> comments;
-
- // The number of unclosed open-parens ("(", '{', '[') at the current point in
- // the stream. Whitespace is handled differently when this is nonzero.
- private int openParenStackDepth = 0;
-
- // List of errors appended to by Lexer and Parser.
- private final List<SyntaxError> errors;
-
- /**
- * True after a NEWLINE token.
- * In other words, we are outside an expression and we have to check the indentation.
- */
- private boolean checkIndentation;
-
- private int dents; // number of saved INDENT (>0) or OUTDENT (<0) tokens to return
-
- /** Constructs a lexer which tokenizes the parser input. Errors are appended to {@code errors}. */
+ // Constructs a lexer which tokenizes the parser input.
+ // Errors are appended to errors.
Lexer(ParserInput input, FileOptions options, List<SyntaxError> errors) {
this.lnt = LineNumberTable.create(input.getContent(), input.getFile());
this.options = options;
@@ -91,7 +95,6 @@
this.checkIndentation = true;
this.comments = new ArrayList<>();
this.dents = 0;
- this.token = new Token(null, -1, -1);
indentStack.push(0);
}
@@ -100,104 +103,47 @@
return comments;
}
- /** Returns the apparent name of the lexer's input file. */
- String getFile() {
- return lnt.getFile();
- }
-
/**
- * Returns the next token, or EOF if it is the end of the file. It is an error to call nextToken()
- * after EOF has been returned.
+ * Reads the next token, updating the Lexer's token fields. It is an error to call nextToken after
+ * an EOF token.
*/
- Token nextToken() {
- boolean afterNewline = token.kind == TokenKind.NEWLINE;
- token.kind = null;
+ void nextToken() {
+ boolean afterNewline = kind == TokenKind.NEWLINE;
tokenize();
- Preconditions.checkState(token.kind != null);
+ Preconditions.checkState(kind != null);
// Like Python, always end with a NEWLINE token, even if no '\n' in input:
- if (token.kind == TokenKind.EOF && !afterNewline) {
- token.kind = TokenKind.NEWLINE;
+ if (kind == TokenKind.EOF && !afterNewline) {
+ kind = TokenKind.NEWLINE;
}
- return token;
}
private void popParen() {
if (openParenStackDepth == 0) {
- error("indentation error");
+ // TODO(adonovan): fix: the input ')' should not report an indentation error.
+ error("indentation error", pos - 1);
} else {
openParenStackDepth--;
}
}
- private void error(String message) {
- error(message, pos - 1, pos - 1);
+ private void error(String message, int pos) {
+ errors.add(new SyntaxError(lnt.getLocation(pos), message));
}
- private void error(String message, int start, int end) {
- errors.add(new SyntaxError(createLocation(start, end), message));
- }
-
- LexerLocation createLocation(int start, int end) {
- return new LexerLocation(lnt, start, end);
- }
-
- // A LexerLocation records the span (both start and end) of a token or grammar production.
- // It implements Location by describing the start position,
- // but it also exposes the end location through getEndLocation.
- // This class will be merged with Location and eliminated when we make the Parser
- // record token offsets in the syntax tree, and create Locations on demand.
- @AutoCodec
- @Immutable
- static final class LexerLocation extends Location {
- private final LineNumberTable lineNumberTable;
- final int startOffset;
- final int endOffset;
-
- LexerLocation(LineNumberTable lineNumberTable, int startOffset, int endOffset) {
- this.startOffset = startOffset;
- this.endOffset = endOffset;
- this.lineNumberTable = lineNumberTable;
- }
-
- @Override
- public String file() {
- return lineNumberTable.getFile();
- }
-
- @Override
- public LineAndColumn getLineAndColumn() {
- return lineNumberTable.getLineAndColumn(startOffset);
- }
-
- // For Node.getEndLocation. This is a temporary measure.
- Location getEndLocation() {
- // The end offset is the location *past* the actual end position --> subtract 1:
- // TODO(adonovan): use half-open intervals again. CL 170723732 was a mistake.
- int endOffset = this.endOffset - 1;
- if (endOffset < 0) {
- endOffset = 0;
- }
- LineAndColumn linecol = lineNumberTable.getLineAndColumn(endOffset);
- return Location.fromFileLineColumn(file(), linecol.line, linecol.column);
- }
- }
-
- /** invariant: symbol positions are half-open intervals. */
private void setToken(TokenKind kind, int left, int right) {
- Preconditions.checkState(token.kind == null);
- token.kind = kind;
- token.left = left;
- token.right = right;
- token.value = null;
+ this.kind = kind;
+ this.left = left;
+ this.right = right;
+ this.value = null;
+ this.raw = null;
}
- private void setToken(TokenKind kind, int left, int right, Object value) {
- Preconditions.checkState(token.kind == null);
- token.kind = kind;
- token.left = left;
- token.right = right;
- token.value = value;
+ // setValue sets the value associated with a STRING, FLOAT, INT,
+ // IDENTIFIER, or COMMENT token, and records the raw text of the token.
+ private void setValue(Object value) {
+ this.value = value;
+ this.raw = bufferSlice(left, right);
}
/**
@@ -240,7 +186,7 @@
} else if (c == '\t') {
indentLen++;
pos++;
- error("Tab characters are not allowed for indentation. Use spaces instead.");
+ error("Tab characters are not allowed for indentation. Use spaces instead.", pos);
} else if (c == '\n') { // entirely blank line: discard
indentLen = 0;
pos++;
@@ -249,7 +195,7 @@
while (pos < buffer.length && c != '\n') {
c = buffer[pos++];
}
- makeComment(oldPos, pos - 1, bufferSlice(oldPos, pos - 1));
+ addComment(oldPos, pos - 1);
indentLen = 0;
} else { // printing character
break;
@@ -273,7 +219,7 @@
}
if (peekedIndent < indentLen) {
- error("indentation error");
+ error("indentation error", pos - 1);
}
}
}
@@ -313,14 +259,16 @@
literal.append(c);
break;
} else {
- error("unterminated string literal at eol", literalStartPos, pos);
- setToken(TokenKind.STRING, literalStartPos, pos, literal.toString());
+ error("unterminated string literal at eol", literalStartPos);
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(literal.toString());
return;
}
case '\\':
if (pos == buffer.length) {
- error("unterminated string literal at eof", literalStartPos, pos);
- setToken(TokenKind.STRING, literalStartPos, pos, literal.toString());
+ error("unterminated string literal at eof", literalStartPos);
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(literal.toString());
return;
}
if (isRaw) {
@@ -395,7 +343,7 @@
}
}
if (octal > 0xff) {
- error("octal escape sequence out of range (maximum is \\377)");
+ error("octal escape sequence out of range (maximum is \\377)", pos - 1);
}
literal.append((char) (octal & 0xff));
break;
@@ -409,7 +357,7 @@
case 'v':
case 'x':
// exists in Python but not implemented in Blaze => error
- error("invalid escape sequence: \\" + c, literalStartPos, pos);
+ error("invalid escape sequence: \\" + c, pos - 1);
break;
default:
// unknown char escape => "\literal"
@@ -419,8 +367,7 @@
+ c
+ ". You can enable unknown escape sequences by passing the flag"
+ " --incompatible_restrict_string_escapes=false",
- pos - 1,
- pos);
+ pos - 1);
}
literal.append('\\');
literal.append(c);
@@ -434,7 +381,8 @@
literal.append(c);
} else {
// Matching close-delimiter, all done.
- setToken(TokenKind.STRING, literalStartPos, pos, literal.toString());
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(literal.toString());
return;
}
break;
@@ -443,8 +391,9 @@
break;
}
}
- error("unterminated string literal at eof", literalStartPos, pos);
- setToken(TokenKind.STRING, literalStartPos, pos, literal.toString());
+ error("unterminated string literal at eof", literalStartPos);
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(literal.toString());
}
/**
@@ -474,8 +423,9 @@
char c = buffer[pos++];
switch (c) {
case '\n':
- error("unterminated string literal at eol", literalStartPos, pos);
- setToken(TokenKind.STRING, literalStartPos, pos, bufferSlice(contentStartPos, pos - 1));
+ error("unterminated string literal at eol", literalStartPos);
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(bufferSlice(contentStartPos, pos - 1));
return;
case '\\':
if (isRaw) {
@@ -498,8 +448,8 @@
case '"':
if (c == quot) {
// close-quote, all done.
- setToken(
- TokenKind.STRING, literalStartPos, pos, bufferSlice(contentStartPos, pos - 1));
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(bufferSlice(contentStartPos, pos - 1));
return;
}
break;
@@ -513,8 +463,9 @@
pos = buffer.length;
}
- error("unterminated string literal at eof", literalStartPos, pos);
- setToken(TokenKind.STRING, literalStartPos, pos, bufferSlice(contentStartPos, pos));
+ error("unterminated string literal at eof", literalStartPos);
+ setToken(TokenKind.STRING, literalStartPos, pos);
+ setValue(bufferSlice(contentStartPos, pos));
}
private static final Map<String, TokenKind> keywordMap = new HashMap<>();
@@ -566,13 +517,16 @@
String id = scanIdentifier();
TokenKind kind = keywordMap.get(id);
if (kind == null) {
- setToken(TokenKind.IDENTIFIER, oldPos, pos, id);
+ setToken(TokenKind.IDENTIFIER, oldPos, pos);
+ setValue(id);
} else {
- setToken(kind, oldPos, pos, null);
+ setToken(kind, oldPos, pos);
}
}
private String scanIdentifier() {
+ // Keep consistent with Identifier.isValid.
+ // TODO(laurentlb): Handle Unicode letters.
int oldPos = pos - 1;
while (pos < buffer.length) {
switch (buffer[pos]) {
@@ -629,10 +583,6 @@
break loop;
}
}
- // TODO(bazel-team): (2009) to do roundtripping when we evaluate the integer
- // constants, we must save the actual text of the tokens, not just their
- // integer value.
-
return bufferSlice(oldPos, pos);
}
@@ -657,7 +607,7 @@
} else if (literal.startsWith("0") && literal.length() > 1) {
radix = 8;
substring = literal.substring(1);
- error("invalid octal value `" + literal + "`, should be: `0o" + substring + "`");
+ error("invalid octal value `" + literal + "`, should be: `0o" + substring + "`", oldPos);
} else {
radix = 10;
substring = literal;
@@ -667,10 +617,11 @@
try {
value = Integer.parseInt(substring, radix);
} catch (NumberFormatException e) {
- error("invalid base-" + radix + " integer constant: " + literal);
+ error("invalid base-" + radix + " integer constant: " + literal, oldPos);
}
- setToken(TokenKind.INT, oldPos, pos, value);
+ setToken(TokenKind.INT, oldPos, pos);
+ setValue(value);
}
/**
@@ -724,6 +675,9 @@
return;
}
+ // TODO(adonovan): cleanup: replace break after setToken with return,
+ // and eliminate null-check of this.kind.
+ kind = null;
while (pos < buffer.length) {
if (tokenizeTwoChars()) {
pos += 2;
@@ -841,7 +795,8 @@
} else if (lookaheadIs(0, '\r') && lookaheadIs(1, '\n')) {
pos += 2; // skip the CRLF at the end of line
} else {
- setToken(TokenKind.ILLEGAL, pos - 1, pos, Character.toString(c));
+ setToken(TokenKind.ILLEGAL, pos - 1, pos);
+ setValue(Character.toString(c));
}
break;
case '\n':
@@ -857,7 +812,7 @@
pos++;
}
}
- makeComment(oldPos, pos, bufferSlice(oldPos, pos));
+ addComment(oldPos, pos);
break;
case '\'':
case '\"':
@@ -877,11 +832,11 @@
} else if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_') {
identifierOrKeyword();
} else {
- error("invalid character: '" + c + "'");
+ error("invalid character: '" + c + "'", pos - 1);
}
break;
} // switch
- if (token.kind != null) { // stop here if we scanned a token
+ if (kind != null) { // stop here if we scanned a token
return;
}
} // while
@@ -905,11 +860,15 @@
* @param end the offset immediately following the slice
* @return the text at offset start with length end - start
*/
- private String bufferSlice(int start, int end) {
+ String bufferSlice(int start, int end) {
return new String(this.buffer, start, end - start);
}
- private void makeComment(int start, int end, String content) {
- comments.add(Node.setLocation(createLocation(start, end), new Comment(content)));
+ // TODO(adonovan): don't retain comments unconditionally.
+ private void addComment(int start, int end) {
+ String content = bufferSlice(start, end);
+ Comment c = new Comment(start, content);
+ c.lnt = lnt;
+ comments.add(c);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LineNumberTable.java b/src/main/java/com/google/devtools/build/lib/syntax/LineNumberTable.java
index 1967437..9c89581 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LineNumberTable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LineNumberTable.java
@@ -22,13 +22,14 @@
import java.util.Objects;
/**
- * A table to keep track of line numbers in source files. The client creates a LineNumberTable for
- * their buffer using {@link #create}. The client can then ask for the line and column given a
- * position using ({@link #getLineAndColumn(int)}).
+ * A LineNumberTable maps each UTF-16 index within a source file to its (file, line, column) triple.
+ * An offset is valid if {@code 0 <= offset <= size}.
*/
@AutoCodec
@Immutable
+// TODO(adonovan): rename to FileLocations.
final class LineNumberTable {
+
private static final Interner<LineNumberTable> LINE_NUMBER_TABLE_INTERNER =
BlazeInterners.newWeakInterner();
@@ -36,21 +37,21 @@
private final int[] linestart;
private final String file;
- private final int bufferLength;
+ private final int size; // size of file in chars
private LineNumberTable(char[] buffer, String file) {
this(computeLinestart(buffer), file, buffer.length);
}
- private LineNumberTable(int[] linestart, String file, int bufferLength) {
+ private LineNumberTable(int[] linestart, String file, int size) {
this.linestart = linestart;
this.file = file;
- this.bufferLength = bufferLength;
+ this.size = size;
}
@AutoCodec.Instantiator
- static LineNumberTable createForSerialization(int[] linestart, String file, int bufferLength) {
- return LINE_NUMBER_TABLE_INTERNER.intern(new LineNumberTable(linestart, file, bufferLength));
+ static LineNumberTable createForSerialization(int[] linestart, String file, int size) {
+ return LINE_NUMBER_TABLE_INTERNER.intern(new LineNumberTable(linestart, file, size));
}
static LineNumberTable create(char[] buffer, String file) {
@@ -58,7 +59,7 @@
}
private int getLineAt(int offset) {
- if (offset < 0) {
+ if (offset < 0 || offset > size) {
throw new IllegalStateException("Illegal position: " + offset);
}
int lowBoundary = 1;
@@ -80,19 +81,19 @@
}
}
- Location.LineAndColumn getLineAndColumn(int offset) {
+ Location getLocation(int offset) {
int line = getLineAt(offset);
int column = offset - linestart[line] + 1;
- return new Location.LineAndColumn(line, column);
+ return new Location(file, line, column);
}
- String getFile() {
- return file;
+ int size() {
+ return size;
}
@Override
public int hashCode() {
- return Objects.hash(Arrays.hashCode(linestart), file, bufferLength);
+ return Objects.hash(Arrays.hashCode(linestart), file, size);
}
@Override
@@ -101,7 +102,7 @@
return false;
}
LineNumberTable that = (LineNumberTable) other;
- return this.bufferLength == that.bufferLength
+ return this.size == that.size
&& Arrays.equals(this.linestart, that.linestart)
&& this.file.equals(that.file);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java
index 9fd22ba..7a4ef85 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListExpression.java
@@ -13,18 +13,30 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
+import com.google.common.base.Preconditions;
import java.util.List;
/** Syntax node for list and tuple expressions. */
public final class ListExpression extends Expression {
// TODO(adonovan): split class into {List,Tuple}Expression, as a tuple may have no parens.
- private final boolean isTuple;
- private final List<Expression> elements;
+ // Materialize all source-level expressions as a separate ParenExpression so that we can roundtrip
+ // faithfully.
- ListExpression(boolean isTuple, List<Expression> elements) {
+ private final boolean isTuple;
+ private final int lbracketOffset; // -1 => unparenthesized non-empty tuple
+ private final List<Expression> elements;
+ private final int rbracketOffset; // -1 => unparenthesized non-empty tuple
+
+ ListExpression(
+ boolean isTuple, int lbracketOffset, List<Expression> elements, int rbracketOffset) {
+ // An unparenthesized tuple must be non-empty.
+ Preconditions.checkArgument(
+ !elements.isEmpty() || (lbracketOffset >= 0 && rbracketOffset >= 0));
+ this.lbracketOffset = lbracketOffset;
this.isTuple = isTuple;
this.elements = elements;
+ this.rbracketOffset = rbracketOffset;
}
public List<Expression> getElements() {
@@ -37,6 +49,19 @@
}
@Override
+ public int getStartOffset() {
+ return lbracketOffset < 0 ? elements.get(0).getStartOffset() : lbracketOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ // Unlike Python, trailing commas are not allowed in unparenthesized tuples.
+ return rbracketOffset < 0
+ ? elements.get(elements.size() - 1).getEndOffset()
+ : rbracketOffset + 1;
+ }
+
+ @Override
public String toString() {
// Print [a, b, c, ...] up to a maximum of 4 elements or 32 chars.
StringBuilder buf = new StringBuilder();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
index a73af95..765f923 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
@@ -41,19 +41,17 @@
}
}
- private final ImmutableList<Binding> bindings;
+ private final int loadOffset;
private final StringLiteral module;
+ private final ImmutableList<Binding> bindings;
+ private final int rparenOffset;
- /**
- * Constructs an import statement.
- *
- * <p>{@code bindings} maps a symbol to the original name under which it was defined in the bzl
- * file that should be loaded. If aliasing is used, the value differs from its key's {@code
- * symbol.getName()}. Otherwise, both values are identical.
- */
- LoadStatement(StringLiteral module, ImmutableList<Binding> bindings) {
+ LoadStatement(
+ int loadOffset, StringLiteral module, ImmutableList<Binding> bindings, int rparenOffset) {
+ this.loadOffset = loadOffset;
this.module = module;
this.bindings = bindings;
+ this.rparenOffset = rparenOffset;
}
public ImmutableList<Binding> getBindings() {
@@ -65,6 +63,16 @@
}
@Override
+ public int getStartOffset() {
+ return loadOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rparenOffset + 1;
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Location.java b/src/main/java/com/google/devtools/build/lib/syntax/Location.java
index cf4ae62..b453f22 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Location.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Location.java
@@ -18,7 +18,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import java.io.Serializable;
-import java.util.Objects;
/**
* A Location denotes a position within a Starlark file.
@@ -29,31 +28,33 @@
* If the line number is also zero, it too is not displayed; in this case, the location denotes the
* file as a whole.
*/
-public abstract class Location implements Serializable, Comparable<Location> {
+@AutoCodec
+@Immutable
+public final class Location implements Serializable, Comparable<Location> {
- // TODO(adonovan): merge with Lexer.LexerLocation and make this the only implementation of
- // Location, once the parser no longer eagerly creates Locations but instead
- // records token offsets and the LineNumberTable in the tree.
- @AutoCodec
- @Immutable
- static class FileLineColumn extends Location {
- final String file;
- final LineAndColumn linecol;
+ private final String file;
+ private final int line;
+ private final int column;
- FileLineColumn(String file, LineAndColumn linecol) {
- this.file = Preconditions.checkNotNull(file);
- this.linecol = linecol;
- }
+ public Location(String file, int line, int column) {
+ this.file = Preconditions.checkNotNull(file);
+ this.line = line;
+ this.column = column;
+ }
- @Override
- public String file() {
- return file;
- }
+ /** Returns the name of the file containing this location. */
+ public String file() {
+ return file;
+ }
- @Override
- protected LineAndColumn getLineAndColumn() {
- return linecol;
- }
+ /** Returns the line number of this location. */
+ public int line() {
+ return line;
+ }
+
+ /** Returns the column number of this location. */
+ public int column() {
+ return column;
}
/**
@@ -62,29 +63,12 @@
*/
public static Location fromFileLineColumn(String file, int line, int column) {
Preconditions.checkArgument(line != 0 || column == 0, "non-zero column but no line number");
- return new FileLineColumn(file, new LineAndColumn(line, column));
+ return new Location(file, line, column);
}
/** Returns a Location for the file as a whole. */
public static Location fromFile(String file) {
- return new FileLineColumn(file, ZERO);
- }
-
- private static final LineAndColumn ZERO = new LineAndColumn(0, 0);
-
- /** Returns the name of the file containing this location. */
- public abstract String file();
-
- protected abstract LineAndColumn getLineAndColumn();
-
- /** Returns the line number of this location. */
- public final int line() {
- return getLineAndColumn().line;
- }
-
- /** Returns the column number of this location. */
- public final int column() {
- return getLineAndColumn().column;
+ return new Location(file, 0, 0);
}
/**
@@ -94,54 +78,16 @@
@Override
public String toString() {
StringBuilder buf = new StringBuilder();
- buf.append(file());
- LineAndColumn linecol = getLineAndColumn();
- if (linecol.line != 0) {
- buf.append(':').append(linecol.line);
- if (linecol.column != 0) {
- buf.append(':').append(linecol.column);
+ buf.append(file);
+ if (line != 0) {
+ buf.append(':').append(line);
+ if (column != 0) {
+ buf.append(':').append(column);
}
}
return buf.toString();
}
- /** A value class that describes the line and column of a location. */
- // TODO(adonovan): make private when we combine with LexerLocation.
- @AutoCodec
- @Immutable
- public static final class LineAndColumn {
-
- public final int line;
- public final int column;
-
- public LineAndColumn(int line, int column) {
- this.line = line;
- this.column = column;
- }
-
- @Override
- public boolean equals(Object o) {
- if (o == this) {
- return true;
- }
- if (!(o instanceof LineAndColumn)) {
- return false;
- }
- LineAndColumn lac = (LineAndColumn) o;
- return lac.line == line && lac.column == column;
- }
-
- @Override
- public int hashCode() {
- return line * 41 + column;
- }
-
- @Override
- public String toString() {
- return line + ":" + column;
- }
- }
-
/** Returns a three-valued lexicographical comparison of two Locations. */
@Override
public final int compareTo(Location that) {
@@ -149,22 +95,22 @@
if (cmp != 0) {
return cmp;
}
- LineAndColumn x = this.getLineAndColumn();
- LineAndColumn y = that.getLineAndColumn();
- return Long.compare(((long) x.line << 32) | x.column, ((long) y.line << 32) | y.column);
+ return Long.compare(
+ ((long) this.line << 32) | this.column, ((long) that.line << 32) | that.column);
}
@Override
- public final int hashCode() {
- return Objects.hash(file(), getLineAndColumn());
+ public int hashCode() {
+ return 97 * file.hashCode() + 37 * line + column;
}
@Override
- public final boolean equals(Object that) {
+ public boolean equals(Object that) {
return this == that
- || that instanceof Location
- && this.file().equals(((Location) that).file())
- && this.getLineAndColumn().equals(((Location) that).getLineAndColumn());
+ || (that instanceof Location
+ && this.file.equals(((Location) that).file)
+ && this.line == ((Location) that).line
+ && this.column == ((Location) that).column);
}
/** A location for built-in functions. */
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Node.java b/src/main/java/com/google/devtools/build/lib/syntax/Node.java
index f6b0e0a..b761590 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Node.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Node.java
@@ -14,8 +14,17 @@
package com.google.devtools.build.lib.syntax;
-
-/** A Node is a node in a Starlark syntax tree. */
+/**
+ * A Node is a node in a Starlark syntax tree.
+ *
+ * <p>Nodes may be constructed only by the parser.
+ *
+ * <p>The syntax tree records the offsets within the file of all salient tokens, such as brackets
+ * that mark the beginning or end of an expression, or operators whose position may be needed in a
+ * run-time error message. The start and end offsets of each Node are computed inductively from
+ * their tokens and subexpressions. Offsets are converted to Locations on demand in methods such as
+ * {@link #getStartLocation}.
+ */
public abstract class Node {
// Use these typical node distributions in Bazel files
@@ -50,42 +59,31 @@
// 1.0% Comprehension
// 6 % all others
- // TODO(adonovan): instead of creating Locations during parsing.
- // record the LineNumberTable and the offsets of key tokens,
- // then create Locations on demand for the node start and end
- // and for key tokens.
- private Lexer.LexerLocation location;
+ // The LNT holds the file name and a compressed mapping from
+ // token char offsets to Locations. It is shared by all nodes
+ // from the same file. For convenience of the parser, it is
+ // set immediately after construction.
+ protected LineNumberTable lnt;
Node() {}
- final void setLocation(Lexer.LexerLocation location) {
- this.location = location;
- }
+ /**
+ * Returns the node's start offset, as a char index (zero-based count of UTF-16 codes) from the
+ * start of the file.
+ */
+ public abstract int getStartOffset();
- /** @return the same node with its location set, in a slightly more fluent style */
- static <NodeT extends Node> NodeT setLocation(Lexer.LexerLocation location, NodeT node) {
- node.setLocation(location);
- return node;
- }
-
- /** Returns the location of the start of this node. */
+ /** Returns the location of the start of this syntax node. */
public final Location getStartLocation() {
- return location;
+ return lnt.getLocation(getStartOffset());
}
- /** Returns the char offset of the start of this node within its file. */
- public final int getStartOffset() {
- return location.startOffset;
- }
+ /** Returns the char offset of the source position immediately after this syntax node. */
+ public abstract int getEndOffset();
- /** Returns the char offset of the end of this node within its file. */
- public final int getEndOffset() {
- return location.endOffset;
- }
-
- /** Returns the Location of the end of this node. */
+ /** Returns the location of the source position immediately after this syntax node. */
public final Location getEndLocation() {
- return location.getEndLocation();
+ return lnt.getLocation(getEndOffset());
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/NodePrinter.java b/src/main/java/com/google/devtools/build/lib/syntax/NodePrinter.java
index cf0b068..934d3b2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/NodePrinter.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/NodePrinter.java
@@ -56,7 +56,7 @@
// it on a single line.
printIndent();
buf.append("# ");
- buf.append(comment.getValue());
+ buf.append(comment.getText());
} else if (n instanceof Argument) {
printArgument((Argument) n);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/NodeVisitor.java b/src/main/java/com/google/devtools/build/lib/syntax/NodeVisitor.java
index c0d7405..bdccb35 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/NodeVisitor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/NodeVisitor.java
@@ -72,22 +72,28 @@
visitAll(node.getArguments());
}
- public void visit(@SuppressWarnings("unused") Identifier node) {}
+ public void visit(Identifier node) {}
public void visit(Comprehension node) {
for (Comprehension.Clause clause : node.getClauses()) {
if (clause instanceof Comprehension.For) {
- Comprehension.For forClause = (Comprehension.For) clause;
- visit(forClause.getVars());
- visit(forClause.getIterable());
+ visit((Comprehension.For) clause);
} else {
- Comprehension.If ifClause = (Comprehension.If) clause;
- visit(ifClause.getCondition());
+ visit((Comprehension.If) clause);
}
}
visit(node.getBody());
}
+ public void visit(Comprehension.For node) {
+ visit(node.getVars());
+ visit(node.getIterable());
+ }
+
+ public void visit(Comprehension.If node) {
+ visit(node.getCondition());
+ }
+
public void visit(ForStatement node) {
visit(node.getCollection());
visit(node.getLHS());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java b/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java
index 19cf096..21422d5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parameter.java
@@ -29,20 +29,20 @@
*/
public abstract class Parameter extends Node {
- @Nullable private final Identifier identifier;
+ @Nullable private final Identifier id;
- private Parameter(@Nullable Identifier identifier) {
- this.identifier = identifier;
+ private Parameter(@Nullable Identifier id) {
+ this.id = id;
}
@Nullable
public String getName() {
- return identifier != null ? identifier.getName() : null;
+ return id != null ? id.getName() : null;
}
@Nullable
public Identifier getIdentifier() {
- return identifier;
+ return id;
}
@Nullable
@@ -55,8 +55,18 @@
* depending on its position.
*/
public static final class Mandatory extends Parameter {
- Mandatory(Identifier identifier) {
- super(identifier);
+ Mandatory(Identifier id) {
+ super(id);
+ }
+
+ @Override
+ public int getStartOffset() {
+ return getIdentifier().getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return getIdentifier().getEndOffset();
}
}
@@ -68,8 +78,8 @@
public final Expression defaultValue;
- Optional(Identifier identifier, @Nullable Expression defaultValue) {
- super(identifier);
+ Optional(Identifier id, @Nullable Expression defaultValue) {
+ super(id);
this.defaultValue = defaultValue;
}
@@ -80,22 +90,58 @@
}
@Override
+ public int getStartOffset() {
+ return getIdentifier().getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return getDefaultValue().getEndOffset();
+ }
+
+ @Override
public String toString() {
return getName() + "=" + defaultValue;
}
}
- /** Syntax node for a star parameter, {@code f(*identifier)} or or {@code f(..., *, ...)}. */
+ /** Syntax node for a star parameter, {@code f(*id)} or or {@code f(..., *, ...)}. */
public static final class Star extends Parameter {
- Star(@Nullable Identifier identifier) {
- super(identifier);
+ private final int starOffset;
+
+ Star(int starOffset, @Nullable Identifier id) {
+ super(id);
+ this.starOffset = starOffset;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return starOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return getIdentifier().getEndOffset();
}
}
- /** Syntax node for a parameter of the form {@code f(**identifier)}. */
+ /** Syntax node for a parameter of the form {@code f(**id)}. */
public static final class StarStar extends Parameter {
- StarStar(Identifier identifier) {
- super(identifier);
+ private final int starStarOffset;
+
+ StarStar(int starStarOffset, Identifier id) {
+ super(id);
+ this.starStarOffset = starStarOffset;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return starStarOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return getIdentifier().getEndOffset();
}
}
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 3ab5148..62e2e7e 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,11 +14,9 @@
package com.google.devtools.build.lib.syntax;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
@@ -29,39 +27,34 @@
import java.util.Map;
import javax.annotation.Nullable;
-/**
- * Recursive descent parser for LL(2) BUILD language. Loosely based on Python 2 grammar. See
- * https://docs.python.org/2/reference/grammar.html
- */
-// TODO(adonovan): break syntax->event.Event dependency.
-@VisibleForTesting
+/** Parser is a recursive-descent parser for Starlark. */
final class Parser {
/** Combines the parser result into a single value object. */
static final class ParseResult {
+ // Maps char offsets in the file to Locations.
+ final LineNumberTable lnt;
+
/** The statements (rules, basically) from the parsed file. */
final List<Statement> statements;
/** The comments from the parsed file. */
final List<Comment> comments;
- /** Represents every statement in the file. */
- final Lexer.LexerLocation location;
-
// Errors encountered during scanning or parsing.
// These lists are ultimately owned by StarlarkFile.
final List<SyntaxError> errors;
ParseResult(
+ LineNumberTable lnt,
List<Statement> statements,
List<Comment> comments,
- Lexer.LexerLocation location,
List<SyntaxError> errors) {
+ this.lnt = lnt;
// No need to copy here; when the object is created, the parser instance is just about to go
// out of scope and be garbage collected.
this.statements = Preconditions.checkNotNull(statements);
this.comments = Preconditions.checkNotNull(comments);
- this.location = location;
this.errors = errors;
}
}
@@ -99,7 +92,7 @@
TokenKind.SLASH);
/** Current lookahead token. May be mutated by the parser. */
- private Token token;
+ private final Lexer token; // token.kind is a prettier alias for lexer.kind
private static final boolean DEBUGGING = false;
@@ -156,6 +149,7 @@
private Parser(Lexer lexer, List<SyntaxError> errors) {
this.lexer = lexer;
this.errors = errors;
+ this.token = lexer;
nextToken();
}
@@ -164,15 +158,11 @@
return prev != null ? prev : s;
}
- private static Lexer.LexerLocation locationFromStatements(
- Lexer lexer, List<Statement> statements) {
- int start = 0;
- int end = 0;
- if (!statements.isEmpty()) {
- start = statements.get(0).getStartOffset();
- end = Iterables.getLast(statements).getEndOffset();
- }
- return lexer.createLocation(start, end);
+ // Returns a token's string form as used in error messages.
+ private static String tokenString(TokenKind kind, @Nullable Object value) {
+ return kind == TokenKind.STRING
+ ? "\"" + value + "\"" // TODO(adonovan): do proper quotation
+ : value == null ? kind.toString() : value.toString();
}
// Main entry point for parsing a file.
@@ -185,14 +175,13 @@
Profiler.instance().profile(ProfilerTask.STARLARK_PARSER, input.getFile())) {
statements = parser.parseFileInput();
}
- return new ParseResult(
- statements, lexer.getComments(), locationFromStatements(lexer, statements), errors);
+ return new ParseResult(lexer.lnt, statements, lexer.getComments(), errors);
}
- // stmt ::= simple_stmt
- // | def_stmt
- // | for_stmt
- // | if_stmt
+ // stmt = simple_stmt
+ // | def_stmt
+ // | for_stmt
+ // | if_stmt
private void parseStatement(List<Statement> list) {
if (token.kind == TokenKind.DEF) {
list.add(parseDefStatement());
@@ -222,77 +211,68 @@
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:
+ // expression. In many cases, we need to use parseTest 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();
+ // A trailing comma is disallowed in an unparenthesized tuple.
+ // This prevents bugs where a one-element tuple is surprisingly created:
+ // e.g. foo = f(x),
+ private Expression parseExpression() {
+ Expression e = parseTest();
if (token.kind != TokenKind.COMMA) {
- return expression;
+ return e;
}
- // 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(
- new ListExpression(/*isTuple=*/ true, tuple), start, Iterables.getLast(tuple));
+ // unparenthesized tuple
+ List<Expression> elems = new ArrayList<>();
+ elems.add(e);
+ parseExprList(elems, /*trailingCommaAllowed=*/ false);
+ return setLNT(new ListExpression(/*isTuple=*/ true, -1, elems, -1));
}
- private void reportError(Location location, String message) {
+ private void reportError(int offset, String message) {
errorsCount++;
// Limit the number of reported errors to avoid spamming output.
if (errorsCount <= 5) {
+ Location location = lexer.lnt.getLocation(offset);
errors.add(new SyntaxError(location, message));
}
}
private void syntaxError(String message) {
if (!recoveryMode) {
- String msg = token.kind == TokenKind.INDENT
- ? "indentation error"
- : "syntax error at '" + token + "': " + message;
- reportError(lexer.createLocation(token.left, token.right), msg);
+ String msg =
+ token.kind == TokenKind.INDENT
+ ? "indentation error"
+ : "syntax error at '" + tokenString(token.kind, token.value) + "': " + message;
+ reportError(token.left, msg);
recoveryMode = true;
}
}
- /**
- * Consumes the current token. If it is not of the specified (expected)
- * kind, reports a syntax error.
- */
- private boolean expect(TokenKind kind) {
- boolean expected = token.kind == kind;
- if (!expected) {
+ // Consumes the current token and returns its position, like nextToken.
+ // Reports a syntax error if the new token is not of the expected kind.
+ private int expect(TokenKind kind) {
+ if (token.kind != kind) {
syntaxError("expected " + kind);
}
- nextToken();
- return expected;
+ return nextToken();
}
- /**
- * Same as expect, but stop the recovery mode if the token was expected.
- */
- private void expectAndRecover(TokenKind kind) {
- if (expect(kind)) {
+ // Like expect, but stops recovery mode if the token was expected.
+ private int expectAndRecover(TokenKind kind) {
+ if (token.kind != kind) {
+ syntaxError("expected " + kind);
+ } else {
recoveryMode = false;
}
+ return nextToken();
}
- /**
- * Consume tokens past the first token that has a kind that is in the set of
- * terminatingTokens.
- * @param terminatingTokens
- * @return the end offset of the terminating token.
- */
+ // Consumes tokens past the first token belonging to terminatingTokens.
+ // It returns the end offset of the terminating token.
+ // TODO(adonovan): always used with makeErrorExpression. Combine and simplify.
private int syncPast(EnumSet<TokenKind> terminatingTokens) {
Preconditions.checkState(terminatingTokens.contains(TokenKind.EOF));
while (!terminatingTokens.contains(token.kind)) {
@@ -366,119 +346,121 @@
error = "keyword '" + token.kind + "' not supported";
break;
}
- reportError(lexer.createLocation(token.left, token.right), error);
+ reportError(token.left, error);
}
- private void nextToken() {
- if (token == null || token.kind != TokenKind.EOF) {
- token = lexer.nextToken();
+ private int nextToken() {
+ int prev = token.left;
+ if (token.kind != TokenKind.EOF) {
+ lexer.nextToken();
}
checkForbiddenKeywords();
+ // TODO(adonovan): move this to lexer so we see the first token too.
if (DEBUGGING) {
- System.err.print(token);
+ System.err.print(tokenString(token.kind, token.value));
}
+ return prev;
}
+ // Returns an "Identifier" whose content is the input from start to end.
private Identifier makeErrorExpression(int start, int end) {
// It's tempting to define a dedicated BadExpression type,
// but it is convenient for parseIdent to return an Identifier
// even when it fails.
- return setLocation(Identifier.of("$error$"), start, end);
+ return setLNT(new Identifier(lexer.bufferSlice(start, end), start));
}
- // Convenience wrapper method around Node.setLocation
- private <NodeT extends Node> NodeT setLocation(NodeT node, int startOffset, int endOffset) {
- return Node.setLocation(lexer.createLocation(startOffset, endOffset), node);
+ // setLNT sets the LineNumberTable associated with a newly created Node.
+ // TODO(adonovan): experiment with making this an explicit parameter
+ // to all Node subclass constructors.
+ private <N extends Node> N setLNT(N n) {
+ n.lnt = lexer.lnt;
+ return n;
}
- // Convenience method that uses end offset from the last node.
- private <NodeT extends Node> NodeT setLocation(NodeT node, int startOffset, Node lastNode) {
- Preconditions.checkNotNull(lastNode, "can't extract end offset from a null node");
- return setLocation(node, startOffset, lastNode.getEndOffset());
- }
-
- // arg ::= IDENTIFIER '=' nontupleexpr
- // | expr
- // | *args
- // | **kwargs
+ // arg = IDENTIFIER '=' test
+ // | expr
+ // | *args
+ // | **kwargs
private Argument parseArgument() {
- final int start = token.left;
Expression expr;
+
// parse **expr
if (token.kind == TokenKind.STAR_STAR) {
- nextToken();
- expr = parseNonTupleExpression();
- return setLocation(new Argument.StarStar(expr), start, expr);
- }
- // parse *expr
- if (token.kind == TokenKind.STAR) {
- nextToken();
- expr = parseNonTupleExpression();
- return setLocation(new Argument.Star(expr), start, expr);
+ int starStarOffset = nextToken();
+ expr = parseTest();
+ return setLNT(new Argument.StarStar(starStarOffset, expr));
}
- expr = parseNonTupleExpression();
+ // parse *expr
+ if (token.kind == TokenKind.STAR) {
+ int starOffset = nextToken();
+ expr = parseTest();
+ return setLNT(new Argument.Star(starOffset, expr));
+ }
+
+ // IDENTIFIER or IDENTIFIER = test
+ expr = parseTest();
if (expr instanceof Identifier) {
+ Identifier id = (Identifier) expr;
// parse a named argument
if (token.kind == TokenKind.EQUALS) {
nextToken();
- Expression val = parseNonTupleExpression();
- return setLocation(new Argument.Keyword(((Identifier) expr), val), start, val);
+ Expression arg = parseTest();
+ return setLNT(new Argument.Keyword(id, arg));
}
}
// parse a positional argument
- return setLocation(new Argument.Positional(expr), start, expr);
+ return setLNT(new Argument.Positional(expr));
}
- // arg ::= IDENTIFIER '=' nontupleexpr
- // | IDENTIFIER
+ // arg = IDENTIFIER '=' test
+ // | IDENTIFIER
private Parameter parseFunctionParameter() {
- int start = token.left;
- if (token.kind == TokenKind.STAR_STAR) { // kwarg
- nextToken();
- Identifier ident = parseIdent();
- return setLocation(new Parameter.StarStar(ident), start, ident);
- } else if (token.kind == TokenKind.STAR) { // stararg
- int end = token.right;
- nextToken();
- if (token.kind == TokenKind.IDENTIFIER) {
- Identifier ident = parseIdent();
- return setLocation(new Parameter.Star(ident), start, ident);
- } else {
- return setLocation(new Parameter.Star(null), start, end);
- }
- } else {
- Identifier ident = parseIdent();
- if (token.kind == TokenKind.EQUALS) { // there's a default value
- nextToken();
- Expression expr = parseNonTupleExpression();
- return setLocation(new Parameter.Optional(ident, expr), start, expr);
- } else {
- return setLocation(new Parameter.Mandatory(ident), start, ident);
- }
+ // **kwargs
+ if (token.kind == TokenKind.STAR_STAR) {
+ int starStarOffset = nextToken();
+ Identifier id = parseIdent();
+ return setLNT(new Parameter.StarStar(starStarOffset, id));
}
+
+ // * or *args
+ if (token.kind == TokenKind.STAR) {
+ int starOffset = nextToken();
+ if (token.kind == TokenKind.IDENTIFIER) {
+ Identifier id = parseIdent();
+ return setLNT(new Parameter.Star(starOffset, id));
+ }
+ return setLNT(new Parameter.Star(starOffset, null));
+ }
+
+ // name=default
+ Identifier id = parseIdent();
+ if (token.kind == TokenKind.EQUALS) {
+ nextToken(); // TODO: save token pos?
+ Expression expr = parseTest();
+ return setLNT(new Parameter.Optional(id, expr));
+ }
+
+ // name
+ return setLNT(new Parameter.Mandatory(id));
}
- // call_suffix ::= '(' arg_list? ')'
- private Expression parseCallSuffix(int start, Expression function) {
+ // call_suffix = '(' arg_list? ')'
+ private Expression parseCallSuffix(Expression fn) {
ImmutableList<Argument> args = ImmutableList.of();
- expect(TokenKind.LPAREN);
- int end;
- if (token.kind == TokenKind.RPAREN) {
- end = token.right;
- nextToken(); // RPAREN
- } else {
+ int lparenOffset = expect(TokenKind.LPAREN);
+ if (token.kind != TokenKind.RPAREN) {
args = parseArguments(); // (includes optional trailing comma)
- end = token.right;
- expect(TokenKind.RPAREN);
}
- return setLocation(new CallExpression(function, args), start, end);
+ int rparenOffset = expect(TokenKind.RPAREN);
+ return setLNT(new CallExpression(fn, lexer.lnt.getLocation(lparenOffset), args, rparenOffset));
}
// Parse a list of call arguments.
//
- // arg_list ::= ( (arg ',')* arg ','? )?
+ // arg_list = ( (arg ',')* arg ','? )?
private ImmutableList<Argument> parseArguments() {
boolean hasArgs = false;
boolean hasStarStar = false;
@@ -494,9 +476,7 @@
}
if (hasStarStar) {
// TODO(adonovan): move this to validation pass too.
- reportError(
- lexer.createLocation(token.left, token.right),
- "unexpected tokens after **kwargs argument");
+ reportError(token.left, "unexpected tokens after **kwargs argument");
break;
}
Argument arg = parseArgument();
@@ -538,66 +518,64 @@
}
Argument arg = arguments.get(i);
- Location loc = arg.getStartLocation();
if (arg instanceof Argument.Positional) {
- reportError(loc, "positional argument is misplaced (positional arguments come first)");
+ reportError(
+ arg.getStartOffset(),
+ "positional argument is misplaced (positional arguments come first)");
return;
}
if (arg instanceof Argument.Keyword) {
reportError(
- loc,
+ arg.getStartOffset(),
"keyword argument is misplaced (keyword arguments must be before any *arg or **kwarg)");
return;
}
if (i < len && arg instanceof Argument.Star) {
- reportError(loc, "*arg argument is misplaced");
+ reportError(arg.getStartOffset(), "*arg argument is misplaced");
return;
}
if (i < len && arg instanceof Argument.StarStar) {
- reportError(loc, "**kwarg argument is misplaced (there can be only one)");
+ reportError(arg.getStartOffset(), "**kwarg argument is misplaced (there can be only one)");
return;
}
}
- // selector_suffix ::= '.' IDENTIFIER
- private Expression parseSelectorSuffix(int start, Expression receiver) {
- expect(TokenKind.DOT);
+ // selector_suffix = '.' IDENTIFIER
+ private Expression parseSelectorSuffix(Expression e) {
+ int dotOffset = expect(TokenKind.DOT);
if (token.kind == TokenKind.IDENTIFIER) {
- Identifier ident = parseIdent();
- return setLocation(new DotExpression(receiver, ident), start, ident);
- } else {
- syntaxError("expected identifier after dot");
- int end = syncTo(EXPR_TERMINATOR_SET);
- return makeErrorExpression(start, end);
+ Identifier id = parseIdent();
+ return setLNT(new DotExpression(e, dotOffset, id));
}
+
+ syntaxError("expected identifier after dot");
+ syncTo(EXPR_TERMINATOR_SET);
+ return e;
}
// expr_list parses a comma-separated list of expression. It assumes that the
// first expression was already parsed, so it starts with a comma.
// It is used to parse tuples and list elements.
- // expr_list ::= ( ',' expr )* ','?
- private List<Expression> parseExprList(boolean trailingColonAllowed) {
- List<Expression> list = new ArrayList<>();
+ //
+ // expr_list = ( ',' expr )* ','?
+ private void parseExprList(List<Expression> list, boolean trailingCommaAllowed) {
// terminating tokens for an expression list
while (token.kind == TokenKind.COMMA) {
expect(TokenKind.COMMA);
if (EXPR_LIST_TERMINATOR_SET.contains(token.kind)) {
- if (!trailingColonAllowed) {
- reportError(
- lexer.createLocation(token.left, token.right),
- "Trailing comma is allowed only in parenthesized tuples.");
+ if (!trailingCommaAllowed) {
+ reportError(token.left, "Trailing comma is allowed only in parenthesized tuples.");
}
break;
}
- list.add(parseNonTupleExpression());
+ list.add(parseTest());
}
- return list;
}
- // dict_entry_list ::= ( (dict_entry ',')* dict_entry ','? )?
+ // dict_entry_list = ( (dict_entry ',')* dict_entry ','? )?
private List<DictExpression.Entry> parseDictEntryList() {
List<DictExpression.Entry> list = new ArrayList<>();
// the terminating token for a dict entry list
@@ -612,101 +590,103 @@
return list;
}
- // dict_entry ::= nontupleexpr ':' nontupleexpr
+ // dict_entry = test ':' test
private DictExpression.Entry parseDictEntry() {
- int start = token.left;
- Expression key = parseNonTupleExpression();
- expect(TokenKind.COLON);
- Expression value = parseNonTupleExpression();
- return setLocation(new DictExpression.Entry(key, value), start, value);
+ Expression key = parseTest();
+ int colonOffset = expect(TokenKind.COLON);
+ Expression value = parseTest();
+ return setLNT(new DictExpression.Entry(key, colonOffset, value));
}
- /**
- * Parse a String literal value, e.g. "str".
- */
+ // expr = STRING
private StringLiteral parseStringLiteral() {
Preconditions.checkState(token.kind == TokenKind.STRING);
- int end = token.right;
StringLiteral literal =
- setLocation(new StringLiteral(intern((String) token.value)), token.left, end);
+ setLNT(new StringLiteral(token.left, intern((String) token.value), token.right));
nextToken();
if (token.kind == TokenKind.STRING) {
- reportError(lexer.createLocation(end, token.left),
- "Implicit string concatenation is forbidden, use the + operator");
+ reportError(token.left, "Implicit string concatenation is forbidden, use the + operator");
}
return literal;
}
- // primary ::= INTEGER
- // | STRING
- // | IDENTIFIER
- // | list_expression
- // | '(' ')' // a tuple with zero elements
- // | '(' expr ')' // a parenthesized expression
- // | dict_expression
- // | '-' primary_with_suffix
+ // primary = INTEGER
+ // | STRING
+ // | IDENTIFIER
+ // | list_expression
+ // | '(' ')' // a tuple with zero elements
+ // | '(' expr ')' // a parenthesized expression
+ // | dict_expression
+ // | '-' primary_with_suffix
private Expression parsePrimary() {
- int start = token.left;
switch (token.kind) {
case INT:
{
- IntegerLiteral literal = new IntegerLiteral((Integer) token.value);
- setLocation(literal, start, token.right);
+ IntegerLiteral literal =
+ setLNT(new IntegerLiteral(token.raw, token.left, (Integer) token.value));
nextToken();
return literal;
}
+
case STRING:
return parseStringLiteral();
+
case IDENTIFIER:
return parseIdent();
- case LBRACKET: // it's a list
+
+ case LBRACKET: // [...]
return parseListMaker();
- case LBRACE: // it's a dictionary
+
+ case LBRACE: // {...}
return parseDictExpression();
+
case LPAREN:
{
- nextToken();
- // check for the empty tuple literal
+ int lparenOffset = nextToken();
+
+ // empty tuple: ()
if (token.kind == TokenKind.RPAREN) {
- ListExpression tuple = new ListExpression(/*isTuple=*/ true, ImmutableList.of());
- setLocation(tuple, start, token.right);
- nextToken();
- return tuple;
+ int rparen = nextToken();
+ return setLNT(
+ new ListExpression(/*isTuple=*/ true, lparenOffset, ImmutableList.of(), rparen));
}
- // parse the first expression
- Expression expression = parseExpression(true);
- setLocation(expression, start, token.right);
+
+ Expression e = parseTest();
+
+ // parenthesized expression: (e)
+ // TODO(adonovan): materialize paren expressions (for fidelity).
if (token.kind == TokenKind.RPAREN) {
nextToken();
- return expression;
+ return e;
}
+
+ // non-empty tuple: (e,) or (e, ..., e)
+ if (token.kind == TokenKind.COMMA) {
+ List<Expression> elems = new ArrayList<>();
+ elems.add(e);
+ parseExprList(elems, /*trailingCommaAllowed=*/ true);
+ int rparenOffset = expect(TokenKind.RPAREN);
+ return setLNT(new ListExpression(/*isTuple=*/ true, lparenOffset, elems, rparenOffset));
+ }
+
expect(TokenKind.RPAREN);
int end = syncTo(EXPR_TERMINATOR_SET);
- return makeErrorExpression(start, end);
+ return makeErrorExpression(lparenOffset, end);
}
+
case MINUS:
- {
- nextToken();
- Expression expr = parsePrimaryWithSuffix();
- UnaryOperatorExpression minus = new UnaryOperatorExpression(TokenKind.MINUS, expr);
- return setLocation(minus, start, expr);
- }
case PLUS:
- {
- nextToken();
- Expression expr = parsePrimaryWithSuffix();
- UnaryOperatorExpression plus = new UnaryOperatorExpression(TokenKind.PLUS, expr);
- return setLocation(plus, start, expr);
- }
case TILDE:
{
- nextToken();
- Expression expr = parsePrimaryWithSuffix();
- UnaryOperatorExpression tilde = new UnaryOperatorExpression(TokenKind.TILDE, expr);
- return setLocation(tilde, start, expr);
+ TokenKind op = token.kind;
+ int offset = nextToken();
+ Expression x = parsePrimaryWithSuffix();
+ return setLNT(new UnaryOperatorExpression(op, offset, x));
}
+
default:
{
+ int start = token.left;
syntaxError("expected expression");
int end = syncTo(EXPR_TERMINATOR_SET);
return makeErrorExpression(start, end);
@@ -714,203 +694,186 @@
}
}
- // primary_with_suffix ::= primary (selector_suffix | substring_suffix | call_suffix)*
+ // primary_with_suffix = primary (selector_suffix | slice_suffix | call_suffix)*
private Expression parsePrimaryWithSuffix() {
- int start = token.left;
- Expression receiver = parsePrimary();
+ Expression e = parsePrimary();
while (true) {
if (token.kind == TokenKind.DOT) {
- receiver = parseSelectorSuffix(start, receiver);
+ e = parseSelectorSuffix(e);
} else if (token.kind == TokenKind.LBRACKET) {
- receiver = parseSubstringSuffix(start, receiver);
+ e = parseSliceSuffix(e);
} else if (token.kind == TokenKind.LPAREN) {
- receiver = parseCallSuffix(start, receiver);
+ e = parseCallSuffix(e);
} else {
- break;
+ return e;
}
}
- return receiver;
}
- // substring_suffix ::= '[' expression? ':' expression? ':' expression? ']'
- // | '[' expression? ':' expression? ']'
- // | '[' expression ']'
- private Expression parseSubstringSuffix(int start, Expression receiver) {
- Expression startExpr;
+ // slice_suffix = '[' expr? ':' expr? ':' expr? ']'
+ // | '[' expr? ':' expr? ']'
+ // | '[' expr ']'
+ private Expression parseSliceSuffix(Expression e) {
+ int lbracketOffset = expect(TokenKind.LBRACKET);
+ Expression start = null;
+ Expression end = null;
+ Expression step = null;
- expect(TokenKind.LBRACKET);
- if (token.kind == TokenKind.COLON) {
- startExpr = null;
- } else {
- startExpr = parseExpression();
- }
- // 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 expr;
- }
- // This is a slice (or substring)
- Expression endExpr = parseSliceArgument();
- Expression stepExpr = parseSliceArgument();
- Expression expr =
- setLocation(
- new SliceExpression(receiver, startExpr, endExpr, stepExpr), start, token.right);
- expect(TokenKind.RBRACKET);
- return expr;
- }
+ if (token.kind != TokenKind.COLON) {
+ start = parseExpression();
- /**
- * 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 null.
- */
- 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.
+ // index x[i]
+ if (token.kind == TokenKind.RBRACKET) {
+ int rbracketOffset = expect(TokenKind.RBRACKET);
+ return setLNT(new IndexExpression(e, lbracketOffset, start, rbracketOffset));
+ }
+ }
+
+ // slice or substring x[i:j] or x[i:j:k]
+ expect(TokenKind.COLON);
+ if (token.kind != TokenKind.COLON && token.kind != TokenKind.RBRACKET) {
+ end = parseTest();
+ }
if (token.kind == TokenKind.COLON) {
expect(TokenKind.COLON);
- if (token.kind != TokenKind.COLON && token.kind != TokenKind.RBRACKET) {
- return parseNonTupleExpression();
+ if (token.kind != TokenKind.RBRACKET) {
+ step = parseTest();
}
}
- return null;
+ int rbracketOffset = expect(TokenKind.RBRACKET);
+ return setLNT(new SliceExpression(e, lbracketOffset, start, end, step, rbracketOffset));
}
// Equivalent to 'exprlist' rule in Python grammar.
- // loop_variables ::= primary_with_suffix ( ',' primary_with_suffix )* ','?
+ // loop_variables = primary_with_suffix ( ',' primary_with_suffix )* ','?
private Expression parseForLoopVariables() {
// We cannot reuse parseExpression because it would parse the 'in' operator.
// e.g. "for i in e: pass" -> we want to parse only "i" here.
- int start = token.left;
Expression e1 = parsePrimaryWithSuffix();
if (token.kind != TokenKind.COMMA) {
return e1;
}
- // It's a tuple
- List<Expression> tuple = new ArrayList<>();
- tuple.add(e1);
+ // unparenthesized tuple
+ List<Expression> elems = new ArrayList<>();
+ elems.add(e1);
while (token.kind == TokenKind.COMMA) {
expect(TokenKind.COMMA);
if (EXPR_LIST_TERMINATOR_SET.contains(token.kind)) {
break;
}
- tuple.add(parsePrimaryWithSuffix());
+ elems.add(parsePrimaryWithSuffix());
}
- return setLocation(
- new ListExpression(/*isTuple=*/ true, tuple), start, Iterables.getLast(tuple));
+ return setLNT(new ListExpression(/*isTuple=*/ true, -1, elems, -1));
}
- // comprehension_suffix ::= 'FOR' loop_variables 'IN' expr comprehension_suffix
- // | 'IF' expr comprehension_suffix
- // | ']' | '}'
- private Expression parseComprehensionSuffix(Node body, TokenKind closingBracket, int offset) {
+ // comprehension_suffix = 'FOR' loop_variables 'IN' expr comprehension_suffix
+ // | 'IF' expr comprehension_suffix
+ // | ']' | '}'
+ private Expression parseComprehensionSuffix(int loffset, Node body, TokenKind closingBracket) {
ImmutableList.Builder<Comprehension.Clause> clauses = ImmutableList.builder();
while (true) {
if (token.kind == TokenKind.FOR) {
- nextToken();
+ int forOffset = nextToken();
Expression vars = parseForLoopVariables();
expect(TokenKind.IN);
// The expression cannot be a ternary expression ('x if y else z') due to
// conflicts in Python grammar ('if' is used by the comprehension).
- Expression seq = parseNonTupleExpression(0);
- clauses.add(new Comprehension.For(vars, seq));
+ Expression seq = parseTest(0);
+ clauses.add(setLNT(new Comprehension.For(forOffset, vars, seq)));
} else if (token.kind == TokenKind.IF) {
- nextToken();
+ int ifOffset = nextToken();
// [x for x in li if 1, 2] # parse error
// [x for x in li if (1, 2)] # ok
- Expression cond = parseNonTupleExpression(0);
- clauses.add(new Comprehension.If(cond));
+ Expression cond = parseTest(0);
+ clauses.add(setLNT(new Comprehension.If(ifOffset, cond)));
} else if (token.kind == closingBracket) {
break;
} else {
syntaxError("expected '" + closingBracket + "', 'for' or 'if'");
- syncPast(LIST_TERMINATOR_SET);
- return makeErrorExpression(offset, token.right);
+ int end = syncPast(LIST_TERMINATOR_SET);
+ return makeErrorExpression(loffset, end);
}
}
+
boolean isDict = closingBracket == TokenKind.RBRACE;
- Comprehension comp = new Comprehension(isDict, body, clauses.build());
- setLocation(comp, offset, token.right);
- nextToken();
- return comp;
+ int roffset = expect(closingBracket);
+ return setLNT(new Comprehension(isDict, loffset, body, clauses.build(), roffset));
}
- // list_maker ::= '[' ']'
- // |'[' expr ']'
- // |'[' expr expr_list ']'
- // |'[' expr comprehension_suffix ']'
+ // list_maker = '[' ']'
+ // | '[' expr ']'
+ // | '[' expr expr_list ']'
+ // | '[' expr comprehension_suffix ']'
private Expression parseListMaker() {
- int start = token.left;
- expect(TokenKind.LBRACKET);
+ int lbracketOffset = expect(TokenKind.LBRACKET);
if (token.kind == TokenKind.RBRACKET) { // empty List
- ListExpression list = new ListExpression(/*isTuple=*/ false, ImmutableList.of());
- setLocation(list, start, token.right);
- nextToken();
- return list;
+ int rbracketOffset = nextToken();
+ return setLNT(
+ new ListExpression(
+ /*isTuple=*/ false, lbracketOffset, ImmutableList.of(), rbracketOffset));
}
- Expression expression = parseNonTupleExpression();
- Preconditions.checkNotNull(expression,
- "null element in list in AST at %s:%s", token.left, token.right);
+
+ Expression expression = parseTest();
switch (token.kind) {
- case RBRACKET: // singleton list
+ case RBRACKET:
+ // [e], singleton list
{
- ListExpression list =
- new ListExpression(/*isTuple=*/ false, ImmutableList.of(expression));
- setLocation(list, start, token.right);
- nextToken();
- return list;
+ int rbracketOffset = nextToken();
+ return setLNT(
+ new ListExpression(
+ /*isTuple=*/ false,
+ lbracketOffset,
+ ImmutableList.of(expression),
+ rbracketOffset));
}
+
case FOR:
- { // list comprehension
- return parseComprehensionSuffix(expression, TokenKind.RBRACKET, start);
- }
+ // [e for x in y], list comprehension
+ return parseComprehensionSuffix(lbracketOffset, expression, TokenKind.RBRACKET);
+
case COMMA:
+ // [e, ...], list expression
{
- List<Expression> elems = parseExprList(true);
- Preconditions.checkState(
- !elems.contains(null),
- "null element in list in AST at %s:%s",
- token.left,
- token.right);
- elems.add(0, expression); // TODO(adonovan): opt: don't do this
+ List<Expression> elems = new ArrayList<>();
+ elems.add(expression);
+ parseExprList(elems, /*trailingCommaAllowed=*/ true);
if (token.kind == TokenKind.RBRACKET) {
- ListExpression list = new ListExpression(/*isTuple=*/ false, elems);
- setLocation(list, start, token.right);
- nextToken();
- return list;
+ int rbracketOffset = nextToken();
+ return setLNT(
+ new ListExpression(/*isTuple=*/ false, lbracketOffset, elems, rbracketOffset));
}
+
expect(TokenKind.RBRACKET);
int end = syncPast(LIST_TERMINATOR_SET);
- return makeErrorExpression(start, end);
+ return makeErrorExpression(lbracketOffset, end);
}
+
default:
{
syntaxError("expected ',', 'for' or ']'");
int end = syncPast(LIST_TERMINATOR_SET);
- return makeErrorExpression(start, end);
+ return makeErrorExpression(lbracketOffset, end);
}
}
}
- // dict_expression ::= '{' '}'
- // |'{' dict_entry_list '}'
- // |'{' dict_entry comprehension_suffix '}'
+ // dict_expression = '{' '}'
+ // | '{' dict_entry_list '}'
+ // | '{' dict_entry comprehension_suffix '}'
private Expression parseDictExpression() {
- int start = token.left;
- expect(TokenKind.LBRACE);
+ int lbraceOffset = expect(TokenKind.LBRACE);
if (token.kind == TokenKind.RBRACE) { // empty Dict
- DictExpression literal = new DictExpression(ImmutableList.of());
- setLocation(literal, start, token.right);
- nextToken();
- return literal;
+ int rbraceOffset = nextToken();
+ return setLNT(new DictExpression(lbraceOffset, ImmutableList.of(), rbraceOffset));
}
+
DictExpression.Entry entry = parseDictEntry();
if (token.kind == TokenKind.FOR) {
// Dict comprehension
- return parseComprehensionSuffix(entry, TokenKind.RBRACE, start);
+ return parseComprehensionSuffix(lbraceOffset, entry, TokenKind.RBRACE);
}
+
List<DictExpression.Entry> entries = new ArrayList<>();
entries.add(entry);
if (token.kind == TokenKind.COMMA) {
@@ -918,34 +881,33 @@
entries.addAll(parseDictEntryList());
}
if (token.kind == TokenKind.RBRACE) {
- DictExpression dict = new DictExpression(entries);
- setLocation(dict, start, token.right);
- nextToken();
- return dict;
+ int rbraceOffset = nextToken();
+ return setLNT(new DictExpression(lbraceOffset, entries, rbraceOffset));
}
+
expect(TokenKind.RBRACE);
int end = syncPast(DICT_TERMINATOR_SET);
- return makeErrorExpression(start, end);
+ return makeErrorExpression(lbraceOffset, end);
}
private Identifier parseIdent() {
if (token.kind != TokenKind.IDENTIFIER) {
- expect(TokenKind.IDENTIFIER);
- return makeErrorExpression(token.left, token.right);
+ int start = token.left;
+ int end = expect(TokenKind.IDENTIFIER);
+ return makeErrorExpression(start, end);
}
- Identifier ident = Identifier.of(((String) token.value));
- setLocation(ident, token.left, token.right);
- nextToken();
- return ident;
+
+ String name = (String) token.value;
+ int offset = nextToken();
+ return setLNT(new Identifier(name, offset));
}
- // binop_expression ::= binop_expression OP binop_expression
- // | parsePrimaryWithSuffix
+ // binop_expression = binop_expression OP binop_expression
+ // | parsePrimaryWithSuffix
// This function takes care of precedence between operators (see operatorPrecedence for
// the order), and it assumes left-to-right associativity.
private Expression parseBinOpExpression(int prec) {
- int start = token.left;
- Expression expr = parseNonTupleExpression(prec + 1);
+ Expression x = parseTest(prec + 1);
// The loop is not strictly needed, but it prevents risks of stack overflow. Depth is
// limited to number of different precedence levels (operatorPrecedence.size()).
TokenKind lastOp = null;
@@ -962,22 +924,21 @@
TokenKind op = token.kind;
if (!operatorPrecedence.get(prec).contains(op)) {
- return expr;
+ return x;
}
// Operator '==' and other operators of the same precedence (e.g. '<', 'in')
// are not associative.
if (lastOp != null && operatorPrecedence.get(prec).contains(TokenKind.EQUALS_EQUALS)) {
reportError(
- lexer.createLocation(token.left, token.right),
+ token.left,
String.format(
"Operator '%s' is not associative with operator '%s'. Use parens.", lastOp, op));
}
- nextToken();
- Expression secondary = parseNonTupleExpression(prec + 1);
- expr = optimizeBinOpExpression(expr, op, secondary);
- setLocation(expr, start, secondary);
+ int opOffset = nextToken();
+ Expression y = parseTest(prec + 1);
+ x = optimizeBinOpExpression(x, op, opOffset, y);
lastOp = op;
}
}
@@ -985,39 +946,38 @@
// Optimize binary expressions.
// string literal + string literal can be concatenated into one string literal
// so we don't have to do the expensive string concatenation at runtime.
- private Expression optimizeBinOpExpression(Expression x, TokenKind op, Expression y) {
- if (op == TokenKind.PLUS) {
- if (x instanceof StringLiteral && y instanceof StringLiteral) {
- StringLiteral left = (StringLiteral) x;
- StringLiteral right = (StringLiteral) y;
- return new StringLiteral(intern(left.getValue() + right.getValue()));
- }
+ private Expression optimizeBinOpExpression(
+ Expression x, TokenKind op, int opOffset, Expression y) {
+ if (op == TokenKind.PLUS && x instanceof StringLiteral && y instanceof StringLiteral) {
+ return setLNT(
+ new StringLiteral(
+ x.getStartOffset(),
+ intern(((StringLiteral) x).getValue() + ((StringLiteral) y).getValue()),
+ y.getEndOffset()));
}
- return new BinaryOperatorExpression(x, op, y);
+ return setLNT(new BinaryOperatorExpression(x, op, opOffset, y));
}
- // Equivalent to 'test' rule in Python grammar.
- private Expression parseNonTupleExpression() {
+ // Parses a non-tuple expression ("test" in Python terminology).
+ private Expression parseTest() {
int start = token.left;
- Expression expr = parseNonTupleExpression(0);
+ Expression expr = parseTest(0);
if (token.kind == TokenKind.IF) {
nextToken();
- Expression condition = parseNonTupleExpression(0);
+ Expression condition = parseTest(0);
if (token.kind == TokenKind.ELSE) {
nextToken();
- Expression elseClause = parseNonTupleExpression();
- return setLocation(new ConditionalExpression(expr, condition, elseClause),
- start, elseClause);
+ Expression elseClause = parseTest();
+ return setLNT(new ConditionalExpression(expr, condition, elseClause));
} else {
- reportError(lexer.createLocation(start, token.left),
- "missing else clause in conditional expression or semicolon before if");
+ reportError(start, "missing else clause in conditional expression or semicolon before if");
return expr; // Try to recover from error: drop the if and the expression after it. Ouch.
}
}
return expr;
}
- private Expression parseNonTupleExpression(int prec) {
+ private Expression parseTest(int prec) {
if (prec >= operatorPrecedence.size()) {
return parsePrimaryWithSuffix();
}
@@ -1027,16 +987,14 @@
return parseBinOpExpression(prec);
}
- // not_expr :== 'not' expr
+ // not_expr = 'not' expr
private Expression parseNotExpression(int prec) {
- int start = token.left;
- expect(TokenKind.NOT);
- Expression expression = parseNonTupleExpression(prec);
- UnaryOperatorExpression not = new UnaryOperatorExpression(TokenKind.NOT, expression);
- return setLocation(not, start, expression);
+ int notOffset = expect(TokenKind.NOT);
+ Expression x = parseTest(prec);
+ return setLNT(new UnaryOperatorExpression(TokenKind.NOT, notOffset, x));
}
- // file_input ::= ('\n' | stmt)* EOF
+ // file_input = ('\n' | stmt)* EOF
private List<Statement> parseFileInput() {
List<Statement> list = new ArrayList<>();
while (token.kind != TokenKind.EOF) {
@@ -1056,19 +1014,19 @@
// load '(' STRING (COMMA [IDENTIFIER EQUALS] STRING)+ COMMA? ')'
private Statement parseLoadStatement() {
- int start = token.left;
- expect(TokenKind.LOAD);
+ int loadOffset = expect(TokenKind.LOAD);
expect(TokenKind.LPAREN);
if (token.kind != TokenKind.STRING) {
- StringLiteral module = setLocation(new StringLiteral(""), token.left, token.right);
+ // error: module is not a string literal.
+ StringLiteral module = setLNT(new StringLiteral(token.left, "", token.right));
expect(TokenKind.STRING);
- return setLocation(new LoadStatement(module, ImmutableList.of()), start, token.right);
+ return setLNT(new LoadStatement(loadOffset, module, ImmutableList.of(), token.right));
}
- StringLiteral importString = parseStringLiteral();
+ StringLiteral module = parseStringLiteral();
if (token.kind == TokenKind.RPAREN) {
syntaxError("expected at least one symbol to load");
- return setLocation(new LoadStatement(importString, ImmutableList.of()), start, token.right);
+ return setLNT(new LoadStatement(loadOffset, module, ImmutableList.of(), token.right));
}
expect(TokenKind.COMMA);
@@ -1084,10 +1042,8 @@
parseLoadSymbol(bindings);
}
- LoadStatement stmt =
- setLocation(new LoadStatement(importString, bindings.build()), start, token.right);
- expect(TokenKind.RPAREN);
- return stmt;
+ int rparen = expect(TokenKind.RPAREN);
+ return setLNT(new LoadStatement(loadOffset, module, bindings.build(), rparen));
}
/**
@@ -1104,7 +1060,8 @@
}
String name = (String) token.value;
- Identifier local = setLocation(Identifier.of(name), token.left, token.right);
+ int nameOffset = token.left + (token.kind == TokenKind.STRING ? 1 : 0);
+ Identifier local = setLNT(new Identifier(name, nameOffset));
Identifier original;
if (token.kind == TokenKind.STRING) {
@@ -1112,19 +1069,22 @@
original = local;
} else {
// load(..., local = "orig")
+ // The name "orig" is morally an identifier but, for legacy reasons (specifically,
+ // a partial implementation of Starlark embedded in a Python interpreter used by
+ // tests of Blaze), it must be a quoted string literal.
expect(TokenKind.IDENTIFIER);
expect(TokenKind.EQUALS);
if (token.kind != TokenKind.STRING) {
syntaxError("expected string");
return;
}
- original = setLocation(Identifier.of((String) token.value), token.left, token.right);
+ original = setLNT(new Identifier((String) token.value, token.left + 1));
}
nextToken();
symbols.add(new LoadStatement.Binding(local, original));
}
- // simple_stmt ::= small_stmt (';' small_stmt)* ';'? NEWLINE
+ // simple_stmt = small_stmt (';' small_stmt)* ';'? NEWLINE
private void parseSimpleStatement(List<Statement> list) {
list.add(parseSmallStatement());
@@ -1138,25 +1098,32 @@
expectAndRecover(TokenKind.NEWLINE);
}
- // small_stmt ::= assign_stmt
- // | expr
- // | load_stmt
- // | return_stmt
- // | BREAK | CONTINUE | PASS
- // assign_stmt ::= expr ('=' | augassign) expr
- // augassign ::= '+=' | '-=' | '*=' | '/=' | '%=' | '//=' | '&=' | '|=' | '^=' |'<<=' | '>>='
+ // small_stmt = assign_stmt
+ // | expr
+ // | load_stmt
+ // | return_stmt
+ // | BREAK | CONTINUE | PASS
+ //
+ // assign_stmt = expr ('=' | augassign) expr
+ //
+ // augassign = '+=' | '-=' | '*=' | '/=' | '%=' | '//=' | '&=' | '|=' | '^=' |'<<=' | '>>='
private Statement parseSmallStatement() {
- int start = token.left;
+ // return
if (token.kind == TokenKind.RETURN) {
return parseReturnStatement();
- } else if (token.kind == TokenKind.BREAK
+ }
+
+ // control flow
+ if (token.kind == TokenKind.BREAK
|| token.kind == TokenKind.CONTINUE
|| token.kind == TokenKind.PASS) {
TokenKind kind = token.kind;
- int end = token.right;
- expect(kind);
- return setLocation(new FlowStatement(kind), start, end);
- } else if (token.kind == TokenKind.LOAD) {
+ int offset = nextToken();
+ return setLNT(new FlowStatement(kind, offset));
+ }
+
+ // load
+ if (token.kind == TokenKind.LOAD) {
return parseLoadStatement();
}
@@ -1165,32 +1132,29 @@
// lhs = rhs or lhs += rhs
TokenKind op = augmentedAssignments.get(token.kind);
if (token.kind == TokenKind.EQUALS || op != null) {
- nextToken();
+ int opOffset = nextToken();
Expression rhs = parseExpression();
- // op == null for ordinary assignment.
- return setLocation(new AssignmentStatement(lhs, op, rhs), start, rhs);
+ // op == null for ordinary assignment. TODO(adonovan): represent as EQUALS.
+ return setLNT(new AssignmentStatement(lhs, op, opOffset, rhs));
} else {
- return setLocation(new ExpressionStatement(lhs), start, lhs);
+ return setLNT(new ExpressionStatement(lhs));
}
}
- // if_stmt ::= IF expr ':' suite [ELIF expr ':' suite]* [ELSE ':' suite]?
+ // if_stmt = IF expr ':' suite [ELIF expr ':' suite]* [ELSE ':' suite]?
private IfStatement parseIfStatement() {
- List<Integer> startOffsets = new ArrayList<>();
- startOffsets.add(token.left);
- expect(TokenKind.IF);
- Expression cond = parseNonTupleExpression();
+ int ifOffset = expect(TokenKind.IF);
+ Expression cond = parseTest();
expect(TokenKind.COLON);
List<Statement> body = parseSuite();
- IfStatement ifStmt = new IfStatement(TokenKind.IF, cond, body);
+ IfStatement ifStmt = setLNT(new IfStatement(TokenKind.IF, ifOffset, cond, body));
IfStatement tail = ifStmt;
while (token.kind == TokenKind.ELIF) {
- startOffsets.add(token.left);
- expect(TokenKind.ELIF);
- cond = parseNonTupleExpression();
+ int elifOffset = expect(TokenKind.ELIF);
+ cond = parseTest();
expect(TokenKind.COLON);
body = parseSuite();
- IfStatement elif = new IfStatement(TokenKind.ELIF, cond, body);
+ IfStatement elif = setLNT(new IfStatement(TokenKind.ELIF, elifOffset, cond, body));
tail.setElseBlock(ImmutableList.of(elif));
tail = elif;
}
@@ -1200,41 +1164,23 @@
body = parseSuite();
tail.setElseBlock(body);
}
-
- // Because locations are allocated and stored redundantly rather
- // than computed on demand from token offsets in the tree, we must
- // wait till the end of the chain, after all setElseBlock calls,
- // before setting the end location of each IfStatement.
- // Body may be empty after a parse error.
- int end = (body.isEmpty() ? tail.getCondition() : Iterables.getLast(body)).getEndOffset();
- IfStatement s = ifStmt;
- setLocation(s, startOffsets.get(0), end);
- for (int i = 1; i < startOffsets.size(); i++) {
- s = (IfStatement) s.elseBlock.get(0);
- setLocation(s, startOffsets.get(i), end);
- }
-
return ifStmt;
}
- // for_stmt ::= FOR IDENTIFIER IN expr ':' suite
+ // for_stmt = FOR IDENTIFIER IN expr ':' suite
private ForStatement parseForStatement() {
- int start = token.left;
- expect(TokenKind.FOR);
+ int forOffset = expect(TokenKind.FOR);
Expression lhs = parseForLoopVariables();
expect(TokenKind.IN);
Expression collection = parseExpression();
expect(TokenKind.COLON);
List<Statement> block = parseSuite();
- ForStatement stmt = new ForStatement(lhs, collection, block);
- int end = block.isEmpty() ? token.left : Iterables.getLast(block).getEndOffset();
- return setLocation(stmt, start, end);
+ return setLNT(new ForStatement(forOffset, lhs, collection, block));
}
- // def_stmt ::= DEF IDENTIFIER '(' arguments ')' ':' suite
+ // def_stmt = DEF IDENTIFIER '(' arguments ')' ':' suite
private DefStatement parseDefStatement() {
- int start = token.left;
- expect(TokenKind.DEF);
+ int defOffset = expect(TokenKind.DEF);
Identifier ident = parseIdent();
expect(TokenKind.LPAREN);
ImmutableList<Parameter> params = parseParameters();
@@ -1243,7 +1189,7 @@
try {
signature = FunctionSignature.fromParameters(params);
} catch (FunctionSignature.SignatureException e) {
- reportError(e.getParameter().getStartLocation(), e.getMessage());
+ reportError(e.getParameter().getStartOffset(), e.getMessage());
// bogus empty signature
signature = FunctionSignature.of();
}
@@ -1251,9 +1197,7 @@
expect(TokenKind.RPAREN);
expect(TokenKind.COLON);
ImmutableList<Statement> block = ImmutableList.copyOf(parseSuite());
- DefStatement stmt = new DefStatement(ident, params, signature, block);
- int end = block.isEmpty() ? token.left : Iterables.getLast(block).getEndOffset();
- return setLocation(stmt, start, end);
+ return setLNT(new DefStatement(defOffset, ident, params, signature, block));
}
// Parse a list of function parameters.
@@ -1276,7 +1220,7 @@
}
if (hasStarStar) {
// TODO(adonovan): move this to validation pass too.
- reportError(lexer.createLocation(token.left, token.right), "unexpected tokens after kwarg");
+ reportError(token.left, "unexpected tokens after kwarg");
break;
}
@@ -1291,15 +1235,14 @@
}
// suite is typically what follows a colon (e.g. after def or for).
- // suite ::= simple_stmt
- // | NEWLINE INDENT stmt+ OUTDENT
+ // suite = simple_stmt
+ // | NEWLINE INDENT stmt+ OUTDENT
private List<Statement> parseSuite() {
List<Statement> list = new ArrayList<>();
if (token.kind == TokenKind.NEWLINE) {
expect(TokenKind.NEWLINE);
if (token.kind != TokenKind.INDENT) {
- reportError(lexer.createLocation(token.left, token.right),
- "expected an indented block");
+ reportError(token.left, "expected an indented block");
return list;
}
expect(TokenKind.INDENT);
@@ -1313,18 +1256,14 @@
return list;
}
- // return_stmt ::= RETURN [expr]
+ // return_stmt = RETURN [expr]
private ReturnStatement parseReturnStatement() {
- int start = token.left;
- int end = token.right;
- expect(TokenKind.RETURN);
+ int returnOffset = expect(TokenKind.RETURN);
- Expression expression = null;
+ Expression result = null;
if (!STATEMENT_TERMINATOR_SET.contains(token.kind)) {
- expression = parseExpression();
- end = expression.getEndOffset();
+ result = parseExpression();
}
- return setLocation(new ReturnStatement(expression), start, end);
+ return setLNT(new ReturnStatement(returnOffset, result));
}
-
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index 0f04b04..3ba54ae 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -13,20 +13,45 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
+import com.google.common.base.Preconditions;
import javax.annotation.Nullable;
-/** A wrapper Statement class for return expressions. */
+/** A syntax node for return statements. */
public final class ReturnStatement extends Statement {
- @Nullable private final Expression returnExpression;
+ private final int returnOffset;
+ @Nullable private final Expression result;
- ReturnStatement(@Nullable Expression returnExpression) {
- this.returnExpression = returnExpression;
+ ReturnStatement(int returnOffset, @Nullable Expression result) {
+ this.returnOffset = returnOffset;
+ this.result = result;
}
+ /**
+ * Returns a new return statement that returns expr. It has a dummy file offset and line number
+ * table. It is provided only for use by the evaluator, and will be removed when it switches to a
+ * compiled representation.
+ */
+ public static ReturnStatement make(Expression expr) {
+ ReturnStatement stmt = new ReturnStatement(0, expr);
+ stmt.lnt = Preconditions.checkNotNull(expr.lnt);
+ return stmt;
+ }
+
+ // TODO(adonovan): rename to getResult.
@Nullable
public Expression getReturnExpression() {
- return returnExpression;
+ return result;
+ }
+
+ @Override
+ public int getStartOffset() {
+ return returnOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return result != null ? result.getEndOffset() : returnOffset + "return".length();
}
@Override
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 7353e1b..d78a850 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
@@ -19,15 +19,25 @@
public final class SliceExpression extends Expression {
private final Expression object;
+ private final int lbracketOffset;
@Nullable private final Expression start;
@Nullable private final Expression stop;
@Nullable private final Expression step;
+ private final int rbracketOffset;
- SliceExpression(Expression object, Expression start, Expression stop, Expression step) {
+ SliceExpression(
+ Expression object,
+ int lbracketOffset,
+ Expression start,
+ Expression stop,
+ Expression step,
+ int rbracketOffset) {
this.object = object;
+ this.lbracketOffset = lbracketOffset;
this.start = start;
this.stop = stop;
this.step = step;
+ this.rbracketOffset = rbracketOffset;
}
public Expression getObject() {
@@ -50,6 +60,20 @@
}
@Override
+ public int getStartOffset() {
+ return object.getStartOffset();
+ }
+
+ @Override
+ public int getEndOffset() {
+ return rbracketOffset + 1;
+ }
+
+ public Location getLbracketLocation() {
+ return lnt.getLocation(lbracketOffset);
+ }
+
+ @Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
index b5d222d..237cd19 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
@@ -34,46 +34,59 @@
final List<SyntaxError> errors; // appended to by ValidationEnvironment
@Nullable private final String contentHashCode;
+ @Override
+ public int getStartOffset() {
+ return 0;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return lnt.size();
+ }
+
private StarlarkFile(
ImmutableList<Statement> statements,
FileOptions options,
ImmutableList<Comment> comments,
List<SyntaxError> errors,
- String contentHashCode,
- Lexer.LexerLocation location) {
+ String contentHashCode) {
this.statements = statements;
this.options = options;
this.comments = comments;
this.errors = errors;
this.contentHashCode = contentHashCode;
- this.setLocation(location);
}
// Creates a StarlarkFile from the given effective list of statements,
// which may include the prelude.
private static StarlarkFile create(
+ LineNumberTable lnt,
ImmutableList<Statement> statements,
FileOptions options,
Parser.ParseResult result,
String contentHashCode) {
- return new StarlarkFile(
- statements,
- options,
- ImmutableList.copyOf(result.comments),
- result.errors,
- contentHashCode,
- result.location);
+ StarlarkFile file =
+ new StarlarkFile(
+ statements,
+ options,
+ ImmutableList.copyOf(result.comments),
+ result.errors,
+ contentHashCode);
+ file.lnt = lnt;
+ return file;
}
/** Extract a subtree containing only statements from i (included) to j (excluded). */
public StarlarkFile subTree(int i, int j) {
- return new StarlarkFile(
- this.statements.subList(i, j),
- this.options,
- /*comments=*/ ImmutableList.of(),
- errors,
- /*contentHashCode=*/ null,
- (Lexer.LexerLocation) this.statements.get(i).getStartLocation());
+ StarlarkFile file =
+ new StarlarkFile(
+ this.statements.subList(i, j),
+ this.options,
+ /*comments=*/ ImmutableList.of(),
+ errors,
+ /*contentHashCode=*/ null);
+ file.lnt = this.lnt;
+ return file;
}
/**
@@ -121,7 +134,7 @@
stmts.addAll(prelude);
stmts.addAll(result.statements);
- return create(stmts.build(), options, result, /*contentHashCode=*/ null);
+ return create(result.lnt, stmts.build(), options, result, /*contentHashCode=*/ null);
}
// TODO(adonovan): make the digest publicly settable, and delete this.
@@ -129,6 +142,7 @@
throws IOException {
Parser.ParseResult result = Parser.parseFile(input, options);
return create(
+ result.lnt,
ImmutableList.copyOf(result.statements),
options,
result,
@@ -152,7 +166,11 @@
public static StarlarkFile parse(ParserInput input, FileOptions options) {
Parser.ParseResult result = Parser.parseFile(input, options);
return create(
- ImmutableList.copyOf(result.statements), options, result, /*contentHashCode=*/ null);
+ result.lnt,
+ ImmutableList.copyOf(result.statements),
+ options,
+ result,
+ /*contentHashCode=*/ null);
}
/** Parse a Starlark file with default options. */
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
index d38f85d..8ca6f1c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
@@ -24,10 +24,14 @@
/** Syntax node for a string literal. */
public final class StringLiteral extends Expression {
+ private final int startOffset;
private final String value;
+ private final int endOffset;
- StringLiteral(String value) {
+ StringLiteral(int startOffset, String value, int endOffset) {
+ this.startOffset = startOffset;
this.value = value;
+ this.endOffset = endOffset;
}
/** Returns the value denoted by the string literal */
@@ -35,6 +39,24 @@
return value;
}
+ public Location getLocation() {
+ return lnt.getLocation(startOffset);
+ }
+
+ @Override
+ public int getStartOffset() {
+ return startOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ // TODO(adonovan): when we switch to compilation,
+ // making syntax trees ephemeral, we can afford to
+ // record the raw literal. This becomes:
+ // return startOffset + raw.length().
+ return endOffset;
+ }
+
@Override
public void accept(NodeVisitor visitor) {
visitor.visit(this);
@@ -52,26 +74,29 @@
}
@Override
- public void serialize(
- SerializationContext context, StringLiteral stringLiteral, CodedOutputStream codedOut)
+ public void serialize(SerializationContext context, StringLiteral lit, CodedOutputStream out)
throws SerializationException, IOException {
// The String instances referred to by StringLiterals are deduped by Parser, so therefore
// memoization is guaranteed to be profitable.
context.serializeWithAdHocMemoizationStrategy(
- stringLiteral.getValue(), MemoizationStrategy.MEMOIZE_AFTER, codedOut);
- context.serialize(stringLiteral.getStartLocation(), codedOut);
+ lit.getValue(), MemoizationStrategy.MEMOIZE_AFTER, out);
+ out.writeInt32NoTag(lit.startOffset);
+ out.writeInt32NoTag(lit.endOffset);
+ context.serializeWithAdHocMemoizationStrategy(
+ lit.lnt, MemoizationStrategy.MEMOIZE_AFTER, out);
}
@Override
- public StringLiteral deserialize(DeserializationContext context, CodedInputStream codedIn)
+ public StringLiteral deserialize(DeserializationContext context, CodedInputStream in)
throws SerializationException, IOException {
String value =
- context.deserializeWithAdHocMemoizationStrategy(
- codedIn, MemoizationStrategy.MEMOIZE_AFTER);
- Lexer.LexerLocation location = context.deserialize(codedIn);
- StringLiteral stringLiteral = new StringLiteral(value);
- stringLiteral.setLocation(location);
- return stringLiteral;
+ context.deserializeWithAdHocMemoizationStrategy(in, MemoizationStrategy.MEMOIZE_AFTER);
+ int startOffset = in.readInt32();
+ int endOffset = in.readInt32();
+ StringLiteral lit = new StringLiteral(startOffset, value, endOffset);
+ lit.lnt =
+ context.deserializeWithAdHocMemoizationStrategy(in, MemoizationStrategy.MEMOIZE_AFTER);
+ return lit;
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Token.java b/src/main/java/com/google/devtools/build/lib/syntax/Token.java
deleted file mode 100644
index e836f992..0000000
--- a/src/main/java/com/google/devtools/build/lib/syntax/Token.java
+++ /dev/null
@@ -1,61 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.syntax;
-
-import javax.annotation.Nullable;
-
-/**
- * A Token represents an actual lexeme; that is, a lexical unit, its location in
- * the input text, its lexical kind (TokenKind) and any associated value.
- */
-class Token {
-
- TokenKind kind;
- int left;
- int right;
- /**
- * value is an Integer if the kind is INT.
- * It is a String if the kind is STRING, IDENTIFIER, or COMMENT.
- * It is null otherwise.
- */
- @Nullable Object value;
-
- Token(TokenKind kind, int left, int right) {
- this(kind, left, right, null);
- }
-
- Token(TokenKind kind, int left, int right, Object value) {
- this.kind = kind;
- this.left = left;
- this.right = right;
- this.value = value;
- }
-
- Token copy() {
- return new Token(kind, left, right, value);
- }
-
- /**
- * Constructs an easy-to-read string representation of token, suitable for use
- * in user error messages.
- */
- @Override
- public String toString() {
- // TODO(bazel-team): do proper escaping of string literals
- return kind == TokenKind.STRING
- ? ("\"" + value + "\"")
- : value == null ? kind.toString() : value.toString();
- }
-
-}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/TokenKind.java b/src/main/java/com/google/devtools/build/lib/syntax/TokenKind.java
index aca3523..e6fc450 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/TokenKind.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/TokenKind.java
@@ -14,9 +14,7 @@
package com.google.devtools.build.lib.syntax;
-/**
- * A TokenKind is an enumeration of each different kind of lexical symbol.
- */
+/** A TokenKind represents the kind of a lexical token. */
public enum TokenKind {
AMPERSAND("&"),
AMPERSAND_EQUALS("&="),
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
index 4a58e94..59d387a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
@@ -18,10 +18,12 @@
public final class UnaryOperatorExpression extends Expression {
private final TokenKind op; // NOT, TILDE, MINUS or PLUS
+ private final int opOffset;
private final Expression x;
- UnaryOperatorExpression(TokenKind op, Expression x) {
+ UnaryOperatorExpression(TokenKind op, int opOffset, Expression x) {
this.op = op;
+ this.opOffset = opOffset;
this.x = x;
}
@@ -30,6 +32,16 @@
return op;
}
+ @Override
+ public int getStartOffset() {
+ return opOffset;
+ }
+
+ @Override
+ public int getEndOffset() {
+ return x.getEndOffset();
+ }
+
/** Returns the operand. */
public Expression getX() {
return x;
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 5392647..8e42109 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
@@ -90,7 +90,7 @@
String getUndeclaredNameError(String name);
}
- private static final Identifier PREDECLARED = new Identifier("");
+ private static final Identifier PREDECLARED = new Identifier("", 0);
private static class Block {
private final Map<String, Identifier> variables = new HashMap<>();
@@ -119,7 +119,13 @@
}
}
- void addError(Location loc, String message) {
+ // Reports an error at the start of the specified node.
+ private void addError(Node node, String message) {
+ addError(node.getStartLocation(), message);
+ }
+
+ // Reports an error at the specified location.
+ private void addError(Location loc, String message) {
errors.add(new SyntaxError(loc, message));
}
@@ -162,9 +168,7 @@
// Reject load('...', '_private').
Identifier orig = b.getOriginalName();
if (orig.isPrivate() && !options.allowLoadPrivateSymbols()) {
- addError(
- orig.getStartLocation(),
- "symbol '" + orig.getName() + "' is private and cannot be imported.");
+ addError(orig, "symbol '" + orig.getName() + "' is private and cannot be imported.");
}
// The allowToplevelRebinding check is not applied to all files
@@ -172,7 +176,7 @@
// and emit a better error message than the generic check.
if (!names.add(b.getLocalName().getName())) {
addError(
- b.getLocalName().getStartLocation(),
+ b.getLocalName(),
String.format(
"load statement defines '%s' more than once", b.getLocalName().getName()));
}
@@ -213,7 +217,7 @@
assign(elem);
}
} else {
- addError(lhs.getStartLocation(), "cannot assign to '" + lhs + "'");
+ addError(lhs, "cannot assign to '" + lhs + "'");
}
}
@@ -229,7 +233,7 @@
// generic error
error = createInvalidIdentifierException(node.getName(), getAllSymbols());
}
- addError(node.getStartLocation(), error);
+ addError(node, error);
return;
}
if (options.recordScope()) {
@@ -238,7 +242,8 @@
}
private static String createInvalidIdentifierException(String name, Set<String> candidates) {
- if (name.equals("$error$")) {
+ if (!Identifier.isValid(name)) {
+ // Identifier was created by Parser.makeErrorExpression and contains misparsed text.
return "contains syntax error(s)";
}
@@ -251,6 +256,7 @@
return "name '" + name + "' is not defined" + suggestion;
}
+ // TODO(adonovan): delete this. It's been long enough.
static String getErrorForObsoleteThreadLocalVars(String name) {
if (name.equals("PACKAGE_NAME")) {
return "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
@@ -268,7 +274,7 @@
@Override
public void visit(ReturnStatement node) {
if (block.scope != Scope.Local) {
- addError(node.getStartLocation(), "return statements must be inside a function");
+ addError(node, "return statements must be inside a function");
}
super.visit(node);
}
@@ -277,7 +283,7 @@
public void visit(ForStatement node) {
if (block.scope != Scope.Local) {
addError(
- node.getStartLocation(),
+ node,
"for loops are not allowed at the top level. You may move it inside a function "
+ "or use a comprehension, [f(x) for x in sequence]");
}
@@ -292,7 +298,7 @@
@Override
public void visit(LoadStatement node) {
if (block.scope == Scope.Local) {
- addError(node.getStartLocation(), "load statement not at top level");
+ addError(node, "load statement not at top level");
}
super.visit(node);
}
@@ -300,7 +306,7 @@
@Override
public void visit(FlowStatement node) {
if (node.getKind() != TokenKind.PASS && loopCount <= 0) {
- addError(node.getStartLocation(), node.getKind() + " statement must be inside a for loop");
+ addError(node, node.getKind() + " statement must be inside a for loop");
}
super.visit(node);
}
@@ -338,9 +344,7 @@
@Override
public void visit(DefStatement node) {
if (block.scope == Scope.Local) {
- addError(
- node.getStartLocation(),
- "nested functions are not allowed. Move the function to the top level.");
+ addError(node, "nested functions are not allowed. Move the function to the top level.");
}
for (Parameter param : node.getParameters()) {
if (param instanceof Parameter.Optional) {
@@ -362,7 +366,7 @@
public void visit(IfStatement node) {
if (block.scope != Scope.Local) {
addError(
- node.getStartLocation(),
+ node,
"if statements are not allowed at the top level. You may move it inside a function "
+ "or use an if expression (x if condition else y).");
}
@@ -377,7 +381,7 @@
// Other bad cases are handled in assign.
if (node.isAugmented() && node.getLHS() instanceof ListExpression) {
addError(
- node.getStartLocation(),
+ node.getOperatorLocation(),
"cannot perform augmented assignment on a list or tuple expression");
}
@@ -391,14 +395,13 @@
// Symbols defined in the module scope cannot be reassigned.
if (prev != null && block.scope == Scope.Module && !options.allowToplevelRebinding()) {
addError(
- id.getStartLocation(),
+ id,
String.format(
"cannot reassign global '%s' (read more at"
+ " https://bazel.build/versions/master/docs/skylark/errors/read-only-variable.html)",
id.getName()));
if (prev != PREDECLARED) {
- addError(
- prev.getStartLocation(), String.format("'%s' previously declared here", id.getName()));
+ addError(prev, String.format("'%s' previously declared here", id.getName()));
}
}
}
@@ -424,7 +427,7 @@
// Report an error if a load statement appears after another kind of statement.
private void checkLoadAfterStatement(List<Statement> statements) {
- Location firstStatement = null;
+ Statement firstStatement = null;
for (Statement statement : statements) {
// Ignore string literals (e.g. docstrings).
@@ -437,15 +440,12 @@
if (firstStatement == null) {
continue;
}
- addError(
- statement.getStartLocation(),
- "load() statements must be called before any other statement. "
- + "First non-load() statement appears at "
- + firstStatement);
+ addError(statement, "load statements must appear before any other statement");
+ addError(firstStatement, "\tfirst non-load statement appears here");
}
if (firstStatement == null) {
- firstStatement = statement.getStartLocation();
+ firstStatement = statement;
}
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 9df6b28..8758dc7 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -189,7 +189,7 @@
scratch.file("foo/BUILD", "load(':rule.bzl', 'gen')", "gen(name = 'a')");
update("//foo:a");
- assertContainsEvent("DEBUG /workspace/foo/rule.bzl:3:3: f owner is //foo:a");
+ assertContainsEvent("DEBUG /workspace/foo/rule.bzl:3:8: f owner is //foo:a");
}
@Test
@@ -518,7 +518,7 @@
reporter.setOutputFilter(RegexOutputFilter.forPattern(Pattern.compile("^//java/a")));
update("//java/a:a");
- assertContainsEvent("DEBUG /workspace/java/b/rules.bzl:2:3: debug in b");
+ assertContainsEvent("DEBUG /workspace/java/b/rules.bzl:2:8: debug in b");
}
@Test
@@ -1249,7 +1249,7 @@
update("//foo");
assertContainsEvent(
- "WARNING /workspace/foo/BUILD:6:1: in deps attribute of custom_rule rule "
+ "WARNING /workspace/foo/BUILD:6:12: in deps attribute of custom_rule rule "
+ "//foo:foo: genrule rule '//foo:genlib' is unexpected here (expected java_library or "
+ "java_binary); continuing anyway");
}
@@ -1312,7 +1312,7 @@
update("//foo");
assertContainsEvent(
- "WARNING /workspace/foo/BUILD:6:1: in deps attribute of custom_rule rule "
+ "WARNING /workspace/foo/BUILD:6:12: in deps attribute of custom_rule rule "
+ "//foo:foo: genrule rule '//foo:genlib' is unexpected here; continuing anyway");
}
@@ -1328,8 +1328,8 @@
"print(existing_rule('bar'))");
reporter.setOutputFilter(RegexOutputFilter.forPattern(Pattern.compile("^//pkg")));
update("//pkg:foo");
- assertContainsEvent("DEBUG /workspace/pkg/BUILD:5:1: genrule");
- assertContainsEvent("DEBUG /workspace/pkg/BUILD:6:1: None");
+ assertContainsEvent("DEBUG /workspace/pkg/BUILD:5:6: genrule");
+ assertContainsEvent("DEBUG /workspace/pkg/BUILD:6:6: None");
}
@Test
@@ -1343,7 +1343,7 @@
"print(existing_rules().keys())");
reporter.setOutputFilter(RegexOutputFilter.forPattern(Pattern.compile("^//pkg")));
update("//pkg:foo");
- assertContainsEvent("DEBUG /workspace/pkg/BUILD:5:1: [\"foo\"]");
+ assertContainsEvent("DEBUG /workspace/pkg/BUILD:5:6: [\"foo\"]");
}
/** Runs the same test with trimmed configurations. */
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
index bb9fed6..77fdbda 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/CircularDependencyTest.java
@@ -98,7 +98,7 @@
}
}
assertThat(foundEvent).isNotNull();
- assertThat(foundEvent.getLocation().toString()).isEqualTo("/workspace/cycle/BUILD:3:1");
+ assertThat(foundEvent.getLocation().toString()).isEqualTo("/workspace/cycle/BUILD:3:14");
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java
index 8c72537..9c6714c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetTest.java
@@ -279,7 +279,7 @@
"# blank line",
"cc_library(name = 'x',",
" srcs = ['a.java'])");
- assertThat(e.getLocation().toString()).isEqualTo("/workspace/x/BUILD:2:1");
+ assertThat(e.getLocation().toString()).isEqualTo("/workspace/x/BUILD:2:11");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
index b0603ca..37cc534 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
@@ -1081,14 +1081,15 @@
reporter.removeHandler(failFastHandler);
// Invalid because "--define mode=a" refines :lib to "compatible_with = []" (empty).
assertThat(getConfiguredTarget("//hello:lib")).isNull();
- assertContainsEvent(""
- + "//hello:lib: the current command line flags disqualify all supported environments "
- + "because of incompatible select() paths:\n"
- + " \n"
- + " environment: //buildenv/foo:b\n"
- + " removed by: //hello:lib (/workspace/hello/BUILD:1:1)\n"
- + " which has a select() that chooses dep: //deps:dep_a\n"
- + " which lacks: //buildenv/foo:b");
+ assertContainsEvent(
+ ""
+ + "//hello:lib: the current command line flags disqualify all supported environments "
+ + "because of incompatible select() paths:\n"
+ + " \n"
+ + " environment: //buildenv/foo:b\n"
+ + " removed by: //hello:lib (/workspace/hello/BUILD:1:11)\n"
+ + " which has a select() that chooses dep: //deps:dep_a\n"
+ + " which lacks: //buildenv/foo:b");
}
@Test
@@ -1136,14 +1137,14 @@
reporter.removeHandler(failFastHandler);
// Invalid because "--define mode=a" refines :lib to "compatible_with = ['//buildenv/foo:a']".
assertThat(getConfiguredTarget("//hello:depender")).isNull();
- assertContainsEvent(""
- + "//hello:depender: the current command line flags disqualify all supported environments "
- + "because of incompatible select() paths:\n"
- + " \n"
- + " environment: //buildenv/foo:b\n"
- + " removed by: //hello:lib (/workspace/hello/BUILD:1:1)\n"
- + " which has a select() that chooses dep: //deps:dep_a\n"
- + " which lacks: //buildenv/foo:b");
+ assertContainsEvent(
+ "//hello:depender: the current command line flags disqualify all supported environments"
+ + " because of incompatible select() paths:\n"
+ + " \n"
+ + " environment: //buildenv/foo:b\n"
+ + " removed by: //hello:lib (/workspace/hello/BUILD:1:11)\n"
+ + " which has a select() that chooses dep: //deps:dep_a\n"
+ + " which lacks: //buildenv/foo:b");
}
@Test
@@ -1176,14 +1177,14 @@
reporter.removeHandler(failFastHandler);
// Invalid because "--define mode=a" refines :lib to "compatible_with = ['//buildenv/foo:a']".
assertThat(getConfiguredTarget("//hello:depender")).isNull();
- assertContainsEvent(""
- + "//hello:depender: the current command line flags disqualify all supported environments "
- + "because of incompatible select() paths:\n"
- + " \n"
- + " environment: //buildenv/foo:b\n"
- + " removed by: //hello:lib2 (/workspace/hello/BUILD:1:1)\n"
- + " which has a select() that chooses dep: //deps:dep_a\n"
- + " which lacks: //buildenv/foo:b");
+ assertContainsEvent(
+ "//hello:depender: the current command line flags disqualify all supported environments"
+ + " because of incompatible select() paths:\n"
+ + " \n"
+ + " environment: //buildenv/foo:b\n"
+ + " removed by: //hello:lib2 (/workspace/hello/BUILD:1:11)\n"
+ + " which has a select() that chooses dep: //deps:dep_a\n"
+ + " which lacks: //buildenv/foo:b");
}
@Test
@@ -1203,14 +1204,15 @@
// Invalid because :lib has an implicit default of ['//buildenv/foo:b'] and "--define mode=a"
// refines it to "compatible_with = []" (empty).
assertThat(getConfiguredTarget("//hello:lib")).isNull();
- assertContainsEvent(""
- + "//hello:lib: the current command line flags disqualify all supported environments "
- + "because of incompatible select() paths:\n"
- + " \n"
- + " environment: //buildenv/foo:b\n"
- + " removed by: //hello:lib (/workspace/hello/BUILD:1:1)\n"
- + " which has a select() that chooses dep: //deps:dep_a\n"
- + " which lacks: //buildenv/foo:b");
+ assertContainsEvent(
+ ""
+ + "//hello:lib: the current command line flags disqualify all supported environments "
+ + "because of incompatible select() paths:\n"
+ + " \n"
+ + " environment: //buildenv/foo:b\n"
+ + " removed by: //hello:lib (/workspace/hello/BUILD:1:11)\n"
+ + " which has a select() that chooses dep: //deps:dep_a\n"
+ + " which lacks: //buildenv/foo:b");
}
@Test
@@ -1240,14 +1242,15 @@
// Invalid because while the //buildenv/foo refinement successfully refines :lib to
// ['//buildenv/foo:a'], the bar refinement refines it to [].
assertThat(getConfiguredTarget("//hello:lib")).isNull();
- assertContainsEvent(""
- + "//hello:lib: the current command line flags disqualify all supported environments "
- + "because of incompatible select() paths:\n"
- + " \n"
- + " environment: //buildenv/bar:c\n"
- + " removed by: //hello:lib (/workspace/hello/BUILD:1:1)\n"
- + " which has a select() that chooses dep: //deps:dep_a\n"
- + " which lacks: //buildenv/bar:c");
+ assertContainsEvent(
+ ""
+ + "//hello:lib: the current command line flags disqualify all supported environments "
+ + "because of incompatible select() paths:\n"
+ + " \n"
+ + " environment: //buildenv/bar:c\n"
+ + " removed by: //hello:lib (/workspace/hello/BUILD:1:11)\n"
+ + " which has a select() that chooses dep: //deps:dep_a\n"
+ + " which lacks: //buildenv/bar:c");
}
/**
@@ -1278,23 +1281,24 @@
useConfiguration("--define", "mode=a");
reporter.removeHandler(failFastHandler);
assertThat(getConfiguredTarget("//hello:lib")).isNull();
- assertContainsEvent(""
- + "//hello:lib: the current command line flags disqualify all supported environments "
- + "because of incompatible select() paths:\n"
- + " \n"
- + "environment group: //buildenv/foo:foo:\n"
- + " \n"
- + " environment: //buildenv/foo:a\n"
- + " removed by: //hello:lib (/workspace/hello/BUILD:9:1)\n"
- + " which has a select() that chooses dep: //hello:all_groups_gone\n"
- + " which lacks: //buildenv/foo:a\n"
- + " \n"
- + "environment group: //buildenv/bar:bar:\n"
- + " \n"
- + " environment: //buildenv/bar:c\n"
- + " removed by: //hello:lib (/workspace/hello/BUILD:9:1)\n"
- + " which has a select() that chooses dep: //hello:all_groups_gone\n"
- + " which lacks: //buildenv/bar:c");
+ assertContainsEvent(
+ ""
+ + "//hello:lib: the current command line flags disqualify all supported environments "
+ + "because of incompatible select() paths:\n"
+ + " \n"
+ + "environment group: //buildenv/foo:foo:\n"
+ + " \n"
+ + " environment: //buildenv/foo:a\n"
+ + " removed by: //hello:lib (/workspace/hello/BUILD:9:11)\n"
+ + " which has a select() that chooses dep: //hello:all_groups_gone\n"
+ + " which lacks: //buildenv/foo:a\n"
+ + " \n"
+ + "environment group: //buildenv/bar:bar:\n"
+ + " \n"
+ + " environment: //buildenv/bar:c\n"
+ + " removed by: //hello:lib (/workspace/hello/BUILD:9:11)\n"
+ + " which has a select() that chooses dep: //hello:all_groups_gone\n"
+ + " which lacks: //buildenv/bar:c");
}
private void writeRulesForRefiningSubsetTests(String topLevelRestrictedTo) throws Exception {
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/NoOutputActionTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/NoOutputActionTest.java
index 30f7786..ca66e1b 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/NoOutputActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/NoOutputActionTest.java
@@ -46,7 +46,7 @@
assertThrows(BuildFailedException.class, () -> buildTarget("//nooutput"));
assertThat(e)
.hasMessageThat()
- .contains("nooutput/BUILD:1:1 not all outputs were created or valid");
+ .contains("nooutput/BUILD:1:8 not all outputs were created or valid");
events.assertContainsError("declared output 'nooutput/out1' was not created by genrule");
events.assertContainsError("declared output 'nooutput/out2' was not created by genrule");
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index d3fc968..aecc48a 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -680,7 +680,7 @@
/*includes=*/ ImmutableList.of("W*", "subdir"),
/*excludes=*/ ImmutableList.<String>of(),
/* excludeDirs= */ true));
- assertThat(e).hasMessageThat().isEqualTo("ERROR /globs/BUILD:2:73: incorrect glob result");
+ assertThat(e).hasMessageThat().isEqualTo("ERROR /globs/BUILD:2:77: incorrect glob result");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/query2/cquery/BuildOutputFormatterCallbackTest.java b/src/test/java/com/google/devtools/build/lib/query2/cquery/BuildOutputFormatterCallbackTest.java
index 674382f..85848e8 100644
--- a/src/test/java/com/google/devtools/build/lib/query2/cquery/BuildOutputFormatterCallbackTest.java
+++ b/src/test/java/com/google/devtools/build/lib/query2/cquery/BuildOutputFormatterCallbackTest.java
@@ -103,7 +103,7 @@
getHelper().useConfiguration("--test_arg=cat");
assertThat(getOutput("//test:my_rule"))
.containsExactly(
- "# /workspace/test/BUILD:1:1",
+ "# /workspace/test/BUILD:1:8",
"my_rule(",
" name = \"my_rule\",",
" deps = [\"//test:lasagna.java\", \"//test:naps.java\"],",
@@ -113,7 +113,7 @@
getHelper().useConfiguration("--test_arg=hound");
assertThat(getOutput("//test:my_rule"))
.containsExactly(
- "# /workspace/test/BUILD:1:1",
+ "# /workspace/test/BUILD:1:8",
"my_rule(",
" name = \"my_rule\",",
" deps = [\"//test:mondays.java\"],",
@@ -143,7 +143,7 @@
assertThat(getOutput("//test:my_alias"))
.containsExactly(
- "# /workspace/test/BUILD:12:1",
+ "# /workspace/test/BUILD:12:6",
"alias(",
" name = \"my_alias\",",
" actual = \"//test:my_rule\",",
@@ -178,7 +178,7 @@
getHelper().useConfiguration("--test_arg=cat");
assertThat(getOutput("//test:my_alias"))
.containsExactly(
- "# /workspace/test/BUILD:13:1",
+ "# /workspace/test/BUILD:13:6",
"alias(",
" name = \"my_alias\",",
" actual = \"//test:my_first_rule\",",
@@ -188,7 +188,7 @@
getHelper().useConfiguration("--test_arg=hound");
assertThat(getOutput("//test:my_alias"))
.containsExactly(
- "# /workspace/test/BUILD:13:1",
+ "# /workspace/test/BUILD:13:6",
"alias(",
" name = \"my_alias\",",
" actual = \"//test:my_second_rule\",",
@@ -235,7 +235,7 @@
getHelper().useConfiguration("--test_arg=cat");
assertThat(getOutput("//test:output.txt"))
.containsExactly(
- "# /workspace/test/BUILD:1:1",
+ "# /workspace/test/BUILD:1:8",
"my_rule(",
" name = \"my_rule\",",
" deps = [\"//test:lasagna.java\", \"//test:naps.java\"],",
@@ -246,7 +246,7 @@
getHelper().useConfiguration("--test_arg=hound");
assertThat(getOutput("//test:output.txt"))
.containsExactly(
- "# /workspace/test/BUILD:1:1",
+ "# /workspace/test/BUILD:1:8",
"my_rule(",
" name = \"my_rule\",",
" deps = [\"//test:mondays.java\"],",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
index 4e3f7d4..03a58f1 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigSettingTest.java
@@ -342,7 +342,7 @@
checkError(
"foo",
"none",
- "ERROR /workspace/foo/BUILD:1:1: //foo:none: "
+ "ERROR /workspace/foo/BUILD:1:15: //foo:none: "
+ "expected value of type 'string' for dict value element, but got None (NoneType)",
"config_setting(",
" name = 'none',",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java
index 824b0ea..33f8731 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java
@@ -96,7 +96,7 @@
" has_py3_only_sources = True,",
")");
PyInfo info = (PyInfo) lookup("info");
- assertThat(info.getCreationLoc().toString()).isEqualTo(":1:8");
+ assertThat(info.getCreationLoc().toString()).isEqualTo(":1:14");
assertHasOrderAndContainsExactly(
info.getTransitiveSources().getSet(Artifact.class), Order.STABLE_ORDER, dummyArtifact);
assertThat(info.getUsesSharedLibraries()).isTrue();
@@ -110,7 +110,7 @@
public void starlarkConstructorDefaults() throws Exception {
exec("info = PyInfo(transitive_sources = depset(direct=[dummy_file]))");
PyInfo info = (PyInfo) lookup("info");
- assertThat(info.getCreationLoc().toString()).isEqualTo(":1:8");
+ assertThat(info.getCreationLoc().toString()).isEqualTo(":1:14");
assertHasOrderAndContainsExactly(
info.getTransitiveSources().getSet(Artifact.class), Order.STABLE_ORDER, dummyArtifact);
assertThat(info.getUsesSharedLibraries()).isFalse();
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
index ecdd4c8..1964ee9 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
@@ -91,7 +91,7 @@
" python_version = 'PY2',",
")");
PyRuntimeInfo info = (PyRuntimeInfo) lookup("info");
- assertThat(info.getCreationLoc().toString()).isEqualTo(":1:8");
+ assertThat(info.getCreationLoc().toString()).isEqualTo(":1:21");
assertThat(info.getInterpreterPath()).isNull();
assertThat(info.getInterpreter()).isEqualTo(dummyInterpreter);
assertHasOrderAndContainsExactly(info.getFiles(), Order.STABLE_ORDER, dummyFile);
@@ -106,7 +106,7 @@
" python_version = 'PY2',",
")");
PyRuntimeInfo info = (PyRuntimeInfo) lookup("info");
- assertThat(info.getCreationLoc().toString()).isEqualTo(":1:8");
+ assertThat(info.getCreationLoc().toString()).isEqualTo(":1:21");
assertThat(info.getInterpreterPath()).isEqualTo(PathFragment.create("/system/interpreter"));
assertThat(info.getInterpreter()).isNull();
assertThat(info.getFiles()).isNull();
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java
index 069659e..73bddfa 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkDefinedAspectsTest.java
@@ -643,7 +643,7 @@
// expected
}
- assertContainsEvent("ERROR /workspace/test/aspect.bzl:11:23");
+ assertContainsEvent("ERROR /workspace/test/aspect.bzl:11:38");
assertContainsEvent("Aspects should be top-level values in extension files that define them.");
}
@@ -680,7 +680,7 @@
// expected
}
- assertContainsEvent("ERROR /workspace/test/rule.bzl:7:23");
+ assertContainsEvent("ERROR /workspace/test/rule.bzl:7:38");
assertContainsEvent(
"Providers should be top-level values in extension files that define them.");
}
@@ -839,7 +839,7 @@
// expect to fail.
}
assertContainsEvent(
- "ERROR /workspace/test/BUILD:1:1: in "
+ "ERROR /workspace/test/BUILD:1:13: in "
+ "//test:aspect.bzl%MyAspect aspect on java_library rule //test:xxx: \n"
+ "Traceback (most recent call last):"
+ LINE_SEPARATOR
@@ -897,7 +897,7 @@
// expect to fail.
}
assertContainsEvent(
- "ERROR /workspace/test/BUILD:1:1: in "
+ "ERROR /workspace/test/BUILD:1:13: in "
+ "//test:aspect.bzl%MyAspect aspect on java_library rule //test:xxx: \n"
+ "\n"
+ "\n"
@@ -989,7 +989,7 @@
} catch (ViewCreationFailedException e) {
// expect to fail.
}
- assertContainsEvent("ERROR /workspace/test/BUILD:3:1: Output group duplicate provided twice");
+ assertContainsEvent("ERROR /workspace/test/BUILD:3:6: Output group duplicate provided twice");
}
@Test
@@ -1170,7 +1170,7 @@
} catch (ViewCreationFailedException e) {
// expected.
}
- assertContainsEvent("ERROR /workspace/test/BUILD:3:1: Output group a1_group provided twice");
+ assertContainsEvent("ERROR /workspace/test/BUILD:3:9: Output group a1_group provided twice");
}
private static Iterable<String> getOutputGroupContents(
@@ -1207,7 +1207,7 @@
} catch (ViewCreationFailedException e) {
// expect to fail.
}
- assertContainsEvent("ERROR /workspace/test/BUILD:3:1: Provider duplicate provided twice");
+ assertContainsEvent("ERROR /workspace/test/BUILD:3:6: Provider duplicate provided twice");
}
@Test
@@ -1383,7 +1383,7 @@
// expect to fail.
}
assertContainsEvent(
- "ERROR /workspace/test/aspect.bzl:5:22: "
+ "ERROR /workspace/test/aspect.bzl:5:28: "
+ "Aspect parameter attribute 'my_attr' has a bad default value: has to be one of 'a' "
+ "instead of 'b'");
}
@@ -1406,7 +1406,9 @@
" 'my_attr' : attr.string() },",
")");
scratch.file(
- "test/BUILD", "load('//test:aspect.bzl', 'my_rule')", "my_rule(name = 'xxx', my_attr='b')");
+ "test/BUILD", //
+ "load('//test:aspect.bzl', 'my_rule')",
+ "my_rule(name = 'xxx', my_attr='b')");
reporter.removeHandler(failFastHandler);
try {
@@ -1417,7 +1419,7 @@
// expect to fail.
}
assertContainsEvent(
- "ERROR /workspace/test/BUILD:2:1: //test:xxx: invalid value in 'my_attr' "
+ "ERROR /workspace/test/BUILD:2:8: //test:xxx: invalid value in 'my_attr' "
+ "attribute: has to be one of 'a' instead of 'b'");
}
@@ -2242,7 +2244,7 @@
// expected
}
assertContainsEvent(
- "ERROR /workspace/test/BUILD:3:1: Aspect //test:aspect.bzl%a2 is"
+ "ERROR /workspace/test/BUILD:3:3: Aspect //test:aspect.bzl%a2 is"
+ " applied twice, both before and after aspect //test:aspect.bzl%a1 "
+ "(when propagating to //test:r1)");
}
@@ -2287,7 +2289,7 @@
// expected
}
assertContainsEvent(
- "ERROR /workspace/test/BUILD:3:1: Aspect //test:aspect.bzl%a2 is"
+ "ERROR /workspace/test/BUILD:3:3: Aspect //test:aspect.bzl%a2 is"
+ " applied twice, both before and after aspect //test:aspect.bzl%a1 "
+ "(when propagating to //test:r1)");
}
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 8394263..a4cc719 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
@@ -240,7 +240,7 @@
AttributeContainer withMacro = getContainerForTarget("macro_target");
assertThat(withMacro.getAttr("generator_name")).isEqualTo("macro_target");
assertThat(withMacro.getAttr("generator_function")).isEqualTo("macro");
- assertThat(withMacro.getAttr("generator_location")).isEqualTo("test/skylark/BUILD:3:1");
+ assertThat(withMacro.getAttr("generator_location")).isEqualTo("test/skylark/BUILD:3:11");
// Attributes are only set when the rule was created by a macro
AttributeContainer noMacro = getContainerForTarget("no_macro_target");
@@ -251,7 +251,7 @@
AttributeContainer nativeMacro = getContainerForTarget("native_macro_target_suffix");
assertThat(nativeMacro.getAttr("generator_name")).isEqualTo("native_macro_target");
assertThat(nativeMacro.getAttr("generator_function")).isEqualTo("native_macro");
- assertThat(nativeMacro.getAttr("generator_location")).isEqualTo("test/skylark/BUILD:5:1");
+ assertThat(nativeMacro.getAttr("generator_location")).isEqualTo("test/skylark/BUILD:5:18");
AttributeContainer ccTarget = getContainerForTarget("cc_target");
assertThat(ccTarget.getAttr("generator_name")).isEqualTo("");
@@ -1731,7 +1731,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:xxx");
- assertContainsEvent("ERROR /workspace/test/BUILD:2:1: in my_rule rule //test:xxx: ");
+ assertContainsEvent("ERROR /workspace/test/BUILD:2:8: in my_rule rule //test:xxx: ");
assertContainsEvent("The following files have no generating action:");
assertContainsEvent("test/xxx");
}
@@ -1777,10 +1777,11 @@
);
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:xxx");
- assertContainsEvent("ERROR /workspace/test/BUILD:2:1: in my_rule rule //test:xxx: ");
- assertContainsEvent("/workspace/test/rule.bzl:5:12: The rule 'my_rule' both accesses "
- + "'ctx.outputs.executable' and provides a different executable 'test/x.sh'. "
- + "Do not use 'ctx.output.executable'.");
+ assertContainsEvent("ERROR /workspace/test/BUILD:2:8: in my_rule rule //test:xxx: ");
+ assertContainsEvent(
+ "/workspace/test/rule.bzl:5:23: The rule 'my_rule' both accesses "
+ + "'ctx.outputs.executable' and provides a different executable 'test/x.sh'. "
+ + "Do not use 'ctx.output.executable'.");
}
@@ -1855,7 +1856,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:yyy");
- assertContainsEvent("ERROR /workspace/test/BUILD:3:1: in my_dep_rule rule //test:yyy: ");
+ assertContainsEvent("ERROR /workspace/test/BUILD:3:12: in my_dep_rule rule //test:yyy: ");
assertContainsEvent("File \"/workspace/test/rule.bzl\", line 8, in _dep_impl");
assertContainsEvent("ctx.attr.dep[PInfo].outputs.executable");
assertContainsEvent("cannot access outputs of rule '//test:xxx' outside "
@@ -1918,7 +1919,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:xxx");
- assertContainsEvent("ERROR /workspace/test/BUILD:2:1: in my_rule rule //test:xxx: ");
+ assertContainsEvent("ERROR /workspace/test/BUILD:2:8: in my_rule rule //test:xxx: ");
assertContainsEvent("/rule.bzl:1:5: The rule 'my_rule' is executable. "
+ "It needs to create an executable File and pass it as the 'executable' "
+ "parameter to the DefaultInfo it returns.");
@@ -1951,7 +1952,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//src:r_tools");
assertContainsEvent(
- "/workspace/src/rulez.bzl:2:12: 'executable' provided by an executable"
+ "/workspace/src/rulez.bzl:2:23: 'executable' provided by an executable"
+ " rule 'r' should be created by the same rule.");
}
@@ -1971,10 +1972,16 @@
" 'out': 'foo/bar/baz',",
" },",
")");
- scratch.file("BUILD", "load(':ext.bzl', 'extrule')", "", "extrule(", " name = 'test'", ")");
+ scratch.file(
+ "BUILD", //
+ "load(':ext.bzl', 'extrule')",
+ "",
+ "extrule(",
+ " name = 'test'",
+ ")");
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//:test");
- assertContainsEvent("ERROR /workspace/BUILD:3:1: in extrule rule //:test:");
+ assertContainsEvent("ERROR /workspace/BUILD:3:8: in extrule rule //:test:");
assertContainsEvent("he following directories were also declared as files:");
assertContainsEvent("foo/bar/baz");
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 57d4724..e87f704 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -193,7 +193,7 @@
setUpAttributeErrorTest();
assertThrows(Exception.class, () -> createRuleContext("//test:m_skylark"));
assertContainsEvent(
- "ERROR /workspace/test/BUILD:4:1: in deps attribute of skylark_rule rule "
+ "ERROR /workspace/test/BUILD:4:19: in deps attribute of skylark_rule rule "
+ "//test:m_skylark: '//test:jlib' does not have mandatory providers:"
+ " 'some_provider'. "
+ "Since this rule was created by the macro 'macro_skylark_rule', the error might "
@@ -215,7 +215,7 @@
setUpAttributeErrorTest();
assertThrows(Exception.class, () -> createRuleContext("//test:skyrule"));
assertContainsEvent(
- "ERROR /workspace/test/BUILD:10:1: in deps attribute of "
+ "ERROR /workspace/test/BUILD:10:13: in deps attribute of "
+ "skylark_rule rule //test:skyrule: '//test:jlib' does not have mandatory providers: "
+ "'some_provider'");
}
@@ -256,7 +256,7 @@
assertThrows(Exception.class, () -> createRuleContext("//test:skyrule2"));
assertContainsEvent(
- "ERROR /workspace/test/BUILD:8:1: in deps attribute of "
+ "ERROR /workspace/test/BUILD:8:13: in deps attribute of "
+ "skylark_rule rule //test:skyrule2: '//test:my_other_lib' does not have "
+ "mandatory providers: 'a' or 'c'");
}
@@ -288,7 +288,7 @@
assertThrows(Exception.class, () -> createRuleContext("//test:skyrule2"));
assertContainsEvent(
- "ERROR /workspace/test/BUILD:8:1: in deps attribute of "
+ "ERROR /workspace/test/BUILD:8:37: in deps attribute of "
+ "testing_rule_for_mandatory_providers rule //test:skyrule2: '//test:my_other_lib' "
+ "does not have mandatory providers: 'a' or 'c'");
}
@@ -303,7 +303,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:cclib");
assertContainsEvent(
- "ERROR /workspace/test/BUILD:1:1: Label '//test:sub/my_sub_lib.h' is invalid because "
+ "ERROR /workspace/test/BUILD:1:11: Label '//test:sub/my_sub_lib.h' is invalid because "
+ "'test/sub' is a subpackage; perhaps you meant to put the colon here: "
+ "'//test/sub:my_sub_lib.h'?");
}
@@ -328,7 +328,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:skyrule");
assertContainsEvent(
- "ERROR /workspace/test/BUILD:2:1: Label '//test:sub/my_sub_lib.h' is invalid because "
+ "ERROR /workspace/test/BUILD:2:13: Label '//test:sub/my_sub_lib.h' is invalid because "
+ "'test/sub' is a subpackage; perhaps you meant to put the colon here: "
+ "'//test/sub:my_sub_lib.h'?");
}
@@ -355,7 +355,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:m_skylark");
assertContainsEvent(
- "ERROR /workspace/test/BUILD:2:1: Label '//test:sub/my_sub_lib.h' is invalid because"
+ "ERROR /workspace/test/BUILD:2:19: Label '//test:sub/my_sub_lib.h' is invalid because"
+ " 'test/sub' is a subpackage; perhaps you meant to put the colon here: "
+ "'//test/sub:my_sub_lib.h'?");
}
@@ -377,7 +377,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//:cclib");
assertContainsEvent(
- "/workspace/BUILD:1:1: Label '//:r/my_sub_lib.h' is invalid because "
+ "/workspace/BUILD:1:11: Label '//:r/my_sub_lib.h' is invalid because "
+ "'@r//' is a subpackage");
}
@@ -396,7 +396,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("@r//:cclib");
assertContainsEvent(
- "/external/r/BUILD:1:1: Label '@r//:sub/my_sub_lib.h' is invalid because "
+ "/external/r/BUILD:1:11: Label '@r//:sub/my_sub_lib.h' is invalid because "
+ "'@r//sub' is a subpackage; perhaps you meant to put the colon here: "
+ "'@r//sub:my_sub_lib.h'?");
}
@@ -431,7 +431,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:m_skylark");
assertContainsEvent(
- "ERROR /workspace/test/BUILD:2:1: Label '//test:sub/my_sub_lib.h' "
+ "ERROR /workspace/test/BUILD:2:19: Label '//test:sub/my_sub_lib.h' "
+ "is invalid because 'test/sub' is a subpackage");
}
@@ -449,7 +449,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:m_native");
assertContainsEvent(
- "ERROR /workspace/test/BUILD:2:1: Label '//test:sub/my_sub_lib.h' "
+ "ERROR /workspace/test/BUILD:2:18: Label '//test:sub/my_sub_lib.h' "
+ "is invalid because 'test/sub' is a subpackage");
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
index c571553..9e52524 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
@@ -467,7 +467,7 @@
Location.newBuilder()
.setPath("/a/build/file/test.bzl")
.setLineNumber(7)
- .setColumnNumber(1))
+ .setColumnNumber(3))
.addScope(
Scope.newBuilder()
.setName("global")
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
index 3b5ee29..a8ba83af 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ExceptionTest.java
@@ -24,12 +24,12 @@
*/
@RunWith(JUnit4.class)
public class ExceptionTest {
- private static final Node DUMMY_NODE = Identifier.of("DUMMY");
@Test
public void testEmptyMessage() throws Exception {
+ Node dummyNode = Expression.parse(ParserInput.fromLines("DUMMY"));
EvalExceptionWithStackTrace ex =
- new EvalExceptionWithStackTrace(new NullPointerException(), DUMMY_NODE);
+ new EvalExceptionWithStackTrace(new NullPointerException(), dummyNode);
assertThat(ex)
.hasMessageThat()
.contains("Null Pointer: ExceptionTest.testEmptyMessage() in ExceptionTest.java:");
@@ -44,8 +44,10 @@
runExceptionTest(ee, iae);
}
- private void runExceptionTest(Exception toThrow, Exception expectedCause) {
- EvalExceptionWithStackTrace ex = new EvalExceptionWithStackTrace(toThrow, DUMMY_NODE);
+ private static void runExceptionTest(Exception toThrow, Exception expectedCause)
+ throws Exception {
+ Node dummyNode = Expression.parse(ParserInput.fromLines("DUMMY"));
+ EvalExceptionWithStackTrace ex = new EvalExceptionWithStackTrace(toThrow, dummyNode);
assertThat(ex).hasCauseThat().isEqualTo(expectedCause);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java b/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java
index 831052c..616a85c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/LexerTest.java
@@ -16,7 +16,6 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.common.base.Joiner;
-import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
import java.util.ArrayList;
import java.util.List;
import org.junit.Test;
@@ -45,13 +44,31 @@
return new Lexer(inputSource, FileOptions.DEFAULT, errors);
}
+ private static class Token {
+ TokenKind kind;
+ int left;
+ int right;
+ Object value;
+
+ @Override
+ public String toString() {
+ return kind == TokenKind.STRING
+ ? "\"" + value + "\""
+ : value == null ? kind.toString() : value.toString();
+ }
+ }
+
private ArrayList<Token> allTokens(Lexer lexer) {
ArrayList<Token> result = new ArrayList<>();
- Token tok;
do {
- tok = lexer.nextToken();
- result.add(tok.copy());
- } while (tok.kind != TokenKind.EOF);
+ lexer.nextToken();
+ Token tok = new Token();
+ tok.kind = lexer.kind;
+ tok.left = lexer.left;
+ tok.right = lexer.right;
+ tok.value = lexer.value;
+ result.add(tok);
+ } while (lexer.kind != TokenKind.EOF);
for (SyntaxError error : errors) {
lastError = error.location().file() + ":" + error.location().line() + ": " + error.message();
@@ -76,7 +93,7 @@
if (buf.length() > 0) {
buf.append(' ');
}
- int line = lexer.createLocation(tok.left, tok.left).line();
+ int line = lexer.lnt.getLocation(tok.left).line();
buf.append(line);
}
return buf.toString();
@@ -511,11 +528,6 @@
+ " instead.");
}
- @Test
- public void testLexerLocationCodec() throws Exception {
- new SerializationTester(createLexer("foo").createLocation(0, 2)).runTests();
- }
-
/**
* Returns the first error whose string form contains the specified substring, or throws an
* informative AssertionError if there is none.
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/LineNumberTableTest.java b/src/test/java/com/google/devtools/build/lib/syntax/LineNumberTableTest.java
index 70d76f4..6116743 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/LineNumberTableTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/LineNumberTableTest.java
@@ -13,43 +13,51 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
-import static com.google.common.truth.Truth.assertThat;
import com.google.devtools.build.lib.skyframe.serialization.testutils.SerializationTester;
-import com.google.devtools.build.lib.syntax.Location.LineAndColumn;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/**
- * Tests for {@link LineNumberTable}.
- */
+/** Tests for {@link LineNumberTable}. */
+// TODO(adonovan): express this test in terms of the public API.
@RunWith(JUnit4.class)
public class LineNumberTableTest {
- private LineNumberTable create(String buffer) {
+ private static LineNumberTable create(String buffer) {
return LineNumberTable.create(buffer.toCharArray(), "/fake/file");
}
+ // Asserts that the specified offset results in a line/column pair of the form "1:2".
+ private static void checkOffset(LineNumberTable table, int offset, String wantLineCol) {
+ Location loc = table.getLocation(offset);
+ String got = String.format("%d:%d", loc.line(), loc.column());
+ if (!got.equals(wantLineCol)) {
+ throw new AssertionError(
+ String.format("location(%d) = %s, want %s", offset, got, wantLineCol));
+ }
+ }
+
@Test
public void testEmpty() {
LineNumberTable table = create("");
- assertThat(table.getLineAndColumn(0)).isEqualTo(new LineAndColumn(1, 1));
+ checkOffset(table, 0, "1:1");
}
@Test
public void testNewline() {
LineNumberTable table = create("\n");
- assertThat(table.getLineAndColumn(0)).isEqualTo(new LineAndColumn(1, 1));
- assertThat(table.getLineAndColumn(1)).isEqualTo(new LineAndColumn(2, 1));
+ checkOffset(table, 0, "1:1");
+ checkOffset(table, 1, "2:1"); // EOF
}
@Test
public void testOneLiner() {
LineNumberTable table = create("foo");
- assertThat(table.getLineAndColumn(0)).isEqualTo(new LineAndColumn(1, 1));
- assertThat(table.getLineAndColumn(1)).isEqualTo(new LineAndColumn(1, 2));
- assertThat(table.getLineAndColumn(2)).isEqualTo(new LineAndColumn(1, 3));
+ checkOffset(table, 0, "1:1");
+ checkOffset(table, 1, "1:2");
+ checkOffset(table, 2, "1:3");
+ checkOffset(table, 3, "1:4"); // EOF
}
@Test
@@ -57,24 +65,27 @@
LineNumberTable table = create("\ntwo\nthree\n\nfive\n");
// \n
- assertThat(table.getLineAndColumn(0)).isEqualTo(new LineAndColumn(1, 1));
+ checkOffset(table, 0, "1:1");
// two\n
- assertThat(table.getLineAndColumn(1)).isEqualTo(new LineAndColumn(2, 1));
- assertThat(table.getLineAndColumn(2)).isEqualTo(new LineAndColumn(2, 2));
- assertThat(table.getLineAndColumn(3)).isEqualTo(new LineAndColumn(2, 3));
- assertThat(table.getLineAndColumn(4)).isEqualTo(new LineAndColumn(2, 4));
+ checkOffset(table, 1, "2:1");
+ checkOffset(table, 2, "2:2");
+ checkOffset(table, 3, "2:3");
+ checkOffset(table, 4, "2:4");
// three\n
- assertThat(table.getLineAndColumn(5)).isEqualTo(new LineAndColumn(3, 1));
- assertThat(table.getLineAndColumn(10)).isEqualTo(new LineAndColumn(3, 6));
+ checkOffset(table, 5, "3:1");
+ checkOffset(table, 10, "3:6");
// \n
- assertThat(table.getLineAndColumn(11)).isEqualTo(new LineAndColumn(4, 1));
+ checkOffset(table, 11, "4:1");
// five\n
- assertThat(table.getLineAndColumn(12)).isEqualTo(new LineAndColumn(5, 1));
- assertThat(table.getLineAndColumn(16)).isEqualTo(new LineAndColumn(5, 5));
+ checkOffset(table, 12, "5:1");
+ checkOffset(table, 16, "5:5");
+
+ // start of final empty line
+ checkOffset(table, 17, "6:1"); // EOF
}
@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 ec8e418..77ad9dd 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
@@ -362,7 +362,8 @@
assertContainsError("syntax error at 'foo'");
- // Test that the actual parameters are: (1, $error$, 3):
+ // Test that the arguments are (1, '[x for foo foo foo foo]', 3),
+ // where the second, errant one is represented as an Identifier.
Identifier ident = (Identifier) e.getFunction();
assertThat(ident.getName()).isEqualTo("f");
@@ -375,9 +376,9 @@
Argument arg1 = e.getArguments().get(1);
Identifier arg1val = ((Identifier) arg1.getValue());
- assertThat(arg1val.getName()).isEqualTo("$error$");
+ assertThat(arg1val.getName()).isEqualTo("[x for foo foo foo foo]");
- assertLocation(5, 29, arg1val);
+ assertLocation(5, 28, arg1val);
assertThat(src.substring(5, 28)).isEqualTo("[x for foo foo foo foo]");
assertThat(arg1val.getEndLocation().column()).isEqualTo(29);
@@ -399,14 +400,6 @@
}
@Test
- public void testSecondaryLocation() throws SyntaxError.Exception {
- String expr = "f(1 % 2)";
- CallExpression call = (CallExpression) parseExpression(expr);
- Argument arg = call.getArguments().get(0);
- assertThat(arg.getEndLocation()).isLessThan(call.getEndLocation());
- }
-
- @Test
public void testPrimaryLocation() throws SyntaxError.Exception {
String expr = "f(1 + 2)";
CallExpression call = (CallExpression) parseExpression(expr);
@@ -494,10 +487,11 @@
}
@Test
- public void testEndLineAndColumnIsInclusive() // <-- this behavior is a mistake
- throws Exception {
+ public void testEndLineAndColumnIsExclusive() throws Exception {
+ // The behavior was 'inclusive' for a couple of years (see CL 170723732),
+ // but this was a mistake. Arithmetic on half-open intervals is much simpler.
AssignmentStatement stmt = (AssignmentStatement) parseStatement("a = b");
- assertThat(stmt.getLHS().getEndLocation().toString()).isEqualTo(":1:1");
+ assertThat(stmt.getLHS().getEndLocation().toString()).isEqualTo(":1:2");
}
@Test
@@ -549,9 +543,11 @@
String input = "for a,b in []: pass";
ForStatement stmt = (ForStatement) parseStatement(input);
assertThat(getText(input, stmt.getLHS())).isEqualTo("a,b");
+
input = "for (a,b) in []: pass";
stmt = (ForStatement) parseStatement(input);
assertThat(getText(input, stmt.getLHS())).isEqualTo("(a,b)");
+
assertExpressionLocationCorrect("a, b");
assertExpressionLocationCorrect("(a, b)");
}
@@ -1099,14 +1095,13 @@
@Test
public void testLoadOneSymbol() throws Exception {
- List<Statement> statements = parseStatements("load('//foo/bar:file.bzl', 'fun_test')\n");
+ String text = "load('//foo/bar:file.bzl', 'fun_test')\n";
+ List<Statement> statements = parseStatements(text);
LoadStatement stmt = (LoadStatement) statements.get(0);
assertThat(stmt.getImport().getValue()).isEqualTo("//foo/bar:file.bzl");
assertThat(stmt.getBindings()).hasSize(1);
Identifier sym = stmt.getBindings().get(0).getLocalName();
- int startOffset = sym.getStartOffset();
- assertWithMessage("getStartOffset()").that(startOffset).isEqualTo(27);
- assertWithMessage("getEndOffset()").that(sym.getEndOffset()).isEqualTo(startOffset + 10);
+ assertThat(getText(text, sym)).isEqualTo("fun_test"); // apparent location within string literal
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/PrettyPrintTest.java b/src/test/java/com/google/devtools/build/lib/syntax/PrettyPrintTest.java
index cf420bb..6293505 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/PrettyPrintTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/PrettyPrintTest.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.common.base.Joiner;
-import com.google.common.collect.ImmutableList;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -292,14 +291,9 @@
@Test
public void flowStatement() throws SyntaxError.Exception {
- // The parser would complain if we tried to construct them from source.
- Node breakNode = new FlowStatement(TokenKind.BREAK);
- assertIndentedPrettyMatches(breakNode, " break\n");
- assertTostringMatches(breakNode, "break\n");
-
- Node continueNode = new FlowStatement(TokenKind.CONTINUE);
- assertIndentedPrettyMatches(continueNode, " continue\n");
- assertTostringMatches(continueNode, "continue\n");
+ assertStmtIndentedPrettyMatches(
+ join("def f():", " pass", " continue", " break"),
+ join(" def f():", " pass", " continue", " break", ""));
}
@Test
@@ -366,41 +360,28 @@
@Test
public void loadStatement() throws SyntaxError.Exception {
- // load("foo.bzl", a="A", "B")
- Node loadStatement =
- new LoadStatement(
- new StringLiteral("foo.bzl"),
- ImmutableList.of(
- new LoadStatement.Binding(Identifier.of("a"), Identifier.of("A")),
- new LoadStatement.Binding(Identifier.of("B"), Identifier.of("B"))));
- assertIndentedPrettyMatches(
- loadStatement,
- " load(\"foo.bzl\", a=\"A\", \"B\")\n");
- assertTostringMatches(
- loadStatement,
- "load(\"foo.bzl\", a=\"A\", \"B\")\n");
+ assertStmtIndentedPrettyMatches(
+ "load(\"foo.bzl\", a=\"A\", \"B\")", " load(\"foo.bzl\", a=\"A\", \"B\")\n");
+ assertStmtTostringMatches(
+ "load(\"foo.bzl\", a=\"A\", \"B\")\n", "load(\"foo.bzl\", a=\"A\", \"B\")\n");
}
@Test
public void returnStatement() throws SyntaxError.Exception {
- assertIndentedPrettyMatches(
- new ReturnStatement(new StringLiteral("foo")),
- " return \"foo\"\n");
- assertTostringMatches(
- new ReturnStatement(new StringLiteral("foo")),
- "return \"foo\"\n");
+ assertStmtIndentedPrettyMatches("return \"foo\"", " return \"foo\"\n");
+ assertStmtTostringMatches("return \"foo\"", "return \"foo\"\n");
- assertIndentedPrettyMatches(new ReturnStatement(Identifier.of("None")), " return None\n");
- assertTostringMatches(new ReturnStatement(Identifier.of("None")), "return None\n");
+ assertStmtIndentedPrettyMatches("return None", " return None\n");
+ assertStmtTostringMatches("return None", "return None\n");
- assertIndentedPrettyMatches(new ReturnStatement(null), " return\n");
- assertTostringMatches(new ReturnStatement(null), "return\n");
+ assertStmtIndentedPrettyMatches("return", " return\n");
+ assertStmtTostringMatches("return", "return\n");
}
// Miscellaneous.
@Test
- public void buildFileAST() throws SyntaxError.Exception {
+ public void file() throws SyntaxError.Exception {
Node node = parseFile("print(x)\nprint(y)");
assertIndentedPrettyMatches(
node,
@@ -412,7 +393,7 @@
@Test
public void comment() throws SyntaxError.Exception {
- Comment node = new Comment("foo");
+ Comment node = new Comment(0, "foo");
assertIndentedPrettyMatches(node, " # foo");
assertTostringMatches(node, "foo");
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java
index 7ffd09a..28b1bbf 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFileTest.java
@@ -91,11 +91,12 @@
@Test
public void testImplicitStringConcatenationFails() throws Exception {
+ // TODO(adonovan): move to ParserTest.
StarlarkFile file = parseFile("a = 'foo' 'bar'");
SyntaxError error =
LexerTest.assertContainsError(
file.errors(), "Implicit string concatenation is forbidden, use the + operator");
- assertThat(error.location().toString()).isEqualTo("foo.star:1:10");
+ assertThat(error.location().toString()).isEqualTo("foo.star:1:11"); // start of 'bar'
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java
index 83df6ce..de3442e 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadDebuggingTest.java
@@ -109,16 +109,16 @@
assertThat(buf.toString())
.isEqualTo(
""
- // location is start of g(4, 5, 6) call:
- + "<toplevel> @ main.star:3:1 local={}\n"
- // location is start of "f()" call:
- + "g @ main.star:2:3 local={a=4, y=5, z=6}\n"
+ // location is paren of g(4, 5, 6) call:
+ + "<toplevel> @ main.star:3:2 local={}\n"
+ // location is paren of "f()" call:
+ + "g @ main.star:2:4 local={a=4, y=5, z=6}\n"
// location is "current PC" in f.
+ "f @ builtin:12 local={}\n");
// Same, with "lite" stack API.
assertThat(result[1].toString()) // an ImmutableList<StarlarkThread.CallStackEntry>
- .isEqualTo("[<toplevel>@main.star:3:1, g@main.star:2:3, f@builtin:12]");
+ .isEqualTo("[<toplevel>@main.star:3:2, g@main.star:2:4, f@builtin:12]");
// TODO(adonovan): more tests:
// - a stack containing functions defined in different modules.
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 402dff6..747691d 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
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
-import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.syntax.LexerTest.assertContainsError;
import java.util.List;
@@ -80,10 +79,9 @@
@Test
public void testLoadAfterStatement() throws Exception {
options.requireLoadStatementsFirst(true);
- assertInvalid(
- "load() statements must be called before any other statement", //
- "a = 5",
- "load(':b.bzl', 'c')");
+ List<SyntaxError> errors = getValidationErrors("a = 5", "load(':b.bzl', 'c')");
+ assertContainsError(errors, ":2:1: load statements must appear before any other statement");
+ assertContainsError(errors, ":1:1: \tfirst non-load statement appears here");
}
@Test
@@ -304,23 +302,6 @@
}
@Test
- public void testDollarErrorDoesNotLeak() throws Exception {
- List<SyntaxError> errors =
- getValidationErrors(
- "def GenerateMapNames():", //
- " a = 2",
- " b = [3, 4]",
- " if a not b:",
- " print(a)");
- assertContainsError(errors, "syntax error at 'b': expected 'in'");
- // Parser uses "$error" symbol for error recovery.
- // It should not be used in error messages.
- for (SyntaxError event : errors) {
- assertThat(event.message()).doesNotContain("$error$");
- }
- }
-
- @Test
public void testPositionalAfterStarArg() throws Exception {
assertInvalid(
"positional argument is misplaced (positional arguments come first)", //
diff --git a/src/test/shell/bazel/bazel_workspaces_test.sh b/src/test/shell/bazel/bazel_workspaces_test.sh
index 5a97570..527fe07 100755
--- a/src/test/shell/bazel/bazel_workspaces_test.sh
+++ b/src/test/shell/bazel/bazel_workspaces_test.sh
@@ -88,11 +88,11 @@
set_workspace_command 'repository_ctx.execute(["echo", "testing!"])'
build_and_process_log
- ensure_contains_exactly "location: .*repos.bzl:2:3" 1
+ ensure_contains_exactly "location: .*repos.bzl:2:25" 1
# Cached executions are not replayed
build_and_process_log
- ensure_contains_exactly "location: .*repos.bzl:2:3" 0
+ ensure_contains_exactly "location: .*repos.bzl:2:25" 0
}
# The workspace is set up so that the function is interrupted and re-executed.
@@ -139,7 +139,7 @@
build_and_process_log
- ensure_contains_atleast "location: .*repos.bzl:2:3" 2
+ ensure_contains_atleast "location: .*repos.bzl:2:" 2
}
@@ -149,7 +149,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:25' 1
ensure_contains_exactly 'arguments: "echo"' 1
ensure_contains_exactly 'arguments: "test_contents"' 1
ensure_contains_exactly 'timeout_seconds: 21' 1
@@ -165,7 +165,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:25' 1
ensure_contains_exactly 'arguments: "echo"' 1
ensure_contains_exactly 'arguments: "test2"' 1
ensure_contains_exactly 'timeout_seconds: 32' 1
@@ -193,7 +193,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:26' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'download_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/file.txt\"" 1
@@ -221,7 +221,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:26' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'download_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/file1.txt\"" 1
@@ -248,7 +248,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:26' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'download_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/file.txt\"" 1
@@ -277,7 +277,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:26' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'download_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/file.txt\"" 1
@@ -337,8 +337,8 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:3:3' 1
- ensure_contains_exactly 'location: .*repos.bzl:4:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:3:26' 1
+ ensure_contains_exactly 'location: .*repos.bzl:4:25' 1
ensure_contains_atleast 'rule: "//external:repo"' 2
ensure_contains_exactly 'download_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/download_then_extract.zip\"" 1
@@ -374,8 +374,8 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:3:3' 1
- ensure_contains_exactly 'location: .*repos.bzl:4:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:3:26' 1
+ ensure_contains_exactly 'location: .*repos.bzl:4:25' 1
ensure_contains_atleast 'rule: "//external:repo"' 2
ensure_contains_exactly 'download_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/download_then_extract.tar.gz\"" 1
@@ -408,7 +408,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:38' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'download_and_extract_event' 1
ensure_contains_exactly "url: \"http://localhost:${fileserver_port}/download_and_extract.zip\"" 1
@@ -425,7 +425,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:22' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
# There are 3 file_event in external:repo as it is currently set up
@@ -440,7 +440,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:22' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
# There are 3 file_event in external:repo as it is currently set up
@@ -471,8 +471,8 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:4:3' 1
- ensure_contains_exactly 'location: .*repos.bzl:5:17' 1
+ ensure_contains_exactly 'location: .*repos.bzl:4:22' 1
+ ensure_contains_exactly 'location: .*repos.bzl:5:36' 1
ensure_contains_atleast 'rule: "//external:repo"' 2
ensure_contains_exactly 'read_event' 1
@@ -521,7 +521,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:3:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:22' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'symlink_event' 1
ensure_contains_exactly 'target: ".*symlink.txt"' 1
@@ -534,7 +534,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:3:3' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:22' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'template_event' 1
ensure_contains_exactly 'path: ".*template_out.txt"' 1
@@ -549,7 +549,7 @@
build_and_process_log --exclude_rule "//external:local_config_cc"
- ensure_contains_exactly 'location: .*repos.bzl:2:9' 1
+ ensure_contains_exactly 'location: .*repos.bzl:2:29' 1
ensure_contains_atleast 'rule: "//external:repo"' 1
ensure_contains_exactly 'which_event' 1
ensure_contains_exactly 'program: "which_prog"' 1
diff --git a/src/test/shell/bazel/external_integration_test.sh b/src/test/shell/bazel/external_integration_test.sh
index 43532a1..63ca019 100755
--- a/src/test/shell/bazel/external_integration_test.sh
+++ b/src/test/shell/bazel/external_integration_test.sh
@@ -2325,11 +2325,11 @@
# We expect to find the call stack for the definition of the repositories
# a and b
- expect_log "WORKSPACE:4:1"
- expect_log "foo.bzl:4:3"
+ expect_log "WORKSPACE:4:4"
+ expect_log "foo.bzl:4:15"
- expect_log "WORKSPACE:5:1"
- expect_log "bar.bzl:4:3"
+ expect_log "WORKSPACE:5:4"
+ expect_log "bar.bzl:4:15"
}
function test_missing_repo_reported() {
diff --git a/src/test/shell/integration/ui_test.sh b/src/test/shell/integration/ui_test.sh
index 1c29872..c875880 100755
--- a/src/test/shell/integration/ui_test.sh
+++ b/src/test/shell/integration/ui_test.sh
@@ -545,11 +545,11 @@
bazel build --attempt_to_print_relative_paths=false \
error:failwitherror > "${TEST_log}" 2>&1 && fail "expected failure"
- expect_log "^ERROR: $(pwd)/error/BUILD:1:1: Executing genrule"
+ expect_log "^ERROR: $(pwd)/error/BUILD:1:8: Executing genrule"
bazel build --attempt_to_print_relative_paths=true \
error:failwitherror > "${TEST_log}" 2>&1 && fail "expected failure"
- expect_log "^ERROR: error/BUILD:1:1: Executing genrule"
+ expect_log "^ERROR: error/BUILD:1:8: Executing genrule"
expect_not_log "$(pwd)/error/BUILD"
}
diff --git a/src/tools/skylark/java/com/google/devtools/skylark/common/LocationRange.java b/src/tools/skylark/java/com/google/devtools/skylark/common/LocationRange.java
index 29d731f..e541416 100644
--- a/src/tools/skylark/java/com/google/devtools/skylark/common/LocationRange.java
+++ b/src/tools/skylark/java/com/google/devtools/skylark/common/LocationRange.java
@@ -15,8 +15,6 @@
package com.google.devtools.skylark.common;
import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.syntax.Location.LineAndColumn;
-import javax.annotation.Nullable;
/**
* Location range of a linter warning.
@@ -61,14 +59,6 @@
this.column = column;
}
- public static Location from(@Nullable LineAndColumn lac) {
- // LineAndColumn may be null, e.g. if a StarlarkFile contains no statements:
- if (lac == null) {
- return new Location(1, 1);
- }
- return new Location(lac.line, lac.column);
- }
-
public static int compare(Location l1, Location l2) {
int cmp = l1.line - l2.line;
if (cmp != 0) {
