starlark: improve "unhashable type" error messages
StarlarkValue.isImmutable is now checkHashable.
Instead of returning a boolean,
it reports an informative error (or not).
Also, rename {EvalUtils->Starlark}.checkHashable and publish.
PiperOrigin-RevId: 336942245
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
index cd8e208..ba1fbbd 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/Args.java
@@ -56,8 +56,9 @@
}
@Override
- public boolean isHashable() {
- return false; // even a frozen Args is not hashable
+ public void checkHashable() throws EvalException {
+ // Even a frozen Args is not hashable.
+ throw Starlark.errorf("unhashable type: '%s'", Starlark.type(this));
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java b/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java
index 3069e1a..0fe9715 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/InternalModule.java
@@ -46,9 +46,4 @@
public boolean isImmutable() {
return true;
}
-
- @Override
- public boolean isHashable() {
- return true;
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java
index d829fe9..0bfeb51 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkInfo.java
@@ -249,13 +249,9 @@
@Override
public boolean isImmutable() {
// If the provider is not yet exported, the hash code of the object is subject to change.
- // TODO(adonovan): implement isHashable?
if (!getProvider().isExported()) {
return false;
}
- // TODO(bazel-team): If we export at the end of a full module's evaluation, instead of at the
- // end of every top-level statement, then we can assume that exported implies frozen, and just
- // return true here without a traversal.
for (int i = table.length / 2; i < table.length; i++) {
if (!Starlark.isImmutable(table[i])) {
return false;
diff --git a/src/main/java/net/starlark/java/eval/Dict.java b/src/main/java/net/starlark/java/eval/Dict.java
index af75076..4d5c9c2 100644
--- a/src/main/java/net/starlark/java/eval/Dict.java
+++ b/src/main/java/net/starlark/java/eval/Dict.java
@@ -142,18 +142,19 @@
}
@Override
- public boolean isHashable() {
- return false; // even a frozen dict is unhashable
+ public void checkHashable() throws EvalException {
+ // Even a frozen dict is unhashable.
+ throw Starlark.errorf("unhashable type: 'dict'");
}
@Override
public int hashCode() {
- return contents.hashCode(); // not called by Dict.put (because !isHashable)
+ return contents.hashCode();
}
@Override
public boolean equals(Object o) {
- return contents.equals(o); // not called by Dict.put (because !isHashable)
+ return contents.equals(o);
}
@Override
@@ -419,7 +420,7 @@
*/
public void put(K key, V value, Location unused) throws EvalException {
Starlark.checkMutable(this);
- EvalUtils.checkHashable(key);
+ Starlark.checkHashable(key);
contents.put(key, value);
}
@@ -435,7 +436,7 @@
Starlark.checkMutable(this);
for (Map.Entry<KK, VV> e : map.entrySet()) {
KK k = e.getKey();
- EvalUtils.checkHashable(k);
+ Starlark.checkHashable(k);
contents.put(k, e.getValue());
}
}
@@ -530,7 +531,7 @@
@Override
public boolean containsKey(StarlarkSemantics semantics, Object key) throws EvalException {
- EvalUtils.checkHashable(key);
+ Starlark.checkHashable(key);
return this.containsKey(key);
}
diff --git a/src/main/java/net/starlark/java/eval/Eval.java b/src/main/java/net/starlark/java/eval/Eval.java
index 2b5779e..df67575 100644
--- a/src/main/java/net/starlark/java/eval/Eval.java
+++ b/src/main/java/net/starlark/java/eval/Eval.java
@@ -795,7 +795,7 @@
DictExpression.Entry body = (DictExpression.Entry) comp.getBody();
Object k = eval(fr, body.getKey());
try {
- EvalUtils.checkHashable(k);
+ Starlark.checkHashable(k);
Object v = eval(fr, body.getValue());
dict.put(k, v, (Location) null);
} catch (EvalException ex) {
diff --git a/src/main/java/net/starlark/java/eval/EvalUtils.java b/src/main/java/net/starlark/java/eval/EvalUtils.java
index 062548d..0dc2f10 100644
--- a/src/main/java/net/starlark/java/eval/EvalUtils.java
+++ b/src/main/java/net/starlark/java/eval/EvalUtils.java
@@ -95,43 +95,6 @@
}
};
- /** Throws EvalException if x is not hashable. */
- static void checkHashable(Object x) throws EvalException {
- if (!isHashable(x)) {
- // This results in confusing errors such as "unhashable type: tuple".
- // TODO(adonovan): ideally the error message would explain which
- // element of, say, a tuple is unhashable. The only practical way
- // to implement this is by implementing isHashable as a call to
- // Object.hashCode within a try/catch, and requiring all
- // unhashable Starlark values to throw a particular unchecked exception
- // with a helpful error message.
- throw Starlark.errorf("unhashable type: '%s'", Starlark.type(x));
- }
- }
-
- /**
- * Reports whether a legal Starlark value is considered hashable to Starlark, and thus suitable as
- * a key in a dict.
- */
- static boolean isHashable(Object o) {
- // Bazel makes widespread assumptions that all Starlark values can be hashed
- // by Java code, so we cannot implement isHashable by having
- // StarlarkValue.hashCode throw an unchecked exception, which would be more
- // efficient. Instead, before inserting a value in a dict, we must first ask
- // it whether it isHashable, and then call its hashCode method only if so.
- // For structs and tuples, this unfortunately visits the object graph twice.
- //
- // One subtlety: the struct.isHashable recursively asks whether its
- // elements are immutable, not hashable. Consequently, even though a list
- // may not be used as a dict key (even if frozen), a struct containing
- // a list is hashable. TODO(adonovan): fix this inconsistency.
- // Requires an incompatible change flag.
- if (o instanceof StarlarkValue) {
- return ((StarlarkValue) o).isHashable();
- }
- return Starlark.isImmutable(o);
- }
-
static void addIterator(Object x) {
if (x instanceof Mutability.Freezable) {
((Mutability.Freezable) x).updateIteratorCount(+1);
diff --git a/src/main/java/net/starlark/java/eval/Starlark.java b/src/main/java/net/starlark/java/eval/Starlark.java
index 1690f77..cc25353 100644
--- a/src/main/java/net/starlark/java/eval/Starlark.java
+++ b/src/main/java/net/starlark/java/eval/Starlark.java
@@ -141,6 +141,20 @@
}
/**
+ * Returns normally if the Starlark value is hashable and thus suitable as a dict key.
+ *
+ * @throws EvalException otherwise.
+ */
+ public static void checkHashable(Object x) throws EvalException {
+ if (x instanceof StarlarkValue) {
+ ((StarlarkValue) x).checkHashable();
+ } else {
+ Starlark.checkValid(x);
+ // String and Boolean are hashable.
+ }
+ }
+
+ /**
* Converts a Java value {@code x} to a Starlark one, if x is not already a valid Starlark value.
* An Integer, Long, or BigInteger is converted to a Starlark int, a Java List or Map is converted
* to a Starlark list or dict, respectively, and null becomes {@link #NONE}. Any other
diff --git a/src/main/java/net/starlark/java/eval/StarlarkList.java b/src/main/java/net/starlark/java/eval/StarlarkList.java
index 4fa3b88..c62ce38 100644
--- a/src/main/java/net/starlark/java/eval/StarlarkList.java
+++ b/src/main/java/net/starlark/java/eval/StarlarkList.java
@@ -79,8 +79,9 @@
}
@Override
- public boolean isHashable() {
- return false; // even a frozen list is unhashable in Starlark
+ public void checkHashable() throws EvalException {
+ // Even a frozen list is unhashable.
+ throw Starlark.errorf("unhashable type: 'list'");
}
@Override
diff --git a/src/main/java/net/starlark/java/eval/StarlarkValue.java b/src/main/java/net/starlark/java/eval/StarlarkValue.java
index 8241244..062ad10 100644
--- a/src/main/java/net/starlark/java/eval/StarlarkValue.java
+++ b/src/main/java/net/starlark/java/eval/StarlarkValue.java
@@ -72,8 +72,31 @@
return false;
}
- /** Reports whether the Starlark value is hashable and thus suitable as a dict key. */
- default boolean isHashable() {
- return this.isImmutable();
+ /**
+ * Returns normally if the Starlark value is hashable and thus suitable as a dict key.
+ *
+ * <p>(A StarlarkValue implementation may define hashCode and equals and thus be a valid
+ * java.util.Map key without being hashable by Starlark code.)
+ *
+ * @throws EvalException otherwise.
+ */
+ default void checkHashable() throws EvalException {
+ // Bazel makes widespread assumptions that all Starlark values can be hashed
+ // by Java code, so we cannot implement checkHashable by having
+ // StarlarkValue.hashCode throw an unchecked exception, which would be more
+ // efficient. Instead, before inserting a value in a dict, we must first check
+ // whether it is hashable by calling this function, and then call its hashCode
+ // method only if so.
+ // For structs and tuples, this unfortunately visits the object graph twice.
+ //
+ // One subtlety: Bazel's lib.packages.StarlarkInfo.checkHashable, by using this
+ // default implementation of checkHashable, which is based on isImmutable,
+ // recursively asks whether its elements are immutable, not hashable.
+ // Consequently, even though a list may not be used as a dict key (even if frozen),
+ // a struct containing a list is hashable.
+ // TODO(adonovan): fix this inconsistency. Requires a Bazel incompatible change.
+ if (!this.isImmutable()) {
+ throw Starlark.errorf("unhashable type: '%s'", Starlark.type(this));
+ }
}
}
diff --git a/src/main/java/net/starlark/java/eval/Tuple.java b/src/main/java/net/starlark/java/eval/Tuple.java
index 83b7f3b..ea3ea29 100644
--- a/src/main/java/net/starlark/java/eval/Tuple.java
+++ b/src/main/java/net/starlark/java/eval/Tuple.java
@@ -115,13 +115,10 @@
}
@Override
- public boolean isHashable() {
+ public void checkHashable() throws EvalException {
for (Object x : elems) {
- if (!EvalUtils.isHashable(x)) {
- return false;
- }
+ Starlark.checkHashable(x);
}
- return true;
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
index ac0aa23..7d1c58e 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
@@ -3241,7 +3241,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:r");
- assertContainsEvent("unhashable type: 'tuple'");
+ assertContainsEvent("unhashable type: 'dict'");
}
@Test
diff --git a/src/test/java/net/starlark/java/eval/testdata/dict.sky b/src/test/java/net/starlark/java/eval/testdata/dict.sky
index 8195d97..c34bdef 100644
--- a/src/test/java/net/starlark/java/eval/testdata/dict.sky
+++ b/src/test/java/net/starlark/java/eval/testdata/dict.sky
@@ -108,3 +108,7 @@
dict().get({}) ### unhashable type: 'dict'
---
{1: 2}.get(len) ### unhashable type: 'function'
+---
+# For composite keys, the error message relates to the
+# unhashable subelement of the key, not the key itself.
+{(0, "", True, [0]): None} ### unhashable type: 'list'
