Introduce --incompatible_depset_is_not_iterable
RELNOTES:
Iterating on a `depset` object is deprecated. If you need an iterable,
call the `.to_list()` method first.
PiperOrigin-RevId: 159672887
diff --git a/site/docs/skylark/concepts.md b/site/docs/skylark/concepts.md
index 7c68d74..0ce1c91 100644
--- a/site/docs/skylark/concepts.md
+++ b/site/docs/skylark/concepts.md
@@ -234,6 +234,26 @@
* Default: `false`
+### Depset is no longer iterable
+
+When the flag is set to true, `depset` objects are not treated as iterable. If
+you need an iterable, call the `.to_list()` method. This affects `for` loops and
+many functions, e.g. `list`, `tuple`, `min`, `max`, `sorted`, `all`, `any`. The
+goal of this change is to avoid accidental iteration on `depset`, which can be
+expensive.
+
+``` python
+deps = depset()
+[x.path for x in deps] # deprecated
+[x.path for x in deps.to_list()] # recommended
+
+sorted(deps) # deprecated
+sorted(deps.to_list()) # recommended
+```
+
+* Flag: `--incompatible_depset_is_not_iterable`
+* Default: `false`
+
## Upcoming changes
The following items are upcoming changes.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java
index a32c6bc..84adc69 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkFileType.java
@@ -24,7 +24,6 @@
import com.google.devtools.build.lib.util.FileType;
import com.google.devtools.build.lib.util.FileTypeSet;
-
/** A wrapper class for FileType and FileTypeSet functionality in Skylark. */
@SkylarkModule(
name = "FileType",
@@ -62,8 +61,7 @@
public ImmutableList<Artifact> filter(Object filesUnchecked) throws EvalException {
return ImmutableList.copyOf(
FileType.filter(
- (Iterable<Artifact>) EvalUtils.toIterableStrict(filesUnchecked, null),
- fileType));
+ (Iterable<Artifact>) EvalUtils.toIterableStrict(filesUnchecked, null, null), fileType));
}
@VisibleForTesting
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
index a25a460..5908834 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleImplementationFunctions.java
@@ -669,7 +669,6 @@
}
};
-
/**
* Ensures the given {@link Map} has keys that have {@link Label} type and values that have either
* {@link Iterable} or {@link SkylarkNestedSet} type, and raises {@link EvalException} otherwise.
@@ -678,8 +677,7 @@
// TODO(bazel-team): find a better way to typecheck this argument.
@SuppressWarnings("unchecked")
private static Map<Label, Iterable<Artifact>> checkLabelDict(
- Map<?, ?> labelDict, Location loc)
- throws EvalException {
+ Map<?, ?> labelDict, Location loc, Environment env) throws EvalException {
Map<Label, Iterable<Artifact>> convertedMap = new HashMap<>();
for (Map.Entry<?, ?> entry : labelDict.entrySet()) {
Object key = entry.getKey();
@@ -691,7 +689,7 @@
Object val = entry.getValue();
Iterable<?> valIter;
try {
- valIter = EvalUtils.toIterableStrict(val, loc);
+ valIter = EvalUtils.toIterableStrict(val, loc, env);
} catch (EvalException ex) {
// EvalException is thrown only if the type is wrong.
throw new EvalException(
@@ -813,7 +811,7 @@
throws ConversionException, EvalException {
ctx.checkMutable("resolve_command");
Label ruleLabel = ctx.getLabel();
- Map<Label, Iterable<Artifact>> labelDict = checkLabelDict(labelDictUnchecked, loc);
+ Map<Label, Iterable<Artifact>> labelDict = checkLabelDict(labelDictUnchecked, loc, env);
// The best way to fix this probably is to convert CommandHelper to Skylark.
CommandHelper helper =
new CommandHelper(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
index 3ec6cd5..deb939e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
@@ -113,7 +113,7 @@
throws EvalException, InterruptedException {
Object listValueObject = list.eval(env);
Location loc = collector.getLocation();
- Iterable<?> listValue = EvalUtils.toIterable(listValueObject, loc);
+ Iterable<?> listValue = EvalUtils.toIterable(listValueObject, loc, env);
EvalUtils.lock(listValueObject, loc);
try {
for (Object listElement : listValue) {
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 34ab3f3..1e3b134 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
@@ -29,6 +29,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
+import javax.annotation.Nullable;
/**
* Utilities used by the evaluator.
@@ -300,7 +301,8 @@
}
}
- public static Collection<?> toCollection(Object o, Location loc) throws EvalException {
+ public static Collection<?> toCollection(Object o, Location loc, @Nullable Environment env)
+ throws EvalException {
if (o instanceof Collection) {
return (Collection<?>) o;
} else if (o instanceof SkylarkList) {
@@ -323,25 +325,36 @@
throw new EvalException(loc, e);
}
} else if (o instanceof SkylarkNestedSet) {
- return ((SkylarkNestedSet) o).toCollection();
+ return nestedSetToCollection((SkylarkNestedSet) o, loc, env);
} else {
throw new EvalException(loc,
"type '" + getDataTypeName(o) + "' is not a collection");
}
}
- public static Iterable<?> toIterable(Object o, Location loc) throws EvalException {
+ private static Collection<?> nestedSetToCollection(
+ SkylarkNestedSet set, Location loc, @Nullable Environment env) throws EvalException {
+ if (env != null && env.getSemantics().incompatibleDepsetIsNotIterable) {
+ throw new EvalException(
+ loc,
+ "type 'depset' is not iterable. Use the `to_list()` method to get a list. "
+ + "Use --incompatible_depset_is_not_iterable to temporarily disable this check.");
+ }
+ return set.toCollection();
+ }
+
+ public static Iterable<?> toIterable(Object o, Location loc, @Nullable Environment env)
+ throws EvalException {
if (o instanceof String) {
// 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();
+ return nestedSetToCollection((SkylarkNestedSet) o, loc, env);
} else if (o instanceof Iterable) {
return (Iterable<?>) o;
} else if (o instanceof Map) {
- return toCollection(o, loc);
+ return toCollection(o, loc, env);
} else {
throw new EvalException(loc,
"type '" + getDataTypeName(o) + "' is not iterable");
@@ -358,18 +371,17 @@
*
* @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.
+ * 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 {
+ public static Iterable<?> toIterableStrict(Object o, Location loc, @Nullable Environment env)
+ 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();
+ return nestedSetToCollection((SkylarkNestedSet) o, loc, env);
} else {
throw new EvalException(loc,
"expected Iterable or depset, but got '" + getDataTypeName(o) + "' (strings and maps "
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index 34d0c30..a43d289 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -61,7 +61,7 @@
@Override
void doExec(Environment env) throws EvalException, InterruptedException {
Object o = collection.eval(env);
- Iterable<?> col = EvalUtils.toIterable(o, getLocation());
+ Iterable<?> col = EvalUtils.toIterable(o, getLocation(), env);
EvalUtils.lock(o, getLocation());
try {
for (Object it : col) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index 52753cc..72c9ac2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -115,7 +115,7 @@
if (lhs instanceof ListLiteral) {
ListLiteral variables = (ListLiteral) lhs;
- Collection<?> rvalue = EvalUtils.toCollection(result, loc);
+ Collection<?> rvalue = EvalUtils.toCollection(result, loc, env);
int len = variables.getElements().size();
if (len != rvalue.size()) {
throw new EvalException(loc, String.format(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index d84fbc4..afa4afd 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -949,13 +949,18 @@
defaultValue = "{}",
doc = "Dictionary of arguments."
),
- useLocation = true
+ useLocation = true,
+ useEnvironment = true
)
private static final BuiltinFunction format =
new BuiltinFunction("format") {
@SuppressWarnings("unused")
public String invoke(
- String self, SkylarkList<Object> args, SkylarkDict<?, ?> kwargs, Location loc)
+ String self,
+ SkylarkList<Object> args,
+ SkylarkDict<?, ?> kwargs,
+ Location loc,
+ Environment env)
throws EvalException {
return new FormatParser(loc)
.format(
@@ -992,14 +997,16 @@
+ "If only one argument is provided, it must be a non-empty iterable.",
extraPositionals =
@Param(name = "args", type = SkylarkList.class, doc = "The elements to be checked."),
- useLocation = true
+ useLocation = true,
+ useEnvironment = true
)
private static final BuiltinFunction min =
new BuiltinFunction("min") {
@SuppressWarnings("unused") // Accessed via Reflection.
- public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
+ public Object invoke(SkylarkList<?> args, Location loc, Environment env)
+ throws EvalException {
try {
- return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc);
+ return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc, env);
} catch (ComparisonException e) {
throw new EvalException(loc, e);
}
@@ -1014,29 +1021,30 @@
+ "If only one argument is provided, it must be a non-empty iterable.",
extraPositionals =
@Param(name = "args", type = SkylarkList.class, doc = "The elements to be checked."),
- useLocation = true
+ useLocation = true,
+ useEnvironment = true
)
private static final BuiltinFunction max =
new BuiltinFunction("max") {
@SuppressWarnings("unused") // Accessed via Reflection.
- public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
+ public Object invoke(SkylarkList<?> args, Location loc, Environment env)
+ throws EvalException {
try {
- return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc);
+ return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc, env);
} catch (ComparisonException e) {
throw new EvalException(loc, e);
}
}
};
- /**
- * Returns the maximum element from this list, as determined by maxOrdering.
- */
- private static Object findExtreme(SkylarkList<?> args, Ordering<Object> maxOrdering, Location loc)
+ /** Returns the maximum element from this list, as determined by maxOrdering. */
+ private static Object findExtreme(
+ SkylarkList<?> args, Ordering<Object> maxOrdering, Location loc, Environment env)
throws EvalException {
// Args can either be a list of items to compare, or a singleton list whose element is an
// iterable of items to compare. In either case, there must be at least one item to compare.
try {
- Iterable<?> items = (args.size() == 1) ? EvalUtils.toIterable(args.get(0), loc) : args;
+ Iterable<?> items = (args.size() == 1) ? EvalUtils.toIterable(args.get(0), loc, env) : args;
return maxOrdering.max(items);
} catch (NoSuchElementException ex) {
throw new EvalException(loc, "expected at least one item");
@@ -1050,13 +1058,15 @@
parameters = {
@Param(name = "elements", type = Object.class, doc = "A string or a collection of elements.")
},
- useLocation = true
+ useLocation = true,
+ useEnvironment = true
)
private static final BuiltinFunction all =
new BuiltinFunction("all") {
@SuppressWarnings("unused") // Accessed via Reflection.
- public Boolean invoke(Object collection, Location loc) throws EvalException {
- return !hasElementWithBooleanValue(collection, false, loc);
+ public Boolean invoke(Object collection, Location loc, Environment env)
+ throws EvalException {
+ return !hasElementWithBooleanValue(collection, false, loc, env);
}
};
@@ -1067,19 +1077,21 @@
parameters = {
@Param(name = "elements", type = Object.class, doc = "A string or a collection of elements.")
},
- useLocation = true
+ useLocation = true,
+ useEnvironment = true
)
private static final BuiltinFunction any =
new BuiltinFunction("any") {
@SuppressWarnings("unused") // Accessed via Reflection.
- public Boolean invoke(Object collection, Location loc) throws EvalException {
- return hasElementWithBooleanValue(collection, true, loc);
+ public Boolean invoke(Object collection, Location loc, Environment env)
+ throws EvalException {
+ return hasElementWithBooleanValue(collection, true, loc, env);
}
};
- private static boolean hasElementWithBooleanValue(Object collection, boolean value, Location loc)
- throws EvalException {
- Iterable<?> iterable = EvalUtils.toIterable(collection, loc);
+ private static boolean hasElementWithBooleanValue(
+ Object collection, boolean value, Location loc, Environment env) throws EvalException {
+ Iterable<?> iterable = EvalUtils.toIterable(collection, loc, env);
for (Object obj : iterable) {
if (EvalUtils.toBoolean(obj) == value) {
return true;
@@ -1105,7 +1117,8 @@
throws EvalException {
try {
return new MutableList(
- EvalUtils.SKYLARK_COMPARATOR.sortedCopy(EvalUtils.toCollection(self, loc)), env);
+ EvalUtils.SKYLARK_COMPARATOR.sortedCopy(EvalUtils.toCollection(self, loc, env)),
+ env);
} catch (EvalUtils.ComparisonException e) {
throw new EvalException(loc, e);
}
@@ -1140,7 +1153,7 @@
loc, "Argument to reversed() must be a sequence, not a depset.");
}
LinkedList<Object> tmpList = new LinkedList<>();
- for (Object element : EvalUtils.toIterable(sequence, loc)) {
+ for (Object element : EvalUtils.toIterable(sequence, loc, env)) {
tmpList.addFirst(element);
}
return new MutableList(tmpList, env);
@@ -1560,12 +1573,13 @@
+ "tuple((2, 3, 2)) == (2, 3, 2)\n"
+ "tuple({5: \"a\", 2: \"b\", 4: \"c\"}) == (5, 2, 4)</pre>",
parameters = {@Param(name = "x", doc = "The object to convert.")},
- useLocation = true
+ useLocation = true,
+ useEnvironment = true
)
private static final BuiltinFunction tuple =
new BuiltinFunction("tuple") {
- public Tuple<?> invoke(Object x, Location loc) throws EvalException {
- return Tuple.create(ImmutableList.copyOf(EvalUtils.toCollection(x, loc)));
+ public Tuple<?> invoke(Object x, Location loc, Environment env) throws EvalException {
+ return Tuple.create(ImmutableList.copyOf(EvalUtils.toCollection(x, loc, env)));
}
};
@@ -1584,7 +1598,7 @@
private static final BuiltinFunction list =
new BuiltinFunction("list") {
public MutableList<?> invoke(Object x, Location loc, Environment env) throws EvalException {
- return new MutableList(EvalUtils.toCollection(x, loc), env);
+ return new MutableList(EvalUtils.toCollection(x, loc, env), env);
}
};
@@ -2114,7 +2128,7 @@
throws EvalException {
Iterator<?>[] iterators = new Iterator<?>[args.size()];
for (int i = 0; i < args.size(); i++) {
- iterators[i] = EvalUtils.toIterable(args.get(i), loc).iterator();
+ iterators[i] = EvalUtils.toIterable(args.get(i), loc, env).iterator();
}
List<Tuple<?>> result = new ArrayList<>();
boolean allHasNext;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
index 9f92cf9..4cca541 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemanticsOptions.java
@@ -118,4 +118,15 @@
+ " comprehension executes."
)
public boolean incompatibleComprehensionVariablesDoNotLeak;
+
+ @Option(
+ name = "incompatible_depset_is_not_iterable",
+ defaultValue = "false",
+ category = "incompatible changes",
+ help =
+ "If set to true, depset type is not iterable. For loops and functions expecting an "
+ + "iterable will reject depset objects. Use the `.to_list` method to explicitly "
+ + "convert to a list."
+ )
+ public boolean incompatibleDepsetIsNotIterable;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Type.java b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
index c83a72b..da2750d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Type.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Type.java
@@ -582,7 +582,7 @@
throws ConversionException {
Iterable<?> iterable;
try {
- iterable = EvalUtils.toIterableStrict(x, null);
+ iterable = EvalUtils.toIterableStrict(x, null, null);
} catch (EvalException ex) {
throw new ConversionException(this, x, what);
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
index edf1edf..b7e6f04 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
@@ -47,12 +47,12 @@
@Test
public void testEmptyStringToIterable() throws Exception {
- assertThat(EvalUtils.toIterable("", null)).isEmpty();
+ assertThat(EvalUtils.toIterable("", null, null)).isEmpty();
}
@Test
public void testStringToIterable() throws Exception {
- assertThat(EvalUtils.toIterable("abc", null)).hasSize(3);
+ assertThat(EvalUtils.toIterable("abc", null, null)).hasSize(3);
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index ba6fd69..a8248ea 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -880,6 +880,26 @@
}
@Test
+ public void testSetIsNotIterable() throws Exception {
+ new SkylarkTest("--incompatible_depset_is_not_iterable=true")
+ .testIfErrorContains("not iterable", "list(depset(['a', 'b']))")
+ .testIfErrorContains("not iterable", "max(depset([1, 2, 3]))")
+ .testIfErrorContains("not iterable", "sorted(depset(['a', 'b']))")
+ .testIfErrorContains("not iterable", "tuple(depset(['a', 'b']))")
+ .testIfErrorContains("not iterable", "[x for x in depset()]");
+ }
+
+ @Test
+ public void testSetIsIterable() throws Exception {
+ new SkylarkTest("--incompatible_depset_is_not_iterable=false")
+ .testStatement("str(list(depset(['a', 'b'])))", "[\"a\", \"b\"]")
+ .testStatement("max(depset([1, 2, 3]))", 3)
+ .testStatement("str(sorted(depset(['b', 'a'])))", "[\"a\", \"b\"]")
+ .testStatement("str(tuple(depset(['a', 'b'])))", "(\"a\", \"b\")")
+ .testStatement("str([x for x in depset()])", "[]");
+ }
+
+ @Test
public void testClassObjectCannotAccessNestedSet() throws Exception {
new SkylarkTest()
.update("mock", new MockClassObject())
