bazel syntax: assert validity of results from Java calls
Other than legal Starlark values, the implicit conversion to Starlark
now accepts only null, List, and Map.
- make Attribute.{Computed,LateBound}Default implement StarlarkValue
- move SkylarkType.convertToSkylark to Starlark.fromJava.
- move SkylarkType.isSkylarkAcceptable to Starlark.valid.
- move support for legacy implicit NestedSet conversion to MethodDescriptor.
I was hoping to delete this but there are still a dozen or so clients to fix.
PiperOrigin-RevId: 281382879
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java
index 2a0ddc9..67d43eb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/AbstractConfiguredTarget.java
@@ -38,10 +38,9 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
-import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.SkylarkClassObject;
-import com.google.devtools.build.lib.syntax.SkylarkType;
+import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
@@ -144,10 +143,7 @@
// Depending on subclass, the 'actions' field will either be unsupported or of type
// java.util.List, which needs to be converted to Sequence before being returned.
Object result = get(name);
- if (result != null) {
- result = SkylarkType.convertToSkylark(result, (Mutability) null);
- }
- return result;
+ return result != null ? Starlark.fromJava(result, null) : null;
default:
return get(name);
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
index d7a61e9..f4ff4a9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttributesCollection.java
@@ -30,9 +30,7 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Sequence;
-import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.Starlark;
-import com.google.devtools.build.lib.syntax.StarlarkThread;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
@@ -166,12 +164,8 @@
// LABEL_DICT_UNARY was previously not treated as a dependency-bearing type, and was put into
// Skylark as a Map<String, Label>; this special case preserves that behavior temporarily.
if (type.getLabelClass() != LabelClass.DEPENDENCY || type == BuildType.LABEL_DICT_UNARY) {
- attrBuilder.put(
- skyname,
- val == null
- ? Starlark.NONE
- // Attribute values should be type safe
- : SkylarkType.convertToSkylark(val, (StarlarkThread) null));
+ // Attribute values should be type safe
+ attrBuilder.put(skyname, Starlark.fromJava(val, null));
return;
}
if (a.isExecutable()) {
@@ -227,8 +221,7 @@
for (TransitiveInfoCollection prereq : allPrereq) {
builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq)));
}
- attrBuilder.put(
- skyname, SkylarkType.convertToSkylark(builder.build(), (StarlarkThread) null));
+ attrBuilder.put(skyname, Starlark.fromJava(builder.build(), null));
} else if (type == BuildType.LABEL_DICT_UNARY) {
Map<Label, TransitiveInfoCollection> prereqsByLabel = new LinkedHashMap<>();
for (TransitiveInfoCollection target :
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java
index ad4314f..d6eb362 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkAttributeTransitionProvider.java
@@ -34,9 +34,7 @@
import com.google.devtools.build.lib.skylarkbuildapi.SplitTransitionProviderApi;
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.Starlark;
-import com.google.devtools.build.lib.syntax.StarlarkThread;
import java.util.LinkedHashMap;
import java.util.List;
@@ -94,9 +92,7 @@
LinkedHashMap<String, Object> attributes = new LinkedHashMap<>();
for (String attribute : attributeMap.getAttributeNames()) {
Object val = attributeMap.get(attribute, attributeMap.getAttributeType(attribute));
- attributes.put(
- Attribute.getSkylarkName(attribute),
- val == null ? Starlark.NONE : SkylarkType.convertToSkylark(val, (StarlarkThread) null));
+ attributes.put(Attribute.getSkylarkName(attribute), Starlark.fromJava(val, null));
}
attrObject = StructProvider.STRUCT.create(attributes, ERROR_MESSAGE_FOR_NO_ATTR);
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java
index df22024..84bf516 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkRuleTransitionProvider.java
@@ -30,9 +30,7 @@
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.StructProvider;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.Starlark;
-import com.google.devtools.build.lib.syntax.StarlarkThread;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Objects;
@@ -90,8 +88,7 @@
continue;
}
attributes.put(
- Attribute.getSkylarkName(attribute.getPublicName()),
- val == null ? Starlark.NONE : SkylarkType.convertToSkylark(val, (StarlarkThread) null));
+ Attribute.getSkylarkName(attribute.getPublicName()), Starlark.fromJava(val, null));
}
attrObject =
StructProvider.STRUCT.create(
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
index b6c0481..1b9973f 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
@@ -54,10 +54,8 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.Sequence;
-import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.OsUtils;
import com.google.devtools.build.lib.util.StringUtilities;
@@ -138,13 +136,9 @@
ImmutableMap.Builder<String, Object> attrBuilder = new ImmutableMap.Builder<>();
for (String name : attrs.getAttributeNames()) {
if (!name.equals("$local")) {
- Object val = attrs.getObject(name);
+ // Attribute values should be type safe
attrBuilder.put(
- Attribute.getSkylarkName(name),
- val == null
- ? Starlark.NONE
- // Attribute values should be type safe
- : SkylarkType.convertToSkylark(val, (StarlarkThread) null));
+ Attribute.getSkylarkName(name), Starlark.fromJava(attrs.getObject(name), null));
}
}
attrObject = StructProvider.STRUCT.create(attrBuilder.build(), "No such attribute '%s'");
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 30c6b62..ce7f1d7 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -41,6 +41,7 @@
import com.google.devtools.build.lib.packages.Type.LabelClass;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
@@ -1287,8 +1288,8 @@
* must be true:
*
* <ol>
- * <li>The other attribute must be declared in the computed default's constructor
- * <li>The other attribute must be non-configurable ({@link Builder#nonconfigurable}
+ * <li>The other attribute must be declared in the computed default's constructor
+ * <li>The other attribute must be non-configurable ({@link Builder#nonconfigurable}
* </ol>
*
* <p>The reason for enforced declarations is that, since attribute values might be configurable,
@@ -1300,7 +1301,7 @@
*
* <p>Implementations of this interface must be immutable.
*/
- public abstract static class ComputedDefault {
+ public abstract static class ComputedDefault implements SkylarkValue {
private final ImmutableList<String> dependencies;
/**
@@ -1610,7 +1611,7 @@
* Label}, or a {@link List} of {@link Label} objects.
*/
@Immutable
- public abstract static class LateBoundDefault<FragmentT, ValueT> {
+ public abstract static class LateBoundDefault<FragmentT, ValueT> implements SkylarkValue {
/**
* Functional interface for computing the value of a late-bound attribute.
*
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
index 85960f9..dcf2a77 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkInfo.java
@@ -30,9 +30,8 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.SkylarkClassObject;
-import com.google.devtools.build.lib.syntax.SkylarkType;
+import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.syntax.StarlarkThread;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
@@ -337,7 +336,10 @@
this.values = new Object[values.length];
for (int i = 0; i < values.length; i++) {
// TODO(b/74396075): Phase out this unnecessary conversion.
- this.values[i] = SkylarkType.convertToSkylark(values[i], (StarlarkThread) null);
+ // NB: fromJava treats null as None, but we need nulls to indicate a field is not present.
+ if (values[i] != null) {
+ this.values[i] = Starlark.fromJava(values[i], null);
+ }
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
index 2ac982d..f15297b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkNativeModule.java
@@ -39,7 +39,6 @@
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.NoneType;
import com.google.devtools.build.lib.syntax.Sequence;
-import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.SkylarkUtils;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.syntax.StarlarkList;
@@ -385,7 +384,7 @@
m.put(key, mapVal);
}
- return SkylarkType.convertToSkylark(m, mu);
+ return Starlark.fromJava(m, mu);
}
if (val.getClass().isAnonymousClass()) {
// Computed defaults. They will be represented as
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
index 51d7c6a..835d9b6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructImpl.java
@@ -31,7 +31,6 @@
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.SkylarkType;
import com.google.devtools.build.lib.syntax.Starlark;
-import com.google.devtools.build.lib.syntax.StarlarkThread;
import com.google.protobuf.TextFormat;
import java.io.Serializable;
import java.util.ArrayList;
@@ -67,9 +66,7 @@
Preconditions.checkNotNull(values);
ImmutableSortedMap.Builder<String, Object> builder = ImmutableSortedMap.naturalOrder();
for (Map.Entry<String, Object> e : values.entrySet()) {
- builder.put(
- Attribute.getSkylarkName(e.getKey()),
- SkylarkType.convertToSkylark(e.getValue(), (StarlarkThread) null));
+ builder.put(Attribute.getSkylarkName(e.getKey()), Starlark.fromJava(e.getValue(), null));
}
return builder.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Dict.java b/src/main/java/com/google/devtools/build/lib/syntax/Dict.java
index 06083b6..a8d7079 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Dict.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Dict.java
@@ -356,7 +356,7 @@
@SuppressWarnings("unchecked")
private <KK extends K, VV extends V> Dict<K, V> putAllUnsafe(Map<KK, VV> m) {
for (Map.Entry<KK, VV> e : m.entrySet()) {
- contents.put(e.getKey(), (VV) SkylarkType.convertToSkylark(e.getValue(), mutability));
+ contents.put(e.getKey(), (VV) Starlark.fromJava(e.getValue(), mutability));
}
return this;
}
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 b468fd5..10401f5 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
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
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.skylarkinterface.SkylarkInterfaceUtils;
@@ -87,8 +86,8 @@
return Integer.compare((Integer) o1, (Integer) o2);
}
- o1 = SkylarkType.convertToSkylark(o1, (StarlarkThread) null);
- o2 = SkylarkType.convertToSkylark(o2, (StarlarkThread) null);
+ o1 = Starlark.fromJava(o1, null);
+ o2 = Starlark.fromJava(o2, null);
if (o1 instanceof Sequence
&& o2 instanceof Sequence
@@ -169,20 +168,6 @@
|| 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)
- // TODO(adonovan): delete those below, and order those above by cost.
- // there is a registered Skylark ancestor class (useful e.g. when using AutoValue)
- || SkylarkInterfaceUtils.getSkylarkModule(c) != null
- || ImmutableMap.class.isAssignableFrom(c); // will be converted to Dict
- }
-
// TODO(bazel-team): move the following few type-related functions to SkylarkType
/**
* Return the Skylark-type of {@code c}
@@ -288,18 +273,6 @@
}
}
- public static Object checkNotNull(Expression expr, Object obj) throws EvalException {
- if (obj == null) {
- throw new EvalException(
- expr.getLocation(),
- "unexpected null value, please send a bug report. "
- + "This was generated by expression '"
- + expr
- + "'");
- }
- return obj;
- }
-
public static Collection<?> toCollection(Object o, Location loc) throws EvalException {
if (o instanceof Collection) {
return (Collection<?>) o;
@@ -549,24 +522,7 @@
// ClassObjects may have fields that are annotated with @SkylarkCallable.
// Since getValue() does not know about those, we cannot expect that result is a valid object.
if (result != null) {
- result = SkylarkType.convertToSkylark(result, thread);
- // If we access NestedSets using ClassObject.getValue() we won't know the generic type,
- // so we have to disable it. This should not happen.
-
- // TODO(bazel-team): Unify this check with the logic in getSkylarkType. Might
- // break some providers whose contents don't implement SkylarkValue, aren't wrapped in
- // Sequence, etc.
- // TODO(adonovan): this is still far too permissive. Replace with isSkylarkAcceptable.
- if (result instanceof NestedSet
- || (result instanceof List && !(result instanceof Sequence))) {
- throw new EvalException(
- loc,
- "internal error: type '"
- + result.getClass().getSimpleName()
- + "' is not allowed as a "
- + "Starlark value");
- }
- return result;
+ return Starlark.fromJava(result, thread.mutability());
}
}
if (method != null) {
@@ -983,10 +939,10 @@
throws EvalException, InterruptedException {
if (object instanceof SkylarkIndexable) {
Object result = ((SkylarkIndexable) object).getIndex(key, loc);
- // TODO(bazel-team): We shouldn't have this convertToSkylark call here. If it's needed at all,
+ // TODO(bazel-team): We shouldn't have this fromJava call here. If it's needed at all,
// it should go in the implementations of SkylarkIndexable#getIndex that produce non-Skylark
// values.
- return SkylarkType.convertToSkylark(result, thread);
+ return result == null ? null : Starlark.fromJava(result, thread.mutability());
} else if (object instanceof String) {
String string = (String) object;
int index = getSequenceIndex(key, string.length(), loc);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
index c9cc1f5..c1414a2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
@@ -17,10 +17,14 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
+import java.lang.reflect.WildcardType;
import java.util.Arrays;
/**
@@ -150,6 +154,9 @@
return Starlark.NONE;
}
if (result == null) {
+ // TODO(adonovan): eliminate allowReturnNones. Given that we convert
+ // String/Integer/Boolean/List/Map, it seems obtuse to crash instead
+ // of converting null too.
if (isAllowReturnNones()) {
return Starlark.NONE;
} else {
@@ -157,15 +164,37 @@
"method invocation returned None: " + getName() + Tuple.copyOf(Arrays.asList(args)));
}
}
- // TODO(bazel-team): get rid of this, by having everyone use the Skylark data structures
- result = SkylarkType.convertToSkylark(result, method, thread);
- if (result != null && !EvalUtils.isSkylarkAcceptable(result.getClass())) {
- throw new EvalException(
- loc,
- Printer.format(
- "method '%s' returns an object of invalid type %r", getName(), result.getClass()));
+
+ // TODO(adonovan): eliminate this hack.
+ // There are about a dozen methods that return NestedSet directly.
+ if (result instanceof NestedSet<?>) {
+ return SkylarkNestedSet.of(resultType(), (NestedSet<?>) result);
}
- return result;
+
+ // Careful: thread may be null when we are called by invokeStructField.
+ return Starlark.fromJava(result, thread != null ? thread.mutability() : null);
+ }
+
+ // legacy hack for NestedSet
+ private SkylarkType resultType() {
+ // This is where we can infer generic type information, so SkylarkNestedSets can be
+ // created in a safe way. Eventually we should probably do something with Lists and Maps too.
+ ParameterizedType t = (ParameterizedType) method.getGenericReturnType();
+ Type type = t.getActualTypeArguments()[0];
+ if (type instanceof Class) {
+ return SkylarkType.of((Class<?>) type);
+ }
+ if (type instanceof WildcardType) {
+ WildcardType wildcard = (WildcardType) type;
+ Type upperBound = wildcard.getUpperBounds()[0];
+ if (upperBound instanceof Class) {
+ // i.e. List<? extends SuperClass>
+ return SkylarkType.of((Class<?>) upperBound);
+ }
+ }
+ // It means someone annotated a method with @SkylarkCallable with no specific generic type info.
+ // We shouldn't annotate methods which return List<?> or List<T>.
+ throw new IllegalStateException("Cannot infer type from method signature " + method);
}
/** @see SkylarkCallable#name() */
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 755a328..d276581 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
@@ -181,7 +181,7 @@
// TODO(adonovan): enforce that we never construct a SkylarkNestedSet with a StarlarkType
// that represents a non-Skylark type (e.g. NestedSet<PathFragment>).
// One way to do that is to disallow constructing StarlarkTypes for classes
- // that would fail isSkylarkAcceptable; however remains the problem that
+ // that would fail Starlark.valid; however remains the problem that
// Object.class means "any Starlark value" but in fact allows any Java value.
public static <T> SkylarkNestedSet of(SkylarkType contentType, NestedSet<T> set) {
return new SkylarkNestedSet(contentType, set, null, null);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index 39fe0bf..7367f40 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -21,16 +21,10 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
-import java.lang.reflect.Method;
-import java.lang.reflect.ParameterizedType;
-import java.lang.reflect.Type;
-import java.lang.reflect.WildcardType;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
@@ -553,9 +547,6 @@
public static SkylarkType of(SkylarkType t1, SkylarkType t2) {
return of(ImmutableList.of(t1, t2));
}
- public static SkylarkType of(Class<?> t1, Class<?> t2) {
- return of(Simple.forClass(t1), Simple.forClass(t2));
- }
}
// TODO(adonovan): eliminate this function: a value may belong to many types.
@@ -728,63 +719,9 @@
return (Map<KEY_TYPE, VALUE_TYPE>) obj;
}
- private static Class<?> getGenericTypeFromMethod(Method method) {
- // This is where we can infer generic type information, so SkylarkNestedSets can be
- // created in a safe way. Eventually we should probably do something with Lists and Maps too.
- ParameterizedType t = (ParameterizedType) method.getGenericReturnType();
- Type type = t.getActualTypeArguments()[0];
- if (type instanceof Class) {
- return (Class<?>) type;
- }
- if (type instanceof WildcardType) {
- WildcardType wildcard = (WildcardType) type;
- Type upperBound = wildcard.getUpperBounds()[0];
- if (upperBound instanceof Class) {
- // i.e. List<? extends SuperClass>
- return (Class<?>) upperBound;
- }
- }
- // It means someone annotated a method with @SkylarkCallable with no specific generic type info.
- // We shouldn't annotate methods which return List<?> or List<T>.
- throw new IllegalStateException("Cannot infer type from method signature " + method);
- }
-
- /** Converts an object retrieved from a Java method to a Skylark-compatible type. */
- static Object convertToSkylark(Object object, Method method, @Nullable StarlarkThread thread) {
- if (object instanceof NestedSet<?>) {
- return SkylarkNestedSet.of(
- SkylarkType.of(getGenericTypeFromMethod(method)), (NestedSet<?>) object);
- }
- return convertToSkylark(object, thread);
- }
-
- /** Converts an object to a Skylark-compatible type if possible. */
- public static Object convertToSkylark(Object object, @Nullable StarlarkThread thread) {
- return convertToSkylark(object, thread == null ? null : thread.mutability());
- }
-
- /**
- * Converts an object to a Skylark-compatible type if possible.
- */
- public static Object convertToSkylark(Object object, @Nullable Mutability mutability) {
- if (object instanceof List && !(object instanceof Sequence)) {
- return StarlarkList.copyOf(mutability, (List<?>) object);
- }
- if (object instanceof SkylarkValue) {
- return object;
- }
- if (object instanceof Map) {
- return Dict.<Object, Object>copyOf(mutability, (Map<?, ?>) object);
- }
- // TODO(bazel-team): ensure everything is a SkylarkValue at all times.
- // Preconditions.checkArgument(EvalUtils.isSkylarkAcceptable(
- // object.getClass()),
- // "invalid object %s of class %s not convertible to a Skylark value",
- // object,
- // object.getClass());
- return object;
- }
-
+ // TODO(adonovan): eliminate 4 uses outside this package and make it private.
+ // The check is trivial (instanceof) and clients can usually produce a better
+ // error in context, without prematurely constructing a description.
public static void checkType(Object object, Class<?> type, @Nullable Object description)
throws EvalException {
if (!type.isInstance(object)) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java b/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
index 875f2d6..79214b2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Starlark.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.util.Pair;
+import java.util.List;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
@@ -79,6 +80,51 @@
}
/**
+ * Reports whether the argument is a legal Starlark value: a string, boolean, integer, or
+ * StarlarkValue.
+ */
+ public static boolean valid(Object x) {
+ return x instanceof SkylarkValue
+ || x instanceof String
+ || x instanceof Boolean
+ || x instanceof Integer;
+ }
+
+ /**
+ * Returns {@code x} if it is a {@link #valid} Starlark value, otherwise throws
+ * IllegalArgumentException.
+ */
+ public static <T> T checkValid(T x) {
+ if (!valid(x)) {
+ throw new IllegalArgumentException("invalid Starlark value: " + x.getClass());
+ }
+ return x;
+ }
+
+ /**
+ * Converts a Java value {@code x} to a Starlark one, if x is not already a valid Starlark value.
+ * A Java List or Map is converted to a Starlark list or dict, respectively, and null becomes
+ * {@link #NONE}. Any other non-Starlark value causes the function to throw
+ * IllegalArgumentException.
+ *
+ * <p>This function is applied to the results of @SkylarkCallable-annotated Java methods.
+ */
+ public static Object fromJava(Object x, @Nullable Mutability mutability) {
+ if (x == null) {
+ return NONE;
+ } else if (Starlark.valid(x)) {
+ return x;
+ } else if (x instanceof List) {
+ return StarlarkList.copyOf(mutability, (List<?>) x);
+ } else if (x instanceof Map) {
+ return Dict.copyOf(mutability, (Map<?, ?>) x);
+ } else {
+ throw new IllegalArgumentException(
+ "cannot expose internal type to Starlark: " + x.getClass());
+ }
+ }
+
+ /**
* Returns the truth value of a valid Starlark value, as if by the Starlark expression {@code
* bool(x)}.
*/
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
index 37e28b4..a75e311 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
@@ -23,6 +23,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.SkylarkInfo.Layout;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.EvalException;
import java.util.Map;
import javax.annotation.Nullable;
@@ -122,8 +123,9 @@
@Test
public void mutableIfContentsAreMutable() throws Exception {
SkylarkProvider provider = makeExportedProvider();
- SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, new Object());
- SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, new Object());
+ SkylarkValue v = new SkylarkValue() {};
+ SkylarkInfo mapInfo = makeSchemalessInfoWithF1F2Values(provider, 5, v);
+ SkylarkInfo compactInfo = makeSchemafulInfoWithF1F2Values(provider, 5, v);
assertThat(mapInfo.isImmutable()).isFalse();
assertThat(compactInfo.isImmutable()).isFalse();
}
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 33363fb..95f27fa 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
@@ -18,7 +18,6 @@
import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.packages.StructProvider;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
@@ -101,7 +100,6 @@
StarlarkList.of(thread, "1", "2", "3"),
Dict.of(thread, "key", 123),
Dict.of(thread, 123, "value"),
- NestedSetBuilder.stableOrder().add(1).add(2).add(3).build(),
StructProvider.STRUCT.create(ImmutableMap.of("key", (Object) "value"), "no field %s"),
};
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 902e964..6b8bb04 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -146,7 +146,7 @@
public void value() {}
@SkylarkCallable(name = "return_bad", documented = false)
public Bad returnBad() {
- return new Bad();
+ return new Bad(); // not a legal Starlark value
}
@SkylarkCallable(name = "struct_field", documented = false, structField = true)
public String structField() {
@@ -520,7 +520,8 @@
public Object getValue(String name) {
switch (name) {
case "field": return "a";
- case "nset": return NestedSetBuilder.stableOrder().build();
+ case "nset":
+ return NestedSetBuilder.stableOrder().build(); // not a legal Starlark value
default: return null;
}
}
@@ -1457,11 +1458,15 @@
}
@Test
- public void testJavaFunctionReturnsMutableObject() throws Exception {
- new SkylarkTest()
- .update("mock", new Mock())
- .testIfExactError(
- "method 'return_bad' returns an object of invalid type Bad", "mock.return_bad()");
+ public void testJavaFunctionReturnsIllegalValue() throws Exception {
+ update("mock", new Mock());
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, () -> eval("mock.return_bad()"));
+ assertThat(e)
+ .hasMessageThat()
+ .contains(
+ "cannot expose internal type to Starlark: class"
+ + " com.google.devtools.build.lib.syntax.SkylarkEvaluationTest$Bad");
}
@Test
@@ -1501,10 +1506,15 @@
}
@Test
- public void testClassObjectCannotAccessNestedSet() throws Exception {
- new SkylarkTest()
- .update("mock", new MockClassObject())
- .testIfErrorContains("internal error: type 'NestedSet' is not allowed", "v = mock.nset");
+ public void testFieldReturnsNestedSet() throws Exception {
+ update("mock", new MockClassObject());
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, () -> eval("mock.nset"));
+ assertThat(e)
+ .hasMessageThat()
+ .contains(
+ "cannot expose internal type to Starlark: class"
+ + " com.google.devtools.build.lib.collect.nestedset.NestedSet");
}
@Test
