Allow dicts to contain non-comparable objects as keys

RELNOTES: Skylark dicts internally don't rely on keys order anymore and accept
any hashable values (i.e. structs with immutable values) as keys. Iteration order of dictionaries is no longer specified.

--
MOS_MIGRATED_REVID=140591710

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -1059,9 +1059,7 @@ static SkylarkDict<String, SkylarkDict<String, Object>> callGetRulesFunction(
     Collection<Target> targets = context.pkgBuilder.getTargets();
     Location loc = ast.getLocation();
 
-    // Sort by name.
-    SkylarkDict<String, SkylarkDict<String, Object>> rules =
-        SkylarkDict.<String, SkylarkDict<String, Object>>of(env);
+    SkylarkDict<String, SkylarkDict<String, Object>> rules = SkylarkDict.of(env);
     for (Target t : targets) {
       if (t instanceof Rule) {
         SkylarkDict<String, Object> m = targetDict(t, loc, env);

src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java

@@ -16,6 +16,7 @@
 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;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -72,6 +73,9 @@ public int compare(Object o1, Object o2) {
       o1 = SkylarkType.convertToSkylark(o1, /*env=*/ null);
       o2 = SkylarkType.convertToSkylark(o2, /*env=*/ null);
 
+      if (o1 instanceof ClassObject && o2 instanceof ClassObject) {
+        throw new ComparisonException("Cannot compare structs");
+      }
       if (o1 instanceof SkylarkNestedSet && o2 instanceof SkylarkNestedSet) {
         throw new ComparisonException("Cannot compare sets");
       }
@@ -312,6 +316,15 @@ public static Collection<?> toCollection(Object o, Location loc) throws EvalExce
       return ((SkylarkList) o).getImmutableList();
     } else if (o instanceof Map) {
       // For dictionaries we iterate through the keys only
+      if (o instanceof SkylarkDict) {
+        // SkylarkDicts handle ordering themselves
+        SkylarkDict<?, ?> dict = (SkylarkDict) 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());

src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java

@@ -801,7 +801,7 @@ private Object callFunction(Object funcValue, Environment env)
     ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>();
     // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or
     // we'd still have to have a HashMap on the side for the sake of properly handling duplicates.
-    Map<String, Object> kwargs = new HashMap<>();
+    Map<String, Object> kwargs = new LinkedHashMap<>();
     BaseFunction function = checkCallable(funcValue, getLocation());
     evalArguments(posargs, kwargs, env);
     return function.call(posargs.build(), ImmutableMap.copyOf(kwargs), this, env);

src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java

@@ -1432,9 +1432,9 @@ public Runtime.NoneType invoke(
     objectType = SkylarkDict.class,
     returnType = MutableList.class,
     doc =
-        "Returns the list of values. Dictionaries are always sorted by their keys:"
+        "Returns the list of values:"
             + "<pre class=\"language-python\">"
-            + "{2: \"a\", 4: \"b\", 1: \"c\"}.values() == [\"c\", \"a\", \"b\"]</pre>\n",
+            + "{2: \"a\", 4: \"b\", 1: \"c\"}.values() == [\"a\", \"b\", \"c\"]</pre>\n",
     parameters = {@Param(name = "self", type = SkylarkDict.class, doc = "This dict.")},
     useEnvironment = true
   )
@@ -1450,9 +1450,9 @@ public MutableList<?> invoke(SkylarkDict<?, ?> self, Environment env) throws Eva
     objectType = SkylarkDict.class,
     returnType = MutableList.class,
     doc =
-        "Returns the list of key-value tuples. Dictionaries are always sorted by their keys:"
+        "Returns the list of key-value tuples:"
             + "<pre class=\"language-python\">"
-            + "{2: \"a\", 4: \"b\", 1: \"c\"}.items() == [(1, \"c\"), (2, \"a\"), (4, \"b\")]"
+            + "{2: \"a\", 4: \"b\", 1: \"c\"}.items() == [(2, \"a\"), (4, \"b\"), (1, \"c\")]"
             + "</pre>\n",
     parameters = {@Param(name = "self", type = SkylarkDict.class, doc = "This dict.")},
     useEnvironment = true
@@ -1470,8 +1470,8 @@ public MutableList<?> invoke(SkylarkDict<?, ?> self, Environment env) throws Eva
 
   @SkylarkSignature(name = "keys", objectType = SkylarkDict.class,
       returnType = MutableList.class,
-      doc = "Returns the list of keys. Dictionaries are always sorted by their keys:"
-          + "<pre class=\"language-python\">{2: \"a\", 4: \"b\", 1: \"c\"}.keys() == [1, 2, 4]"
+      doc = "Returns the list of keys:"
+          + "<pre class=\"language-python\">{2: \"a\", 4: \"b\", 1: \"c\"}.keys() == [2, 4, 1]"
           + "</pre>\n",
       parameters = {
         @Param(name = "self", type = SkylarkDict.class, doc = "This dict.")},
@@ -1482,9 +1482,11 @@ public MutableList<?> invoke(SkylarkDict<?, ?> self, Environment env) throws Eva
     @SuppressWarnings("unchecked")
     public MutableList<?> invoke(SkylarkDict<?, ?> self,
         Environment env) throws EvalException {
-      return new MutableList(
-          Ordering.natural().sortedCopy((Set<Comparable<?>>) (Set<?>) self.keySet()),
-          env);
+      List<Object> list = Lists.newArrayListWithCapacity(self.size());
+      for (Map.Entry<?, ?> entries : self.entrySet()) {
+        list.add(entries.getKey());
+      }
+      return new MutableList(list, env);
     }
   };
 

src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java

@@ -18,8 +18,8 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.syntax.SkylarkMutable.MutableMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.TreeMap;
 import javax.annotation.Nullable;
 
 /**
@@ -38,15 +38,15 @@
     + "d = {\"a\" : 1} + {\"b\" : 2}   # d == {\"a\" : 1, \"b\" : 2}\n"
     + "d += {\"c\" : 3}              # d == {\"a\" : 1, \"b\" : 2, \"c\" : 3}\n"
     + "d = d + {\"c\" : 5}           # d == {\"a\" : 1, \"b\" : 2, \"c\" : 5}</pre>"
-    + "Iterating on a dict is equivalent to iterating on its keys (in sorted order).<br>"
+    + "Iterating on a dict is equivalent to iterating on its keys (order is not specified).<br>"
     + "Dicts support the <code>in</code> operator, testing membership in the keyset of the dict. "
     + "Example:<br>"
     + "<pre class=\"language-python\">\"a\" in {\"a\" : 2, \"b\" : 5}   # evaluates as True"
     + "</pre>")
 public final class SkylarkDict<K, V>
     extends MutableMap<K, V> implements Map<K, V>, SkylarkIndexable {
 
-  private final TreeMap<K, V> contents = new TreeMap<>(EvalUtils.SKYLARK_COMPARATOR);
+  private final LinkedHashMap<K, V> contents = new LinkedHashMap<>();
 
   private final Mutability mutability;
 
@@ -139,7 +139,7 @@ public <KK extends K, VV extends V> void putAll(Map<KK, VV> m, Location loc, Env
 
   /** @return the first key in the dict */
   K firstKey() {
-    return contents.firstKey();
+    return contents.entrySet().iterator().next().getKey();
   }
 
   /**

src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java

@@ -656,6 +656,11 @@ public void testSimpleTextMessages() throws Exception {
         "struct(a=struct(b=struct(c='c'))).to_proto()", "a {", "  b {", "    c: \"c\"", "  }", "}");
   }
 
+  @Test
+  public void testProtoFieldsOrder() throws Exception {
+    checkTextMessage("struct(d=4, b=2, c=3, a=1).to_proto()", "a: 1", "b: 2", "c: 3", "d: 4");
+  }
+
   @Test
   public void testTextMessageEscapes() throws Exception {
     checkTextMessage("struct(name='a\"b').to_proto()", "name: \"a\\\"b\"");
@@ -809,6 +814,14 @@ public void testStructEquality() throws Exception {
     assertTrue((Boolean) eval("foo(a = 1) == foo(a = 1)"));
   }
 
+  @Test
+  public void testStructIncomparability() throws Exception {
+    checkErrorContains("Cannot compare structs", "struct(a = 1) < struct(a = 2)");
+    checkErrorContains("Cannot compare structs", "struct(a = 1) > struct(a = 2)");
+    checkErrorContains("Cannot compare structs", "struct(a = 1) <= struct(a = 2)");
+    checkErrorContains("Cannot compare structs", "struct(a = 1) >= struct(a = 2)");
+  }
+
   @Test
   public void testStructAccessingFieldsFromSkylark() throws Exception {
     eval("x = struct(a = 1, b = 2)", "x1 = x.a", "x2 = x.b");
@@ -933,6 +946,18 @@ public void testStructsInSets() throws Exception {
     eval("set([struct(a='a')])");
   }
 
+  @Test
+  public void testStructsInDicts() throws Exception {
+    eval("d = {struct(a = 1): 'aa', struct(b = 2): 'bb'}");
+    assertThat(eval("d[struct(a = 1)]")).isEqualTo("aa");
+    assertThat(eval("d[struct(b = 2)]")).isEqualTo("bb");
+    assertThat(eval("str([d[k] for k in d])")).isEqualTo("[\"aa\", \"bb\"]");
+
+    checkErrorContains(
+        "unhashable type: 'struct'",
+        "{struct(a = []): 'foo'}");
+  }
+
   @Test
   public void testStructMembersAreImmutable() throws Exception {
     checkErrorContains(

src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java

@@ -547,27 +547,27 @@ public void testGetRule() throws Exception {
     assertEquals(
         SkylarkList.createImmutable(
             ImmutableList.<String>of(
-                "cmd",
-                "compatible_with",
-                "executable",
-                "features",
+                "name",
+                "visibility",
+                "tags",
+                "generator_name",
                 "generator_function",
                 "generator_location",
-                "generator_name",
-                "heuristic_label_expansion",
-                "kind",
-                "local",
-                "message",
-                "name",
-                "output_to_bindir",
-                "outs",
+                "features",
+                "compatible_with",
                 "restricted_to",
                 "srcs",
-                "stamp",
-                "tags",
-                "toolchains",
                 "tools",
-                "visibility")),
+                "toolchains",
+                "outs",
+                "cmd",
+                "output_to_bindir",
+                "local",
+                "message",
+                "executable",
+                "stamp",
+                "heuristic_label_expansion",
+                "kind")),
         result);
   }
 

src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java

@@ -146,7 +146,11 @@ public void testEquality() throws Exception {
         .testStatement("'hello' == 'bye'", false)
         .testStatement("None == None", true)
         .testStatement("[1, 2] == [1, 2]", true)
-        .testStatement("[1, 2] == [2, 1]", false);
+        .testStatement("[1, 2] == [2, 1]", false)
+        .testStatement("{'a': 1, 'b': 2} == {'b': 2, 'a': 1}", true)
+        .testStatement("{'a': 1, 'b': 2} == {'a': 1}", false)
+        .testStatement("{'a': 1, 'b': 2} == {'a': 1, 'b': 2, 'c': 3}", false)
+        .testStatement("{'a': 1, 'b': 2} == {'a': 1, 'b': 3}", false);
   }
 
   @Test
@@ -157,7 +161,11 @@ public void testInequality() throws Exception {
         .testStatement("'hello' != 'hel' + 'lo'", false)
         .testStatement("'hello' != 'bye'", true)
         .testStatement("[1, 2] != [1, 2]", false)
-        .testStatement("[1, 2] != [2, 1]", true);
+        .testStatement("[1, 2] != [2, 1]", true)
+        .testStatement("{'a': 1, 'b': 2} != {'b': 2, 'a': 1}", false)
+        .testStatement("{'a': 1, 'b': 2} != {'a': 1}", true)
+        .testStatement("{'a': 1, 'b': 2} != {'a': 1, 'b': 2, 'c': 3}", true)
+        .testStatement("{'a': 1, 'b': 2} != {'a': 1, 'b': 3}", true);
   }
 
   @Test
@@ -272,7 +280,10 @@ public Object call(List<Object> args,
         .update(kwargs.getName(), kwargs)
         .testEval(
             "kwargs(foo=1, bar='bar', wiz=[1,2,3]).items()",
-            "[('bar', 'bar'), ('foo', 1), ('wiz', [1, 2, 3])]");
+            "[('foo', 1), ('bar', 'bar'), ('wiz', [1, 2, 3])]")
+        .testEval(
+            "kwargs(wiz=[1,2,3], bar='bar', foo=1).items()",
+            "[('wiz', [1, 2, 3]), ('bar', 'bar'), ('foo', 1)]");
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java

@@ -1311,8 +1311,8 @@ public void testDictionaryValues() throws Exception {
     new BothModesTest()
         .testEval("{1: 'foo'}.values()", "['foo']")
         .testEval("{}.values()", "[]")
-        .testEval("{True: 3, False: 5}.values()", "[5, 3]")
-        .testEval("{'a': 5, 'c': 2, 'b': 4, 'd': 3}.values()", "[5, 4, 2, 3]");
+        .testEval("{True: 3, False: 5}.values()", "[3, 5]")
+        .testEval("{'a': 5, 'c': 2, 'b': 4, 'd': 3}.values()", "[5, 2, 4, 3]");
     // sorted by keys
   }
 
@@ -1321,9 +1321,9 @@ public void testDictionaryKeys() throws Exception {
     new BothModesTest()
         .testEval("{1: 'foo'}.keys()", "[1]")
         .testEval("{}.keys()", "[]")
-        .testEval("{True: 3, False: 5}.keys()", "[False, True]")
+        .testEval("{True: 3, False: 5}.keys()", "[True, False]")
         .testEval(
-            "{1:'a', 2:'b', 6:'c', 0:'d', 5:'e', 4:'f', 3:'g'}.keys()", "[0, 1, 2, 3, 4, 5, 6]");
+            "{1:'a', 2:'b', 6:'c', 0:'d', 5:'e', 4:'f', 3:'g'}.keys()", "[1, 2, 6, 0, 5, 4, 3]");
   }
 
   @Test
@@ -1342,7 +1342,7 @@ public void testDictionaryItems() throws Exception {
         .testEval("{'a': 'foo'}.items()", "[('a', 'foo')]")
         .testEval("{}.items()", "[]")
         .testEval("{1: 3, 2: 5}.items()", "[(1, 3), (2, 5)]")
-        .testEval("{'a': 5, 'c': 2, 'b': 4}.items()", "[('a', 5), ('b', 4), ('c', 2)]");
+        .testEval("{'a': 5, 'c': 2, 'b': 4}.items()", "[('a', 5), ('c', 2), ('b', 4)]");
   }
 
   @Test
@@ -1378,9 +1378,9 @@ public void testDictionaryPopItem() throws Exception {
             "popitem(): dictionary is empty",
             "d = {2: 'bar', 3: 'baz', 1: 'foo'}\n"
                 + "if len(d) != 3: fail('popitem 0')\n"
-                + "if d.popitem() != (1, 'foo'): fail('popitem 1')\n"
                 + "if d.popitem() != (2, 'bar'): fail('popitem 2')\n"
                 + "if d.popitem() != (3, 'baz'): fail('popitem 3')\n"
+                + "if d.popitem() != (1, 'foo'): fail('popitem 1')\n"
                 + "if d != {}: fail('popitem 4')\n"
                 + "d.popitem()");
   }

src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java

@@ -919,7 +919,7 @@ public void testDictComprehensions_IterationOrder() throws Exception {
         "  for a in d:",
         "    s += a",
         "  return s",
-        "s = foo()").testLookup("s", "abc");
+        "s = foo()").testLookup("s", "cab");
   }
 
   @Test