Rollback of commit 984d6d48d0e07ac3be2bbfec667158165390eb4f.

--
MOS_MIGRATED_REVID=140121290

6 files changed, 9 insertions, 34 deletions

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 2785b6486f..c83ed98793 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
@@ -72,9 +72,6 @@ public final class EvalUtils {
       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");
       }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 7ca5793cc9..5748f63bdb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -801,7 +801,7 @@ public final class FuncallExpression extends Expression {
     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 LinkedHashMap<>();
+    Map<String, Object> kwargs = new HashMap<>();
     BaseFunction function = checkCallable(funcValue, getLocation());
     evalArguments(posargs, kwargs, env);
     return function.call(posargs.build(), ImmutableMap.copyOf(kwargs), this, env);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
index cf371aff2c..6ae2ae5f2a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
@@ -18,8 +18,8 @@ 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.syntax.SkylarkMutable.MutableMap;
-import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.TreeMap;
 import javax.annotation.Nullable;
 
 /**
@@ -46,7 +46,7 @@ import javax.annotation.Nullable;
 public final class SkylarkDict<K, V>
     extends MutableMap<K, V> implements Map<K, V>, SkylarkIndexable {
 
-  private final LinkedHashMap<K, V> contents = new LinkedHashMap<>();
+  private final TreeMap<K, V> contents = new TreeMap<>(EvalUtils.SKYLARK_COMPARATOR);
 
   private final Mutability mutability;
 
@@ -139,7 +139,7 @@ public final class SkylarkDict<K, V>
 
   /** @return the first key in the dict */
   K firstKey() {
-    return contents.entrySet().iterator().next().getKey();
+    return contents.firstKey();
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index b5cb1f652e..17f5484e29 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -810,14 +810,6 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   }
 
   @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");
     assertThat(lookup("x1")).isEqualTo(1);
@@ -942,17 +934,6 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase {
   }
 
   @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");
-
-    checkErrorContains(
-        "unhashable type: 'struct'",
-        "{struct(a = []): 'foo'}");
-  }
-
-  @Test
   public void testStructMembersAreImmutable() throws Exception {
     checkErrorContains(
         "cannot assign to 's.x'",
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 75e42eb312..fa72014247 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
@@ -272,10 +272,7 @@ public class EvaluationTest extends EvaluationTestCase {
         .update(kwargs.getName(), kwargs)
         .testEval(
             "kwargs(foo=1, bar='bar', wiz=[1,2,3]).items()",
-            "[('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)]");
+            "[('bar', 'bar'), ('foo', 1), ('wiz', [1, 2, 3])]");
   }
 
   @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 d416c304d0..ea81adb35d 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
@@ -1311,8 +1311,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
     new BothModesTest()
         .testEval("{1: 'foo'}.values()", "['foo']")
         .testEval("{}.values()", "[]")
-        .testEval("{True: 3, False: 5}.values()", "[3, 5]")
-        .testEval("{'a': 5, 'c': 2, 'b': 4, 'd': 3}.values()", "[5, 2, 4, 3]");
+        .testEval("{True: 3, False: 5}.values()", "[5, 3]")
+        .testEval("{'a': 5, 'c': 2, 'b': 4, 'd': 3}.values()", "[5, 4, 2, 3]");
     // sorted by keys
   }
 
@@ -1342,7 +1342,7 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .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), ('c', 2), ('b', 4)]");
+        .testEval("{'a': 5, 'c': 2, 'b': 4}.items()", "[('a', 5), ('b', 4), ('c', 2)]");
   }
 
   @Test
@@ -1378,9 +1378,9 @@ public class MethodLibraryTest extends EvaluationTestCase {
             "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()");
   }