Disallow comparison of objects of different types in Skylark

RELNOTES[INC]: It's not allowed anymore to compare objects of different types
(i.e. a string to an integer) and objects for which comparison rules are not
defined (i.e. a dict to another dict) using order operators.

--
PiperOrigin-RevId: 147710942
MOS_MIGRATED_REVID=147710942

2 files changed, 46 insertions, 24 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 59daa250ff..e0b48b8e4b 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
@@ -22,11 +22,13 @@ import static org.junit.Assert.assertTrue;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.packages.SkylarkClassObjectConstructor;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.syntax.EvalUtils.ComparisonException;
 import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
 import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
-import java.util.TreeMap;
+import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -115,21 +117,43 @@ public class EvalUtilsTest extends EvaluationTestCase {
 
   @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(true, 1);
-    map.put(Runtime.NONE, 0);
-
-    int expected = 0;
-    // Expected order of keys is NoneType -> Double -> Integers -> Strings
-    for (Object obj : map.values()) {
-      assertThat(obj).isEqualTo(expected);
-      ++expected;
+    Object[] objects = {
+        "1",
+        2,
+        true,
+        Runtime.NONE,
+        SkylarkList.Tuple.of(1, 2, 3),
+        SkylarkList.Tuple.of("1", "2", "3"),
+        SkylarkList.MutableList.of(env, 1, 2, 3),
+        SkylarkList.MutableList.of(env, "1", "2", "3"),
+        SkylarkDict.of(env, "key", 123),
+        SkylarkDict.of(env, 123, "value"),
+        NestedSetBuilder.stableOrder().add(1).add(2).add(3).build(),
+        SkylarkClassObjectConstructor.STRUCT.create(
+            ImmutableMap.of("key", (Object) "value"), "no field %s"),
+    };
+
+    for (int i = 0; i < objects.length; ++i) {
+      for (int j = 0; j < objects.length; ++j) {
+        if (i != j) {
+          try {
+            EvalUtils.SKYLARK_COMPARATOR.compare(objects[i], objects[j]);
+            Assert.fail("Shouldn't have compared different types");
+          } catch (ComparisonException e) {
+            // expected
+          }
+        }
+      }
+    }
+  }
+
+  @Test
+  public void testComparatorWithNones() throws Exception {
+    try {
+      EvalUtils.SKYLARK_COMPARATOR.compare(Runtime.NONE, Runtime.NONE);
+      Assert.fail("Shouldn't have compared nones");
+    } catch (ComparisonException e) {
+      // expected
     }
   }
 }
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 31810415f4..1d93eb92b0 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
@@ -89,9 +89,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
   @Test
   public void testMinWithDifferentTypes() throws Exception {
     new BothModesTest()
-        .testStatement("min(1, '2', True)", true)
-        .testStatement("min([1, '2', True])", true)
-        .testStatement("min(None, 1, 'test')", Runtime.NONE);
+        .testIfExactError("Cannot compare int with string", "min(1, '2', True)")
+        .testIfExactError("Cannot compare int with string", "min([1, '2', True])");
   }
 
   @Test
@@ -143,9 +142,8 @@ public class MethodLibraryTest extends EvaluationTestCase {
   @Test
   public void testMaxWithDifferentTypes() throws Exception {
     new BothModesTest()
-        .testStatement("max(1, '2', True)", "2")
-        .testStatement("max([1, '2', True])", "2")
-        .testStatement("max(None, 1, 'test')", "test");
+        .testIfExactError("Cannot compare int with string", "max(1, '2', True)")
+        .testIfExactError("Cannot compare int with string", "max([1, '2', True])");
   }
 
   @Test
@@ -1105,9 +1103,9 @@ public class MethodLibraryTest extends EvaluationTestCase {
         .testEval("sorted([[1], [], [2], [1, 2]])", "[[], [1], [1, 2], [2]]")
         .testEval("sorted([True, False, True])", "[False, True, True]")
         .testEval("sorted(['a','x','b','z'])", "[\"a\", \"b\", \"x\", \"z\"]")
-        .testEval("sorted([sorted, sorted])", "[sorted, sorted]")
         .testEval("sorted({1: True, 5: True, 4: False})", "[1, 4, 5]")
-        .testEval("sorted(depset([1, 5, 4]))", "[1, 4, 5]");
+        .testEval("sorted(depset([1, 5, 4]))", "[1, 4, 5]")
+        .testIfExactError("Cannot compare function with function", "sorted([sorted, sorted])");
   }
 
   @Test