Cleanup javadoc around Mutability / SkylarkMutable

Also throw IllegalArgumentException instead of AssertionError.

A follow-up CL will look to eliminate the SkylarkMutable#checkMutable(Location, Environment) override in favor of checkMutable(Location, Mutability). This will make it easier to manipulate Skylark values in specialized contexts aside from normal evaluation.

RELNOTES: None
PiperOrigin-RevId: 163978262

5 files changed, 150 insertions, 98 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 4bec1c6012..0ae54ff2fd 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -238,7 +238,7 @@ public final class Environment implements Freezable {
      */
     public void put(Environment env, String varname, Object value)
         throws MutabilityException {
-      Mutability.checkMutable(this, env);
+      Mutability.checkMutable(this, env.mutability());
       bindings.put(varname, value);
     }
 
@@ -247,7 +247,7 @@ public final class Environment implements Freezable {
      * be part of the public interface.
      */
     void remove(Environment env, String varname) throws MutabilityException {
-      Mutability.checkMutable(this, env);
+      Mutability.checkMutable(this, env.mutability());
       bindings.remove(varname);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java b/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java
index e1e77c1325..2ac152c024 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java
@@ -22,24 +22,42 @@ import java.util.IdentityHashMap;
 import java.util.List;
 
 /**
- * Manages the capability to mutate an {@link Environment} and its contained Skylark objects.
+ * An object that manages the capability to mutate Skylark objects and their {@link Environment}s.
+ * Collectively, the managed objects are called {@link Freezable}s.
  *
- * <p>Once the {@code Environment} is done evaluating, its {@code Mutability} is irreversibly closed
- * ("frozen"). At that point, it is no longer possible to mutate either the {@code Environment} or
- * its objects. This protects each {@code Environment} from unintentional and unsafe modification.
- * Before freezing, only a single thread may use the {@code Environment}, but after freezing, any
- * number of threads may access it.
+ * <p>Each {@code Environment}, and each of the mutable Skylark values (i.e., {@link
+ * SkylarkMutable}s) that are created in that {@code Environment}, holds a pointer to the same
+ * {@code Mutability} instance. Once the {@code Environment} is done evaluating, its {@code
+ * Mutability} is irreversibly closed ("frozen"). At that point, it is no longer possible to change
+ * either the bindings in that {@code Environment} or the state of its objects. This protects each
+ * {@code Environment} from unintentional and unsafe modification. Before freezing, only a single
+ * thread may use the contents of the {@code Environment}, but after freezing, any number of threads
+ * may access it.
  *
  * <p>It is illegal for an evaluation in one {@code Environment} to affect another {@code
- * Environment}, even if the second {@code Environment} has not yet been frozen.
+ * Environment}, even if the second {@code Environment} has not yet been frozen. In practice, the
+ * only unfrozen values that any {@code Environment} should be able to access are its own.
  *
- * <p>A {@code Mutability} also tracks which {@link Freezable} objects in its {@code Environment}
+ * <p>A {@code Mutability} also tracks which {@code Freezable} objects in its {@code Environment}
  * are temporarily locked from mutation. This is used to prevent modification of iterables during
  * loops. A {@code Freezable} may be locked multiple times (e.g., nested loops over the same
  * iterable). Locking an object does not prohibit mutating its deeply contained values, such as in
  * the case of a list of lists.
  *
- * <p>To ensure safety, {@code Mutability}s must be created using the try-with-resource style:
+ * We follow two disciplines to ensure safety. First, all mutation methods of a {@code Freezable}
+ * must take in a {@code Mutability} as a parameter, and confirm that
+ * <ol>
+ *   <li>the {@code Freezable} is not yet frozen,
+ *   <li>the given {@code Mutability} matches the one referred to by the {@code Freezable}, and
+ *   <li>the {@code Freezable} is not locked.
+ * </ol>
+ * It is a high-level error ({@link MutabilityException}, which gets translated to {@link
+ * EvalException}) to attempt to modify a frozen or locked value. But it is a low-level error
+ * ({@link IllegalArgumentException}) to attempt to modify a value using the wrong {@link
+ * Mutability} instance, since the user shouldn't be able to trigger this situation under normal
+ * circumstances.
+ *
+ * <p>Second, {@code Mutability}s are created using the try-with-resource style:
  * <pre>{@code
  * try (Mutability mutability = Mutability.create(fmt, ...)) { ... }
  * }</pre>
@@ -49,25 +67,27 @@ import java.util.List;
  * the result is used. The only code that should create a {@code Mutability} without using
  * try-with-resource is test code that is not part of the Bazel jar.
  */
-// TODO(bazel-team): When we start using Java 8, this safe usage pattern can be enforced
-// through the use of a higher-order function.
+// TODO(bazel-team): This safe usage pattern can be enforced through the use of a higher-order
+// function.
 public final class Mutability implements AutoCloseable, Serializable {
 
+  /**
+   * If true, mutation of any {@link Freezable} associated with this {@code Mutability} is
+   * disallowed.
+   */
   private boolean isFrozen;
 
-  // For each locked Freezable, store all Locations where it is locked.
-  // This field is set null once the Mutability is closed. This saves some
-  // space, and avoids a concurrency bug from multiple Skylark modules
-  // accessing the same Mutability at once.
+  /**
+   * For each locked {@link Freezable}, stores all {@link Location}s where it is locked.
+   *
+   * This field is set null once the {@code Mutability} is closed. This saves some space, and avoids
+   * a concurrency bug from multiple Skylark modules accessing the same {@code Mutability} at once.
+   */
   private IdentityHashMap<Freezable, List<Location>> lockedItems;
 
-  private final String annotation; // For error reporting.
+  /** For error reporting; a name for the context in which this {@code Mutability} is used. */
+  private final String annotation;
 
-  /**
-   * Creates a Mutability.
-   * @param annotation an Object used for error reporting,
-   * describing to the user the context in which this Mutability was active.
-   */
   private Mutability(String annotation) {
     this.isFrozen = false;
     // Seems unlikely that we'll often lock more than 10 things at once.
@@ -77,8 +97,9 @@ public final class Mutability implements AutoCloseable, Serializable {
 
   /**
    * Creates a Mutability.
-   * @param pattern is a {@link Printer#format} pattern used to lazily produce a string
-   * for error reporting
+   *
+   * @param pattern is a {@link Printer#format} pattern used to lazily produce a string name
+   *     for error reporting
    * @param arguments are the optional {@link Printer#format} arguments to produce that string
    */
   public static Mutability create(String pattern, Object... arguments) {
@@ -103,11 +124,13 @@ public final class Mutability implements AutoCloseable, Serializable {
   /**
    * Return whether a {@link Freezable} belonging to this {@code Mutability} is currently locked.
    * Frozen objects are not considered locked, though they are of course immutable nonetheless.
+   *
+   * @throws IllegalArgumentException if the {@code Freezable} does not belong to this {@code
+   *     Mutability}
    */
   public boolean isLocked(Freezable object) {
-    if (!object.mutability().equals(this)) {
-      throw new AssertionError("trying to check the lock of an object from a different context");
-    }
+    Preconditions.checkArgument(object.mutability().equals(this),
+        "trying to check the lock of an object from a different context");
     if (isFrozen) {
       return false;
     }
@@ -115,25 +138,29 @@ public final class Mutability implements AutoCloseable, Serializable {
   }
 
   /**
-   * For a locked {@link Freezable} that belongs to this Mutability, return a List of the
+   * For a locked {@link Freezable} that belongs to this {@code Mutability}, return a List of the
    * {@link Location}s corresponding to its current locks.
+   *
+   * @throws IllegalArgumentException if the {@code Freezable} does not belong to this {@code
+   *     Mutability}
    */
   public List<Location> getLockLocations(Freezable object) {
-    if (!isLocked(object)) {
-      throw new AssertionError("trying to get lock locations for an object that is not locked");
-    }
+    Preconditions.checkArgument(isLocked(object),
+        "trying to get lock locations for an object that is not locked");
     return lockedItems.get(object);
   }
 
   /**
-   * Add a lock on a {@link Freezable} belonging to this Mutability. The object cannot be
+   * Add a lock on a {@link Freezable} belonging to this {@code Mutability}. The object cannot be
    * mutated until all locks on it are gone. For error reporting purposes each lock is
    * associated with its originating {@link Location}.
+   *
+   * @throws IllegalArgumentException if the {@code Freezable} does not belong to this {@code
+   *     Mutability}
    */
   public void lock(Freezable object, Location loc) {
-    if (!object.mutability().equals(this)) {
-      throw new AssertionError("trying to lock an object from a different context");
-    }
+    Preconditions.checkArgument(object.mutability().equals(this),
+        "trying to lock an object from a different context");
     if (isFrozen) {
       return;
     }
@@ -149,53 +176,51 @@ public final class Mutability implements AutoCloseable, Serializable {
 
   /**
    * Remove the lock for a given {@link Freezable} that is associated with the given {@link
-   * Location}. It is an error if {@code object} does not belong to this mutability, or has no lock
-   * corresponding to {@code loc}.
+   * Location}.
+   *
+   * @throws IllegalArgumentException if the object does not belong to this {@code Mutability}, or
+   *     if the object has no lock corresponding to {@code loc}
    */
   public void unlock(Freezable object, Location loc) {
-    if (!object.mutability().equals(this)) {
-      throw new AssertionError("trying to unlock an object from a different context");
-    }
+    Preconditions.checkArgument(object.mutability().equals(this),
+        "trying to unlock an object from a different context");
     if (isFrozen) {
       // It's okay if we somehow got frozen while there were still locked objects.
       return;
     }
-    if (!lockedItems.containsKey(object)) {
-      throw new AssertionError("trying to unlock an object that is not locked");
-    }
+    Preconditions.checkArgument(lockedItems.containsKey(object),
+        "trying to unlock an object that is not locked");
     List<Location> locList = lockedItems.get(object);
     boolean changed = locList.remove(loc);
-    if (!changed) {
-      throw new AssertionError(Printer.format("trying to unlock an object for a location at which "
-          + "it was not locked (%r)", loc));
-    }
+    Preconditions.checkArgument(changed, Printer.format(
+        "trying to unlock an object for a location at which it was not locked (%r)", loc));
     if (locList.isEmpty()) {
       lockedItems.remove(object);
     }
   }
 
   /**
-   * Freezes this Mutability, marking as immutable all {@link Freezable} objects that use it.
+   * Freezes this {@code Mutability}, rendering all {@link Freezable} objects that refer to it
+   * immutable.
+   *
+   * Note that freezing does not directly touch all the {@code Freezables}, so this operation is
+   * constant-time.
+   *
+   * @return this object, in the fluent style
    */
-  @Override
-  public void close() {
+  public Mutability freeze() {
     // No need to track per-Freezable info since everything is immutable now.
     lockedItems = null;
     isFrozen = true;
+    return this;
   }
 
-  /**
-   * Freezes this Mutability
-   * @return it in fluent style.
-   */
-  public Mutability freeze() {
-    close();
-    return this;
+  @Override
+  public void close() {
+    freeze();
   }
 
-  /**
-   * A MutabilityException will be thrown when the user attempts to mutate an object he shouldn't.
-   */
+  /** Indicates an illegal attempt to mutate a frozen or locked {@link Freezable}. */
   static class MutabilityException extends Exception {
     MutabilityException(String message) {
       super(message);
@@ -203,49 +228,48 @@ public final class Mutability implements AutoCloseable, Serializable {
   }
 
   /**
-   * Each {@code Freezable} object possesses a {@link Mutability} that determines whether the object
-   * is still mutable. All {@code Freezable} objects created in the same {@link Environment} will
-   * share the same {@code Mutability}, inherited from this {@code Environment}. Only evaluation in
-   * the same {@code Environment} is allowed to mutate these objects, and only until the
-   * {@code Mutability} is irreversibly frozen.
+   * An object that refers to a {@link Mutability} to decide whether to allow mutation. All
+   * {@link Freezable} Skylark objects created within a given {@link Environment} will share the
+   * same {@code Mutability} as that {@code Environment}.
    */
   public interface Freezable {
     /**
-     * Returns the {@link Mutability} associated with this Freezable object.
-     * This should not change over the lifetime of the object.
+     * Returns the {@link Mutability} associated with this {@code Freezable}. This should not change
+     * over the lifetime of the object.
      */
     Mutability mutability();
   }
 
   /**
-   * Checks that this Freezable object can be mutated from the given {@link Environment}.
-   * If the object is mutable, it must be from the environment.
-   * @param object a Freezable object that we check is still mutable.
-   * @param env the {@link Environment} attempting the mutation.
-   * @throws MutabilityException when the object was frozen already, or is locked.
+   * Checks that the given {@code Freezable} can be mutated using the given {@code Mutability}, and
+   * throws an exception if it cannot.
+   *
+   * @throws MutabilityException if the object is either frozen or locked
+   * @throws IllegalArgumentException if the given {@code Mutability} is not the same as the one
+   *     the {@code Freezable} is associated with
    */
-  public static void checkMutable(Freezable object, Environment env)
+  public static void checkMutable(Freezable object, Mutability mutability)
       throws MutabilityException {
     if (object.mutability().isFrozen()) {
-      // Throw MutabilityException, not AssertionError, even if the object was from
+      // Throw MutabilityException, not IllegalArgumentException, even if the object was from
       // another context.
       throw new MutabilityException("trying to mutate a frozen object");
     }
 
-    // Consider an {@link Environment} e1, in which is created {@link UserDefinedFunction} f1,
-    // that closes over some variable v1 bound to list l1. If somehow, via the magic of callbacks,
-    // f1 or l1 is passed as an argument to some function f2 evaluated in {@link Environment} e2
-    // while e1 is still mutable, then e2, being a different {@link Environment}, should not be
-    // allowed to mutate objects from e1. It's a bug, that shouldn't happen in our current code
-    // base, so we throw an AssertionError. If in the future such situations are allowed to happen,
+    // Consider an {@link Environment} e1, in which is created {@link UserDefinedFunction} f1, that
+    // closes over some variable v1 bound to list l1. If somehow, via the magic of callbacks, f1 or
+    // l1 is passed as an argument to some function f2 evaluated in {@link Environment} e2 while e1
+    // is still mutable, then e2, being a different {@link Environment}, should not be allowed to
+    // mutate objects from e1. It's a bug, that shouldn't happen in our current code base, so we
+    // throw an IllegalArgumentException. If in the future such situations are allowed to happen,
     // then we should throw a MutabilityException instead.
-    if (!object.mutability().equals(env.mutability())) {
-      throw new AssertionError("trying to mutate an object from a different context");
+    if (!object.mutability().equals(mutability)) {
+      throw new IllegalArgumentException("trying to mutate an object from a different context");
     }
 
-    if (env.mutability().isLocked(object)) {
+    if (mutability.isLocked(object)) {
       Iterable<String> locs =
-          Iterables.transform(env.mutability().getLockLocations(object), Location::print);
+          Iterables.transform(mutability.getLockLocations(object), Location::print);
       throw new MutabilityException(
           "trying to mutate a locked object (is it currently being iterated over by a for loop "
           + "or comprehension?)\n"
@@ -254,5 +278,6 @@ public final class Mutability implements AutoCloseable, Serializable {
     }
   }
 
+  /** A singular instance for permanently immutable things. */
   public static final Mutability IMMUTABLE = create("IMMUTABLE").freeze();
 }
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 40c9c9ddb3..bb9dbbab55 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
@@ -233,6 +233,11 @@ 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,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
index eb642d2200..abb5031599 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkList.java
@@ -203,6 +203,11 @@ public abstract class SkylarkList<E> extends MutableCollection<E>
     return getClass().hashCode() + 31 * getContentsUnsafe().hashCode();
   }
 
+  @Override
+  public String toString() {
+    return Printer.repr(this);
+  }
+
   /**
    * Cast a {@code List<?>} to a {@code List<T>} after checking its current contents.
    * @param list the List to cast
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java
index a1da805bf0..d95456e855 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkMutable.java
@@ -27,19 +27,33 @@ import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
- * Base class for data structures that are only mutable with a proper Mutability.
+ * Base class for data structures that are only mutable using a proper, unfrozen {@link Mutability}.
  */
 public abstract class SkylarkMutable implements Freezable, SkylarkValue {
 
-  protected SkylarkMutable() {}
-
   /**
-   * Check whether this object is mutable in the current evaluation Environment.
-   * @throws EvalException if the object was not mutable.
+   * Checks whether this object is currently mutable in the given {@link Environment}, and throws
+   * an exception if it is not.
+   *
+   * @deprecated prefer {@link #checkMutable(Location, Mutability)} instead
    */
+  @Deprecated
   protected void checkMutable(Location loc, Environment env) throws EvalException {
+    checkMutable(loc, env.mutability());
+  }
+
+  /**
+   * Checks whether this object is currently mutable using the given {@link Mutability}, and throws
+   * an exception if it is not.
+   *
+   * @throws EvalException if the object is not mutable. This may be because the object (i.e., its
+   *     {@code Mutability} was frozen, or because it is temporarily locked from mutation (due to
+   *     being iterated over by a loop), or because it is associated with a different {@code
+   *     Mutability} than the one given.
+   */
+  protected void checkMutable(Location loc, Mutability mutability) throws EvalException {
     try {
-      Mutability.checkMutable(this, env);
+      Mutability.checkMutable(this, mutability);
     } catch (MutabilityException ex) {
       throw new EvalException(loc, ex);
     }
@@ -50,11 +64,6 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
     return mutability().isFrozen();
   }
 
-  @Override
-  public String toString() {
-    return Printer.repr(this);
-  }
-
   /**
    * Add a new lock at {@code loc}. No effect if frozen.
    */
@@ -69,6 +78,11 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
     mutability().unlock(this, loc);
   }
 
+  /**
+   * Base class for a {@link SkylarkMutable} that is also a {@link Collection}.
+   *
+   * <p>The mutating methods from {@code Collection} are not supported.
+   */
   abstract static class MutableCollection<E> extends SkylarkMutable implements Collection<E> {
 
     protected MutableCollection() {}
@@ -167,10 +181,13 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
     }
   }
 
+  /**
+   * Base class for a {@link SkylarkMutable} that is also a {@link Map}.
+   *
+   * <p>The mutating methods from {@code Map} are not supported.
+   */
   abstract static class MutableMap<K, V> extends SkylarkMutable implements Map<K, V> {
 
-    MutableMap() {}
-
     /**
      * The underlying contents is a (usually) mutable data structure.
      * Read access is forwarded to these contents.