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
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 8ca1d1a..f8ebe6b 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.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 @@
// 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 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 7619d67..7ba3d24 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 @@
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 @@
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 6803800..a8a6fdc 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 @@
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 832f038..a113754 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 @@
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 6b6608c..dd59ec9 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.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 @@
/**
* 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 @@
}
};
- // 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 @@
return ImmutableList.class.isAssignableFrom(c);
}
- /**
- * @return true if the specified value is immutable (suitable for use as a
- * dictionary key) according to the rules of the Build language.
- */
- 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
+ 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;
}
/**
- * Returns true if the type is immutable in the skylark 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 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;
- }
+ 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()));
+ }
+
+ /**
+ * 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 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 f47e939..cf16b68 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 @@
}
}
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 6c712c3..983f7fd 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 @@
*/
@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 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 2586306..bc00795 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 @@
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 @@
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 @@
} 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 @@
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 @@
* @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 9230d6b..098836a 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 @@
* 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 fb4f245..282a7af 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 @@
+ "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 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 4370ece..556c2db 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 @@
+ "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 @@
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 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, ")");
+ }
}
