Cleanup Skylark types some more

Clarify the criterion for being a valid Skylark value;
stop claiming immutability is "the" criterion when Skylark now has mutable values;
stop relying on a reflection with a magic list (this also fixes the SkylarkShell build).
Clarify the criterion for determining immutable types when making a SkylarkNestedSet.
Clarify and use the criterion for being a valid Skylark dict key.

--
MOS_MIGRATED_REVID=103313934

13 files changed, 189 insertions, 93 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index b5d8339df9..623ea96d96 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -28,6 +28,8 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.EventKind;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.Label;
+import com.google.devtools.build.lib.syntax.Printer;
+import com.google.devtools.build.lib.syntax.SkylarkValue;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -44,7 +46,7 @@ import javax.annotation.Nullable;
  * immutable.
  */
 @Immutable @ThreadSafe
-public abstract class AbstractAction implements Action {
+public abstract class AbstractAction implements Action, SkylarkValue {
 
   /**
    * An arbitrary default resource set. Currently 250MB of memory, 50% CPU and 0% of total I/O.
@@ -243,6 +245,16 @@ public abstract class AbstractAction implements Action {
     return "action '" + describe() + "'";
   }
 
+  @Override
+  public boolean isImmutable() {
+    return false;
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    Printer.append(buffer, prettyPrint()); // TODO(bazel-team): implement a readable representation
+  }
+
   /**
    * Deletes all of the action's output files, if they exist. If any of the
    * Artifacts refers to a directory recursively removes the contents of the
diff --git a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
index e620ef63c6..d9193966b6 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/Artifact.java
@@ -26,8 +26,10 @@ import com.google.devtools.build.lib.actions.Action.MiddlemanType;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.syntax.Label;
+import com.google.devtools.build.lib.syntax.Printer;
 import com.google.devtools.build.lib.syntax.SkylarkCallable;
 import com.google.devtools.build.lib.syntax.SkylarkModule;
+import com.google.devtools.build.lib.syntax.SkylarkValue;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -70,7 +72,7 @@ import javax.annotation.Nullable;
 @SkylarkModule(name = "File",
     doc = "This type represents a file used by the build system. It can be "
         + "either a source file or a derived file produced by a rule.")
-public class Artifact implements FileType.HasFilename, ActionInput {
+public class Artifact implements FileType.HasFilename, ActionInput, SkylarkValue {
 
   /**
    * Compares artifact according to their exec paths. Sorts null values first.
@@ -694,4 +696,14 @@ public class Artifact implements FileType.HasFilename, ActionInput {
     public Label getLabel() {
       return null;
     }};
+
+  @Override
+  public boolean isImmutable() {
+    return true;
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    Printer.append(buffer, toString()); // TODO(bazel-team): implement a readable representation
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index 8ca1d1ab67..f8ebe6b396 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -23,7 +23,6 @@ import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 
-import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -60,7 +59,7 @@ import javax.annotation.Nullable;
 // Provide optimized argument frobbing depending of FunctionSignature and CallerSignature
 // (that FuncallExpression must supply), optimizing for the all-positional and all-keyword cases.
 // Also, use better pure maps to minimize map O(n) re-creation events when processing keyword maps.
-public abstract class BaseFunction implements Serializable {
+public abstract class BaseFunction implements SkylarkValue {
 
   // The name of the function
   private final String name;
@@ -562,4 +561,14 @@ public abstract class BaseFunction implements Serializable {
   public Location getLocation() {
     return location;
   }
+
+  @Override
+  public boolean isImmutable() {
+    return true;
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    Printer.append(buffer, "<function " + getName() + ">");
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index 7619d67d43..7ba3d24282 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -262,7 +262,7 @@ public final class BinaryOperatorExpression extends Expression {
     if (lval instanceof List<?> && rval instanceof List<?>) {
       List<?> llist = (List<?>) lval;
       List<?> rlist = (List<?>) rval;
-      if (EvalUtils.isImmutable(llist) != EvalUtils.isImmutable(rlist)) {
+      if (EvalUtils.isTuple(llist) != EvalUtils.isTuple(rlist)) {
         throw new EvalException(getLocation(), "can only concatenate "
             + EvalUtils.getDataTypeName(rlist) + " (not \"" + EvalUtils.getDataTypeName(llist)
             + "\") to " + EvalUtils.getDataTypeName(rlist));
@@ -273,7 +273,7 @@ public final class BinaryOperatorExpression extends Expression {
         List<Object> result = Lists.newArrayListWithCapacity(llist.size() + rlist.size());
         result.addAll(llist);
         result.addAll(rlist);
-        return EvalUtils.makeSequence(result, EvalUtils.isImmutable(llist));
+        return EvalUtils.makeSequence(result, EvalUtils.isTuple(llist));
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
index 6803800c9c..a8a6fdcf06 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictComprehension.java
@@ -60,6 +60,7 @@ public class DictComprehension extends Expression {
     for (Object element : elements) {
       loopVar.assign(env, getLocation(), element);
       Object key = keyExpression.eval(env);
+      EvalUtils.checkValidDictKey(key);
       map.put(key, valueExpression.eval(env));
     }
     return ImmutableMap.copyOf(map);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
index 832f038401..a113754bf9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
@@ -77,7 +77,10 @@ public class DictionaryLiteral extends Expression {
       if (entry == null) {
         throw new EvalException(getLocation(), "null expression in " + this);
       }
-      map.put(entry.key.eval(env), entry.value.eval(env));
+      Object key = entry.key.eval(env);
+      EvalUtils.checkValidDictKey(key);
+      Object val = entry.value.eval(env);
+      map.put(key, val);
     }
     return ImmutableMap.copyOf(map);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 6b6608c89d..dd59ec9ff2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -17,12 +17,12 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Ordering;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.util.Collection;
 import java.util.Comparator;
@@ -33,7 +33,9 @@ import java.util.Set;
 /**
  * Utilities used by the evaluator.
  */
-public abstract class EvalUtils {
+public final class EvalUtils {
+
+  private EvalUtils() {}
 
   /**
    * The exception that SKYLARK_COMPARATOR might throw. This is an unchecked exception
@@ -91,26 +93,6 @@ public abstract class EvalUtils {
     }
   };
 
-  // TODO(bazel-team): Yet an other hack committed in the name of Skylark. One problem is that the
-  // syntax package cannot depend on actions so we have to have this until Actions are immutable.
-  // The other is that BuildConfigurations are technically not immutable but they cannot be modified
-  // from Skylark.
-  private static final ImmutableSet<Class<?>> quasiImmutableClasses;
-  static {
-    try {
-      ImmutableSet.Builder<Class<?>> builder = ImmutableSet.builder();
-      builder.add(Class.forName("com.google.devtools.build.lib.actions.Action"));
-      builder.add(Class.forName("com.google.devtools.build.lib.analysis.config.BuildConfiguration"));
-      builder.add(Class.forName("com.google.devtools.build.lib.actions.Root"));
-      quasiImmutableClasses = builder.build();
-    } catch (ClassNotFoundException e) {
-      throw new RuntimeException(e);
-    }
-  }
-
-  private EvalUtils() {
-  }
-
   /**
    * @return true if the specified sequence is a tuple; false if it's a modifiable list.
    */
@@ -123,40 +105,80 @@ public abstract class EvalUtils {
     return ImmutableList.class.isAssignableFrom(c);
   }
 
+  public static boolean isTuple(Object o) {
+    if (o instanceof SkylarkList) {
+      return ((SkylarkList) o).isTuple(); // tuples are immutable, lists are not.
+    }
+    if (o instanceof List<?>) {
+      return isTuple(o.getClass());
+    }
+    return false;
+  }
+
   /**
-   * @return true if the specified value is immutable (suitable for use as a
-   *         dictionary key) according to the rules of the Build language.
+   * Checks that an Object is a valid key for a Skylark dict.
+   * @param o an Object to validate
+   * @throws an EvalException if o is not a valid key
    */
-  public static boolean isImmutable(Object o) {
-    if (o instanceof Map<?, ?> || o instanceof BaseFunction) {
-      return false;
-    } else if (o instanceof List<?>) {
-      return isTuple((List<?>) o); // tuples are immutable, lists are not.
-    } else if (o instanceof SkylarkValue) {
-      return ((SkylarkValue) o).isImmutable();
-    } else {
-      return true; // string/int
+  static void checkValidDictKey(Object o) throws EvalException {
+    // TODO(bazel-team): check that all recursive elements are both Immutable AND Comparable.
+    if (isImmutable(o)) {
+      return;
     }
+    // Same error message as Python (that makes it a TypeError).
+    throw new EvalException(null, Printer.format("unhashable type: '%r'", o.getClass()));
   }
 
   /**
-   * Returns true if the type is immutable in the skylark language.
+   * Is this object known or assumed to be recursively immutable by Skylark?
+   * @param o an Object
+   * @return true if the object is known to be an immutable value.
    */
-  public static boolean isSkylarkImmutable(Class<?> c) {
-    if (c.isAnnotationPresent(Immutable.class)) {
-      return true;
-    } else if (c.equals(String.class) || c.equals(Integer.class) || c.equals(Boolean.class)
-        || SkylarkList.class.isAssignableFrom(c) || ImmutableMap.class.isAssignableFrom(c)
-        || NestedSet.class.isAssignableFrom(c)) {
-      return true;
-    } else {
-      for (Class<?> classObject : quasiImmutableClasses) {
-        if (classObject.isAssignableFrom(c)) {
-          return true;
-        }
+  public static boolean isImmutable(Object o) {
+    if (o instanceof SkylarkValue) {
+      return ((SkylarkValue) o).isImmutable();
+    }
+    if (!(o instanceof List<?>)) {
+      return isImmutable(o.getClass());
+    }
+    if (!isTuple((List<?>) o)) {
+      return false;
+    }
+    for (Object item : (List<?>) o) {
+      if (!isImmutable(item)) {
+        return false;
       }
     }
-    return false;
+    return true;
+  }
+
+  /**
+   * Is this class known to be *recursively* immutable by Skylark?
+   * For instance, class Tuple is not it, because it can contain mutable values.
+   * @param c a Class
+   * @return true if the class is known to represent only recursively immutable values.
+   */
+  // NB: This is used as the basis for accepting objects in SkylarkNestedSet-s,
+  // as well as for accepting objects as keys for Skylark dict-s.
+  static boolean isImmutable(Class<?> c) {
+    return c.isAnnotationPresent(Immutable.class) // TODO(bazel-team): beware of containers!
+        || c.equals(String.class)
+        || c.equals(Integer.class)
+        || c.equals(Boolean.class);
+  }
+
+  /**
+   * Returns true if the type is acceptable to be returned to the Skylark language.
+   */
+  public static boolean isSkylarkAcceptable(Class<?> c) {
+    return SkylarkValue.class.isAssignableFrom(c) // implements SkylarkValue
+        || c.equals(String.class) // basic values
+        || c.equals(Integer.class)
+        || c.equals(Boolean.class)
+        || c.isAnnotationPresent(SkylarkModule.class) // registered Skylark class
+        || ImmutableMap.class.isAssignableFrom(c) // will be converted to SkylarkDict
+        || NestedSet.class.isAssignableFrom(c) // will be converted to SkylarkNestedSet
+        || c.equals(PathFragment.class); // other known class
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index f47e939181..cf16b68f5d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -341,9 +341,9 @@ public final class FuncallExpression extends Expression {
         }
       }
       result = SkylarkType.convertToSkylark(result, method);
-      if (result != null && !EvalUtils.isSkylarkImmutable(result.getClass())) {
-        throw new EvalException(loc, "Method '" + methodName
-            + "' returns a mutable object (type of " + EvalUtils.getDataTypeName(result) + ")");
+      if (result != null && !EvalUtils.isSkylarkAcceptable(result.getClass())) {
+        throw new EvalException(loc, Printer.format(
+            "Method '%s' returns an object of invalid type %r", methodName, result.getClass()));
       }
       return result;
     } catch (IllegalAccessException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Label.java b/src/main/java/com/google/devtools/build/lib/syntax/Label.java
index 6c712c3ead..983f7fd84d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Label.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Label.java
@@ -39,7 +39,7 @@ import java.io.Serializable;
  */
 @SkylarkModule(name = "Label", doc = "A BUILD target identifier.")
 @Immutable @ThreadSafe
-public final class Label implements Comparable<Label>, Serializable {
+public final class Label implements Comparable<Label>, Serializable, SkylarkValue {
 
   /**
    * Factory for Labels from absolute string form. e.g.
@@ -406,4 +406,16 @@ public final class Label implements Comparable<Label>, Serializable {
   public static String print(Label label) {
     return label == null ? "(unknown)" : label.toString();
   }
+
+  @Override
+  public boolean isImmutable() {
+    return true;
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    // TODO(bazel-team): make the representation readable Label(//foo),
+    // and isolate the legacy functions that want the unreadable variant.
+    Printer.write(buffer, toString(), quotationMark);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
index 2586306bf0..bc00795625 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
-import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.io.IOException;
@@ -108,15 +107,15 @@ public final class Printer {
     if (o == null) {
       throw new NullPointerException(); // Java null is not a build language value.
 
+    } else if (o instanceof SkylarkValue) {
+      ((SkylarkValue) o).write(buffer, quotationMark);
+
     } else if (o instanceof String) {
       writeString(buffer, (String) o, quotationMark);
 
     } else if (o instanceof Integer || o instanceof Double) {
       append(buffer, o.toString());
 
-    } else if (o == Runtime.NONE) {
-      append(buffer, "None");
-
     } else if (o == Boolean.TRUE) {
       append(buffer, "True");
 
@@ -125,11 +124,7 @@ public final class Printer {
 
     } else if (o instanceof List<?>) {
       List<?> seq = (List<?>) o;
-      printList(buffer, seq, EvalUtils.isImmutable(seq), quotationMark);
-
-    } else if (o instanceof SkylarkList) {
-      SkylarkList list = (SkylarkList) o;
-      printList(buffer, list.toList(), list.isTuple(), quotationMark);
+      printList(buffer, seq, EvalUtils.isTuple(seq), quotationMark);
 
     } else if (o instanceof Map<?, ?>) {
       Map<?, ?> dict = (Map<?, ?>) o;
@@ -141,28 +136,11 @@ public final class Printer {
       append(buffer, ": ");
       write(buffer, entry.getValue(), quotationMark);
 
-    } else if (o instanceof SkylarkNestedSet) {
-      SkylarkNestedSet set = (SkylarkNestedSet) o;
-      append(buffer, "set(");
-      printList(buffer, set, "[", ", ", "]", null, quotationMark);
-      Order order = set.getOrder();
-      if (order != Order.STABLE_ORDER) {
-        append(buffer, ", order = \"" + order.getName() + "\"");
-      }
-      append(buffer, ")");
-
-    } else if (o instanceof BaseFunction) {
-      BaseFunction func = (BaseFunction) o;
-      append(buffer, "<function " + func.getName() + ">");
-
-    } else if (o instanceof Label) {
-      write(buffer, o.toString(), quotationMark);
-
     } else if (o instanceof PathFragment) {
       append(buffer, ((PathFragment) o).getPathString());
 
-    } else if (o instanceof SkylarkValue) {
-      ((SkylarkValue) o).write(buffer, quotationMark);
+    } else if (o instanceof Class<?>) {
+      append(buffer, EvalUtils.getDataTypeNameFromClass((Class<?>) o));
 
     } else {
       append(buffer, o.toString());
@@ -271,7 +249,7 @@ public final class Printer {
    * @param quotationMark The quotation mark to be used (' or ")
    * @return the Appendable, in fluent style.
    */
-  private static Appendable printList(
+  public static Appendable printList(
       Appendable buffer,
       Iterable<?> list,
       String before,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
index 9230d6be72..098836a26d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
@@ -43,10 +43,23 @@ public final class Runtime {
    * There should be only one instance of this type to allow "== None" tests.
    */
   @Immutable
-  public static final class NoneType {
-    @Override
-    public String toString() { return "None"; }
+  public static final class NoneType implements SkylarkValue {
     private NoneType() {}
+
+    @Override
+    public String toString() {
+      return "None";
+    }
+
+    @Override
+    public boolean isImmutable() {
+      return true;
+    }
+
+    @Override
+    public void write(Appendable buffer, char quotationMark) {
+      Printer.append(buffer, "None");
+    }
   }
 
   @SkylarkSignature(name = "None", returnType = NoneType.class,
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 fb4f2457d4..282a7af590 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
@@ -41,7 +41,7 @@ import java.util.List;
         + "error. Lists - just like everything - are immutable, therefore <code>x[1] = \"a\""
         + "</code> is not supported.")
 // TODO(bazel-team): should we instead have it implement List<Object> like ImmutableList does?
-public abstract class SkylarkList implements Iterable<Object> {
+public abstract class SkylarkList implements Iterable<Object>, SkylarkValue {
 
   private final boolean tuple;
   private final SkylarkType contentType;
@@ -412,4 +412,22 @@ public abstract class SkylarkList implements Iterable<Object> {
   public static SkylarkList tuple(Object... elements) {
     return new SimpleSkylarkList(ImmutableList.copyOf(elements), true, SkylarkType.TOP);
   }
+
+  @Override
+  public boolean isImmutable() {
+    if (!isTuple()) {
+      return false;
+    }
+    for (Object item : this) {
+      if (!EvalUtils.isImmutable(item)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    Printer.printList(buffer, toList(), isTuple(), quotationMark);
+  }
 }
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 4370eceb5c..556c2db4c3 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
@@ -74,7 +74,7 @@ import javax.annotation.Nullable;
         + "determined by the outer set, thus ignoring the <code>order</code> parameter of "
         + "nested sets.")
 @Immutable
-public final class SkylarkNestedSet implements Iterable<Object> {
+public final class SkylarkNestedSet implements Iterable<Object>, SkylarkValue {
 
   private final SkylarkType contentType;
   @Nullable private final List<Object> items;
@@ -185,7 +185,7 @@ public final class SkylarkNestedSet implements Iterable<Object> {
       throw new EvalException(
           loc, String.format("sets cannot contain items of type '%s'", itemType));
     }
-    if (!EvalUtils.isSkylarkImmutable(itemType.getType())) {
+    if (!EvalUtils.isImmutable(itemType.getType())) {
       throw new EvalException(
           loc, String.format("sets cannot contain items of type '%s' (mutable type)", itemType));
     }
@@ -247,4 +247,20 @@ public final class SkylarkNestedSet implements Iterable<Object> {
   public Order getOrder() {
     return set.getOrder();
   }
+
+  @Override
+  public boolean isImmutable() {
+    return true;
+  }
+
+  @Override
+  public void write(Appendable buffer, char quotationMark) {
+    Printer.append(buffer, "set(");
+    Printer.printList(buffer, this, "[", ", ", "]", null, quotationMark);
+    Order order = getOrder();
+    if (order != Order.STABLE_ORDER) {
+      Printer.append(buffer, ", order = \"" + order.getName() + "\"");
+    }
+    Printer.append(buffer, ")");
+  }
 }