bazel syntax: simplify StarlarkMutable
A sequence may be mutable (list) or not (tuple, range).
This change breaks the subclass relationship between Sequence
and StarlarkMutable so that Tuple and RangeList do not implement it.
This change also deletes the StarlarkMutable abstract base classes
(BaseMutableWrapper, MutableCollection, BaseMutableList, MutableMap), partly
because they would now require Mutable and non-Mutable variants,
but more importantly because, through getContentsUnsafe, they force
inefficiency on all concrete implementations: either the internal representation
must itself be a List, which adds another allocation and indirection, or
getContentsUnsafe must lazily allocate a List wrapper, which generates
unnecessary garbage. A follow-up change will move the accessor methods
down into list and tuple and specialize them to the representation.
Pushing MutableMap down into Dict caused it to have two methods named get,
one annotated and one not. This exposed a bug in which
SkylarkInterfaceUtils.getSkylarkCallable gets confused
as two which method has the annotation.
For now I renamed Dict.get to get2 as the fix is slightly tricky.
StarlarkMutable's lock and unlock methods were in effect moved to EvalUtils.
Its isImmutable and isHashable methods are left abstract. They are
easy enough for subclasses to implement.
The StarlarkMutable class was effectively a "mix in", a base class that one
extends to get some help with implementation. Mixins are not a good
fit with single-inheritance OO languages. This change makes it an interface
with a default implementation of its sole remaining method, checkMutability,
that subclasses should really not override.
Also:
- update the two flavors (mutable/immutable) of Args
- delete mutability methods of RangeList and Tuple.
- delete SHALLOW_IMMUTABLE, which is no longer needed.
- remove unused Mutability from test of serialization.
- add some TODOs.
PiperOrigin-RevId: 280466438
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java b/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java
index 9067066..ea2004c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Tuple.java
@@ -43,127 +43,120 @@
+ "Tuples are immutable, therefore <code>x[1] = \"a\"</code> is not supported.")
public final class Tuple<E> extends Sequence<E> {
- private final ImmutableList<E> contents;
+ private final ImmutableList<E> contents;
- private Tuple(ImmutableList<E> contents) {
- this.contents = contents;
+ private Tuple(ImmutableList<E> contents) {
+ this.contents = contents;
+ }
+
+ /**
+ * A shared instance for the empty tuple.
+ *
+ * <p>This instance should be the only empty tuple.
+ */
+ private static final Tuple<?> EMPTY = new Tuple<>(ImmutableList.of());
+
+ /** Returns the empty tuple, cast to have an arbitrary content type. */
+ @SuppressWarnings("unchecked")
+ public static <T> Tuple<T> empty() {
+ return (Tuple<T>) EMPTY;
+ }
+
+ /**
+ * Creates a {@code Tuple} from an {@link ImmutableList}, reusing the empty instance if
+ * applicable.
+ */
+ private static <T> Tuple<T> create(ImmutableList<T> contents) {
+ if (contents.isEmpty()) {
+ return empty();
}
+ return new Tuple<>(contents);
+ }
- /**
- * A shared instance for the empty tuple.
- *
- * <p>This instance should be the only empty tuple.
- */
- private static final Tuple<?> EMPTY = new Tuple<>(ImmutableList.of());
+ /** Returns a {@code Tuple} whose items are given by an iterable. */
+ public static <T> Tuple<T> copyOf(Iterable<? extends T> contents) {
+ return create(ImmutableList.<T>copyOf(contents));
+ }
- /** Returns the empty tuple, cast to have an arbitrary content type. */
- @SuppressWarnings("unchecked")
- public static <T> Tuple<T> empty() {
- return (Tuple<T>) EMPTY;
- }
+ /**
+ * Returns a {@code Tuple} whose items are given by an immutable list.
+ *
+ * <p>This method is a specialization of a {@link #copyOf(Iterable)} that avoids an unnecessary
+ * {@code copyOf} invocation.
+ */
+ public static <T> Tuple<T> copyOf(ImmutableList<T> contents) {
+ return create(contents);
+ }
- /**
- * Creates a {@code Tuple} from an {@link ImmutableList}, reusing the empty instance if
- * applicable.
- */
- private static <T> Tuple<T> create(ImmutableList<T> contents) {
- if (contents.isEmpty()) {
- return empty();
+ /** Returns a {@code Tuple} with the given items. */
+ public static <T> Tuple<T> of(T... elements) {
+ return Tuple.create(ImmutableList.copyOf(elements));
+ }
+
+ @Override
+ public boolean isImmutable() {
+ for (Object item : this) {
+ if (!EvalUtils.isImmutable(item)) {
+ return false;
}
- return new Tuple<>(contents);
}
+ return true;
+ }
- /** Returns a {@code Tuple} whose items are given by an iterable. */
- public static <T> Tuple<T> copyOf(Iterable<? extends T> contents) {
- return create(ImmutableList.<T>copyOf(contents));
- }
-
- /**
- * Returns a {@code Tuple} whose items are given by an immutable list.
- *
- * <p>This method is a specialization of a {@link #copyOf(Iterable)} that avoids an unnecessary
- * {@code copyOf} invocation.
- */
- public static <T> Tuple<T> copyOf(ImmutableList<T> contents) {
- return create(contents);
- }
-
- /** Returns a {@code Tuple} with the given items. */
- public static <T> Tuple<T> of(T... elements) {
- return Tuple.create(ImmutableList.copyOf(elements));
- }
-
- // Overridden to recurse over children, since tuples use SHALLOW_IMMUTABLE and other
- // StarlarkMutable subclasses do not.
- @Override
- public boolean isImmutable() {
- for (Object item : this) {
- if (!EvalUtils.isImmutable(item)) {
- return false;
- }
+ @Override
+ public boolean isHashable() {
+ for (Object item : this) {
+ if (!EvalUtils.isHashable(item)) {
+ return false;
}
- return true;
+ }
+ return true;
+ }
+
+ @Override
+ public ImmutableList<E> getImmutableList() {
+ return contents;
+ }
+
+ @Override
+ protected List<E> getContentsUnsafe() {
+ return contents;
+ }
+
+ /** Returns a {@code Tuple} that is the concatenation of two {@code Tuple}s. */
+ public static <T> Tuple<T> concat(Tuple<? extends T> left, Tuple<? extends T> right) {
+ // Build the ImmutableList directly rather than use Iterables.concat, to avoid unnecessary
+ // array resizing.
+ return create(
+ ImmutableList.<T>builderWithExpectedSize(left.size() + right.size())
+ .addAll(left)
+ .addAll(right)
+ .build());
+ }
+
+ @Override
+ public Tuple<E> getSlice(
+ Object start, Object end, Object step, Location loc, Mutability mutability)
+ throws EvalException {
+ List<Integer> sliceIndices = EvalUtils.getSliceIndices(start, end, step, this.size(), loc);
+ ImmutableList.Builder<E> builder = ImmutableList.builderWithExpectedSize(sliceIndices.size());
+ // foreach is not used to avoid iterator overhead
+ for (int i = 0; i < sliceIndices.size(); ++i) {
+ builder.add(this.get(sliceIndices.get(i)));
+ }
+ return copyOf(builder.build());
+ }
+
+ @Override
+ public Tuple<E> repeat(int times, Mutability mutability) {
+ if (times <= 0) {
+ return empty();
}
- @Override
- public boolean isHashable() {
- for (Object item : this) {
- if (!EvalUtils.isHashable(item)) {
- return false;
- }
- }
- return true;
+ ImmutableList.Builder<E> builder = ImmutableList.builderWithExpectedSize(this.size() * times);
+ for (int i = 0; i < times; i++) {
+ builder.addAll(this);
}
-
- @Override
- public Mutability mutability() {
- return Mutability.SHALLOW_IMMUTABLE;
- }
-
- @Override
- public ImmutableList<E> getImmutableList() {
- return contents;
- }
-
- @Override
- protected List<E> getContentsUnsafe() {
- return contents;
- }
-
- /** Returns a {@code Tuple} that is the concatenation of two {@code Tuple}s. */
- public static <T> Tuple<T> concat(Tuple<? extends T> left, Tuple<? extends T> right) {
- // Build the ImmutableList directly rather than use Iterables.concat, to avoid unnecessary
- // array resizing.
- return create(
- ImmutableList.<T>builderWithExpectedSize(left.size() + right.size())
- .addAll(left)
- .addAll(right)
- .build());
- }
-
- @Override
- public Tuple<E> getSlice(
- Object start, Object end, Object step, Location loc, Mutability mutability)
- throws EvalException {
- List<Integer> sliceIndices = EvalUtils.getSliceIndices(start, end, step, this.size(), loc);
- ImmutableList.Builder<E> builder = ImmutableList.builderWithExpectedSize(sliceIndices.size());
- // foreach is not used to avoid iterator overhead
- for (int i = 0; i < sliceIndices.size(); ++i) {
- builder.add(this.get(sliceIndices.get(i)));
- }
- return copyOf(builder.build());
- }
-
- @Override
- public Tuple<E> repeat(int times, Mutability mutability) {
- if (times <= 0) {
- return empty();
- }
-
- ImmutableList.Builder<E> builder = ImmutableList.builderWithExpectedSize(this.size() * times);
- for (int i = 0; i < times; i++) {
- builder.addAll(this);
- }
- return copyOf(builder.build());
- }
+ return copyOf(builder.build());
+ }
}
