From 6c10eac70123104a2b48eaf58075374e155ed12d Mon Sep 17 00:00:00 2001 From: Francois-Rene Rideau Date: Thu, 17 Sep 2015 19:17:20 +0000 Subject: 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 --- .../devtools/build/lib/actions/AbstractAction.java | 14 ++- .../devtools/build/lib/actions/Artifact.java | 14 ++- .../devtools/build/lib/syntax/BaseFunction.java | 13 ++- .../build/lib/syntax/BinaryOperatorExpression.java | 4 +- .../build/lib/syntax/DictComprehension.java | 1 + .../build/lib/syntax/DictionaryLiteral.java | 5 +- .../devtools/build/lib/syntax/EvalUtils.java | 116 ++++++++++++--------- .../build/lib/syntax/FuncallExpression.java | 6 +- .../google/devtools/build/lib/syntax/Label.java | 14 ++- .../google/devtools/build/lib/syntax/Printer.java | 36 ++----- .../google/devtools/build/lib/syntax/Runtime.java | 19 +++- .../devtools/build/lib/syntax/SkylarkList.java | 20 +++- .../build/lib/syntax/SkylarkNestedSet.java | 20 +++- 13 files changed, 189 insertions(+), 93 deletions(-) (limited to 'src/main/java') 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, ""); + } } 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 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> quasiImmutableClasses; - static { - try { - ImmutableSet.Builder> 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