Cleanup SkylarkDict API

Reorder a few methods, change some mutators to take Mutability instead of Environment, make it clear that the casting methods return read-only maps (which may be views). Also make putAll/putAllUnsafe public with an appropriately scary javadocs.

RELNOTES: None
PiperOrigin-RevId: 164776346

2 files changed, 114 insertions, 80 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index a0e1c5c076..c4104623c2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -1358,7 +1358,7 @@ public class MethodLibrary {
             throws EvalException {
           Object value = self.get(key);
           if (value != null) {
-            self.remove(key, loc, env);
+            self.remove(key, loc, env.mutability());
             return value;
           }
           if (defaultValue != Runtime.UNBOUND) {
@@ -1392,9 +1392,9 @@ public class MethodLibrary {
           if (self.isEmpty()) {
             throw new EvalException(loc, "popitem(): dictionary is empty");
           }
-          Object key = self.firstKey();
+          Object key = self.keySet().iterator().next();
           Object value = self.get(key);
-          self.remove(key, loc, env);
+          self.remove(key, loc, env.mutability());
           return Tuple.of(key, value);
         }
       };
@@ -1415,7 +1415,7 @@ public class MethodLibrary {
         public Runtime.NoneType invoke(SkylarkDict<Object, Object> self,
             Location loc, Environment env)
             throws EvalException {
-          self.clear(loc, env);
+          self.clear(loc, env.mutability());
           return Runtime.NONE;
         }
       };
@@ -1480,7 +1480,7 @@ public class MethodLibrary {
             Location loc,
             Environment env)
             throws EvalException {
-          self.putAll(other, loc, env);
+          self.putAll(other, loc, env.mutability());
           return Runtime.NONE;
         }
       };
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
index a5fafaeedc..a0e639b4b8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
@@ -20,11 +20,17 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.syntax.SkylarkMutable.MutableMap;
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
 
-/** Skylark Dict module. */
+/**
+ * A Skylark dictionary (dict).
+ *
+ * <p>Although this implements the {@link Map} interface, it is not mutable via that interface's
+ * methods. Instead, use the mutators that take in a {@link Mutability} object.
+ */
 @SkylarkModule(
   name = "dict",
   category = SkylarkModuleCategory.BUILTIN,
@@ -54,11 +60,6 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
 
   private final Mutability mutability;
 
-  @Override
-  public Mutability mutability() {
-    return mutability;
-  }
-
   private SkylarkDict(@Nullable Mutability mutability) {
     this.mutability = mutability == null ? Mutability.IMMUTABLE : mutability;
   }
@@ -67,6 +68,15 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     this.mutability = env == null ? Mutability.IMMUTABLE : env.mutability();
   }
 
+  private static final SkylarkDict<?, ?> EMPTY = of((Mutability) null);
+
+  /** Returns an immutable empty dict. */
+  // Safe because the empty singleton is immutable.
+  @SuppressWarnings("unchecked")
+  public static <K, V> SkylarkDict<K, V> empty() {
+    return (SkylarkDict<K, V>) EMPTY;
+  }
+
   private static <K, V> SkylarkDict<K, V> of(@Nullable Mutability mutability) {
     return new SkylarkDict<>(mutability);
   }
@@ -101,103 +111,136 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     return SkylarkDict.<K, V>of(env).putAllUnsafe(m);
   }
 
-  private SkylarkDict<K, V> putUnsafe(K k, V v) {
+  /**
+   * Puts the given entry into the dict, without calling {@link #checkMutable}.
+   *
+   * <p><em>Warning:</em> This method should never be used by a caller that cares about respecting
+   * mutability restrictions. Such callers should instead use the safe {@link #put(K, V, Location,
+   * Mutability)} method below. This unsafe variant is only public in order to provide
+   * an "escape hatch" for when ordinary mutability restrictions are inapplicable, e.g. for
+   * constructing dicts from outside a Skylark environment and where it's impossible for multiple
+   * threads to observe the value at once.
+   */
+  public SkylarkDict<K, V> putUnsafe(K k, V v) {
     contents.put(k, v);
     return this;
   }
 
-  private <KK extends K, VV extends V> SkylarkDict<K, V> putAllUnsafe(Map<KK, VV> m) {
+  /**
+   * Puts all entries of the given map into the dict, without calling {@link #checkMutable}.
+   *
+   * <p><em>Warning:</em> This method should never be used by a caller that cares about respecting
+   * mutability restrictions. Such callers should instead use the safe {@link #putAll(Map,
+   * Location, Mutability)} method below. This unsafe variant is only public in order to provide
+   * an "escape hatch" for when ordinary mutability restrictions are inapplicable, e.g. for
+   * constructing dicts from outside a Skylark environment and where it's impossible for multiple
+   * threads to observe the value at once.
+   */
+  public <KK extends K, VV extends V> SkylarkDict<K, V> putAllUnsafe(Map<KK, VV> m) {
     for (Map.Entry<KK, VV> e : m.entrySet()) {
       contents.put(e.getKey(), e.getValue());
     }
     return this;
   }
 
-  /**
-   * @return The underlying contents is a (usually) mutable data structure.
-   * Read access is forwarded to these contents.
-   * This object must not be modified outside an {@link Environment}
-   * with a correct matching {@link Mutability},
-   * which should be checked beforehand using {@link #checkMutable}.
-   * it need not be an instance of {@link com.google.common.collect.ImmutableMap}.
-   */
+  @Override
+  public Mutability mutability() {
+    return mutability;
+  }
+
   @Override
   protected Map<K, V> getContentsUnsafe() {
     return contents;
   }
 
   /**
-   * Put an entry into a SkylarkDict.
-   * @param k the key
-   * @param v the associated value
-   * @param loc a {@link Location} in case of error
-   * @param env an {@link Environment}, to check Mutability
-   * @throws EvalException if the key is invalid
+   * Puts an entry into a dict, after validating that mutation is allowed.
+   *
+   * @param key the key of the added entry
+   * @param value the value of the added entry
+   * @param loc the location to use for error reporting
+   * @param mutability the {@link Mutability} associated with the opreation
+   * @throws EvalException if the key is invalid or the dict is frozen
    */
-  public void put(K k, V v, Location loc, Environment env) throws EvalException {
-    checkMutable(loc, env);
-    EvalUtils.checkValidDictKey(k);
-    contents.put(k, v);
+  public void put(K key, V value, Location loc, Mutability mutability) throws EvalException {
+    checkMutable(loc, mutability);
+    EvalUtils.checkValidDictKey(key);
+    contents.put(key, value);
   }
 
   /**
-   * Put all the entries from a given dict into the SkylarkDict.
-   * @param m the map to copy
-   * @param loc a {@link Location} in case of error
-   * @param env an {@link Environment}, to check Mutability
-   * @throws EvalException if some key is invalid
+   * Convenience version of {@link #put(K, V, Location, Mutability)} that uses the {@link
+   * Mutability} of an {@link Environment}.
    */
-  public <KK extends K, VV extends V> void putAll(Map<KK, VV> m, Location loc, Environment env)
-      throws EvalException {
-    checkMutable(loc, env);
-    for (Map.Entry<KK, VV> e : m.entrySet()) {
+  // TODO(bazel-team): Decide whether to eliminate this overload.
+  public void put(K key, V value, Location loc, Environment env) throws EvalException {
+    put(key, value, loc, env.mutability());
+  }
+
+  /**
+   * Puts all the entries from a given map into the dict, after validating that mutation is allowed.
+   *
+   * @param map the map whose entries are added
+   * @param loc the location to use for error reporting
+   * @param mutability the {@link Mutability} associated with the operation
+   * @throws EvalException if some key is invalid or the dict is frozen
+   */
+  public <KK extends K, VV extends V> void putAll(
+      Map<KK, VV> map, Location loc, Mutability mutability) throws EvalException {
+    checkMutable(loc, mutability);
+    for (Map.Entry<KK, VV> e : map.entrySet()) {
       KK k = e.getKey();
       EvalUtils.checkValidDictKey(k);
       contents.put(k, e.getValue());
     }
   }
 
-  /** @return the first key in the dict */
-  K firstKey() {
-    return contents.entrySet().iterator().next().getKey();
-  }
-
   /**
-   * Delete the entry associated to a key.
+   * Deletes the entry associated with the given key.
+   *
    * @param key the key to delete
-   * @param loc a {@link Location} in case of error
-   * @param env an {@link Environment}, to check Mutability
+   * @param loc the location to use for error reporting
+   * @param mutability the {@link Mutability} associated with the operation
    * @return the value associated to the key, or {@code null} if not present
-   * @throws EvalException if the dict is frozen.
+   * @throws EvalException if the dict is frozen
    */
-  V remove(Object key, Location loc, Environment env) throws EvalException {
-    checkMutable(loc, env);
+  V remove(Object key, Location loc, Mutability mutability) throws EvalException {
+    checkMutable(loc, mutability);
     return contents.remove(key);
   }
 
   /**
-   * Clear the dict.
-   * @param loc a {@link Location} in case of error
-   * @param env an {@link Environment}, to check Mutability
-   * @throws EvalException if the dict is frozen.
+   * Clears the dict.
+   *
+   * @param loc the location to use for error reporting
+   * @param mutability the {@link Mutability} associated with the operation
+   * @throws EvalException if the dict is frozen
    */
-  void clear(Location loc, Environment env) throws EvalException {
-    checkMutable(loc, env);
+  void clear(Location loc, Mutability mutability) throws EvalException {
+    checkMutable(loc, mutability);
     contents.clear();
   }
 
-  // Other methods
   @Override
   public void repr(SkylarkPrinter printer) {
     printer.printList(entrySet(), "{", ", ", "}", null);
   }
 
+  @Override
+  public String toString() {
+    return Printer.repr(this);
+  }
+
   /**
-   * Cast a SkylarkDict to a {@code Map<K, V>} after checking its current contents.
-   * Treat None as meaning the empty ImmutableMap.
-   * @param obj the Object to cast. null and None are treated as an empty list.
-   * @param keyType the expected class of keys
-   * @param valueType the expected class of values
+   * If {@code obj} is a {@code SkylarkDict}, casts it to an unmodifiable {@code Map<K, V>} after
+   * checking that each of its entries has key type {@code keyType} and value type {@code
+   * valueType}. If {@code obj} is {@code None} or null, treats it as an empty dict.
+   *
+   * <p>The returned map may or may not be a view that is affected by updates to the original dict.
+   *
+   * @param obj the object to cast. null and None are treated as an empty dict.
+   * @param keyType the expected type of all the dict's keys
+   * @param valueType the expected type of all the dict's values
    * @param description a description of the argument being converted, or null, for debugging
    */
   public static <K, V> Map<K, V> castSkylarkDictOrNoneToDict(
@@ -217,14 +260,18 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
   }
 
   /**
-   * Cast a SkylarkDict to a {@code SkylarkDict<K, V>} after checking its current contents.
+   * Casts this dict to an unmodifiable {@code SkylarkDict<X, Y>}, after checking that all keys and
+   * values have types {@code keyType} and {@code valueType} respectively.
+   *
+   * <p>The returned map may or may not be a view that is affected by updates to the original dict.
+   *
    * @param keyType the expected class of keys
    * @param valueType the expected class of values
    * @param description a description of the argument being converted, or null, for debugging
    */
   @SuppressWarnings("unchecked")
-  public <KeyType, ValueType> SkylarkDict<KeyType, ValueType> getContents(
-      Class<KeyType> keyType, Class<ValueType> valueType, @Nullable String description)
+  public <X, Y> Map<X, Y> getContents(
+      Class<X> keyType, Class<Y> valueType, @Nullable String description)
       throws EvalException {
     Object keyDescription = description == null
         ? null : Printer.formattable("'%s' key", description);
@@ -234,7 +281,7 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
       SkylarkType.checkType(e.getKey(), keyType, keyDescription);
       SkylarkType.checkType(e.getValue(), valueType, valueDescription);
     }
-    return (SkylarkDict<KeyType, ValueType>) this;
+    return Collections.unmodifiableMap((SkylarkDict<X, Y>) this);
   }
 
   @Override
@@ -250,11 +297,6 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     return this.containsKey(key);
   }
 
-  @Override
-  public String toString() {
-    return Printer.repr(this);
-  }
-
   public static <K, V> SkylarkDict<K, V> plus(
       SkylarkDict<? extends K, ? extends V> left,
       SkylarkDict<? extends K, ? extends V> right,
@@ -264,12 +306,4 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     result.putAllUnsafe(right);
     return result;
   }
-
-  private static final SkylarkDict<?, ?> EMPTY = of((Mutability) null);
-
-  // Safe because the empty singleton is immutable.
-  @SuppressWarnings("unchecked")
-  public static <K, V> SkylarkDict<K, V> empty() {
-    return (SkylarkDict<K, V>) EMPTY;
-  }
 }