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

2 files changed, 18 insertions, 13 deletions

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 9448c32b2b..5d44265578 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 @@ public abstract class EvalUtils {
     } 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 2272be68a2..f6cde1241a 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 abstract class SkylarkType {
   }
 
   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 @@ public abstract class SkylarkType {
 
   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 abstract class SkylarkType {
   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 @@ public abstract class SkylarkType {
     @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 @@ public abstract class SkylarkType {
     // 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);