starlark: more validity checks
...in Starlark.call and StarlarkList.{of,copyOf,immutableCopyOf}.
Benchmarks showed no significant cost.
Several tests that passed Integer needed fixing.
PiperOrigin-RevId: 341727596
diff --git a/src/main/java/net/starlark/java/eval/Starlark.java b/src/main/java/net/starlark/java/eval/Starlark.java
index 3a96b4f..086be09 100644
--- a/src/main/java/net/starlark/java/eval/Starlark.java
+++ b/src/main/java/net/starlark/java/eval/Starlark.java
@@ -568,7 +568,7 @@
int i = 0;
for (Map.Entry<String, Object> e : kwargs.entrySet()) {
named[i++] = e.getKey();
- named[i++] = e.getValue();
+ named[i++] = Starlark.checkValid(e.getValue());
}
return fastcall(thread, fn, args.toArray(), named);
}
diff --git a/src/main/java/net/starlark/java/eval/StarlarkList.java b/src/main/java/net/starlark/java/eval/StarlarkList.java
index c11bfcc..9383d3a 100644
--- a/src/main/java/net/starlark/java/eval/StarlarkList.java
+++ b/src/main/java/net/starlark/java/eval/StarlarkList.java
@@ -158,7 +158,15 @@
return list;
}
- return wrap(mutability, Iterables.toArray(elems, Object.class));
+ Object[] array = Iterables.toArray(elems, Object.class);
+ checkElemsValid(array);
+ return wrap(mutability, array);
+ }
+
+ private static void checkElemsValid(Object[] elems) {
+ for (Object elem : elems) {
+ Starlark.checkValid(elem);
+ }
}
/**
@@ -173,11 +181,13 @@
* mutability} is null, the list is immutable.
*/
public static <T> StarlarkList<T> of(@Nullable Mutability mutability, T... elems) {
+ checkElemsValid(elems);
return wrap(mutability, Arrays.copyOf(elems, elems.length));
}
/** Returns an immutable {@code StarlarkList} with the given items. */
public static <T> StarlarkList<T> immutableOf(T... elems) {
+ checkElemsValid(elems);
return wrap(null, Arrays.copyOf(elems, elems.length));
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/StarlarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/StarlarkProviderTest.java
index b6632de..215e616 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/StarlarkProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/StarlarkProviderTest.java
@@ -23,6 +23,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import net.starlark.java.eval.Mutability;
import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkInt;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import org.junit.Test;
@@ -140,7 +141,8 @@
thread,
provider,
/*args=*/ ImmutableList.of(),
- /*kwargs=*/ ImmutableMap.of("a", 1, "b", 2, "c", 3));
+ /*kwargs=*/ ImmutableMap.of(
+ "a", StarlarkInt.of(1), "b", StarlarkInt.of(2), "c", StarlarkInt.of(3)));
assertThat(result).isInstanceOf(StarlarkInfo.class);
return (StarlarkInfo) result;
}
@@ -149,8 +151,8 @@
/** Asserts that a {@link StarlarkInfo} has fields a=1, b=2, c=3 (and nothing else). */
private static void assertHasExactlyValuesA1B2C3(StarlarkInfo info) throws Exception {
assertThat(info.getFieldNames()).containsExactly("a", "b", "c");
- assertThat(info.getValue("a")).isEqualTo(1);
- assertThat(info.getValue("b")).isEqualTo(2);
- assertThat(info.getValue("c")).isEqualTo(3);
+ assertThat(info.getValue("a")).isEqualTo(StarlarkInt.of(1));
+ assertThat(info.getValue("b")).isEqualTo(StarlarkInt.of(2));
+ assertThat(info.getValue("c")).isEqualTo(StarlarkInt.of(3));
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/starlarkdebug/server/StarlarkDebugServerTest.java b/src/test/java/com/google/devtools/build/lib/starlarkdebug/server/StarlarkDebugServerTest.java
index 95a81a7..8f32c63 100644
--- a/src/test/java/com/google/devtools/build/lib/starlarkdebug/server/StarlarkDebugServerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlarkdebug/server/StarlarkDebugServerTest.java
@@ -375,7 +375,11 @@
.addScope(
Scope.newBuilder()
.setName("global")
- .addBinding(getValueProto("x", StarlarkList.of(/*mutability=*/ null, 1, 2, 3))))
+ .addBinding(
+ getValueProto(
+ "x",
+ StarlarkList.immutableOf(
+ StarlarkInt.of(1), StarlarkInt.of(2), StarlarkInt.of(3)))))
.build());
}
@@ -398,7 +402,10 @@
Value xValue = frames.getFrame(0).getScope(0).getBinding(0);
assertValuesEqualIgnoringId(
- xValue, getValueProto("x", StarlarkList.of(/*mutability=*/ null, 1, 2, 3)));
+ xValue,
+ getValueProto(
+ "x",
+ StarlarkList.immutableOf(StarlarkInt.of(1), StarlarkInt.of(2), StarlarkInt.of(3))));
List<Value> children = getChildren(xValue);
@@ -563,7 +570,11 @@
ListFramesResponse frames = listFrames(threadId);
assertThat(frames.getFrame(0).getScope(0).getBindingList())
- .contains(getValueProto("x", StarlarkList.of(/*mutability=*/ null, 1, 2, 3, 4)));
+ .contains(
+ getValueProto(
+ "x",
+ StarlarkList.immutableOf(
+ StarlarkInt.of(1), StarlarkInt.of(2), StarlarkInt.of(3), StarlarkInt.of(4))));
}
@Test
diff --git a/src/test/java/net/starlark/java/eval/PrinterTest.java b/src/test/java/net/starlark/java/eval/PrinterTest.java
index e5accf4..92957a6 100644
--- a/src/test/java/net/starlark/java/eval/PrinterTest.java
+++ b/src/test/java/net/starlark/java/eval/PrinterTest.java
@@ -45,22 +45,25 @@
assertThat(Starlark.repr("'")).isEqualTo("\"'\"");
assertThat(Starlark.str("\"")).isEqualTo("\"");
assertThat(Starlark.repr("\"")).isEqualTo("\"\\\"\"");
- assertThat(Starlark.str(3)).isEqualTo("3");
- assertThat(Starlark.repr(3)).isEqualTo("3");
+ assertThat(Starlark.str(StarlarkInt.of(3))).isEqualTo("3");
+ assertThat(Starlark.repr(StarlarkInt.of(3))).isEqualTo("3");
assertThat(Starlark.repr(Starlark.NONE)).isEqualTo("None");
List<?> list = StarlarkList.of(null, "foo", "bar");
List<?> tuple = Tuple.of("foo", "bar");
- assertThat(Starlark.str(Tuple.of(1, list, 3))).isEqualTo("(1, [\"foo\", \"bar\"], 3)");
- assertThat(Starlark.repr(Tuple.of(1, list, 3))).isEqualTo("(1, [\"foo\", \"bar\"], 3)");
- assertThat(Starlark.str(StarlarkList.of(null, 1, tuple, 3)))
+ assertThat(Starlark.str(Tuple.of(StarlarkInt.of(1), list, StarlarkInt.of(3))))
+ .isEqualTo("(1, [\"foo\", \"bar\"], 3)");
+ assertThat(Starlark.repr(Tuple.of(StarlarkInt.of(1), list, StarlarkInt.of(3))))
+ .isEqualTo("(1, [\"foo\", \"bar\"], 3)");
+ assertThat(Starlark.str(StarlarkList.of(null, StarlarkInt.of(1), tuple, StarlarkInt.of(3))))
.isEqualTo("[1, (\"foo\", \"bar\"), 3]");
- assertThat(Starlark.repr(StarlarkList.of(null, 1, tuple, 3)))
+ assertThat(Starlark.repr(StarlarkList.of(null, StarlarkInt.of(1), tuple, StarlarkInt.of(3))))
.isEqualTo("[1, (\"foo\", \"bar\"), 3]");
Map<Object, Object> dict =
- ImmutableMap.<Object, Object>of(1, tuple, 2, list, "foo", StarlarkList.of(null));
+ ImmutableMap.<Object, Object>of(
+ StarlarkInt.of(1), tuple, StarlarkInt.of(2), list, "foo", StarlarkList.of(null));
assertThat(Starlark.str(dict))
.isEqualTo("{1: (\"foo\", \"bar\"), 2: [\"foo\", \"bar\"], \"foo\": []}");
assertThat(Starlark.repr(dict))
@@ -76,11 +79,12 @@
@Test
public void testOutputOrderOfMap() throws Exception {
Map<Object, Object> map = new LinkedHashMap<>();
- map.put(5, 5);
- map.put(3, 3);
- map.put("foo", 42);
- map.put(7, "bar");
- assertThat(Starlark.str(map)).isEqualTo("{5: 5, 3: 3, \"foo\": 42, 7: \"bar\"}");
+ map.put(StarlarkInt.of(5), StarlarkInt.of(5));
+ map.put(StarlarkInt.of(3), StarlarkInt.of(3));
+ map.put("foo", StarlarkInt.of(42));
+ map.put(StarlarkInt.of(7), "bar");
+ assertThat(Starlark.str(Starlark.fromJava(map, null)))
+ .isEqualTo("{5: 5, 3: 3, \"foo\": 42, 7: \"bar\"}");
}
@Test
diff --git a/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java b/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java
index 6e8e9bf..e7cc32c 100644
--- a/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java
+++ b/src/test/java/net/starlark/java/eval/StarlarkMutableTest.java
@@ -33,7 +33,9 @@
@Test
public void testListViewsCheckMutability() throws Exception {
Mutability mutability = Mutability.create("test");
- StarlarkList<Object> list = StarlarkList.copyOf(mutability, ImmutableList.of(1, 2, 3));
+ StarlarkList<Object> list =
+ StarlarkList.copyOf(
+ mutability, ImmutableList.of(StarlarkInt.of(1), StarlarkInt.of(2), StarlarkInt.of(3)));
mutability.freeze();
{
