Fix race condition in SkylarkType.of
Fix race condition in SkylarkType.of(), whereby .equals() but not same (==) types were created, by making Simple.of() synchronized.
Also make SkylarkType#includes() more robust by using .equals() instead of == (it's a little bit slower in the case of Simple types once fixed, but also works on complex types that don't hash-cons their values).
Also, distinguish SkylarkList (printed as list) and java.util.List (printed as List) and similarly for tuple vs Tuple, when printing types in debugging messages.
--
MOS_MIGRATED_REVID=87490176
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 9448c32..5d44265 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
@@ -212,10 +212,10 @@
} else if (c.equals(Void.TYPE) || c.equals(Environment.NoneType.class)) {
return "None";
} else if (List.class.isAssignableFrom(c)) {
- // TODO(bazel-team): for better debugging, we should distinguish "java tuple" and "java list"
- // from "tuple" and "list" -- or better yet, only use one set of pure data structures
- // everywhere and eliminate all calls to .append and .extend from the code base.
- return isTuple(c) ? "tuple" : "list";
+ // NB: the capital here is a subtle way to distinguish java Tuple and java List
+ // from native SkylarkList tuple and list.
+ // TODO(bazel-team): refactor SkylarkList and use it everywhere.
+ return isTuple(c) ? "Tuple" : "List";
} else if (GlobList.class.isAssignableFrom(c)) {
return "glob list";
} else if (Map.class.isAssignableFrom(c)) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index 2272be6..f6cde12 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -136,7 +136,7 @@
}
public boolean includes(SkylarkType other) {
- return intersection(this, other) == other;
+ return intersection(this, other).equals(other);
}
public boolean includes(Class<?> other) {
@@ -149,9 +149,6 @@
private final class Empty { }; // Empty type, used as basis for Bottom
- private static Map<Class<?>, Simple> simpleCache = // cache used by Simple
- new HashMap<Class<?>, Simple>();
-
// Notable types
/** A singleton for the TOP type, that at analysis time means that any type is possible. */
@@ -250,12 +247,12 @@
public static class Simple extends SkylarkType {
private final Class<?> type;
- public Simple(Class<?> type) {
+ private Simple(Class<?> type) {
this.type = type;
}
@Override public boolean contains(Object value) {
- return type.isAssignableFrom(value.getClass());
+ return value != null && type.isInstance(value);
}
@Override public Class<?> getType() {
return type;
@@ -273,7 +270,15 @@
@Override public boolean canBeCastTo(Class<?> type) {
return this.type == type || super.canBeCastTo(type);
}
- public static Simple of(Class<?> type) {
+ private static HashMap<Class<?>, Simple> simpleCache = new HashMap<>();
+
+ /**
+ * The public way to create a Simple type
+ * @param type a Class
+ * @return the Simple type that contains exactly the instances of that Class
+ */
+ // NB: synchronized to avoid race conditions filling that cache.
+ public static synchronized Simple of(Class<?> type) {
Simple cached = simpleCache.get(type);
if (cached != null) {
return cached;
@@ -308,14 +313,14 @@
// and in practice actually one of SkylarkList or SkylarkNestedSet
private final SkylarkType genericType; // actually always a Simple, for now.
private final SkylarkType argType; // not always Simple
- public Combination(SkylarkType genericType, SkylarkType argType) {
+ private Combination(SkylarkType genericType, SkylarkType argType) {
this.genericType = genericType;
this.argType = argType;
}
public boolean contains(Object value) {
// The empty collection is member of compatible types
- if (!genericType.contains(value)) {
+ if (value == null || !genericType.contains(value)) {
return false;
} else {
SkylarkType valueArgType = getGenericArgType(value);
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
index e320050..3d463f3 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
@@ -97,7 +97,7 @@
assertFalse(buildfile.exec(env, reporter));
Event e = JunitTestUtils.assertContainsEvent(collector,
- "unsupported operand type(s) for +: 'int' and 'list'");
+ "unsupported operand type(s) for +: 'int' and 'List'");
assertEquals(4, e.getLocation().getStartLineAndColumn().getLine());
}
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 7aad74f..9da4ed2 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
@@ -61,8 +61,8 @@
public void testDataTypeNames() throws Exception {
assertEquals("string", EvalUtils.getDataTypeName("foo"));
assertEquals("int", EvalUtils.getDataTypeName(3));
- assertEquals("tuple", EvalUtils.getDataTypeName(makeTuple(1, 2, 3)));
- assertEquals("list", EvalUtils.getDataTypeName(makeList(1, 2, 3)));
+ assertEquals("Tuple", EvalUtils.getDataTypeName(makeTuple(1, 2, 3)));
+ assertEquals("List", EvalUtils.getDataTypeName(makeList(1, 2, 3)));
assertEquals("dict", EvalUtils.getDataTypeName(makeDict()));
assertEquals("FilesetEntry", EvalUtils.getDataTypeName(makeFilesetEntry()));
assertEquals("None", EvalUtils.getDataTypeName(Environment.NONE));
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 f33b500..c31b17b 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
@@ -302,7 +302,7 @@
assertTrue(EvalUtils.isImmutable(x));
checkEvalError("(1,2) + [3,4]", // list + tuple
- "can only concatenate list (not \"tuple\") to list");
+ "can only concatenate List (not \"Tuple\") to List");
}
@SuppressWarnings("unchecked")
@@ -380,8 +380,8 @@
public void testListConcatenation() throws Exception {
assertEquals(Arrays.asList(1, 2, 3, 4), eval("[1, 2] + [3, 4]", env));
assertEquals(ImmutableList.of(1, 2, 3, 4), eval("(1, 2) + (3, 4)", env));
- checkEvalError("[1, 2] + (3, 4)", "can only concatenate tuple (not \"list\") to tuple");
- checkEvalError("(1, 2) + [3, 4]", "can only concatenate list (not \"tuple\") to list");
+ checkEvalError("[1, 2] + (3, 4)", "can only concatenate Tuple (not \"List\") to Tuple");
+ checkEvalError("(1, 2) + [3, 4]", "can only concatenate List (not \"Tuple\") to List");
}
@Test
