Check that deeply nested values in sets are immutable

--
MOS_MIGRATED_REVID=129331086

5 files changed, 94 insertions, 26 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.
    */
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
index 95423b6544..26763ea796 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvalUtilsTest.java
@@ -19,16 +19,16 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject;
 import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
 import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
-
+import java.util.TreeMap;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-import java.util.TreeMap;
-
 /**
  *  Test properties of the evaluator's datatypes and utility functions
  *  without actually creating any parse trees.
@@ -44,6 +44,21 @@ public class EvalUtilsTest extends EvaluationTestCase {
     return SkylarkDict.<Object, Object>of(env, 1, 1, 2, 2);
   }
 
+  private static SkylarkClassObject makeStruct(String field, Object value) {
+    return new SkylarkClassObject(ImmutableMap.of(field, value));
+  }
+
+  private static SkylarkClassObject makeBigStruct(Environment env) {
+    // struct(a=[struct(x={1:1}), ()], b=(), c={2:2})
+    return new SkylarkClassObject(ImmutableMap.<String, Object>of(
+        "a", MutableList.<Object>of(env,
+            new SkylarkClassObject(ImmutableMap.<String, Object>of(
+                "x", SkylarkDict.<Object, Object>of(env, 1, 1))),
+            Tuple.of()),
+        "b", Tuple.of(),
+        "c", SkylarkDict.<Object, Object>of(env, 2, 2)));
+  }
+
   @Test
   public void testEmptyStringToIterable() throws Exception {
     assertThat(EvalUtils.toIterable("", null)).isEmpty();
@@ -65,10 +80,15 @@ public class EvalUtilsTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testDatatypeMutability() throws Exception {
+  public void testDatatypeMutabilityPrimitive() throws Exception {
     assertTrue(EvalUtils.isImmutable("foo"));
     assertTrue(EvalUtils.isImmutable(3));
+  }
+
+  @Test
+  public void testDatatypeMutabilityShallow() throws Exception {
     assertTrue(EvalUtils.isImmutable(Tuple.of(1, 2, 3)));
+    assertTrue(EvalUtils.isImmutable(makeStruct("a", 1)));
 
     // Mutability depends on the environment.
     assertTrue(EvalUtils.isImmutable(makeList(null)));
@@ -78,6 +98,17 @@ public class EvalUtilsTest extends EvaluationTestCase {
   }
 
   @Test
+  public void testDatatypeMutabilityDeep() throws Exception {
+    assertTrue(EvalUtils.isImmutable(Tuple.<Object>of(makeList(null))));
+    assertTrue(EvalUtils.isImmutable(makeStruct("a", makeList(null))));
+    assertTrue(EvalUtils.isImmutable(makeBigStruct(null)));
+
+    assertFalse(EvalUtils.isImmutable(Tuple.<Object>of(makeList(env))));
+    assertFalse(EvalUtils.isImmutable(makeStruct("a", makeList(env))));
+    assertFalse(EvalUtils.isImmutable(makeBigStruct(env)));
+  }
+
+  @Test
   public void testComparatorWithDifferentTypes() throws Exception {
     TreeMap<Object, Object> map = new TreeMap<>(EvalUtils.SKYLARK_COMPARATOR);
     map.put(2, 3);
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java
index dae2667d54..5c6535e8c9 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java
@@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject;
 import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
 
@@ -26,6 +27,7 @@ import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -154,8 +156,19 @@ public class SkylarkNestedSetTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testNsetBadCompositeItem() throws Exception {
-    checkEvalError("sets cannot contain items of type 'struct'", "set([struct(a='a')])");
+  public void testNsetGoodCompositeItem() throws Exception {
+    eval("def func():",
+         "  return set([struct(a='a')])",
+         "s = func()");
+    Collection<Object> result = get("s").toCollection();
+    assertThat(result).hasSize(1);
+    assertThat(result.iterator().next()).isInstanceOf(SkylarkClassObject.class);
+  }
+
+  @Test
+  public void testNsetBadMutableItem() throws Exception {
+    checkEvalError("sets cannot contain mutable items", "set([([],)])");
+    checkEvalError("sets cannot contain mutable items", "set([struct(a=[])])");
   }
 
   @Test