bazel syntax: rationalize Depset constructors
This change renames the various functions formerly called 'of'.
Overloading is fine for two methods that apply same logic to different
representations of their input (e.g. int vs long, CharSequence vs
String), but it is an abuse to use it for different algorithms.
Also, remove unnecessary Location parameters. (Locations are not
needed for EvalExceptions as they are filled in by the call
machinery.)
PiperOrigin-RevId: 282004067
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
index 5beb8fa..ac958a3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
@@ -668,7 +668,7 @@
protected Object skylarkListToDepset(Object o) throws EvalException {
if (o instanceof Sequence) {
Sequence<String> list = (Sequence<String>) o;
- Depset.Builder builder = Depset.builder(Order.STABLE_ORDER, Location.BUILTIN);
+ Depset.Builder builder = Depset.builder(Order.STABLE_ORDER);
for (Object entry : list) {
builder.addDirect(entry);
}
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 ae7b72e..bc655b9 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
@@ -20,7 +20,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
@@ -107,8 +106,8 @@
this.transitiveItems = transitiveItems;
}
- static Depset of(
- Order order, SkylarkType contentType, Object item, Location loc, @Nullable Depset left)
+ private static Depset create(
+ Order order, SkylarkType contentType, Object item, @Nullable Depset left)
throws EvalException {
ImmutableList.Builder<Object> itemsBuilder = ImmutableList.builder();
ImmutableList.Builder<NestedSet<?>> transitiveItemsBuilder = ImmutableList.builder();
@@ -124,19 +123,19 @@
if (item instanceof Depset) {
Depset nestedSet = (Depset) item;
if (!nestedSet.isEmpty()) {
- contentType = checkType(contentType, nestedSet.contentType, loc);
+ contentType = checkType(contentType, nestedSet.contentType);
transitiveItemsBuilder.add(nestedSet.set);
}
} else if (item instanceof Sequence) {
for (Object x : (Sequence) item) {
EvalUtils.checkValidDictKey(x);
SkylarkType xt = SkylarkType.of(x);
- contentType = checkType(contentType, xt, loc);
+ contentType = checkType(contentType, xt);
itemsBuilder.add(x);
}
} else {
throw new EvalException(
- loc,
+ null,
String.format(
"cannot union value of type '%s' to a depset", EvalUtils.getDataTypeName(item)));
}
@@ -151,23 +150,25 @@
}
} catch (IllegalArgumentException e) {
// Order mismatch between item and builder.
- throw new EvalException(loc, e.getMessage());
+ throw new EvalException(null, e.getMessage());
}
return new Depset(contentType, builder.build(), items, transitiveItems);
}
- static Depset of(Order order, Object item, Location loc) throws EvalException {
+ // 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
- // (should be BOTTOM) if item is an empty Depset or Sequence.
- return of(order, SkylarkType.TOP, item, loc, null);
+ // (should be BOTTOM) if items is an empty Depset or Sequence.
+ return create(order, SkylarkType.TOP, items, null);
}
- static Depset of(Depset left, Object right, Location loc) throws EvalException {
- return of(left.set.getOrder(), left.contentType, right, loc, left);
+ // implementation of deprecated depset+x, depset.union(x), depset|x
+ static Depset unionOf(Depset left, Object right) throws EvalException {
+ return create(left.set.getOrder(), left.contentType, right, left);
}
/**
- * Returns a type-safe Depset. Use this instead of the constructor if possible.
+ * Returns a Depset that wraps the specified NestedSet.
*
* <p>This operation is type-safe only if the specified element type is appropriate for every
* element of the set.
@@ -182,20 +183,20 @@
}
/**
- * Returns a type safe Depset. Use this instead of the constructor if possible.
+ * Returns a Depset that wraps the specified NestedSet.
*
* <p>This operation is type-safe only if the specified element type is appropriate for every
* element of the set.
*/
public static <T> Depset of(Class<T> contentType, NestedSet<T> set) {
- return of(SkylarkType.of(contentType), set);
+ return new Depset(SkylarkType.of(contentType), set, null, null);
}
/**
* Checks that an item type is allowed in a given set type, and returns the type of a new depset
* with that item inserted.
*/
- private static SkylarkType checkType(SkylarkType depsetType, SkylarkType itemType, Location loc)
+ private static SkylarkType checkType(SkylarkType depsetType, SkylarkType itemType)
throws EvalException {
// An initially empty depset takes its type from the first element added.
// Otherwise, the types of the item and depset must match exactly.
@@ -211,7 +212,7 @@
return itemType;
}
throw new EvalException(
- loc,
+ null,
String.format("cannot add an item of type '%s' to a depset of '%s'", itemType, depsetType));
}
@@ -385,13 +386,11 @@
parameters = {
@Param(name = "new_elements", type = Object.class, doc = "The elements to be added.")
},
- useLocation = true,
useStarlarkThread = true)
- public Depset union(Object newElements, Location loc, StarlarkThread thread)
- throws EvalException {
+ public Depset union(Object newElements, StarlarkThread thread) throws EvalException {
if (thread.getSemantics().incompatibleDepsetUnion()) {
throw new EvalException(
- loc,
+ null,
"depset method `.union` has been removed. See "
+ "https://docs.bazel.build/versions/master/skylark/depsets.html for "
+ "recommendations. Use --incompatible_depset_union=false "
@@ -400,7 +399,7 @@
// newElements' type is Object because of the polymorphism on unioning two
// Depsets versus a set and another kind of iterable.
// Can't use EvalUtils#toIterable since that would discard this information.
- return Depset.of(this, newElements, loc);
+ return Depset.unionOf(this, newElements);
}
@SkylarkCallable(
@@ -412,15 +411,13 @@
+ "</code>-ordered depsets, and for elements of child depsets whose order differs "
+ "from that of the parent depset. The list is a copy; modifying it has no effect "
+ "on the depset and vice versa.",
- useStarlarkThread = true,
- useLocation = true)
- public StarlarkList<Object> toList(Location location, StarlarkThread thread)
- throws EvalException {
+ useStarlarkThread = true)
+ public StarlarkList<Object> toList(StarlarkThread thread) throws EvalException {
try {
return StarlarkList.copyOf(thread.mutability(), this.toCollection());
} catch (NestedSetDepthException exception) {
throw new EvalException(
- location,
+ null,
"depset exceeded maximum depth "
+ exception.getDepthLimit()
+ ". This was only discovered when attempting to flatten the depset for to_list(), "
@@ -430,13 +427,9 @@
}
}
- /**
- * Create a {@link Builder} with specified order.
- *
- * <p>The {@code Builder} will use {@code location} to report errors.
- */
- public static Builder builder(Order order, Location location) {
- return new Builder(order, location);
+ /** Create a {@link Builder} with specified order. */
+ public static Builder builder(Order order) {
+ return new Builder(order);
}
/**
@@ -449,14 +442,11 @@
private final Order order;
private final NestedSetBuilder<Object> builder;
- /** Location for error messages */
- private final Location location;
private SkylarkType contentType = SkylarkType.TOP;
- private Builder(Order order, Location location) {
+ private Builder(Order order) {
this.order = order;
- this.location = location;
this.builder = new NestedSetBuilder<>(order);
}
@@ -466,7 +456,7 @@
// Investigate how/why user code is exploiting the weak check.
// EvalUtils.checkValidDictKey(x);
SkylarkType xt = SkylarkType.of(x);
- this.contentType = checkType(contentType, xt, location);
+ this.contentType = checkType(contentType, xt);
builder.add(x);
return this;
}
@@ -477,12 +467,14 @@
return this;
}
- this.contentType = checkType(contentType, transitive.getContentType(), location);
+ this.contentType = checkType(contentType, transitive.getContentType());
if (!order.isCompatible(transitive.getOrder())) {
- throw new EvalException(location,
- String.format("Order '%s' is incompatible with order '%s'",
- order.getSkylarkName(), transitive.getOrder().getSkylarkName()));
+ throw new EvalException(
+ null,
+ String.format(
+ "Order '%s' is incompatible with order '%s'",
+ order.getSkylarkName(), transitive.getOrder().getSkylarkName()));
}
builder.addTransitive(transitive.getSet());
return this;
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 1ac7a6d..d02ed95 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
@@ -702,7 +702,7 @@
+ "recommendations. Use --incompatible_depset_union=false "
+ "to temporarily disable this check.");
}
- return Depset.of((Depset) x, y, location);
+ return Depset.unionOf((Depset) x, y);
}
throw unknownBinaryOperator(x, y, TokenKind.PLUS, location);
}
@@ -721,7 +721,7 @@
+ "recommendations. Use --incompatible_depset_union=false "
+ "to temporarily disable this check.");
}
- return Depset.of((Depset) x, y, location);
+ return Depset.unionOf((Depset) x, y);
}
throw unknownBinaryOperator(x, y, TokenKind.PIPE, location);
}
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 2a339fd..47d3682 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
@@ -999,7 +999,6 @@
valueWhenDisabled = "[]",
named = true),
},
- useLocation = true,
useStarlarkSemantics = true)
public Depset depset(
Object x,
@@ -1007,7 +1006,6 @@
Object direct,
Object transitive,
Object items,
- Location loc,
StarlarkSemantics semantics)
throws EvalException {
Order order;
@@ -1015,27 +1013,27 @@
try {
order = Order.parse(orderString);
} catch (IllegalArgumentException ex) {
- throw new EvalException(loc, ex);
+ throw new EvalException(null, ex);
}
if (semantics.incompatibleDisableDepsetItems()) {
if (x != Starlark.NONE) {
if (direct != Starlark.NONE) {
throw new EvalException(
- loc, "parameter 'direct' cannot be specified both positionally and by keyword");
+ null, "parameter 'direct' cannot be specified both positionally and by keyword");
}
direct = x;
}
- result = depsetConstructor(direct, order, transitive, loc);
+ result = depsetConstructor(direct, order, transitive);
} else {
if (x != Starlark.NONE) {
if (!isEmptySkylarkList(items)) {
throw new EvalException(
- loc, "parameter 'items' cannot be specified both positionally and by keyword");
+ null, "parameter 'items' cannot be specified both positionally and by keyword");
}
items = x;
}
- result = legacyDepsetConstructor(items, order, direct, transitive, loc);
+ result = legacyDepsetConstructor(items, order, direct, transitive);
}
if (semantics.debugDepsetDepth()) {
@@ -1052,18 +1050,18 @@
return result;
}
- private static Depset depsetConstructor(
- Object direct, Order order, Object transitive, Location loc) throws EvalException {
+ private static Depset depsetConstructor(Object direct, Order order, Object transitive)
+ throws EvalException {
if (direct instanceof Depset) {
throw new EvalException(
- loc,
+ null,
"parameter 'direct' must contain a list of elements, and may "
+ "no longer accept a depset. The deprecated behavior may be temporarily re-enabled "
+ "by setting --incompatible_disable_depset_inputs=false");
}
- Depset.Builder builder = Depset.builder(order, loc);
+ Depset.Builder builder = Depset.builder(order);
for (Object directElement : listFromNoneable(direct, Object.class, "direct")) {
builder.addDirect(directElement);
}
@@ -1084,17 +1082,16 @@
}
private static Depset legacyDepsetConstructor(
- Object items, Order order, Object direct, Object transitive, Location loc)
- throws EvalException {
+ Object items, Order order, Object direct, Object transitive) throws EvalException {
if (transitive == Starlark.NONE && direct == Starlark.NONE) {
// Legacy behavior.
- return Depset.of(order, items, loc);
+ return Depset.legacyOf(order, items);
}
if (direct != Starlark.NONE && !isEmptySkylarkList(items)) {
throw new EvalException(
- loc, "Do not pass both 'direct' and 'items' argument to depset constructor.");
+ null, "Do not pass both 'direct' and 'items' argument to depset constructor.");
}
// Non-legacy behavior: either 'transitive' or 'direct' were specified.
@@ -1114,7 +1111,7 @@
} else {
transitiveList = ImmutableList.of();
}
- Depset.Builder builder = Depset.builder(order, loc);
+ Depset.Builder builder = Depset.builder(order);
for (Object directElement : directElements) {
builder.addDirect(directElement);
}
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 1047dbd..b8ada81 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
@@ -493,15 +493,15 @@
// (b) at least one order is "default"
for (Order first : Order.values()) {
- Depset s1 = Depset.of(first, Tuple.of("1", "11"), null);
+ Depset s1 = Depset.legacyOf(first, Tuple.of("1", "11"));
for (Order second : Order.values()) {
- Depset s2 = Depset.of(second, Tuple.of("2", "22"), null);
+ Depset s2 = Depset.legacyOf(second, Tuple.of("2", "22"));
boolean compatible = true;
try {
- Depset.of(s1, s2, null);
+ Depset.unionOf(s1, s2);
} catch (Exception ex) {
compatible = false;
}
@@ -525,9 +525,9 @@
new MergeStrategy() {
@Override
public Depset merge(Depset[] sets) throws Exception {
- Depset union = Depset.of(sets[0], sets[1], null);
- union = Depset.of(union, sets[2], null);
- union = Depset.of(union, sets[3], null);
+ Depset union = Depset.unionOf(sets[0], sets[1]);
+ union = Depset.unionOf(union, sets[2]);
+ union = Depset.unionOf(union, sets[3]);
return union;
}
@@ -546,9 +546,9 @@
new MergeStrategy() {
@Override
public Depset merge(Depset[] sets) throws Exception {
- Depset leftUnion = Depset.of(sets[0], sets[1], null);
- Depset rightUnion = Depset.of(sets[2], sets[3], null);
- Depset union = Depset.of(leftUnion, rightUnion, null);
+ Depset leftUnion = Depset.unionOf(sets[0], sets[1]);
+ Depset rightUnion = Depset.unionOf(sets[2], sets[3]);
+ Depset union = Depset.unionOf(leftUnion, rightUnion);
return union;
}
@@ -567,9 +567,9 @@
new MergeStrategy() {
@Override
public Depset merge(Depset[] sets) throws Exception {
- Depset union = Depset.of(sets[2], sets[3], null);
- union = Depset.of(sets[1], union, null);
- union = Depset.of(sets[0], union, null);
+ Depset union = Depset.unionOf(sets[2], sets[3]);
+ union = Depset.unionOf(sets[1], union);
+ union = Depset.unionOf(sets[0], union);
return union;
}
@@ -624,10 +624,10 @@
private Depset[] makeFourSets(Order order) throws Exception {
return new Depset[] {
- Depset.of(order, Tuple.of("1", "11"), null),
- Depset.of(order, Tuple.of("2", "22"), null),
- Depset.of(order, Tuple.of("3", "33"), null),
- Depset.of(order, Tuple.of("4", "44"), null)
+ Depset.legacyOf(order, Tuple.of("1", "11")),
+ Depset.legacyOf(order, Tuple.of("2", "22")),
+ Depset.legacyOf(order, Tuple.of("3", "33")),
+ Depset.legacyOf(order, Tuple.of("4", "44"))
};
}
}
