Refactor SkylarkNestedSet to not implement Iterable
This is not intended to be a user-visible semantic change, aside from error messages.
This is to help avoid unintentional flattening of depsets, and to narrow down the number of call sites where this can occur, to help us print warning/deprecation messages.
EvalUtils#toIterable will now return an ImmutableList in place of SkylarkNestedSet. This should be ok since the caller shouldn't be relying on the result being a Skylark-safe type. Code that takes Iterable because it accepts either a list or set, can instead be changed to take Object and use EvalUtils#toIterableStrict for validation.
Note that NestedSet still implements Iterable, so native code can still easily and accidentally flatten sets.
--
PiperOrigin-RevId: 145044023
MOS_MIGRATED_REVID=145044023
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 83d2b9f..b3d7bc0 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
@@ -353,6 +353,9 @@
// This is not as efficient as special casing String in for and dict and list comprehension
// statements. However this is a more unified way.
return split((String) o);
+ } else if (o instanceof SkylarkNestedSet) {
+ // TODO(bazel-team): Add a deprecation warning: don't implicitly flatten depsets.
+ return ((SkylarkNestedSet) o).toCollection();
} else if (o instanceof Iterable) {
return (Iterable<?>) o;
} else if (o instanceof Map) {
@@ -363,6 +366,35 @@
}
}
+ /**
+ * Given an {@link Iterable}, returns it as-is. Given a {@link SkylarkNestedSet}, returns its
+ * contents as an iterable. Throws {@link EvalException} for any other value.
+ *
+ * <p>This is a kludge for the change that made {@code SkylarkNestedSet} not implement {@code
+ * Iterable}. It is different from {@link #toIterable} in its behavior for strings and other types
+ * that are not strictly Java-iterable.
+ *
+ * @throws EvalException if {@code o} is not an iterable or set
+ * @deprecated avoid writing APIs that implicitly treat depsets as iterables. It encourages
+ * unnecessary flattening of depsets.
+ *
+ * <p>TODO(bazel-team): Remove this if/when implicit iteration over {@code SkylarkNestedSet} is no
+ * longer supported.
+ */
+ @Deprecated
+ public static Iterable<?> toIterableStrict(Object o, Location loc) throws EvalException {
+ if (o instanceof Iterable) {
+ return (Iterable<?>) o;
+ } else if (o instanceof SkylarkNestedSet) {
+ // TODO(bazel-team): Add a deprecation warning: don't implicitly flatten depsets.
+ return ((SkylarkNestedSet) o).toCollection();
+ } else {
+ throw new EvalException(loc,
+ "expected Iterable or depset, but got '" + getDataTypeName(o) + "' (strings and maps "
+ + "are not allowed here)");
+ }
+ }
+
public static void lock(Object object, Location loc) {
if (object instanceof SkylarkMutable) {
((SkylarkMutable) object).lock(loc);
@@ -392,7 +424,10 @@
} else if (arg instanceof Map) {
return ((Map<?, ?>) arg).size();
} else if (arg instanceof SkylarkList) {
- return ((SkylarkList) arg).size();
+ return ((SkylarkList<?>) arg).size();
+ } else if (arg instanceof SkylarkNestedSet) {
+ // TODO(bazel-team): Add a deprecation warning: don't implicitly flatten depsets.
+ return ((SkylarkNestedSet) arg).toCollection().size();
} else if (arg instanceof Iterable) {
// Iterables.size() checks if arg is a Collection so it's efficient in that sense.
return Iterables.size((Iterable<?>) arg);
