starlark: improve error message for "for k,v in dict(...)"
Was: type 'string' is not iterable
Now: got 'string' in sequence assignment (want 2-element sequence)
Also:
- add test (testdata/loop.sky)
- migrate related tests from Java to loop.sky
- naming tweaks
PiperOrigin-RevId: 334874972
diff --git a/src/main/java/net/starlark/java/eval/Eval.java b/src/main/java/net/starlark/java/eval/Eval.java
index 14a8b87..2b5779e 100644
--- a/src/main/java/net/starlark/java/eval/Eval.java
+++ b/src/main/java/net/starlark/java/eval/Eval.java
@@ -355,11 +355,12 @@
// assignments fail when the left side aliases the right,
// which is a tricky case in Python assignment semantics.
int nrhs = Starlark.len(x);
- if (nrhs < 0) {
- throw Starlark.errorf("got '%s' in sequence assignment", Starlark.type(x));
- }
- Iterable<?> rhs = Starlark.toIterable(x); // fails if x is a string
int nlhs = lhs.size();
+ if (nrhs < 0 || x instanceof String) { // strings are not iterable
+ throw Starlark.errorf(
+ "got '%s' in sequence assignment (want %d-element sequence)", Starlark.type(x), nlhs);
+ }
+ Iterable<?> rhs = Starlark.toIterable(x);
if (nlhs != nrhs) {
throw Starlark.errorf(
"too %s values to unpack (got %d, want %d)", nrhs < nlhs ? "few" : "many", nrhs, nlhs);
@@ -766,10 +767,10 @@
Comprehension.For forClause = (Comprehension.For) clause;
Object iterable = eval(fr, forClause.getIterable());
- Iterable<?> listValue = Starlark.toIterable(iterable);
+ Iterable<?> seq = Starlark.toIterable(iterable);
EvalUtils.addIterator(iterable);
try {
- for (Object elem : listValue) {
+ for (Object elem : seq) {
assign(fr, forClause.getVars(), elem);
execClauses(index + 1);
}
diff --git a/src/main/java/net/starlark/java/syntax/ForStatement.java b/src/main/java/net/starlark/java/syntax/ForStatement.java
index 68cad7b..dffddf4 100644
--- a/src/main/java/net/starlark/java/syntax/ForStatement.java
+++ b/src/main/java/net/starlark/java/syntax/ForStatement.java
@@ -16,12 +16,12 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-/** Syntax node for a for loop statement. */
+/** Syntax node for a for loop statement, {@code for vars in iterable: ...}. */
public final class ForStatement extends Statement {
private final int forOffset;
private final Expression vars;
- private final Expression collection;
+ private final Expression iterable;
private final ImmutableList<Statement> body; // non-empty if well formed
/** Constructs a for loop statement. */
@@ -29,26 +29,30 @@
FileLocations locs,
int forOffset,
Expression vars,
- Expression collection,
+ Expression iterable,
ImmutableList<Statement> body) {
super(locs);
this.forOffset = forOffset;
this.vars = Preconditions.checkNotNull(vars);
- this.collection = Preconditions.checkNotNull(collection);
+ this.iterable = Preconditions.checkNotNull(iterable);
this.body = body;
}
+ /**
+ * Returns variables assigned by each iteration. May be a compound target such as {@code (a[b],
+ * c.d)}.
+ */
public Expression getVars() {
return vars;
}
- /**
- * @return The collection we iterate on, e.g. `col` in `for x in col:`
- */
+ /** Returns the iterable value. */
+ // TODO(adonovan): rename to getIterable.
public Expression getCollection() {
- return collection;
+ return iterable;
}
+ /** Returns the statements of the loop body. Non-empty if parsing succeeded. */
public ImmutableList<Statement> getBody() {
return body;
}
@@ -61,13 +65,13 @@
@Override
public int getEndOffset() {
return body.isEmpty()
- ? collection.getEndOffset() // wrong, but tree is ill formed
+ ? iterable.getEndOffset() // wrong, but tree is ill formed
: body.get(body.size() - 1).getEndOffset();
}
@Override
public String toString() {
- return "for " + vars + " in " + collection + ": ...\n";
+ return "for " + vars + " in " + iterable + ": ...\n";
}
@Override
diff --git a/src/test/java/net/starlark/java/eval/EvaluationTest.java b/src/test/java/net/starlark/java/eval/EvaluationTest.java
index f1c47f8..e397239 100644
--- a/src/test/java/net/starlark/java/eval/EvaluationTest.java
+++ b/src/test/java/net/starlark/java/eval/EvaluationTest.java
@@ -401,168 +401,6 @@
}
@Test
- public void testListComprehensions() throws Exception {
- ev.new Scenario()
- .testExactOrder("['foo/%s.java' % x for x in []]")
- .testExactOrder(
- "['foo/%s.java' % y for y in ['bar', 'wiz', 'quux']]",
- "foo/bar.java", "foo/wiz.java", "foo/quux.java")
- .testExactOrder(
- "['%s/%s.java' % (z, t) for z in ['foo', 'bar'] " + "for t in ['baz', 'wiz', 'quux']]",
- "foo/baz.java",
- "foo/wiz.java",
- "foo/quux.java",
- "bar/baz.java",
- "bar/wiz.java",
- "bar/quux.java")
- .testExactOrder(
- "['%s/%s.java' % (b, b) for a in ['foo', 'bar'] " + "for b in ['baz', 'wiz', 'quux']]",
- "baz/baz.java",
- "wiz/wiz.java",
- "quux/quux.java",
- "baz/baz.java",
- "wiz/wiz.java",
- "quux/quux.java")
- .testExactOrder(
- "['%s/%s.%s' % (c, d, e) for c in ['foo', 'bar'] "
- + "for d in ['baz', 'wiz', 'quux'] for e in ['java', 'cc']]",
- "foo/baz.java",
- "foo/baz.cc",
- "foo/wiz.java",
- "foo/wiz.cc",
- "foo/quux.java",
- "foo/quux.cc",
- "bar/baz.java",
- "bar/baz.cc",
- "bar/wiz.java",
- "bar/wiz.cc",
- "bar/quux.java",
- "bar/quux.cc")
- .testExactOrder("[i for i in (1, 2)]", StarlarkInt.of(1), StarlarkInt.of(2))
- .testExactOrder("[i for i in [2, 3] or [1, 2]]", StarlarkInt.of(2), StarlarkInt.of(3));
- }
-
- @Test
- public void testNestedListComprehensions() throws Exception {
- ev.new Scenario()
- .setUp("li = [[1, 2], [3, 4]]")
- .testExactOrder(
- "[j for i in li for j in i]",
- StarlarkInt.of(1),
- StarlarkInt.of(2),
- StarlarkInt.of(3),
- StarlarkInt.of(4));
- ev.new Scenario()
- .setUp("input = [['abc'], ['def', 'ghi']]\n")
- .testExactOrder(
- "['%s %s' % (b, c) for a in input for b in a for c in b.elems()]",
- "abc a", "abc b", "abc c", "def d", "def e", "def f", "ghi g", "ghi h", "ghi i");
- }
-
- @Test
- public void testListComprehensionsMultipleVariables() throws Exception {
- ev.new Scenario()
- .testEval("[x + y for x, y in [(1, 2), (3, 4)]]", "[3, 7]")
- .testEval("[z + t for (z, t) in [[1, 2], [3, 4]]]", "[3, 7]");
- }
-
- @Test
- public void testSequenceAssignment() throws Exception {
- // Assignment to empty list/tuple is permitted.
- // See https://github.com/bazelbuild/starlark/issues/93 for discussion.
- ev.exec("() = ()");
- ev.exec("[] = ()");
-
- // RHS not iterable
- ev.checkEvalError(
- "got 'int' in sequence assignment", //
- "x, y = 1");
- ev.checkEvalError(
- "got 'int' in sequence assignment", //
- "(x,) = 1");
- ev.checkEvalError(
- "got 'int' in sequence assignment", //
- "[x] = 1");
-
- // too few
- ev.checkEvalError(
- "too few values to unpack (got 0, want 2)", //
- "x, y = ()");
- ev.checkEvalError(
- "too few values to unpack (got 0, want 2)", //
- "[x, y] = ()");
-
- // just right
- ev.exec("x, y = 1, 2");
- ev.exec("[x, y] = 1, 2");
- ev.exec("(x,) = [1]");
-
- // too many
- ev.checkEvalError(
- "got 'int' in sequence assignment", //
- "() = 1");
- ev.checkEvalError(
- "too many values to unpack (got 1, want 0)", //
- "() = (1,)");
- ev.checkEvalError(
- "too many values to unpack (got 3, want 2)", //
- "x, y = 1, 2, 3");
- ev.checkEvalError(
- "too many values to unpack (got 3, want 2)", //
- "[x, y] = 1, 2, 3");
- }
-
- @Test
- public void testListComprehensionsMultipleVariablesFail() throws Exception {
- ev.new Scenario()
- .testIfErrorContains(
- "too few values to unpack (got 2, want 3)", //
- "[x + y for x, y, z in [(1, 2), (3, 4)]]")
- .testIfExactError(
- "got 'int' in sequence assignment", //
- "[x + y for x, y in (1, 2)]");
-
- ev.new Scenario()
- .testIfErrorContains(
- "too few values to unpack (got 2, want 3)", //
- "def foo (): return [x + y for x, y, z in [(1, 2), (3, 4)]]",
- "foo()");
-
- ev.new Scenario()
- .testIfErrorContains(
- "got 'int' in sequence assignment", //
- "def bar (): return [x + y for x, y in (1, 2)]",
- "bar()");
-
- ev.new Scenario()
- .testIfErrorContains(
- "too few values to unpack (got 2, want 3)", //
- "[x + y for x, y, z in [(1, 2), (3, 4)]]");
-
- ev.new Scenario()
- .testIfErrorContains(
- "got 'int' in sequence assignment", //
- "[x2 + y2 for x2, y2 in (1, 2)]");
-
- // Assignment to empty tuple is permitted.
- // See https://github.com/bazelbuild/starlark/issues/93 for discussion.
- ev.new Scenario().testEval("[1 for [] in [(), []]]", "[1, 1]");
- }
-
- @Test
- public void testListComprehensionsWithFiltering() throws Exception {
- ev.new Scenario()
- .setUp("range3 = [0, 1, 2]")
- .testEval("[a for a in (4, None, 2, None, 1) if a != None]", "[4, 2, 1]")
- .testEval("[b+c for b in [0, 1, 2] for c in [0, 1, 2] if b + c > 2]", "[3, 3, 4]")
- .testEval("[d+e for d in range3 if d % 2 == 1 for e in range3]", "[1, 2, 3]")
- .testEval(
- "[[f,g] for f in [0, 1, 2, 3, 4] if f for g in [5, 6, 7, 8] if f * g % 12 == 0 ]",
- "[[2, 6], [3, 8], [4, 6]]")
- .testEval("[h for h in [4, 2, 0, 1] if h]", "[4, 2, 1]");
- }
-
- @Test
public void testListComprehensionDefinitionOrder() throws Exception {
// This exercises the .bzl file behavior. This is a dynamic error.
// (The error message for BUILD files is slightly different (no "local")
diff --git a/src/test/java/net/starlark/java/eval/testdata/loop.sky b/src/test/java/net/starlark/java/eval/testdata/loop.sky
new file mode 100644
index 0000000..1ef38c1
--- /dev/null
+++ b/src/test/java/net/starlark/java/eval/testdata/loop.sky
@@ -0,0 +1,150 @@
+# for statements and list comprehensions
+
+# --- for statements ---
+
+# sequence assignment
+
+# Assignment to empty list/tuple is permitted.
+# https://github.com/bazelbuild/starlark/issues/93 for discussion.
+() = ()
+[] = ()
+
+# RHS not iterable
+x, y = 1 ### got 'int' in sequence assignment
+---
+(x,) = 1 ### got 'int' in sequence assignment
+---
+[x] = 1 ### got 'int' in sequence assignment
+---
+
+# too few
+x, y = () ### too few values to unpack (got 0, want 2)
+---
+[x, y] = () ### too few values to unpack (got 0, want 2)
+---
+
+# just right
+x, y = 1, 2
+---
+[x, y] = 1, 2
+---
+(x,) = [1]
+---
+
+# too many
+() = 1 ### got 'int' in sequence assignment
+---
+() = (1,) ### too many values to unpack (got 1, want 0)
+---
+x, y = 1, 2, 3 ### too many values to unpack (got 3, want 2)
+---
+[x, y] = 1, 2, 3 ### too many values to unpack (got 3, want 2)
+---
+
+# Assignment to empty tuple is permitted.
+# See https://github.com/bazelbuild/starlark/issues/93 for discussion.
+assert_eq([1 for [] in [(), []]], [1, 1])
+
+# Iterating over dict without .items() gives informative error.
+assert_eq([v for v in dict(a = "b")], ["a"])
+[None for () in dict(a = "b")] ### got 'string' in sequence assignment (want 0-element sequence)
+---
+[None for (v1,) in dict(a = "b")] ### got 'string' in sequence assignment (want 1-element sequence)
+---
+[None for v1, v2 in dict(a = "b")] ### got 'string' in sequence assignment (want 2-element sequence)
+---
+
+# --- list comprehensions ---
+
+assert_eq(["foo/%s.java" % x for x in []], [])
+assert_eq(
+ ["foo/%s.java" % y for y in ["bar", "wiz", "quux"]],
+ ["foo/bar.java", "foo/wiz.java", "foo/quux.java"])
+assert_eq(
+ ["%s/%s.java" % (z, t)
+ for z in ["foo", "bar"]
+ for t in ["baz", "wiz", "quux"]],
+ ["foo/baz.java",
+ "foo/wiz.java",
+ "foo/quux.java",
+ "bar/baz.java",
+ "bar/wiz.java",
+ "bar/quux.java"])
+assert_eq(
+ ["%s/%s.java" % (b, b)
+ for a in ["foo", "bar"]
+ for b in ["baz", "wiz", "quux"]],
+ ["baz/baz.java",
+ "wiz/wiz.java",
+ "quux/quux.java",
+ "baz/baz.java",
+ "wiz/wiz.java",
+ "quux/quux.java"])
+assert_eq(
+ ["%s/%s.%s" % (c, d, e)
+ for c in ["foo", "bar"]
+ for d in ["baz", "wiz", "quux"]
+ for e in ["java", "cc"]],
+ ["foo/baz.java",
+ "foo/baz.cc",
+ "foo/wiz.java",
+ "foo/wiz.cc",
+ "foo/quux.java",
+ "foo/quux.cc",
+ "bar/baz.java",
+ "bar/baz.cc",
+ "bar/wiz.java",
+ "bar/wiz.cc",
+ "bar/quux.java",
+ "bar/quux.cc"])
+assert_eq([i for i in (1, 2)], [1,2])
+assert_eq([i for i in [2, 3] or [1, 2]], [2, 3])
+
+# nested list comprehensions
+li = [[1, 2], [3, 4]]
+assert_eq([j for i in li for j in i], [1,2,3,4])
+input = [["abc"], ["def", "ghi"]]
+assert_eq(
+ ["%s %s" % (b, c)
+ for a in input
+ for b in a
+ for c in b.elems()],
+ ["abc a", "abc b", "abc c", "def d", "def e", "def f", "ghi g", "ghi h", "ghi i"])
+
+# filtering
+range3 = [0, 1, 2]
+assert_eq([a for a in (4, None, 2, None, 1)
+ if a != None],
+ [4, 2, 1])
+assert_eq([b+c for b in [0, 1, 2]
+ for c in [0, 1, 2]
+ if b + c > 2],
+ [3, 3, 4])
+assert_eq([d+e for d in range3
+ if d % 2 == 1
+ for e in range3],
+ [1, 2, 3])
+assert_eq([[f, g] for f in [0, 1, 2, 3, 4]
+ if f
+ for g in [5, 6, 7, 8]
+ if f * g % 12 == 0],
+ [[2, 6], [3, 8], [4, 6]])
+assert_eq([h for h in [4, 2, 0, 1] if h], [4, 2, 1])
+
+# multiple variables, ok
+assert_eq([x + y for x, y in [(1, 2), (3, 4)]], [3, 7])
+assert_eq([z + t for (z, t) in [[1, 2], [3, 4]]], [3, 7])
+
+# multiple variables, fail
+[x + y for x, y, z in [(1, 2), (3, 4)]] ### too few values to unpack (got 2, want 3)
+---
+[x + y for x, y in (1, 2)] ### got 'int' in sequence assignment (want 2-element sequence)
+---
+[x + y for x, y, z in [(1, 2), (3, 4)]] ### too few values to unpack (got 2, want 3)
+---
+[x + y for x, y in (1, 2)] ### got 'int' in sequence assignment (want 2-element sequence)
+---
+[x + y for x, y, z in [(1, 2), (3, 4)]] ### too few values to unpack (got 2, want 3)
+---
+[x2 + y2 for x2, y2 in (1, 2)] ### got 'int' in sequence assignment (want 2-element sequence)
+---
