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
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 4bec1c6..0ae54ff 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 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 @@
* 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 e1e77c1..2ac152c 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.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 @@
* 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 @@
/**
* 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 @@
/**
* 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 @@
}
/**
- * 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 @@
/**
* 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;
- }
-
- /**
- * Freezes this Mutability
- * @return it in fluent style.
- */
- public Mutability freeze() {
- close();
return this;
}
- /**
- * A MutabilityException will be thrown when the user attempts to mutate an object he shouldn't.
- */
+ @Override
+ public void close() {
+ freeze();
+ }
+
+ /** 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 @@
}
/**
- * 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 @@
}
}
+ /** 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 40c9c9d..bb9dbba 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 @@
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 eb642d2..abb5031 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 @@
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 a1da805..d95456e 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 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() {}
+ /**
+ * 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());
+ }
/**
- * 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 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, Environment env) throws EvalException {
+ 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 @@
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 @@
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 @@
}
}
+ /**
+ * 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.
