From af105c4901d33ed4b3279c839d733992841676f2 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Tue, 22 May 2018 20:40:36 -0700 Subject: [PATCH 1/3] [Skylark] Make range function lazy. range used to use MutableList which would eagerly allocate an array list with all range elements, which is not efficient for very large ranges or when only a small number of its elements are used. This implementation uses a constant amount of RAM and computes a value for each requested index. --- .../lib/syntax/BinaryOperatorExpression.java | 11 ++ .../build/lib/syntax/MethodLibrary.java | 20 +-- .../build/lib/syntax/SkylarkList.java | 138 +++++++++++++++++- .../build/lib/syntax/MethodLibraryTest.java | 1 + 4 files changed, 153 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java index 283fd5720dc5b6..11191786783f3c 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java @@ -17,7 +17,9 @@ import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.syntax.Concatable.Concatter; import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; +import com.google.devtools.build.lib.syntax.SkylarkList.MutableListLike; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; +import com.google.devtools.build.lib.syntax.SkylarkList.TupleLike; import java.io.IOException; import java.util.IllegalFormatException; @@ -306,6 +308,15 @@ private static Object plus( } } + if (lval instanceof MutableListLike && rval instanceof MutableListLike) { + return MutableList.concat(((MutableListLike) lval).toMutableList(), + ((MutableListLike) rval).toMutableList(), env.mutability()); + } + + if (lval instanceof TupleLike && rval instanceof TupleLike) { + return Tuple.concat(((TupleLike) lval).toTuple(), ((TupleLike) rval).toTuple()); + } + if (lval instanceof SkylarkDict && rval instanceof SkylarkDict) { if (env.getSemantics().incompatibleDisallowDictPlus()) { throw new EvalException( 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 cbf8ce4e17d067..f23caef3430278 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 @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; import com.google.devtools.build.lib.syntax.EvalUtils.ComparisonException; import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; +import com.google.devtools.build.lib.syntax.SkylarkList.RangeList; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.syntax.Type.ConversionException; import java.util.ArrayDeque; @@ -621,7 +622,7 @@ public Integer invoke(String value) throws EvalException { @SkylarkSignature( name = "range", - returnType = MutableList.class, + returnType = RangeList.class, doc = "Creates a list where items go from start to stop, using a " + "step increment. If a single argument is provided, items will " @@ -658,7 +659,7 @@ public Integer invoke(String value) throws EvalException { ) private static final BuiltinFunction range = new BuiltinFunction("range") { - public MutableList invoke( + public RangeList invoke( Integer startOrStop, Object stopOrNone, Integer step, Location loc, Environment env) throws EvalException { int start; @@ -673,19 +674,12 @@ public MutableList invoke( if (step == 0) { throw new EvalException(loc, "step cannot be 0"); } - ArrayList result = new ArrayList<>(Math.abs((stop - start) / step)); if (step > 0) { - while (start < stop) { - result.add(start); - start += step; - } - } else { - while (start > stop) { - result.add(start); - start += step; - } + // simplify handling of a case like range(-2) or its equivalent range(-2, 0) + // by turning it into range(0, 0) + stop = Math.max(start, stop); } - return MutableList.wrapUnsafe(env, result); + return RangeList.of(start, stop, step); } }; diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java index 6be38d5f8fd5eb..e777ae6652f9c2 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; import com.google.devtools.build.lib.syntax.SkylarkMutable.BaseMutableList; +import java.util.AbstractList; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -116,8 +117,10 @@ public String toString() { @Override public boolean equals(Object object) { return (this == object) - || ((object != null) && (this.getClass() == object.getClass()) - && getContentsUnsafe().equals(((SkylarkList) object).getContentsUnsafe())); + || ((object != null) && (this.getClass() == object.getClass() + || this instanceof MutableListLike && object instanceof MutableListLike + || this instanceof TupleLike && object instanceof TupleLike) + && getContentsUnsafe().equals(((SkylarkList) object).getContentsUnsafe())); } @Override @@ -201,6 +204,123 @@ public static SkylarkList createImmutable(Iterable contents) return MutableList.copyOf(Mutability.IMMUTABLE, contents); } + /** An interface for classes that can be converted to {@link MutableList}. */ + public interface MutableListLike { + MutableList toMutableList(); + } + + /** An interface for classes that can be converted to {@link Tuple}. */ + public interface TupleLike { + Tuple toTuple(); + } + + /** + * A sequence returned by the range function invocation. + * + * Instead of eagerly allocating an array with all elements of the sequence, this class uses + * simple math to compute a value at each index. This is particularly useful when range is huge + * or only a few elements from it are used. + */ + public static final class RangeList extends SkylarkList implements + MutableListLike, TupleLike { + + /** Provides access to range elements based on their index. */ + private static class RangeListView extends AbstractList { + private static int computeSize(int start, int stop, int step) { + final int length = Math.abs(stop - start); + final int absolute_step = Math.abs(step); + // round up (length / absolute_step) without using floats + return 1 + ((length - 1) / absolute_step); + } + + private final int start; + private final int stop; + private final int step; + private final int size; + + private RangeListView(int start, int stop, int step) { + this.start = start; + this.stop = stop; + this.step = step; + this.size = computeSize(start, stop, step); + } + + @Override + public Integer get(int index) { + int value = start + step * index; + if ((step > 0 && value > stop) || (step < 0 && value < stop)) { + throw new ArrayIndexOutOfBoundsException(index); + } + return value; + } + + @Override + public int size() { + return size; + } + } + + private final AbstractList contents; + + private RangeList(int start, int stop, int step) { + this.contents = new RangeListView(start, stop, step); + } + + @Override + public boolean isTuple() { + return false; + } + + @Override + public ImmutableList getImmutableList() { + return ImmutableList.copyOf(contents); + } + + @Override + public SkylarkList getSlice(Object start, Object end, Object step, Location loc, + Mutability mutability) throws EvalException { + List sliceIndices = EvalUtils.getSliceIndices(start, end, step, this.size(), loc); + ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(sliceIndices.size()); + for (int pos : sliceIndices) { + builder.add(this.get(pos)); + } + return MutableList.createImmutable(builder.build()); + } + + @Override + public SkylarkList repeat(int times, Mutability mutability) { + ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(this.size() * times); + for (int i = 0; i < times; i++) { + builder.addAll(this); + } + return MutableList.createImmutable(builder.build()); + } + + @Override + protected List getContentsUnsafe() { + return contents; + } + + @Override + public Mutability mutability() { + return Mutability.IMMUTABLE; + } + + @Override + public MutableList toMutableList() { + return MutableList.copyOf(Mutability.IMMUTABLE, contents); + } + + @Override + public Tuple toTuple() { + return Tuple.copyOf(contents); + } + + public static RangeList of(int start, int stop, int step) { + return new RangeList(start, stop, step); + } + } + /** * A Skylark list, i.e., the value represented by {@code [1, 2, 3]}. Lists are mutable datatypes. */ @@ -222,7 +342,7 @@ public static SkylarkList createImmutable(Iterable contents) + "['a', 'b', 'c', 'd'][3:0:-1] # ['d', 'c', 'b']" + "Lists are mutable, as in Python." ) - public static final class MutableList extends SkylarkList { + public static final class MutableList extends SkylarkList implements MutableListLike { private final ArrayList contents; @@ -565,6 +685,11 @@ public Object pop(Object i, Location loc, Environment env) remove(index, loc, env.mutability()); return result; } + + @Override + public MutableList toMutableList() { + return this; + } } /** @@ -589,7 +714,7 @@ public Object pop(Object i, Location loc, Environment env) + "('a', 'b', 'c', 'd')[3:0:-1] # ('d', 'c', 'b')" + "Tuples are immutable, therefore x[1] = \"a\" is not supported." ) - public static final class Tuple extends SkylarkList { + public static final class Tuple extends SkylarkList implements TupleLike { private final ImmutableList contents; @@ -698,5 +823,10 @@ public Tuple repeat(int times, Mutability mutability) { } return copyOf(builder.build()); } + + @Override + public Tuple toTuple() { + return this; + } } } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java index 072b9c8e1c6d45..84945cf83920cf 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java @@ -486,6 +486,7 @@ public void testRange() throws Exception { .testStatement("str(range(5, 0, -1))", "[5, 4, 3, 2, 1]") .testStatement("str(range(5, 0, -10))", "[5]") .testStatement("str(range(0, -3, -2))", "[0, -2]") + .testStatement("str(range(3)[1:2])", "[1]") .testIfErrorContains("step cannot be 0", "range(2, 3, 0)"); } From 0c258137e35fd25ebe0b372a3187e4a4f11e1228 Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Wed, 23 May 2018 11:24:22 -0700 Subject: [PATCH 2/3] [Skylark] Implement specialized RangeListView iterators. --- .../build/lib/syntax/SkylarkList.java | 79 ++++++++++++++++++- .../build/lib/syntax/MethodLibraryTest.java | 9 ++- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java index e777ae6652f9c2..7c5b9f16da2b5e 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.UnmodifiableIterator; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.Param; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; @@ -28,7 +29,9 @@ import java.util.AbstractList; import java.util.ArrayList; import java.util.Collections; +import java.util.Iterator; import java.util.List; +import java.util.NoSuchElementException; import java.util.RandomAccess; import javax.annotation.Nullable; @@ -215,7 +218,7 @@ public interface TupleLike { } /** - * A sequence returned by the range function invocation. + * A sequence returned by the {@code range} function invocation. * * Instead of eagerly allocating an array with all elements of the sequence, this class uses * simple math to compute a value at each index. This is particularly useful when range is huge @@ -226,6 +229,65 @@ public static final class RangeList extends SkylarkList implements /** Provides access to range elements based on their index. */ private static class RangeListView extends AbstractList { + + /** Iterator for increasing sequences. */ + private static class IncreasingIterator extends UnmodifiableIterator { + private final int stop; + private final int step; + + private int cursor; + + private IncreasingIterator(int start, int stop, int step) { + this.cursor = start; + this.stop = stop; + this.step = step; + } + + @Override + public boolean hasNext() { + return cursor < stop; + } + + @Override + public Integer next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + int current = cursor; + cursor += step; + return current; + } + } + + /** Iterator for decreasing sequences. */ + private static class DecreasingIterator extends UnmodifiableIterator { + private final int stop; + private final int step; + + private int cursor; + + private DecreasingIterator(int start, int stop, int step) { + this.cursor = start; + this.stop = stop; + this.step = step; + } + + @Override + public boolean hasNext() { + return cursor > stop; + } + + @Override + public Integer next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + int current = cursor; + cursor += step; + return current; + } + } + private static int computeSize(int start, int stop, int step) { final int length = Math.abs(stop - start); final int absolute_step = Math.abs(step); @@ -258,6 +320,19 @@ public Integer get(int index) { public int size() { return size; } + + /** + * Returns an iterator optimized for traversing range elements, since it's the most frequent + * operation for which ranges are used. + */ + @Override + public Iterator iterator() { + if (step > 0) { + return new IncreasingIterator(start, stop, step); + } else { + return new DecreasingIterator(start, stop, step); + } + } } private final AbstractList contents; @@ -279,6 +354,7 @@ public ImmutableList getImmutableList() { @Override public SkylarkList getSlice(Object start, Object end, Object step, Location loc, Mutability mutability) throws EvalException { + // TODO: use lazy slice implementation List sliceIndices = EvalUtils.getSliceIndices(start, end, step, this.size(), loc); ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(sliceIndices.size()); for (int pos : sliceIndices) { @@ -317,6 +393,7 @@ public Tuple toTuple() { } public static RangeList of(int start, int stop, int step) { + Preconditions.checkArgument(step != 0); return new RangeList(start, stop, step); } } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java index 84945cf83920cf..b024f8a2241784 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java @@ -486,7 +486,14 @@ public void testRange() throws Exception { .testStatement("str(range(5, 0, -1))", "[5, 4, 3, 2, 1]") .testStatement("str(range(5, 0, -10))", "[5]") .testStatement("str(range(0, -3, -2))", "[0, -2]") - .testStatement("str(range(3)[1:2])", "[1]") + .testStatement("str(range(5)[1:])", "[1, 2, 3, 4]") + .testStatement("len(range(5)[1:])", 4) + .testStatement("str(range(5)[:2])", "[0, 1]") + .testStatement("str(range(10)[1:9:2])", "[1, 3, 5, 7]") + .testStatement("str(range(10)[1:10:2])", "[1, 3, 5, 7, 9]") + .testStatement("str(range(10)[1:11:2])", "[1, 3, 5, 7, 9]") + .testStatement("str(range(0, 10, 2)[::2])", "[0, 4, 8]") + .testStatement("str(range(0, 10, 2)[::-2])", "[8, 4, 0]") .testIfErrorContains("step cannot be 0", "range(2, 3, 0)"); } From aa7d616e4bb36dac33ecb5653446e14e7a6f72df Mon Sep 17 00:00:00 2001 From: Taras Tsugrii Date: Tue, 29 May 2018 19:37:27 -0700 Subject: [PATCH 3/3] [Skylark] Control whether range type is used with --incompatible_range_type. --- site/docs/skylark/backward-compatibility.md | 9 + .../lib/packages/SkylarkSemanticsCodec.java | 2 + .../lib/packages/SkylarkSemanticsOptions.java | 14 + .../lib/syntax/BinaryOperatorExpression.java | 13 +- .../build/lib/syntax/MethodLibrary.java | 13 +- .../devtools/build/lib/syntax/RangeList.java | 316 ++++++++++++++++++ .../build/lib/syntax/SkylarkList.java | 216 +----------- .../build/lib/syntax/SkylarkSemantics.java | 5 + .../SkylarkSemanticsConsistencyTest.java | 2 + .../build/lib/syntax/MethodLibraryTest.java | 45 +++ 10 files changed, 405 insertions(+), 230 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/syntax/RangeList.java diff --git a/site/docs/skylark/backward-compatibility.md b/site/docs/skylark/backward-compatibility.md index aac35278cec5dd..6c27fa676ce7b9 100644 --- a/site/docs/skylark/backward-compatibility.md +++ b/site/docs/skylark/backward-compatibility.md @@ -231,6 +231,15 @@ no user-visible impact. * Default: `true` +### Python 3 range behavior. +When set, the result of `range(...)` function is a lazy `range` type instead of +a `list`. Because of this repetitions using `*` operator are no longer +supported and `range` slices are also lazy `range` instances. + +* Flag: `--incompatible_range_type` +* Default: `false` + + ### Disable objc provider resources This flag disables certain deprecated resource fields on diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java index 7f147cb422f52a..e6671f89c0d2e0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java @@ -58,6 +58,7 @@ public void serialize( codedOut.writeBoolNoTag(semantics.incompatibleNewActionsApi()); codedOut.writeBoolNoTag(semantics.incompatibleNoSupportToolsInActionInputs()); codedOut.writeBoolNoTag(semantics.incompatiblePackageNameIsAFunction()); + codedOut.writeBoolNoTag(semantics.incompatibleRangeType()); codedOut.writeBoolNoTag(semantics.incompatibleRemoveNativeGitRepository()); codedOut.writeBoolNoTag(semantics.incompatibleRemoveNativeHttpArchive()); codedOut.writeBoolNoTag(semantics.incompatibleStringIsNotIterable()); @@ -85,6 +86,7 @@ public SkylarkSemantics deserialize(DeserializationContext context, CodedInputSt builder.incompatibleNewActionsApi(codedIn.readBool()); builder.incompatibleNoSupportToolsInActionInputs(codedIn.readBool()); builder.incompatiblePackageNameIsAFunction(codedIn.readBool()); + builder.incompatibleRangeType(codedIn.readBool()); builder.incompatibleRemoveNativeGitRepository(codedIn.readBool()); builder.incompatibleRemoveNativeHttpArchive(codedIn.readBool()); builder.incompatibleStringIsNotIterable(codedIn.readBool()); diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java index 8781bb5b5944b2..e29243fee36081 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java @@ -265,6 +265,19 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable ) public boolean incompatiblePackageNameIsAFunction; + @Option( + name = "incompatible_range_type", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.UNKNOWN}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = "If set to true, range() will use the 'range' type instead of 'list'." + ) + public boolean incompatibleRangeType; + @Option( name = "incompatible_remove_native_git_repository", defaultValue = "false", @@ -338,6 +351,7 @@ public SkylarkSemantics toSkylarkSemantics() { .incompatibleNewActionsApi(incompatibleNewActionsApi) .incompatibleNoSupportToolsInActionInputs(incompatibleNoSupportToolsInActionInputs) .incompatiblePackageNameIsAFunction(incompatiblePackageNameIsAFunction) + .incompatibleRangeType(incompatibleRangeType) .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository) .incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive) .incompatibleStringIsNotIterable(incompatibleStringIsNotIterable) diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java index 11191786783f3c..5ac2fb345fb901 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java @@ -17,9 +17,7 @@ import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.syntax.Concatable.Concatter; import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; -import com.google.devtools.build.lib.syntax.SkylarkList.MutableListLike; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; -import com.google.devtools.build.lib.syntax.SkylarkList.TupleLike; import java.io.IOException; import java.util.IllegalFormatException; @@ -308,15 +306,6 @@ private static Object plus( } } - if (lval instanceof MutableListLike && rval instanceof MutableListLike) { - return MutableList.concat(((MutableListLike) lval).toMutableList(), - ((MutableListLike) rval).toMutableList(), env.mutability()); - } - - if (lval instanceof TupleLike && rval instanceof TupleLike) { - return Tuple.concat(((TupleLike) lval).toTuple(), ((TupleLike) rval).toTuple()); - } - if (lval instanceof SkylarkDict && rval instanceof SkylarkDict) { if (env.getSemantics().incompatibleDisallowDictPlus()) { throw new EvalException( @@ -399,7 +388,7 @@ private static Object mult(Object lval, Object rval, Environment env, Location l } else if (otherFactor instanceof String) { // Similar to Python, a factor < 1 leads to an empty string. return Strings.repeat((String) otherFactor, Math.max(0, number)); - } else if (otherFactor instanceof SkylarkList) { + } else if (otherFactor instanceof SkylarkList && !(otherFactor instanceof RangeList)) { // Similar to Python, a factor < 1 leads to an empty string. return ((SkylarkList) otherFactor).repeat(number, env.mutability()); } 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 f23caef3430278..88f0c2b8bf7dbc 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 @@ -30,7 +30,6 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; import com.google.devtools.build.lib.syntax.EvalUtils.ComparisonException; import com.google.devtools.build.lib.syntax.SkylarkList.MutableList; -import com.google.devtools.build.lib.syntax.SkylarkList.RangeList; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.syntax.Type.ConversionException; import java.util.ArrayDeque; @@ -622,7 +621,7 @@ public Integer invoke(String value) throws EvalException { @SkylarkSignature( name = "range", - returnType = RangeList.class, + returnType = SkylarkList.class, doc = "Creates a list where items go from start to stop, using a " + "step increment. If a single argument is provided, items will " @@ -659,7 +658,7 @@ public Integer invoke(String value) throws EvalException { ) private static final BuiltinFunction range = new BuiltinFunction("range") { - public RangeList invoke( + public SkylarkList invoke( Integer startOrStop, Object stopOrNone, Integer step, Location loc, Environment env) throws EvalException { int start; @@ -674,12 +673,8 @@ public RangeList invoke( if (step == 0) { throw new EvalException(loc, "step cannot be 0"); } - if (step > 0) { - // simplify handling of a case like range(-2) or its equivalent range(-2, 0) - // by turning it into range(0, 0) - stop = Math.max(start, stop); - } - return RangeList.of(start, stop, step); + RangeList range = RangeList.of(start, stop, step); + return env.getSemantics().incompatibleRangeType() ? range : range.toMutableList(env); } }; diff --git a/src/main/java/com/google/devtools/build/lib/syntax/RangeList.java b/src/main/java/com/google/devtools/build/lib/syntax/RangeList.java new file mode 100644 index 00000000000000..9562926a7b4f28 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/syntax/RangeList.java @@ -0,0 +1,316 @@ +package com.google.devtools.build.lib.syntax; + +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.UnmodifiableIterator; +import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; +import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; +import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter; +import java.util.AbstractList; +import java.util.Iterator; +import java.util.List; +import java.util.NoSuchElementException; + +/** + * A sequence returned by the {@code range} function invocation. + * + *

Instead of eagerly allocating an array with all elements of the sequence, this class uses + * simple math to compute a value at each index. This is particularly useful when range is huge or + * only a few elements from it are used. + * + *

Eventually {@code range} function should produce an instance of the {@code range} type as is + * the case in Python 3, but for now to preserve backwards compatibility with Python 2, {@code list} + * is returned. + */ +@SkylarkModule( + name = "range", + category = SkylarkModuleCategory.BUILTIN, + doc = + "A language built-in type to support ranges. Example of range literal:
" + + "

x = range(1, 10, 3)
" + + "Accessing elements is possible using indexing (starts from 0):
" + + "
e = x[1]   # e == 2
" + + "Ranges do not support the + operator for concatenation." + + "Similar to strings, ranges support slice operations:" + + "
range(10)[1:3]   # range(1, 3)\n"
+            + "range(10)[::2]  # range(0, 10, 2)\n"
+            + "range(10)[3:0:-1]  # range(3, 0, -1)
" + + "Ranges are immutable, as in Python 3.") +public final class RangeList extends SkylarkList { + + private final int step; + private final int start; + + private static int computeItem(int start, int step, int index) { + return start + step * index; + } + + /** Provides access to range elements based on their index. */ + private static class RangeListView extends AbstractList { + + /** Iterator for increasing/decreasing sequences. */ + private static class RangeListIterator extends UnmodifiableIterator { + private final int stop; + private final int step; + + private int cursor; + + private RangeListIterator(int start, int stop, int step) { + this.cursor = start; + this.stop = stop; + this.step = step; + } + + @Override + public boolean hasNext() { + return (step > 0) ? cursor < stop : cursor > stop; + } + + @Override + public Integer next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + int current = cursor; + cursor += step; + return current; + } + } + + /** + * @return The size of the range specified by {@code start}, {@code stop} and {@code step}. + * Python version: + * https://github.com/python/cpython/blob/09bb918a61031377d720f1a0fa1fe53c962791b6/Objects/rangeobject.c#L144 + */ + private static int computeSize(int start, int stop, int step) { + // low and high represent bounds of the interval with only one of the sides being open. + int low; + int high; + if (step > 0) { + low = start; + high = stop; + } else { + low = stop; + high = start; + step = -step; + } + if (low >= high) return 0; + + int diff = high - low - 1; + return diff / step + 1; + } + + private final int start; + private final int stop; + private final int step; + private final int size; + + private RangeListView(int start, int stop, int step) { + this.start = start; + this.stop = stop; + this.step = step; + this.size = computeSize(start, stop, step); + } + + @Override + public Integer get(int index) { + if (index < 0 || index >= size()) { + throw new ArrayIndexOutOfBoundsException(index); + } + return computeItem(start, step, index); + } + + @Override + public int size() { + return size; + } + + /** + * Returns an iterator optimized for traversing range elements, since it's the most frequent + * operation for which ranges are used. + */ + @Override + public Iterator iterator() { + return new RangeListIterator(start, stop, step); + } + + /** @return the start of the range. */ + public int getStart() { + return start; + } + + /** @return the stop element (next after the last one) of the range. */ + public int getStop() { + return stop; + } + + /** @return the step between each element of the range. */ + public int getStep() { + return step; + } + } + + private final RangeListView contents; + + private RangeList(int start, int stop, int step) { + this.step = step; + this.start = start; + this.contents = new RangeListView(start, stop, step); + } + + @Override + public boolean isTuple() { + return false; + } + + @Override + public ImmutableList getImmutableList() { + return ImmutableList.copyOf(contents); + } + + @Override + public SkylarkList getSlice( + Object start, Object end, Object step, Location loc, Mutability mutability) + throws EvalException { + Slice slice = Slice.from(size(), start, end, step, loc); + int substep = slice.step * this.step; + int substart = computeItem(this.start, this.step, slice.start); + int substop = computeItem(this.start, this.step, slice.stop); + return RangeList.of(substart, substop, substep); + } + + @Override + public SkylarkList repeat(int times, Mutability mutability) { + throw new UnsupportedOperationException("Ranges do not support repetition."); + } + + @Override + protected List getContentsUnsafe() { + return contents; + } + + @Override + public Mutability mutability() { + return Mutability.IMMUTABLE; + } + + @Override + public void repr(SkylarkPrinter printer) { + if (contents.getStep() == 1) { + printer.format("range(%d, %d)", contents.getStart(), contents.getStop()); + } else { + printer.format( + "range(%d, %d, %d)", contents.getStart(), contents.getStop(), contents.getStep()); + } + } + + /** + * Converts this range sequence into a materialized list. + * + *

Usage of this method is not recommended, since it completely defeats the purpose of lazy + * computation by eagerly computing the result. + * + * @return A materialized version of the range that can be used as a + * 

list
+ * type. + */ + MutableList toMutableList(Environment env) { + return MutableList.copyOf(env, contents); + } + + /** + * @return A half-opened range defined by its starting value (inclusive), stop value (exclusive) + * and a step from previous value to the next one. + */ + public static RangeList of(int start, int stop, int step) { + Preconditions.checkArgument(step != 0); + return new RangeList(start, stop, step); + } + + /** + * Represents a slice produced by applying {@code [start:end:step]} to a {@code range}. + * + *

{@code start} and {@code stop} define a half-open interval + * + * 

[start, stop)
+ */ + private static class Slice { + + private final int start; + private final int stop; + private final int step; + + private Slice(int start, int stop, int step) { + this.start = start; + this.stop = stop; + this.step = step; + } + + /** + * Computes slice indices for the requested range slice. + * + *

The implementation is based on CPython + * https://github.com/python/cpython/blob/09bb918a61031377d720f1a0fa1fe53c962791b6/Objects/sliceobject.c#L366-L509 + */ + public static Slice from( + int length, Object startObj, Object endObj, Object stepObj, Location loc) + throws EvalException { + int start; + int stop; + int step; + + if (stepObj == Runtime.NONE) { + step = 1; + } else if (stepObj instanceof Integer) { + step = (Integer) stepObj; + } else { + throw new EvalException( + loc, String.format("slice step must be an integer, not '%s'", stepObj)); + } + if (step == 0) { + throw new EvalException(loc, "slice step cannot be zero"); + } + + int upper; // upper bound for stop (exclusive) + int lower; // lower bound for start (inclusive) + if (step < 0) { + lower = -1; + upper = length - 1; + } else { + lower = 0; + upper = length; + } + + if (startObj == Runtime.NONE) { + start = step < 0 ? upper : lower; + } else if (startObj instanceof Integer) { + start = (Integer) startObj; + if (start < 0) { + start += length; + start = Math.max(start, lower); + } else { + start = Math.min(start, upper); + } + } else { + throw new EvalException( + loc, String.format("slice start must be an integer, not '%s'", startObj)); + } + if (endObj == Runtime.NONE) { + stop = step < 0 ? lower : upper; + } else if (endObj instanceof Integer) { + stop = (Integer) endObj; + if (stop < 0) { + stop += length; + stop = Math.max(stop, lower); + } else { + stop = Math.min(stop, upper); + } + } else { + throw new EvalException( + loc, String.format("slice end must be an integer, not '%s'", endObj)); + } + return new Slice(start, stop, step); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java index 7c5b9f16da2b5e..023a87d1d08079 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java @@ -120,10 +120,9 @@ public String toString() { @Override public boolean equals(Object object) { return (this == object) - || ((object != null) && (this.getClass() == object.getClass() - || this instanceof MutableListLike && object instanceof MutableListLike - || this instanceof TupleLike && object instanceof TupleLike) - && getContentsUnsafe().equals(((SkylarkList) object).getContentsUnsafe())); + || ((object != null) + && (this.getClass() == object.getClass()) + && getContentsUnsafe().equals(((SkylarkList) object).getContentsUnsafe())); } @Override @@ -207,197 +206,6 @@ public static SkylarkList createImmutable(Iterable contents) return MutableList.copyOf(Mutability.IMMUTABLE, contents); } - /** An interface for classes that can be converted to {@link MutableList}. */ - public interface MutableListLike { - MutableList toMutableList(); - } - - /** An interface for classes that can be converted to {@link Tuple}. */ - public interface TupleLike { - Tuple toTuple(); - } - - /** - * A sequence returned by the {@code range} function invocation. - * - * Instead of eagerly allocating an array with all elements of the sequence, this class uses - * simple math to compute a value at each index. This is particularly useful when range is huge - * or only a few elements from it are used. - */ - public static final class RangeList extends SkylarkList implements - MutableListLike, TupleLike { - - /** Provides access to range elements based on their index. */ - private static class RangeListView extends AbstractList { - - /** Iterator for increasing sequences. */ - private static class IncreasingIterator extends UnmodifiableIterator { - private final int stop; - private final int step; - - private int cursor; - - private IncreasingIterator(int start, int stop, int step) { - this.cursor = start; - this.stop = stop; - this.step = step; - } - - @Override - public boolean hasNext() { - return cursor < stop; - } - - @Override - public Integer next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - int current = cursor; - cursor += step; - return current; - } - } - - /** Iterator for decreasing sequences. */ - private static class DecreasingIterator extends UnmodifiableIterator { - private final int stop; - private final int step; - - private int cursor; - - private DecreasingIterator(int start, int stop, int step) { - this.cursor = start; - this.stop = stop; - this.step = step; - } - - @Override - public boolean hasNext() { - return cursor > stop; - } - - @Override - public Integer next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - int current = cursor; - cursor += step; - return current; - } - } - - private static int computeSize(int start, int stop, int step) { - final int length = Math.abs(stop - start); - final int absolute_step = Math.abs(step); - // round up (length / absolute_step) without using floats - return 1 + ((length - 1) / absolute_step); - } - - private final int start; - private final int stop; - private final int step; - private final int size; - - private RangeListView(int start, int stop, int step) { - this.start = start; - this.stop = stop; - this.step = step; - this.size = computeSize(start, stop, step); - } - - @Override - public Integer get(int index) { - int value = start + step * index; - if ((step > 0 && value > stop) || (step < 0 && value < stop)) { - throw new ArrayIndexOutOfBoundsException(index); - } - return value; - } - - @Override - public int size() { - return size; - } - - /** - * Returns an iterator optimized for traversing range elements, since it's the most frequent - * operation for which ranges are used. - */ - @Override - public Iterator iterator() { - if (step > 0) { - return new IncreasingIterator(start, stop, step); - } else { - return new DecreasingIterator(start, stop, step); - } - } - } - - private final AbstractList contents; - - private RangeList(int start, int stop, int step) { - this.contents = new RangeListView(start, stop, step); - } - - @Override - public boolean isTuple() { - return false; - } - - @Override - public ImmutableList getImmutableList() { - return ImmutableList.copyOf(contents); - } - - @Override - public SkylarkList getSlice(Object start, Object end, Object step, Location loc, - Mutability mutability) throws EvalException { - // TODO: use lazy slice implementation - List sliceIndices = EvalUtils.getSliceIndices(start, end, step, this.size(), loc); - ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(sliceIndices.size()); - for (int pos : sliceIndices) { - builder.add(this.get(pos)); - } - return MutableList.createImmutable(builder.build()); - } - - @Override - public SkylarkList repeat(int times, Mutability mutability) { - ImmutableList.Builder builder = ImmutableList.builderWithExpectedSize(this.size() * times); - for (int i = 0; i < times; i++) { - builder.addAll(this); - } - return MutableList.createImmutable(builder.build()); - } - - @Override - protected List getContentsUnsafe() { - return contents; - } - - @Override - public Mutability mutability() { - return Mutability.IMMUTABLE; - } - - @Override - public MutableList toMutableList() { - return MutableList.copyOf(Mutability.IMMUTABLE, contents); - } - - @Override - public Tuple toTuple() { - return Tuple.copyOf(contents); - } - - public static RangeList of(int start, int stop, int step) { - Preconditions.checkArgument(step != 0); - return new RangeList(start, stop, step); - } - } - /** * A Skylark list, i.e., the value represented by {@code [1, 2, 3]}. Lists are mutable datatypes. */ @@ -419,7 +227,7 @@ public static RangeList of(int start, int stop, int step) { + "['a', 'b', 'c', 'd'][3:0:-1] # ['d', 'c', 'b']" + "Lists are mutable, as in Python." ) - public static final class MutableList extends SkylarkList implements MutableListLike { + public static final class MutableList extends SkylarkList { private final ArrayList contents; @@ -542,7 +350,7 @@ public static MutableList concat( return new MutableList<>(newContents, mutability); } - /** More efficient {@link List#addAll} replacement when both lists are {@link ArrayList}s. */ + /** More efficient {@link List#addAll} replacement when both lists are {@link ArrayList}s. */ private static void addAll(ArrayList addTo, ArrayList addFrom) { // Hot code path, skip iterator. for (int i = 0; i < addFrom.size(); i++) { @@ -762,11 +570,6 @@ public Object pop(Object i, Location loc, Environment env) remove(index, loc, env.mutability()); return result; } - - @Override - public MutableList toMutableList() { - return this; - } } /** @@ -791,7 +594,7 @@ public MutableList toMutableList() { + "('a', 'b', 'c', 'd')[3:0:-1] # ('d', 'c', 'b')" + "Tuples are immutable, therefore x[1] = \"a\" is not supported." ) - public static final class Tuple extends SkylarkList implements TupleLike { + public static final class Tuple extends SkylarkList { private final ImmutableList contents; @@ -816,7 +619,7 @@ public static Tuple empty() { * Creates a {@code Tuple} from an {@link ImmutableList}, reusing the empty instance if * applicable. */ - private static Tuple create(ImmutableList contents) { + private static Tuple create(ImmutableList contents) { if (contents.isEmpty()) { return empty(); } @@ -900,10 +703,5 @@ public Tuple repeat(int times, Mutability mutability) { } return copyOf(builder.build()); } - - @Override - public Tuple toTuple() { - return this; - } } } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index 5bd711b75edb59..51ff0134f8eb6c 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java @@ -69,6 +69,8 @@ public abstract class SkylarkSemantics { public abstract boolean incompatiblePackageNameIsAFunction(); + public abstract boolean incompatibleRangeType(); + public abstract boolean incompatibleRemoveNativeGitRepository(); public abstract boolean incompatibleRemoveNativeHttpArchive(); @@ -107,6 +109,7 @@ public static Builder builderWithDefaults() { .incompatibleNewActionsApi(false) .incompatibleNoSupportToolsInActionInputs(false) .incompatiblePackageNameIsAFunction(false) + .incompatibleRangeType(false) .incompatibleRemoveNativeGitRepository(false) .incompatibleRemoveNativeHttpArchive(false) .incompatibleStringIsNotIterable(false) @@ -148,6 +151,8 @@ public abstract static class Builder { public abstract Builder incompatiblePackageNameIsAFunction(boolean value); + public abstract Builder incompatibleRangeType(boolean value); + public abstract Builder incompatibleRemoveNativeGitRepository(boolean value); public abstract Builder incompatibleRemoveNativeHttpArchive(boolean value); diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index 4a8e303a5063eb..561e9afdf247c5 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -134,6 +134,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex "--incompatible_new_actions_api=" + rand.nextBoolean(), "--incompatible_no_support_tools_in_action_inputs=" + rand.nextBoolean(), "--incompatible_package_name_is_a_function=" + rand.nextBoolean(), + "--incompatible_range_type=" + rand.nextBoolean(), "--incompatible_remove_native_git_repository=" + rand.nextBoolean(), "--incompatible_remove_native_http_archive=" + rand.nextBoolean(), "--incompatible_string_is_not_iterable=" + rand.nextBoolean(), @@ -162,6 +163,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) { .incompatibleNewActionsApi(rand.nextBoolean()) .incompatibleNoSupportToolsInActionInputs(rand.nextBoolean()) .incompatiblePackageNameIsAFunction(rand.nextBoolean()) + .incompatibleRangeType(rand.nextBoolean()) .incompatibleRemoveNativeGitRepository(rand.nextBoolean()) .incompatibleRemoveNativeHttpArchive(rand.nextBoolean()) .incompatibleStringIsNotIterable(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java index b024f8a2241784..103a35d137dae5 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java @@ -507,6 +507,51 @@ public void testRangeIsList() throws Exception { runRangeIsListAssertions("range(4)[:3]"); } + @Test + public void testRangeType() throws Exception { + new BothModesTest("--incompatible_range_type=true") + .setUp("a = range(3)") + .testStatement("len(a)", 3) + .testStatement("str(a)", "range(0, 3)") + .testStatement("str(range(1,2,3))", "range(1, 2, 3)") + .testStatement("repr(a)", "range(0, 3)") + .testStatement("repr(range(1,2,3))", "range(1, 2, 3)") + .testStatement("type(a)", "range") + .testIfErrorContains("unsupported operand type(s) for +: 'range' and 'range'", "a + a") + .testIfErrorContains("type 'range' has no method append(int)", "a.append(3)") + .testStatement("str(list(range(5)))", "[0, 1, 2, 3, 4]") + .testStatement("str(list(range(0)))", "[]") + .testStatement("str(list(range(1)))", "[0]") + .testStatement("str(list(range(-2)))", "[]") + .testStatement("str(list(range(-3, 2)))", "[-3, -2, -1, 0, 1]") + .testStatement("str(list(range(3, 2)))", "[]") + .testStatement("str(list(range(3, 3)))", "[]") + .testStatement("str(list(range(3, 4)))", "[3]") + .testStatement("str(list(range(3, 5)))", "[3, 4]") + .testStatement("str(list(range(-3, 5, 2)))", "[-3, -1, 1, 3]") + .testStatement("str(list(range(-3, 6, 2)))", "[-3, -1, 1, 3, 5]") + .testStatement("str(list(range(5, 0, -1)))", "[5, 4, 3, 2, 1]") + .testStatement("str(list(range(5, 0, -10)))", "[5]") + .testStatement("str(list(range(0, -3, -2)))", "[0, -2]") + .testStatement("range(3)[-1]", 2) + .testIfErrorContains( + "index out of range (index is 3, but sequence has 3 elements)", "range(3)[3]") + .testStatement("str(range(5)[1:])", "range(1, 5)") + .testStatement("len(range(5)[1:])", 4) + .testStatement("str(range(5)[:2])", "range(0, 2)") + .testStatement("str(range(10)[1:9:2])", "range(1, 9, 2)") + .testStatement("str(list(range(10)[1:9:2]))", "[1, 3, 5, 7]") + .testStatement("str(range(10)[1:10:2])", "range(1, 10, 2)") + .testStatement("str(range(10)[1:11:2])", "range(1, 10, 2)") + .testStatement("str(range(0, 10, 2)[::2])", "range(0, 10, 4)") + .testStatement("str(range(0, 10, 2)[::-2])", "range(8, -2, -4)") + .testStatement("str(range(5)[1::-1])", "range(1, -1, -1)") + .testIfErrorContains("step cannot be 0", "range(2, 3, 0)") + .testIfErrorContains( + "unsupported operand type(s) for *: 'range' and 'int'", "range(3) * 3"); + ; + } + /** * Helper function for testRangeIsList that expects a range or range slice expression producing * the range value containing [0, 1, 2].