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
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java index 29b9902..4b7adbc 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java +++ b/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 @@ 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)); }
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 7c0af2c..1580fb4 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,20 +72,44 @@ 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); } }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java index 77c708c..3a01f76 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java +++ b/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 @@ "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 @@ "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 @@ 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
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java index af6515f..730a830 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
@@ -171,7 +171,9 @@ */ 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; }