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

site/versions/master/docs/skylark/concepts.md

@@ -199,6 +199,12 @@ Python:
   declaration. However, it is fine to define `f()` before `g()`, even if `f()`
   calls `g()`.
 
+* The order comparison operators (<, <=, >=, >) are not defined across different
+  types of values, e.g., you can't compare `5 < 'foo'` (however you still can
+  compare them using == or !=). This is a difference with Python 2, but
+  consistent with Python 3. Note that this means you are unable to sort lists
+  that contain mixed types of values.
+
 The following Python features are not supported:
 
 * `class` (see [`struct`](lib/globals.html#struct) function)
@@ -244,11 +250,6 @@ The following items are upcoming changes.
 *   The `|` operator is defined for depsets as a synonym for `+`. This will be
     going away; use `+` instead.
 
-*   The order comparison operators (<, <=, >=, >) are currently defined across
-    different types of values, e.g., you can write `5 < 'foo'`. This will be an
-    error, just like in Python 3. Note that this means you will be unable to
-    sort lists that contain mixed types of values.
-
 *   The structure of the set that you get back from using the `+` or `|`
     operator is changing. Previously `a + b`, where `a` is a set, would include
     as its direct items all of `a`'s direct items. Under the upcoming way, the

src/main/java/com/google/devtools/build/lib/actions/Artifact.java

@@ -31,6 +31,7 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.EvalUtils.ComparisonException;
 import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -135,7 +136,7 @@ public int compareTo(Object o) {
     if (o instanceof Artifact) {
       return EXEC_PATH_COMPARATOR.compare(this, (Artifact) o);
     }
-    return EvalUtils.compareByClass(this, o);
+    throw new ComparisonException("Cannot compare artifact with " + EvalUtils.getDataTypeName(o));
   }
 
 

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

@@ -72,20 +72,44 @@ 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 depsets");
-          }
           if (o1 instanceof SkylarkList
               && o2 instanceof SkylarkList
               && ((SkylarkList) o1).isTuple() == ((SkylarkList) o2).isTuple()) {
             return compareLists((SkylarkList) o1, (SkylarkList) o2);
           }
+          if (!o1.getClass().equals(o2.getClass())) {
+            throw new ComparisonException(
+                "Cannot compare " + getDataTypeName(o1) + " with " + getDataTypeName(o2));
+          }
+
+          if (o1 instanceof ClassObject) {
+            throw new ComparisonException("Cannot compare structs");
+          }
+          if (o1 instanceof SkylarkNestedSet) {
+            throw new ComparisonException("Cannot compare depsets");
+          }
           try {
             return ((Comparable<Object>) o1).compareTo(o2);
           } catch (ClassCastException e) {
+            throw new ComparisonException(
+                "Cannot compare " + getDataTypeName(o1) + " with " + getDataTypeName(o2));
+          }
+        }
+      };
+
+  /**
+   * Legacy Skylark comparator.
+   *
+   * <p>Falls back to comparing by class if objects are not comparable otherwise.
+   */
+  public static final Ordering<Object> SAFE_SKYLARK_COMPARATOR =
+      new Ordering<Object>() {
+        @Override
+        @SuppressWarnings("unchecked")
+        public int compare(Object o1, Object o2) {
+          try {
+            return SKYLARK_COMPARATOR.compare(o1, o2);
+          } catch (ComparisonException e) {
             return compareByClass(o1, o2);
           }
         }

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

@@ -28,6 +28,7 @@
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
+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.Type.ConversionException;
@@ -992,15 +993,20 @@ public Boolean invoke(String self, String sub, Integer start, Object end)
         "Returns the smallest one of all given arguments. "
             + "If only one argument is provided, it must be a non-empty iterable.",
     extraPositionals =
-      @Param(name = "args", type = SkylarkList.class, doc = "The elements to be checked."),
+        @Param(name = "args", type = SkylarkList.class, doc = "The elements to be checked."),
     useLocation = true
   )
-  private static final BuiltinFunction min = new BuiltinFunction("min") {
-    @SuppressWarnings("unused") // Accessed via Reflection.
-    public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
-      return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc);
-    }
-  };
+  private static final BuiltinFunction min =
+      new BuiltinFunction("min") {
+        @SuppressWarnings("unused") // Accessed via Reflection.
+        public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
+          try {
+            return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc);
+          } catch (ComparisonException e) {
+            throw new EvalException(loc, e);
+          }
+        }
+      };
 
   @SkylarkSignature(
     name = "max",
@@ -1009,15 +1015,20 @@ public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
         "Returns the largest one of all given arguments. "
             + "If only one argument is provided, it must be a non-empty iterable.",
     extraPositionals =
-      @Param(name = "args", type = SkylarkList.class, doc = "The elements to be checked."),
+        @Param(name = "args", type = SkylarkList.class, doc = "The elements to be checked."),
     useLocation = true
   )
-  private static final BuiltinFunction max = new BuiltinFunction("max") {
-    @SuppressWarnings("unused") // Accessed via Reflection.
-    public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
-      return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc);
-    }
-  };
+  private static final BuiltinFunction max =
+      new BuiltinFunction("max") {
+        @SuppressWarnings("unused") // Accessed via Reflection.
+        public Object invoke(SkylarkList<?> args, Location loc) throws EvalException {
+          try {
+            return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc);
+          } catch (ComparisonException e) {
+            throw new EvalException(loc, e);
+          }
+        }
+      };
 
   /**
    * Returns the maximum element from this list, as determined by maxOrdering.
@@ -1084,8 +1095,8 @@ private static boolean hasElementWithBooleanValue(Object collection, boolean val
     name = "sorted",
     returnType = MutableList.class,
     doc =
-        "Sort a collection. Elements are sorted first by their type, "
-            + "then by their value (in ascending order).",
+        "Sort a collection. Elements should all belong to the same orderable type, they are sorted "
+            + "by their value (in ascending order).",
     parameters = {@Param(name = "self", type = Object.class, doc = "This collection.")},
     useLocation = true,
     useEnvironment = true

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

@@ -171,7 +171,9 @@ public static Appendable write(Appendable buffer, Object o, char quotationMark)
    */
   private static <K, V> Set<Map.Entry<K, V>> getSortedEntrySet(Map<K, V> dict) {
     if (!(dict instanceof SortedMap<?, ?>)) {
-      Map<K, V> tmp = new TreeMap<>(EvalUtils.SKYLARK_COMPARATOR);
+      // TODO(bazel-team): Dict keys should not be sorted, because comparison of objects of
+      // potentially different types is not supported anymore in Skylark.
+      Map<K, V> tmp = new TreeMap<>(EvalUtils.SAFE_SKYLARK_COMPARATOR);
       tmp.putAll(dict);
       dict = tmp;
     }

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

@@ -22,11 +22,13 @@
 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 void testDatatypeMutabilityDeep() throws Exception {
 
   @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
     }
   }
 }

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

@@ -89,9 +89,8 @@ public void testMinWithSameValues() throws Exception {
   @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 void testMaxWithSameValues() throws Exception {
   @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 void testListSort() throws Exception {
         .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