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
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 8db7648..52b7e1f 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
@@ -80,8 +80,13 @@ try { return ((Comparable<Object>) o1).compareTo(o2); } catch (ClassCastException e) { - throw new ComparisonException( - "Cannot compare " + getDataTypeName(o1) + " with " + EvalUtils.getDataTypeName(o2)); + try { + // Different types -> let the class names decide + return o1.getClass().getName().compareTo(o2.getClass().getName()); + } catch (NullPointerException ex) { + throw new ComparisonException( + "Cannot compare " + getDataTypeName(o1) + " with " + EvalUtils.getDataTypeName(o2)); + } } } };
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 dc1e10b..3c89024 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
@@ -27,6 +27,9 @@ import java.util.List; import java.util.Map; import java.util.MissingFormatWidthException; +import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; /** * (Pretty) Printing of Skylark values @@ -130,7 +133,7 @@ } else if (o instanceof Map<?, ?>) { Map<?, ?> dict = (Map<?, ?>) o; - printList(buffer, dict.entrySet(), "{", ", ", "}", null, quotationMark); + printList(buffer, getSortedEntrySet(dict), "{", ", ", "}", null, quotationMark); } else if (o instanceof Map.Entry<?, ?>) { Map.Entry<?, ?> entry = (Map.Entry<?, ?>) o; @@ -183,6 +186,19 @@ return buffer; } + /** + * Returns the sorted entry set of the given map + */ + 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); + tmp.putAll(dict); + dict = tmp; + } + + return dict.entrySet(); + } + public static Appendable write(Appendable buffer, Object o) { return write(buffer, o, SKYLARK_QUOTATION_MARK); }
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 fa98c70..9445ace 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.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 @@ 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 9b0ce0b..14988cb 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 @@ .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 692a338..7be1c86 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 @@ @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 3dbcfb1..baa1fcb 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.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 @@ } @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 f43232a..744e6db 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 @@ @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)]]",