Check that deeply nested values in sets are immutable

--
MOS_MIGRATED_REVID=129331086

3 files changed, 44 insertions, 20 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
index 638936d9c3..79c19610e0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
@@ -19,10 +19,10 @@ import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.common.collect.Sets.SetView;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.util.Preconditions;
 
 import java.io.Serializable;
@@ -56,7 +56,6 @@ public interface ClassObject {
 
   /** An implementation class of ClassObject for structs created in Skylark code. */
   // TODO(bazel-team): maybe move the SkylarkModule annotation to the ClassObject interface?
-  @Immutable
   @SkylarkModule(
     name = "struct",
     category = SkylarkModuleCategory.BUILTIN,
@@ -65,7 +64,7 @@ public interface ClassObject {
             + "See the global <a href=\"globals.html#struct\">struct</a> function "
             + "for more details."
   )
-  public class SkylarkClassObject implements ClassObject, Serializable {
+  public class SkylarkClassObject implements ClassObject, SkylarkValue, Serializable {
     /** Error message to use when errorMessage argument is null. */
     private static final String DEFAULT_ERROR_MESSAGE = "'struct' object has no attribute '%s'";
 
@@ -74,6 +73,16 @@ public interface ClassObject {
     private final String errorMessage;
 
     /**
+     * Primarily for testing purposes where no location is available and the default
+     * errorMessage suffices.
+     */
+    public SkylarkClassObject(Map<String, Object> values) {
+      this.values = copyValues(values);
+      this.creationLoc = null;
+      this.errorMessage = DEFAULT_ERROR_MESSAGE;
+    }
+
+    /**
      * Creates a built-in struct (i.e. without creation loc). The errorMessage has to have
      * exactly one '%s' parameter to substitute the struct field name.
      */
@@ -144,27 +153,40 @@ public interface ClassObject {
       return String.format(errorMessage, name) + "\n" + suffix;
     }
 
+    @Override
+    public boolean isImmutable() {
+      for (Object item : values.values()) {
+        if (!EvalUtils.isImmutable(item)) {
+          return false;
+        }
+      }
+      return true;
+    }
+
     /**
      * Convert the object to string using Skylark syntax. The output tries to be
      * reversible (but there is no guarantee, it depends on the actual values).
      */
     @Override
-    public String toString() {
-      StringBuilder builder = new StringBuilder();
+    public void write(Appendable buffer, char quotationMark) {
       boolean first = true;
-      builder.append("struct(");
+      Printer.append(buffer, "struct(");
       // Sort by key to ensure deterministic output.
       for (String key : Ordering.natural().sortedCopy(values.keySet())) {
         if (!first) {
-          builder.append(", ");
+          Printer.append(buffer, ", ");
         }
         first = false;
-        builder.append(key);
-        builder.append(" = ");
-        Printer.write(builder, values.get(key));
+        Printer.append(buffer, key);
+        Printer.append(buffer, " = ");
+        Printer.write(buffer, values.get(key), quotationMark);
       }
-      builder.append(")");
-      return builder.toString();
+      Printer.append(buffer, ")");
+    }
+
+    @Override
+    public String toString() {
+      return Printer.repr(this);
     }
   }
 }
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 debe96856f..51be3e9b2d 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
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory;
@@ -268,7 +267,7 @@ public abstract class SkylarkList<E> extends MutableCollection<E> implements Lis
     }
 
     /**
-     * Builds a Skylark list (actually immutable) from a variable number of arguments.
+     * Builds a Skylark list from a variable number of arguments.
      * @param env an Environment from which to inherit Mutability, or null for immutable
      * @param contents the contents of the list
      * @return a Skylark list containing the specified arguments as elements.
@@ -421,7 +420,6 @@ public abstract class SkylarkList<E> extends MutableCollection<E> implements Lis
             + "('a', 'b', 'c', 'd')[3:0:-1]  # ('d', 'c', 'b')</pre>"
             + "Tuples are immutable, therefore <code>x[1] = \"a\"</code> is not supported."
   )
-  @Immutable
   public static final class Tuple<E> extends SkylarkList<E> {
 
     private final ImmutableList<E> contents;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java
index bd4be21685..0f8a3d31bc 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java
@@ -125,6 +125,7 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue {
       // TODO(bazel-team): we should check ImmutableList here but it screws up genrule at line 43
       for (Object object : (SkylarkList) item) {
         contentType = checkType(contentType, SkylarkType.of(object.getClass()), loc);
+        checkImmutable(object, loc);
         items.add(object);
       }
     } else {
@@ -186,15 +187,11 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue {
   private static SkylarkType checkType(SkylarkType builderType, SkylarkType itemType, Location loc)
       throws EvalException {
     if (SkylarkType.intersection(
-        SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST, SkylarkType.STRUCT),
+        SkylarkType.Union.of(SkylarkType.DICT, SkylarkType.LIST),
         itemType) != SkylarkType.BOTTOM) {
       throw new EvalException(
           loc, String.format("sets cannot contain items of type '%s'", itemType));
     }
-    if (!EvalUtils.isImmutable(itemType.getType())) {
-      throw new EvalException(
-          loc, String.format("sets cannot contain items of type '%s' (mutable type)", itemType));
-    }
     SkylarkType newType = SkylarkType.intersection(builderType, itemType);
     if (newType == SkylarkType.BOTTOM) {
       throw new EvalException(
@@ -204,6 +201,13 @@ public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue {
     return newType;
   }
 
+  private static void checkImmutable(Object o, Location loc) throws EvalException {
+    if (!EvalUtils.isImmutable(o)) {
+      throw new EvalException(
+          loc, "sets cannot contain mutable items");
+    }
+  }
+
   /**
    * Returns the NestedSet embedded in this SkylarkNestedSet if it is of the parameter type.
    */