Fix mutability bug in SkylarkList/SkylarkMap Java APIs

Previously, you could modify a frozen MutableList indirectly via its iterator(). This CL wraps the underlying list with a Collections.unmodifiableList when obtaining the iterator, and likewise for other methods that return views over Skylark data structures.

This also eliminates the SkylarkList#getContents method in favor of just using the SkylarkList object itself.

RELNOTES: None
PiperOrigin-RevId: 164402129

3 files changed, 32 insertions, 53 deletions

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 bb9dbbab55..a5fafaeedc 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
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.syntax;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
@@ -58,8 +59,16 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     return mutability;
   }
 
+  private SkylarkDict(@Nullable Mutability mutability) {
+    this.mutability = mutability == null ? Mutability.IMMUTABLE : mutability;
+  }
+
   private SkylarkDict(@Nullable Environment env) {
-    mutability = env == null ? Mutability.IMMUTABLE : env.mutability();
+    this.mutability = env == null ? Mutability.IMMUTABLE : env.mutability();
+  }
+
+  private static <K, V> SkylarkDict<K, V> of(@Nullable Mutability mutability) {
+    return new SkylarkDict<>(mutability);
   }
 
   /** @return a dict mutable in given environment only */
@@ -78,6 +87,14 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     return SkylarkDict.<K, V>of(env).putUnsafe(k1, v1).putUnsafe(k2, v2);
   }
 
+  // TODO(bazel-team): Make other methods that take in mutabilities instead of environments, make
+  // this method public.
+  @VisibleForTesting
+  static <K, V> SkylarkDict<K, V> copyOf(
+      @Nullable Mutability mutability, Map<? extends K, ? extends V> m) {
+    return SkylarkDict.<K, V>of(mutability).putAllUnsafe(m);
+  }
+
   /** @return a dict mutable in given environment only, with contents copied from given map */
   public static <K, V> SkylarkDict<K, V> copyOf(
       @Nullable Environment env, Map<? extends K, ? extends V> m) {
@@ -248,7 +265,7 @@ public final class SkylarkDict<K, V> extends MutableMap<K, V>
     return result;
   }
 
-  private static final SkylarkDict<?, ?> EMPTY = of(null);
+  private static final SkylarkDict<?, ?> EMPTY = of((Mutability) null);
 
   // Safe because the empty singleton is immutable.
   @SuppressWarnings("unchecked")
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 13f693ee0d..f9e92ca4cf 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
@@ -51,26 +51,6 @@ public abstract class SkylarkList<E> extends BaseMutableList<E>
   public abstract ImmutableList<E> getImmutableList();
 
   /**
-   * Returns a List object with the current underlying contents of this SkylarkList.
-   * This object must not be mutated, but need not be an {@link ImmutableList}.
-   * Indeed it can sometimes be a {@link GlobList}.
-   */
-  // TODO(bazel-team): move GlobList out of Skylark, into an extension.
-  // TODO(bazel-team): Remove this function, prefer getImmutableList()? Looks like a possible source
-  // of mutability bugs.
-  public abstract List<E> getContents();
-
-  // For subList, use the immutable getContents() rather than getContentsUnsafe,
-  // to prevent subsequent mutation. To get a mutable SkylarkList,
-  // use a method that takes a Mutability into account.
-  @Override
-  public List<E> subList(int fromIndex, int toIndex) {
-    // TODO(bazel-team): remove this, use the version in BaseMutableList instead, fix that version
-    // to make a copy if that's what's needed here.
-    return getContents().subList(fromIndex, toIndex);
-  }
-
-  /**
    * Retrieve an entry from a SkylarkList.
    *
    * @param key the index
@@ -241,6 +221,7 @@ public abstract class SkylarkList<E> extends BaseMutableList<E>
     // TODO(bazel-team): make data structures *and binary operators* extensible
     // (via e.g. interface classes for each binary operator) so that GlobList
     // can be implemented outside of the core of Skylark.
+    // TODO(bazel-team): move GlobList out of Skylark, into an extension.
     @Nullable private GlobList<E> globList;
 
     private final Mutability mutability;
@@ -357,19 +338,6 @@ public abstract class SkylarkList<E> extends BaseMutableList<E>
       return ImmutableList.copyOf(contents);
     }
 
-    /**
-     * Returns the {@link GlobList} if there is one, otherwise an {@link ImmutableList} copy of the
-     * regular contents.
-     */
-    @Override
-    @SuppressWarnings("unchecked")
-    public List<E> getContents() {
-      if (globList != null) {
-        return globList;
-      }
-      return getImmutableList();
-    }
-
     @Override
     protected List<E> getContentsUnsafe() {
       return contents;
@@ -598,11 +566,6 @@ public abstract class SkylarkList<E> extends BaseMutableList<E>
     }
 
     @Override
-    public List<E> getContents() {
-      return contents;
-    }
-
-    @Override
     protected List<E> getContentsUnsafe() {
       return contents;
     }
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 6d28fff616..947d2d6689 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
@@ -89,9 +89,10 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
    *
    * <p>A subclass implements a specific data structure interface, say {@link List}, and refines the
    * return type of {@link #getContentsUnsafe} to be that interface. The subclass implements all of
-   * the interface's accessors such that they defer to the result of {@code getContentsUnsafe}. The
-   * subclass implements final versions of all the interface's mutating methods such that they
-   * are marked {@code @Deprecated} and throw {@link UnsupportedOperationException}.
+   * the interface's accessors such that they defer to the result of {@code getContentsUnsafe}.
+   * Accessors such as {@link Collection#iterator()} must return unmodifiable views. The subclass
+   * implements final versions of all the interface's mutating methods such that they are marked
+   * {@code @Deprecated} and throw {@link UnsupportedOperationException}.
    *
    * <p>A concrete subclass may provide alternative mutating methods that take in a {@link
    * Mutability} and validate that the mutation is allowed using {@link #checkMutable}. This
@@ -145,12 +146,9 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
       return getContentsUnsafe().isEmpty();
     }
 
-    // TODO(bazel-team): Is exposing this a mutability violation? Same for listIterator() and
-    // subList() below, as well as the entrySet(), keySet(), and values() map methods. Could address
-    // by returning views over a Collections.unmodifiableCollection(), etc.
     @Override
     public Iterator<E> iterator() {
-      return getContentsUnsafe().iterator();
+      return Collections.unmodifiableCollection(getContentsUnsafe()).iterator();
     }
 
     @Override
@@ -158,6 +156,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
       return getContentsUnsafe().size();
     }
 
+    // toArray() and toArray(T[]) return copies, so we don't need an unmodifiable view.
     @Override
     public Object[] toArray() {
       return getContentsUnsafe().toArray();
@@ -232,17 +231,17 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
 
     @Override
     public ListIterator<E> listIterator() {
-      return getContentsUnsafe().listIterator();
+      return Collections.unmodifiableList(getContentsUnsafe()).listIterator();
     }
 
     @Override
     public ListIterator<E> listIterator(int index) {
-      return getContentsUnsafe().listIterator(index);
+      return Collections.unmodifiableList(getContentsUnsafe()).listIterator(index);
     }
 
     @Override
     public List<E> subList(int fromIndex, int toIndex) {
-      return getContentsUnsafe().subList(fromIndex, toIndex);
+      return Collections.unmodifiableList(getContentsUnsafe()).subList(fromIndex, toIndex);
     }
 
     // (Disallowed) writing methods of List, in alphabetic order.
@@ -292,7 +291,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
 
     @Override
     public Set<Map.Entry<K, V>> entrySet() {
-      return getContentsUnsafe().entrySet();
+      return Collections.unmodifiableMap(getContentsUnsafe()).entrySet();
     }
 
     @Override
@@ -307,7 +306,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
 
     @Override
     public Set<K> keySet() {
-      return getContentsUnsafe().keySet();
+      return Collections.unmodifiableMap(getContentsUnsafe()).keySet();
     }
 
     @Override
@@ -317,7 +316,7 @@ public abstract class SkylarkMutable implements Freezable, SkylarkValue {
 
     @Override
     public Collection<V> values() {
-      return getContentsUnsafe().values();
+      return Collections.unmodifiableMap(getContentsUnsafe()).values();
     }
 
     // (Disallowed) writing methods of Map, in alphabetic order.