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)]]",
