bazel syntax: relax depset element validity check to isImmutable
The nightly uncovered cases of depsets containing lists of strings,
which are Starlark-unhashable even when frozen.
Also document the two problems related to the check.
PiperOrigin-RevId: 282377809
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Depset.java b/src/main/java/com/google/devtools/build/lib/syntax/Depset.java
index 30f6251..a796816 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Depset.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Depset.java
@@ -128,7 +128,7 @@
}
} else if (item instanceof Sequence) {
for (Object x : (Sequence) item) {
- EvalUtils.checkHashable(x);
+ checkElement(x);
SkylarkType xt = SkylarkType.of(x);
contentType = checkType(contentType, xt);
itemsBuilder.add(x);
@@ -155,6 +155,30 @@
return new Depset(contentType, builder.build(), items, transitiveItems);
}
+ private static void checkElement(Object x) throws EvalException {
+ // Historically the requirement for a depset element was isImmutable(x).
+ // However, this check is neither necessary not sufficient.
+ // It is unnecessary because elements need only be hashable,
+ // as with dicts, whose keys may be mutable so long as mutations
+ // don't affect the hash code. (Elements of a NestedSet must be
+ // hashable because a hash-based set is used to de-duplicate
+ // elements during iteration.)
+ // And it is insufficient because some values are immutable
+ // but not Starlark-hashable, such as frozen lists.
+ // NestedSet calls its hashCode method regardless.
+ //
+ // TODO(adonovan): use this check instead:
+ // EvalUtils.checkHashable(x);
+ // and delete the StarlarkValue.isImmutable and EvalUtils.isImmutable.
+ // Unfortunately this is a breaking change because some users
+ // construct depsets whose elements contain lists of strings,
+ // which are Starlark-unhashable even if frozen.
+ // TODO(adonovan): also remove StarlarkList.hashCode.
+ if (!EvalUtils.isImmutable(x)) {
+ throw new EvalException(null, "depset elements must not be mutable values");
+ }
+ }
+
// implementation of deprecated depset(x) constructor
static Depset legacyOf(Order order, Object items) throws EvalException {
// TODO(adonovan): rethink this API. TOP is a pessimistic type for item, and it's wrong
@@ -438,6 +462,8 @@
* <p>Use this to construct typesafe Skylark nested sets (depsets). Encapsulates content type
* checking logic.
*/
+ // TODO(adonovan): make this private. The sole external user in rules/cpp could
+ // instead call Depset.of(NestedSet.wrap(Order, elems)).
public static final class Builder {
private final Order order;
@@ -452,8 +478,17 @@
/** Adds a direct element, checking that its type is equal to the elements already added. */
public Builder addDirect(Object x) throws EvalException {
- // In case of problems, see b/144992997 or github.com/bazelbuild/bazel/issues/10289.
- EvalUtils.checkHashable(x);
+ // Historically, checkElement was called only by some depset constructors,
+ // but not this one, depset(direct=[x]).
+ // This was a regrettable oversight that allowed users to put
+ // mutable values into depsets, doubly so because we have just forced our
+ // users to migrate away from the legacy constructor which applied the check.
+ // We are currently discovering and fixing existing violations, for example
+ // marking the relevant Starlark types as immutable where appropriate
+ // (e.g. ConfiguredTarget), but if violations are too numerous we may need
+ // to suppress the checkElement call below and reintroduce it as a breaking change.
+ // See b/144992997 or github.com/bazelbuild/bazel/issues/10289.
+ checkElement(x);
SkylarkType xt = SkylarkType.of(x);
this.contentType = checkType(contentType, xt);
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 1043d7e3..15ce13a 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
@@ -113,6 +113,13 @@
/** 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 new EvalException(
null, Starlark.format("unhashable type: '%s'", EvalUtils.getDataTypeName(x)));
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 9d650c2..5a71e61 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -1265,12 +1265,6 @@
assertThat(result.iterator().next()).isInstanceOf(StructImpl.class);
}
- @Test
- public void testNsetBadMutableItem() throws Exception {
- checkEvalErrorContains("unhashable type: 'tuple'", "depset([([],)])");
- checkEvalErrorContains("unhashable type: 'struct'", "depset([struct(a=[])])");
- }
-
private static StructImpl makeStruct(String field, Object value) {
return StructProvider.STRUCT.create(ImmutableMap.of(field, value), "no field '%'");
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index 340745f..5f2bb99 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -962,7 +962,8 @@
@Test
public void testNsetContainsList() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
- checkEvalErrorContains("unhashable type: 'list'", "depset([[ruleContext.files.srcs]])");
+ checkEvalErrorContains(
+ "depset elements must not be mutable values", "depset([[ruleContext.files.srcs]])");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
index b8ada81..0530589 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
@@ -579,6 +579,24 @@
}
@Test
+ public void testElementsMustBeImmutable() throws Exception {
+ // Test legacy and new constructors.
+
+ // mutable list: error
+ checkEvalError("depset elements must not be mutable values", "depset([[1,2,3]])");
+ checkEvalError("depset elements must not be mutable values", "depset(direct=[[1,2,3]])");
+
+ // struct containing mutable list: error
+ checkEvalError("depset elements must not be mutable values", "depset([struct(a=[])])");
+ checkEvalError("depset elements must not be mutable values", "depset(direct=[struct(a=[])])");
+
+ // frozen list: no error
+ update("x", StarlarkList.empty());
+ eval("depset([x])");
+ eval("depset(direct=[x])");
+ }
+
+ @Test
public void testDepthExceedsLimitDuringIteration() throws Exception {
NestedSet.setApplicationDepthLimit(2000);
new SkylarkTest()
