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);
