Repository: bazelbuild/bazel
Issue: enumerate, join, extend functions not accepting dicts
Labels: type: feature request
P3

### Description of the problem:

When trying to pass a `dict` object to any of the Starlark functions `enumerate`, `string.join` or `list.extend`, the error `expected value of type 'sequence' for parameter ...` is thrown.

According to the spec:
- A `dict` is both an `Iterable` and a `Sequence`. (see [sequence types](https://github.com/bazelbuild/starlark/blob/master/spec.md#sequence-types))
- The arguments of [`enumerate`](https://github.com/bazelbuild/starlark/blob/master/spec.md#enumerate), [`extend`](https://github.com/bazelbuild/starlark/blob/master/spec.md#listextend) and [`join`](https://github.com/bazelbuild/starlark/blob/master/spec.md#stringjoin) are iterable sequences.

Also, both Python and the Go implementation accept dictionaries being passed to the above functions.

Issue comments:

Pull request review comments:
That cast isn't sound, and it should result in an "unchecked" warning. Do you know how (and why) these are suppressed in the Bazel codebase? They should really be treated as errors.
I am not sure actually why they are suppressed. I removed the cast anyway.
This incurs an unnecessary copy that was not present in the old code. Essentially the problem has been pushed to 'new ArrayList', which has a raw type, which ought to be a compile error if our build was correctly configured. Let's revert to the previous version.

(I think the right solution is to abolish the type parameter in StarlarkList, so that it extends `Collection<Object>`. If you pass a `StarlarkList<X>` to the interpreter, you have no guarantee that it doesn't insert objects of class Y into the list. The only reason this doesn't happen today is that Blaze never passes mutable lists (other than of `<Object>)` to Starlark functions, but an application that did would start to see very confusing crashes. 
But that's a much bigger change.)
fyi, I've merged this PR using Friday's state (without the unnecessary copy, but with the cast).

Patch that resolved the issue:
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 62fe34b901f259..830ca5672e75fe 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
@@ -605,12 +605,12 @@ private String getIntegerPrefix(String value) {
       name = "enumerate",
       doc =
           "Returns a list of pairs (two-element tuples), with the index (int) and the item from"
-              + " the input list.\n<pre class=\"language-python\">"
+              + " the input sequence.\n<pre class=\"language-python\">"
               + "enumerate([24, 21, 84]) == [(0, 24), (1, 21), (2, 84)]</pre>\n",
       parameters = {
         // Note Python uses 'sequence' keyword instead of 'list'. We may want to change tihs
         // some day.
-        @Param(name = "list", type = SkylarkList.class, doc = "input list.", named = true),
+        @Param(name = "list", type = Object.class, doc = "input sequence.", named = true),
         @Param(
             name = "start",
             type = Integer.class,
@@ -618,12 +618,13 @@ private String getIntegerPrefix(String value) {
             defaultValue = "0",
             named = true)
       },
-      useEnvironment = true)
-  public MutableList<?> enumerate(SkylarkList<?> input, Integer start, Environment env)
+      useEnvironment = true,
+      useLocation = true)
+  public MutableList<?> enumerate(Object input, Integer start, Location loc, Environment env)
       throws EvalException {
     int count = start;
-    ArrayList<SkylarkList<?>> result = new ArrayList<>(input.size());
-    for (Object obj : input) {
+    ArrayList<SkylarkList<?>> result = new ArrayList<>();
+    for (Object obj : EvalUtils.toCollection(input, loc, env)) {
       result.add(Tuple.of(count, obj));
       count++;
     }
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 fe1ffc3e5f96f3..686ed3d7f9a753 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
@@ -28,6 +28,7 @@
 import com.google.devtools.build.lib.syntax.StarlarkMutable.BaseMutableList;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Collection;
 import java.util.List;
 import java.util.RandomAccess;
 import javax.annotation.Nullable;
@@ -521,15 +522,15 @@ public Runtime.NoneType insert(
       name = "extend",
       doc = "Adds all items to the end of the list.",
       parameters = {
-          @Param(name = "items", type = SkylarkList.class, doc = "Items to add at the end.")
+          @Param(name = "items", type = Object.class, doc = "Items to add at the end.")
       },
       useLocation = true,
       useEnvironment = true
     )
     public Runtime.NoneType extend(
-        SkylarkList<E> items, Location loc, Environment env)
+        Object items, Location loc, Environment env)
         throws EvalException {
-      addAll(items, loc, env.mutability());
+      addAll(new ArrayList(EvalUtils.toCollection(items, loc, env)), loc, env.mutability());
       return Runtime.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 97255220e0a42e..f274d03f370ef1 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
@@ -32,6 +32,7 @@
 import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.Collection;
 
 /**
  * Skylark String module.
@@ -103,15 +104,16 @@ private static String pythonSubstring(String str, int start, Object end, String
             name = "elements",
             // TODO(cparsons): This parameter should be positional-only.
             legacyNamed = true,
-            type = SkylarkList.class,
+            type = Object.class,
             doc = "The objects to join.")
       },
       useLocation = true,
       useEnvironment = true)
-  public String join(String self, SkylarkList<?> elements, Location loc, Environment env)
+  public String join(String self, Object elements, Location loc, Environment env)
       throws ConversionException, EvalException {
+    Collection<?> items = EvalUtils.toCollection(elements, loc, env);
     if (env.getSemantics().incompatibleStringJoinRequiresStrings()) {
-      for (Object item : elements) {
+      for (Object item : items) {
         if (!(item instanceof String)) {
           throw new EvalException(
               loc,
@@ -122,7 +124,7 @@ public String join(String self, SkylarkList<?> elements, Location loc, Environme
         }
       }
     }
-    return Joiner.on(self).join(elements);
+    return Joiner.on(self).join(items);
   }
 
   @SkylarkCallable(


Test patch:
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 62875a76b6ef09..7f024d79ebcd57 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
@@ -535,8 +535,7 @@ public void testEnumerate() throws Exception {
   public void testEnumerateBadArg() throws Exception {
     new BothModesTest()
         .testIfErrorContains(
-            "expected value of type 'sequence' for parameter 'list', "
-                + "for call to function enumerate(list, start = 0)",
+            "type 'string' is not a collection",
             "enumerate('a')");
   }
 
diff --git a/src/test/starlark/testdata/list_mutation.sky b/src/test/starlark/testdata/list_mutation.sky
index 3dbcf7e4bd4989..c27d74938f3d06 100644
--- a/src/test/starlark/testdata/list_mutation.sky
+++ b/src/test/starlark/testdata/list_mutation.sky
@@ -46,12 +46,13 @@ assert_eq(foo, ['a', 'b', 'c', 'd'])
 foo = ['a', 'b']
 foo.extend(['c', 'd'])
 foo.extend(('e', 'f'))
-assert_eq(foo, ['a', 'b', 'c', 'd', 'e', 'f'])
+foo.extend({'g': None})
+assert_eq(foo, ['a', 'b', 'c', 'd', 'e', 'f', 'g'])
 
 ---
 (1, 2).extend([3, 4]) ### type 'tuple' has no method extend()
 ---
-[1, 2].extend(3) ### expected value of type 'sequence' for parameter 'items'
+[1, 2].extend(3) ### type 'int' is not a collection
 
 # remove
 
diff --git a/src/test/starlark/testdata/string_misc.sky b/src/test/starlark/testdata/string_misc.sky
index 398e263ef7ccce..d80187a9925381 100644
--- a/src/test/starlark/testdata/string_misc.sky
+++ b/src/test/starlark/testdata/string_misc.sky
@@ -1,5 +1,6 @@
 # join
 assert_eq('-'.join(['a', 'b', 'c']), "a-b-c")
+assert_eq('-'.join({'a': 0, 'b': None, 'c': True}), "a-b-c")
 
 ---
 join(' ', ['a', 'b', 'c']) ### name 'join' is not defined
@@ -162,6 +163,8 @@ assert_eq("\"", '"')
 # enumerate
 
 assert_eq(enumerate("abc".elems()), [(0, "a"), (1, "b"), (2, "c")])
+assert_eq(enumerate({'a': 0, 2: 1, 'ab': 3}), [(0, 'a'), (1, 2), (2, 'ab')])
+assert_eq(enumerate({}), [])
 assert_eq(enumerate([False, True, None], 42), [(42, False), (43, True), (44, None)])
 ---
-enumerate("ab") ### expected value of type 'sequence'
+enumerate("ab") ### type 'string' is not a collection