Changes related to the order of Skylark dictionaries:

- Objects of different types can now be compared. 
- Printer now prints dictionaries in a deterministic order, even when the keys have different types.
- testEval() in EvaluationTestCases evaluates both expressions instead of comparing expression strings. Consequently, if a statement describes a collection, its order does no longer matter when doing the comparison.

--
MOS_MIGRATED_REVID=99829458

5 files changed, 40 insertions, 18 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
index fa98c70e3d..9445aced22 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -28,6 +29,7 @@ import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.TreeMap;
 
 /**
  *  Test properties of the evaluator's datatypes and utility functions
@@ -78,4 +80,24 @@ public class EvalUtilsTest {
     assertFalse(EvalUtils.isImmutable(makeDict()));
     assertFalse(EvalUtils.isImmutable(makeFilesetEntry()));
   }
+
+  @Test
+  public void testComparatorWithDifferentTypes() throws Exception {
+    TreeMap<Object, Object> map = new TreeMap<>(EvalUtils.SKYLARK_COMPARATOR);
+    map.put(2, 3);
+    map.put("1", 5);
+    map.put(42, 4);
+    map.put("test", 7);
+    map.put(-1, 2);
+    map.put("4", 6);
+    map.put(2.0, 1);
+    map.put(Environment.NONE, 0);
+
+    int expected = 0;
+    // Expected order of keys is NoneType -> Double -> Integers -> Strings
+    for (Object obj : map.values()) {
+      assertThat(obj).isEqualTo(expected);
+      ++expected;
+    }
+  }
 }
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 9b0ce0b347..14988cbac0 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
@@ -218,15 +218,7 @@ public class EvaluationTest extends EvaluationTestCase {
         .testStatement("('a', 'b') <= ('a', 'b')", true)
 
         .testStatement("[[1, 1]] > [[1, 1], []]", false)
-        .testStatement("[[1, 1]] < [[1, 1], []]", true)
-
-        .testIfExactError("Cannot compare int with string", "[1] < ['a']")
-        .testIfExactError("Cannot compare list with int", "[1] < 1");
-  }
-
-  @Test
-  public void testCompareStringInt() throws Exception {
-    newTest().testIfExactError("Cannot compare string with int", "'a' >= 1");
+        .testStatement("[[1, 1]] < [[1, 1], []]", true);
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java
index 692a338e52..7be1c86545 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTestCase.java
@@ -367,13 +367,15 @@ public class EvaluationTestCase {
         @Override
         public void run() throws Exception {
           Object actual = eval(statement);
+          Object realExpected = expected;
 
-          // Prints the actual object instead of evaluating the expected expression
+          // We could also print the actual object and compare the string to the expected
+          // expression, but then the order of elements would matter.
           if (expectedIsExpression) {
-            actual = Printer.repr(actual, '\'');
+            realExpected = eval((String) expected);
           }
 
-          assertThat(actual).isEqualTo(expected);
+          assertThat(actual).isEqualTo(realExpected);
         }
       };
     }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
index 3dbcfb1e9a..baa1fcb847 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
@@ -27,6 +27,7 @@ import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.IllegalFormatException;
 import java.util.List;
 import java.util.Map;
@@ -109,6 +110,17 @@ public class PrinterTest {
   }
 
   @Test
+  public void testSortedOutputOfUnsortedMap() throws Exception {
+    Map<Integer, Integer> map = new HashMap<>();
+    int[] data = {5, 7, 3};
+
+    for (int current : data) {
+      map.put(current, current);
+    }
+    assertThat(Printer.str(map)).isEqualTo("{3: 3, 5: 5, 7: 7}");
+  }
+
+  @Test
   public void testFormatPositional() throws Exception {
     assertEquals("foo 3", Printer.formatString("%s %d", makeTuple("foo", 3)));
     assertEquals("foo 3", Printer.format("%s %d", "foo", 3));
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 f43232a2ca..744e6db31b 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
@@ -981,12 +981,6 @@ public class SkylarkEvaluationTest extends EvaluationTest {
 
   @Override
   @Test
-  public void testCompareStringInt() throws Exception {
-    new SkylarkTest().testIfExactError("Cannot compare string with int", "'a' >= 1");
-  }
-
-  @Override
-  @Test
   public void testListComprehensionsMultipleVariablesFail() throws Exception {
     new SkylarkTest().testIfExactError("lvalue has length 3, but rvalue has has length 2",
         "def foo (): return [x + y for x, y, z in [(1, 2), (3, 4)]]",