bazel syntax: eliminate unnecessary array copying Before, EvalUtils.toCollection would return a view (if given a Sequence) but a copy if given a Dict. This makes it inefficient and also hard to reason about. This change replaces it with Starlark.toIterable and Starlark.toArray: the first always returns a view, the second a copy. Also: - CallUtils: eliminate array copying in handling of *args and **kwargs. - Start removing Location parameters from everything we touch. Location is automatically added to EvalExceptions by the interpreter. - MethodLibrary: eliminate array copies in sorted, reverse, tuple, list, enumerate. - Also in string.join, string.partition. Simplify the latter function. - Improve text of an error message. - Declare Starlark.len, which returns the length of a string or iterable. This change removes support for Starlark Map other than Dict, because there are none today. PiperOrigin-RevId: 282682290
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java index fad3a77..3c96fd8 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
@@ -301,8 +301,6 @@ // *args, **kwargs, location, ast, thread, skylark semantics final int extraArgsCount = 6; List<Object> builder = new ArrayList<>(parameters.size() + extraArgsCount); - boolean acceptsExtraArgs = method.isAcceptsExtraArgs(); - boolean acceptsExtraKwargs = method.isAcceptsExtraKwargs(); int argIndex = 0; @@ -374,45 +372,30 @@ builder.add(value); } - ImmutableList<Object> extraArgs = ImmutableList.of(); - if (argIndex < args.size()) { - if (acceptsExtraArgs) { - ImmutableList.Builder<Object> extraArgsBuilder = - ImmutableList.builderWithExpectedSize(args.size() - argIndex); - for (; argIndex < args.size(); argIndex++) { - extraArgsBuilder.add(args.get(argIndex)); - } - extraArgs = extraArgsBuilder.build(); - } else { - throw argumentMismatchException( - call, - String.format( - "expected no more than %s positional arguments, but got %s", argIndex, args.size()), - method, - objClass); - } - } - ImmutableMap<String, Object> extraKwargs = ImmutableMap.of(); - if (!keys.isEmpty()) { - if (acceptsExtraKwargs) { - ImmutableMap.Builder<String, Object> extraKwargsBuilder = - ImmutableMap.builderWithExpectedSize(keys.size()); - for (String key : keys) { - extraKwargsBuilder.put(key, kwargs.get(key)); - } - extraKwargs = extraKwargsBuilder.build(); - } else { - throw unexpectedKeywordArgumentException(call, keys, method, objClass, thread); - } + // *args + if (method.isAcceptsExtraArgs()) { + builder.add(Tuple.copyOf(args.subList(argIndex, args.size()))); + } else if (argIndex < args.size()) { + throw argumentMismatchException( + call, + String.format( + "expected no more than %s positional arguments, but got %s", argIndex, args.size()), + method, + objClass); } - // Then add any skylark-interpreter arguments (for example kwargs or the StarlarkThread). - if (acceptsExtraArgs) { - builder.add(Tuple.copyOf(extraArgs)); + // **kwargs + if (method.isAcceptsExtraKwargs()) { + Dict<String, Object> dict = Dict.of(thread.mutability()); + for (String k : keys) { + dict.put(k, kwargs.get(k), (Location) null); + } + builder.add(dict); + } else if (!keys.isEmpty()) { + throw unexpectedKeywordArgumentException(call, keys, method, objClass, thread); } - if (acceptsExtraKwargs) { - builder.add(Dict.copyOf(thread.mutability(), extraKwargs)); - } + + // Add Location, FuncallExpression, and/or StarlarkThread. appendExtraInterpreterArgs(builder, method, call, call.getLocation(), thread); return builder.toArray();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java index 9382bd3..b133c39 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -18,7 +18,6 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.events.Location; import java.util.ArrayList; -import java.util.Collection; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -253,7 +252,7 @@ assignItem(object, key, value, loc); } else if (expr instanceof ListExpression) { ListExpression list = (ListExpression) expr; - assignList(list, value, thread, loc); + assignList(list.getElements(), value, thread, loc); } else { // Not possible for validated ASTs. throw new EvalException(loc, "cannot assign to '" + expr + "'"); @@ -299,25 +298,32 @@ * matching length */ private static void assignList( - ListExpression list, Object value, StarlarkThread thread, Location loc) + List<Expression> lhs, Object x, StarlarkThread thread, Location loc) throws EvalException, InterruptedException { - Collection<?> collection = EvalUtils.toCollection(value, loc); - int len = list.getElements().size(); + // TODO(adonovan): lock/unlock rhs during iteration so that + // assignments fail when the left side aliases the right, + // which is a tricky case in Python assignment semantics. + int nrhs = Starlark.len(x); + if (nrhs < 0) { + throw new EvalException(null, "type '" + EvalUtils.getDataTypeName(x) + "' is not iterable"); + } + Iterable<?> rhs = Starlark.toIterable(x); // fails if x is a string + int len = lhs.size(); if (len == 0) { throw new EvalException( loc, "lists or tuples on the left-hand side of assignments must have at least one item"); } - if (len != collection.size()) { + if (len != nrhs) { throw new EvalException( loc, String.format( "assignment length mismatch: left-hand side has length %d, but right-hand side" + " evaluates to value of length %d", - len, collection.size())); + len, nrhs)); } int i = 0; - for (Object item : collection) { - assign(list.getElements().get(i), item, thread, loc); + for (Object item : rhs) { + assign(lhs.get(i), item, thread, loc); i++; } } @@ -360,7 +366,7 @@ // TODO(b/141263526): following Python, allow list+=iterable (but not list+iterable). if (op == TokenKind.PLUS && x instanceof StarlarkList && y instanceof StarlarkList) { StarlarkList<?> list = (StarlarkList) x; - list.extend(y, location); + list.extend(y); return list; } return EvalUtils.binaryOp(op, x, y, thread, location);
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 dbea44e..6a90213 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
@@ -17,7 +17,6 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Lists; import com.google.common.collect.Ordering; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Location; @@ -25,7 +24,6 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.syntax.Concatable.Concatter; import com.google.devtools.build.lib.util.SpellChecker; -import java.util.Collection; import java.util.IllegalFormatException; import java.util.List; import java.util.Map; @@ -272,34 +270,6 @@ } } - public static Collection<?> toCollection(Object o, Location loc) throws EvalException { - if (o instanceof Collection) { - return (Collection<?>) o; - } else if (o instanceof Sequence) { - return ((Sequence) o).getImmutableList(); - } else if (o instanceof Map) { - // For dictionaries we iterate through the keys only - if (o instanceof Dict) { - // Dicts handle ordering themselves - Dict<?, ?> dict = (Dict) o; - List<Object> list = Lists.newArrayListWithCapacity(dict.size()); - for (Map.Entry<?, ?> entries : dict.entrySet()) { - list.add(entries.getKey()); - } - return ImmutableList.copyOf(list); - } - // For determinism, we sort the keys. - try { - return SKYLARK_COMPARATOR.sortedCopy(((Map<?, ?>) o).keySet()); - } catch (ComparisonException e) { - throw new EvalException(loc, e); - } - } else { - throw new EvalException(loc, - "type '" + getDataTypeName(o) + "' is not a collection"); - } - } - public static void lock(Object object, Location loc) { if (object instanceof StarlarkMutable) { StarlarkMutable x = (StarlarkMutable) object;
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 e85a250..93d5952 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
@@ -18,7 +18,6 @@ import com.google.common.base.Ascii; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Ordering; import com.google.devtools.build.lib.collect.nestedset.NestedSet.NestedSetDepthException; @@ -32,15 +31,12 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.EvalUtils.ComparisonException; import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier; -import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.NoSuchElementException; import java.util.Set; import java.util.TreeSet; @@ -59,13 +55,12 @@ + "<pre class=\"language-python\">min(2, 5, 4) == 2\n" + "min([5, 6, 3]) == 3</pre>", extraPositionals = - @Param(name = "args", type = Sequence.class, doc = "The elements to be checked."), - useLocation = true) - public Object min(Sequence<?> args, Location loc) throws EvalException { + @Param(name = "args", type = Sequence.class, doc = "The elements to be checked.")) + public Object min(Sequence<?> args) throws EvalException { try { - return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc); + return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse()); } catch (ComparisonException e) { - throw new EvalException(loc, e); + throw new EvalException(null, e); } } @@ -78,18 +73,17 @@ + "<pre class=\"language-python\">max(2, 5, 4) == 5\n" + "max([5, 6, 3]) == 6</pre>", extraPositionals = - @Param(name = "args", type = Sequence.class, doc = "The elements to be checked."), - useLocation = true) - public Object max(Sequence<?> args, Location loc) throws EvalException { + @Param(name = "args", type = Sequence.class, doc = "The elements to be checked.")) + public Object max(Sequence<?> args) throws EvalException { try { - return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc); + return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR); } catch (ComparisonException e) { - throw new EvalException(loc, e); + throw new EvalException(null, e); } } /** Returns the maximum element from this list, as determined by maxOrdering. */ - private static Object findExtreme(Sequence<?> args, Ordering<Object> maxOrdering, Location loc) + private static Object findExtreme(Sequence<?> args, Ordering<Object> maxOrdering) 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. @@ -97,7 +91,7 @@ Iterable<?> items = (args.size() == 1) ? Starlark.toIterable(args.get(0)) : args; return maxOrdering.max(items); } catch (NoSuchElementException ex) { - throw new EvalException(loc, "expected at least one item", ex); + throw new EvalException(null, "expected at least one item", ex); } } @@ -189,8 +183,7 @@ final Location loc, final StarlarkThread thread) throws EvalException, InterruptedException { - - Object[] array = EvalUtils.toCollection(iterable, loc).toArray(); + Object[] array = Starlark.toArray(iterable); if (key == Starlark.NONE) { try { Arrays.sort(array, EvalUtils.SKYLARK_COMPARATOR); @@ -240,15 +233,19 @@ } if (reverse) { - for (int i = 0, j = array.length - 1; i < j; i++, j--) { - Object tmp = array[i]; - array[i] = array[j]; - array[j] = tmp; - } + reverse(array); } return StarlarkList.wrap(thread.mutability(), array); } + private static void reverse(Object[] array) { + for (int i = 0, j = array.length - 1; i < j; i++, j--) { + Object tmp = array[i]; + array[i] = array[j]; + array[j] = tmp; + } + } + @SkylarkCallable( name = "reversed", doc = @@ -257,27 +254,23 @@ parameters = { @Param( name = "sequence", - type = Object.class, - doc = "The sequence to be reversed (string, list or tuple).", + type = Sequence.class, + doc = "The sequence (list or tuple) to be reversed.", // TODO(cparsons): This parameter should be positional-only. legacyNamed = true), }, useStarlarkThread = true) - public StarlarkList<?> reversed(Object sequence, StarlarkThread thread) throws EvalException { - if (sequence instanceof Dict) { - throw new EvalException(null, "Argument to reversed() must be a sequence, not a dictionary."); - } - ArrayDeque<Object> tmpList = new ArrayDeque<>(); - for (Object element : Starlark.toIterable(sequence)) { - tmpList.addFirst(element); - } - return StarlarkList.copyOf(thread.mutability(), tmpList); + public StarlarkList<?> reversed(Sequence<?> sequence, StarlarkThread thread) + throws EvalException { + Object[] array = Starlark.toArray(sequence); + reverse(array); + return StarlarkList.wrap(thread.mutability(), array); } @SkylarkCallable( name = "tuple", doc = - "Converts a collection (e.g. list, tuple or dictionary) to a tuple." + "Returns a tuple with the same elements as the given iterable value." + "<pre class=\"language-python\">tuple([1, 2]) == (1, 2)\n" + "tuple((2, 3, 2)) == (2, 3, 2)\n" + "tuple({5: \"a\", 2: \"b\", 4: \"c\"}) == (5, 2, 4)</pre>", @@ -288,16 +281,18 @@ doc = "The object to convert.", // TODO(cparsons): This parameter should be positional-only. legacyNamed = true) - }, - useLocation = true) - public Tuple<?> tuple(Object x, Location loc) throws EvalException { - return Tuple.copyOf(EvalUtils.toCollection(x, loc)); + }) + public Tuple<?> tuple(Object x) throws EvalException { + if (x instanceof Tuple) { + return (Tuple<?>) x; + } + return Tuple.wrap(Starlark.toArray(x)); } @SkylarkCallable( name = "list", doc = - "Converts a collection (e.g. list, tuple or dictionary) to a list." + "Returns a new list with the same elements as the given iterable value." + "<pre class=\"language-python\">list([1, 2]) == [1, 2]\n" + "list((2, 3, 2)) == [2, 3, 2]\n" + "list({5: \"a\", 2: \"b\", 4: \"c\"}) == [5, 2, 4]</pre>", @@ -312,7 +307,7 @@ useLocation = true, useStarlarkThread = true) public StarlarkList<?> list(Object x, Location loc, StarlarkThread thread) throws EvalException { - return StarlarkList.copyOf(thread.mutability(), EvalUtils.toCollection(x, loc)); + return StarlarkList.wrap(thread.mutability(), Starlark.toArray(x)); } @SkylarkCallable( @@ -328,18 +323,11 @@ useLocation = true, useStarlarkThread = true) public Integer len(Object x, Location loc, StarlarkThread thread) throws EvalException { - if (x instanceof String) { - return ((String) x).length(); - } else if (x instanceof Map) { - return ((Map<?, ?>) x).size(); - } else if (x instanceof Sequence) { - return ((Sequence<?>) x).size(); - } else if (x instanceof StarlarkIterable) { - // Iterables.size() checks if x is a Collection so it's efficient in that sense. - return Iterables.size((Iterable<?>) x); - } else { + int len = Starlark.len(x); + if (len < 0) { throw new EvalException(loc, EvalUtils.getDataTypeName(x) + " is not iterable"); } + return len; } @SkylarkCallable( @@ -617,12 +605,9 @@ useLocation = true) public StarlarkList<?> enumerate(Object input, Integer start, Location loc, StarlarkThread thread) throws EvalException { - Collection<?> src = EvalUtils.toCollection(input, loc); - Object[] array = new Object[src.size()]; - int i = 0; - for (Object x : src) { - array[i] = Tuple.pair(i + start, x); - i++; + Object[] array = Starlark.toArray(input); + for (int i = 0; i < array.length; i++) { + array[i] = Tuple.pair(i + start, array[i]); // update in place } return StarlarkList.wrap(thread.mutability(), array); } @@ -941,7 +926,7 @@ + "currently checked consistently in all constructors. Use the " + "--incompatible_always_check_depset_elements flag to enable " + "consistent checking; this will be the default behavior in future releases; " - + " see https://github.com/bazelbuild/bazel/issues/10313." + + " see <a href='https://github.com/bazelbuild/bazel/issues/10313'>Issue 10313</a>." + "" + "<p>In addition, elements must currently be immutable, though this restriction " + "will be relaxed in future."
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java b/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java index 6b99cea..b82d653 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
@@ -14,6 +14,7 @@ package com.google.devtools.build.lib.syntax; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; @@ -156,6 +157,40 @@ throw new EvalException(null, "type '" + EvalUtils.getDataTypeName(x) + "' is not iterable"); } + /** + * Returns a new array containing the elements of Starlark iterable value {@code x}. A Starlark + * value is iterable if it implements {@link StarlarkIterable}. + */ + public static Object[] toArray(Object x) throws EvalException { + // Specialize Sequence and Dict to avoid allocation and/or indirection. + if (x instanceof Sequence) { + return ((Sequence<?>) x).toArray(); + } else if (x instanceof Dict) { + return ((Dict<?, ?>) x).keySet().toArray(); + } else { + return Iterables.toArray(toIterable(x), Object.class); + } + } + + /** + * Returns the length of a legal Starlark value as if by the expression {@code len(x)}, or -1 if + * the value is not a string or iterable. + */ + public static int len(Object x) { + if (x instanceof String) { + return ((String) x).length(); + } else if (x instanceof Sequence) { + return ((Sequence) x).size(); + } else if (x instanceof Dict) { + return ((Dict) x).size(); + } else if (x instanceof StarlarkIterable) { + // Iterables.size() checks if x is a Collection so it's efficient in that sense. + return Iterables.size((Iterable<?>) x); + } else { + return -1; + } + } + /** Returns the string form of a value as if by the Starlark expression {@code str(x)}. */ public static String str(Object x) { return Printer.getPrinter().str(x).toString();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkList.java index dd150c2..33301d0 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkList.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkList.java
@@ -392,12 +392,11 @@ @SkylarkCallable( name = "extend", doc = "Adds all items to the end of the list.", - parameters = {@Param(name = "items", type = Object.class, doc = "Items to add at the end.")}, - useLocation = true) - public NoneType extend(Object items, Location loc) throws EvalException { + parameters = {@Param(name = "items", type = Object.class, doc = "Items to add at the end.")}) + public NoneType extend(Object items) throws EvalException { @SuppressWarnings("unchecked") - Collection<? extends E> src = (Collection<? extends E>) EvalUtils.toCollection(items, loc); - addAll(src, loc); + Iterable<? extends E> src = (Iterable<? extends E>) Starlark.toIterable(items); + addAll(src, (Location) null); return Starlark.NONE; }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java b/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java index e593cda..29fbe9c 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StringModule.java
@@ -25,7 +25,6 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.regex.Matcher; @@ -107,15 +106,14 @@ legacyNamed = true, type = Object.class, doc = "The objects to join.") - }, - useLocation = true) - public String join(String self, Object elements, Location loc) throws EvalException { - Collection<?> items = EvalUtils.toCollection(elements, loc); + }) + public String join(String self, Object elements) throws EvalException { + Iterable<?> items = Starlark.toIterable(elements); int i = 0; for (Object item : items) { if (!(item instanceof String)) { throw new EvalException( - loc, + null, String.format( "expected string for sequence element %d, got '%s'", i, EvalUtils.getDataTypeName(item))); @@ -479,28 +477,8 @@ } /** - * Wraps the stringPartition() method and converts its results and exceptions - * to the expected types. - * - * @param self The input string - * @param separator The string to split on - * @param forward A flag that controls whether the input string is split around - * the first ({@code true}) or last ({@code false}) occurrence of the separator. - * @param loc The location that is used for potential exceptions - * @return A list with three elements - */ - private static Tuple<String> partitionWrapper( - String self, String separator, boolean forward, Location loc) throws EvalException { - try { - return Tuple.copyOf(stringPartition(self, separator, forward)); - } catch (IllegalArgumentException ex) { - throw new EvalException(loc, ex); - } - } - - /** - * Splits the input string at the {first|last} occurrence of the given separator and returns the - * resulting partition as a three-tuple of Strings, contained in a {@code StarlarkList}. + * Splits the input string at the first/last occurrence of the given separator and returns the + * resulting partition as a three-tuple of Strings. * * <p>If the input string does not contain the separator, the tuple will consist of the original * input string and two empty strings. @@ -512,32 +490,40 @@ * @param separator The string to split on * @param forward A flag that controls whether the input string is split around the first ({@code * true}) or last ({@code false}) occurrence of the separator. - * @return A three-tuple (List) of the form [part_before_separator, separator, - * part_after_separator]. + * @param loc The location that is used for potential exceptions + * @return a 3-Tuple of the form (part_before_separator, separator, part_after_separator). */ - private static ImmutableList<String> stringPartition( - String input, String separator, boolean forward) { + private static Tuple<String> partitionWrapper( + String input, String separator, boolean forward, Location loc) throws EvalException { if (separator.isEmpty()) { - throw new IllegalArgumentException("Empty separator"); + throw new EvalException(loc, "empty separator"); } - int pos = forward ? input.indexOf(separator) : input.lastIndexOf(separator); + String a = ""; + String b = ""; + String c = ""; + int pos = forward ? input.indexOf(separator) : input.lastIndexOf(separator); if (pos < 0) { // Following Python's implementation of str.partition() and str.rpartition(), // the input string is copied to either the first or the last position in the // list, depending on the value of the forward flag. - return forward ? ImmutableList.of(input, "", "") : ImmutableList.of("", "", input); + if (forward) { + a = input; + } else { + c = input; + } } else { - return ImmutableList.of( - input.substring(0, pos), - separator, - // pos + sep.length() is at most equal to input.length(). This worst-case - // happens when the separator is at the end of the input string. However, - // substring() will return an empty string in this scenario, thus making - // any additional safety checks obsolete. - input.substring(pos + separator.length())); + a = input.substring(0, pos); + b = separator; + // pos + sep.length() is at most equal to input.length(). This worst-case + // happens when the separator is at the end of the input string. However, + // substring() will return an empty string in this scenario, thus making + // any additional safety checks obsolete. + c = input.substring(pos + separator.length()); } + + return Tuple.triple(a, b, c); } @SkylarkCallable(
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java index 2ff2eee..f4bd832 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -353,11 +353,12 @@ @Test public void testListComprehensionsMultipleVariablesFail() throws Exception { - newTest().testIfErrorContains( - "assignment length mismatch: left-hand side has length 3, but right-hand side evaluates to " - + "value of length 2", - "[x + y for x, y, z in [(1, 2), (3, 4)]]").testIfExactError( - "type 'int' is not a collection", "[x + y for x, y in (1, 2)]"); + newTest() + .testIfErrorContains( + "assignment length mismatch: left-hand side has length 3, but right-hand side" + + " evaluates to value of length 2", + "[x + y for x, y, z in [(1, 2), (3, 4)]]") + .testIfExactError("type 'int' is not iterable", "[x + y for x, y in (1, 2)]"); } @Test
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 91e6864..125cda7 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
@@ -534,7 +534,7 @@ @Test public void testEnumerateBadArg() throws Exception { - new BothModesTest().testIfErrorContains("type 'string' is not a collection", "enumerate('a')"); + new BothModesTest().testIfErrorContains("type 'string' is not iterable", "enumerate('a')"); } @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 7e938ef..13bb23a 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
@@ -1508,11 +1508,11 @@ @Test public void testSetIsNotIterable() throws Exception { new SkylarkTest() - .testIfErrorContains("not a collection", "list(depset(['a', 'b']))") + .testIfErrorContains("not iterable", "list(depset(['a', 'b']))") .testIfErrorContains("not iterable", "max(depset([1, 2, 3]))") .testIfErrorContains("not iterable", "1 in depset([1, 2, 3])") - .testIfErrorContains("not a collection", "sorted(depset(['a', 'b']))") - .testIfErrorContains("not a collection", "tuple(depset(['a', 'b']))") + .testIfErrorContains("not iterable", "sorted(depset(['a', 'b']))") + .testIfErrorContains("not iterable", "tuple(depset(['a', 'b']))") .testIfErrorContains("not iterable", "[x for x in depset()]") .testIfErrorContains("not iterable", "len(depset(['a']))"); } @@ -2035,9 +2035,7 @@ new SkylarkTest() .testIfErrorContains( - "type 'int' is not a collection", - "def bar (): return [x + y for x, y in (1, 2)]", - "bar()"); + "type 'int' is not iterable", "def bar (): return [x + y for x, y in (1, 2)]", "bar()"); new SkylarkTest() .testIfErrorContains( @@ -2046,7 +2044,7 @@ "[x + y for x, y, z in [(1, 2), (3, 4)]]"); new SkylarkTest() - .testIfErrorContains("type 'int' is not a collection", "[x2 + y2 for x2, y2 in (1, 2)]"); + .testIfErrorContains("type 'int' is not iterable", "[x2 + y2 for x2, y2 in (1, 2)]"); new SkylarkTest() // returns [2] in Python, it's an error in Skylark
diff --git a/src/test/starlark/testdata/list_mutation.sky b/src/test/starlark/testdata/list_mutation.sky index c27d749..e3321e0 100644 --- a/src/test/starlark/testdata/list_mutation.sky +++ b/src/test/starlark/testdata/list_mutation.sky
@@ -52,7 +52,7 @@ --- (1, 2).extend([3, 4]) ### type 'tuple' has no method extend() --- -[1, 2].extend(3) ### type 'int' is not a collection +[1, 2].extend(3) ### type 'int' is not iterable # remove @@ -102,4 +102,4 @@ foo.clear() assert_eq(foo, []) -assert_eq(['a', 'b'].clear(), None) \ No newline at end of file +assert_eq(['a', 'b'].clear(), None)
diff --git a/src/test/starlark/testdata/reversed.sky b/src/test/starlark/testdata/reversed.sky index bf14c68..9d07c40 100644 --- a/src/test/starlark/testdata/reversed.sky +++ b/src/test/starlark/testdata/reversed.sky
@@ -7,11 +7,11 @@ assert_eq(reversed('bbb'.elems()), ['b', 'b', 'b']) --- -reversed(None) ### parameter 'sequence' cannot be None, for call to function reversed(sequence) +reversed(None) ### expected value of type 'sequence' --- -reversed(1) ### type 'int' is not iterable +reversed(1) ### expected value of type 'sequence' --- -reversed({1: 3}) ### Argument to reversed() must be a sequence, not a dictionary +reversed({1: 3}) ### expected value of type 'sequence' --- x = ['a', 'b'] @@ -27,4 +27,3 @@ reverse_equivalence([]) reverse_equivalence([1]) reverse_equivalence(["a", "b"]) -
diff --git a/src/test/starlark/testdata/sorted.sky b/src/test/starlark/testdata/sorted.sky index d7fa3ab..1006e62 100644 --- a/src/test/starlark/testdata/sorted.sky +++ b/src/test/starlark/testdata/sorted.sky
@@ -28,7 +28,7 @@ [[1, 4, 2], [5, 2, 1], [2, 6, 1]]) --- -sorted(1) ### type 'int' is not a collection +sorted(1) ### type 'int' is not iterable --- sorted([1, 2, None, 3]) ### Cannot compare NoneType with int ---
diff --git a/src/test/starlark/testdata/string_misc.sky b/src/test/starlark/testdata/string_misc.sky index d339f38..edd1f07 100644 --- a/src/test/starlark/testdata/string_misc.sky +++ b/src/test/starlark/testdata/string_misc.sky
@@ -179,4 +179,4 @@ assert_eq(enumerate({}), []) assert_eq(enumerate([False, True, None], 42), [(42, False), (43, True), (44, None)]) --- -enumerate("ab") ### type 'string' is not a collection +enumerate("ab") ### type 'string' is not iterable
diff --git a/src/test/starlark/testdata/string_partition.sky b/src/test/starlark/testdata/string_partition.sky index 12f075b..9bf352d 100644 --- a/src/test/starlark/testdata/string_partition.sky +++ b/src/test/starlark/testdata/string_partition.sky
@@ -29,9 +29,9 @@ assert_eq('google'.rpartition('google'), ('', 'google', '')) --- -'google'.partition('') ### Empty separator +'google'.partition('') ### empty separator --- -'google'.rpartition('') ### Empty separator +'google'.rpartition('') ### empty separator --- 'google'.partition() ### parameter 'sep' has no default value ---