starlark: relax Param.noneable to ease its removal
The semantics of noneable are now additive, not an override:
a client may specify allowedTypes={..., NoneType} and not mention
noneable, and the parameter will accept None.
Follow-up changes will remove all uses of noneable
in copybara (CL 336078992) and Bazel.
PiperOrigin-RevId: 336117855
diff --git a/src/main/java/net/starlark/java/annot/Param.java b/src/main/java/net/starlark/java/annot/Param.java
index 7cad3c5..210477c 100644
--- a/src/main/java/net/starlark/java/annot/Param.java
+++ b/src/main/java/net/starlark/java/annot/Param.java
@@ -51,12 +51,17 @@
* #allowedTypes}. Specifying neither {@code type} nor {@code allowedTypes} is equivalent to
* specifying the class of the parameter variable.
*/
- // TODO(adonovan): abolish this (and noneable). It creates ambiguity, complexity,and bugs.
- // Just specify allowedTypes, with empty array meaning "use the class of the parameter itself".
+ // Deprecated. Use allowedTypes.
Class<?> type() default Void.class;
/**
- * List of allowed types for the parameter if multiple types are allowed.
+ * List of allowed types for the parameter.
+ *
+ * <p>The array may be omitted, in which case the parameter accepts any value whose class is
+ * assignable to the class of the parameter variable.
+ *
+ * <p>If a function should accept None, NoneType should be in this list. (Currently one may set
+ * {@link #noneable} to achieve the same effect, but it is going away.)
*
* <p>May not be used in conjunction with {@link #type}.
*/
@@ -71,24 +76,18 @@
* runtime, so the Java method signature should use a generic type of Object and cast
* appropriately.
*/
+ // Deprecated. Use allowedTypes.
Class<?> generic1() default Object.class;
/**
- * Indicates whether this parameter accepts {@code None} as a value, its allowed types
- * notwithstanding.
+ * Indicates whether this parameter accepts {@code None} as a value, even if NoneType was not
+ * among {@link #allowedTypes}.
*
* <p>If true, {@code None} is accepted as a valid input in addition to the types mentioned by
* {@link #type} or {@link #allowedTypes}. In this case, the Java type of the corresponding method
* parameter must be {@code Object}.
- *
- * <p>If false, this parameter cannot be passed {@code None}, even if it would otherwise be
- * allowed by {@code type} or {@code allowedTypes}.
*/
- // TODO(starlark-team): Allow None as a value when noneable is false and the type is Object. But
- // look out for unwanted user-visible changes in the signatures of builtins.
- // TODO(140932420): Consider simplifying noneable by converting None to null, so that the Java
- // type need not be Object. But note that we still have the same problem for params whose default
- // value is the special "unbound" sentinel.
+ // Deprecated. Use allowedTypes={..., @ParamType(type=NoneType)}.
boolean noneable() default false;
/**
@@ -160,8 +159,4 @@
* {@link #defaultValue}.)
*/
String valueWhenDisabled() default "";
-
- // TODO(bazel-team): parse the type from a single field in Starlark syntax,
- // and allow a Union as "ThisType or ThatType or NoneType":
- // String type() default "Object";
}
diff --git a/src/main/java/net/starlark/java/annot/ParamType.java b/src/main/java/net/starlark/java/annot/ParamType.java
index 6ac3bb1..de1f8ca 100644
--- a/src/main/java/net/starlark/java/annot/ParamType.java
+++ b/src/main/java/net/starlark/java/annot/ParamType.java
@@ -34,5 +34,7 @@
* runtime, so the Java method signature should use a generic type of Object and cast
* appropriately.
*/
+ // TODO(adonovan): make this an array---a non-breaking change for most clients---
+ // ideally of the same length as the number of type parameters of type().
Class<?> generic1() default Object.class;
}
diff --git a/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java b/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java
index 9103eb0..7227937 100644
--- a/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java
+++ b/src/main/java/net/starlark/java/annot/processor/StarlarkMethodProcessor.java
@@ -309,26 +309,6 @@
Element param, Param paramAnnot, TypeMirror objectType, TypeMirror voidType) {
TypeMirror paramType = param.asType(); // type of the Java method parameter
- // A "noneable" parameter variable must accept the value None.
- // A parameter whose default is None must be noneable.
- if (paramAnnot.noneable()) {
- if (!types.isSameType(paramType, objectType)) {
- errorf(
- param,
- "Expected type 'Object' but got type '%s' for noneable parameter '%s'. The argument"
- + " for a noneable parameter may be None, so the java parameter must be"
- + " compatible with the type of None as well as possible non-None values.",
- paramType,
- param.getSimpleName());
- }
- } else if (paramAnnot.defaultValue().equals("None")) {
- errorf(
- param,
- "Parameter '%s' has 'None' default value but is not noneable. (If this is intended"
- + " as a mandatory parameter, leave the defaultValue field empty)",
- paramAnnot.name());
- }
-
// Give helpful hint for parameter of type Integer.
TypeMirror integerType = getType("java.lang.Integer");
if (types.isSameType(paramType, integerType)) {
@@ -361,17 +341,24 @@
}
}
- // Reject an Param.allowed_type if not assignable to parameter variable.
+ TypeMirror noneType = getType("net.starlark.java.eval.NoneType");
+ boolean allowsNoneType = false;
+
+ // Reject an entry of Param.allowedTypes if not assignable to the parameter variable.
for (ParamType paramTypeAnnot : paramAnnot.allowedTypes()) {
TypeMirror t = getParamTypeType(paramTypeAnnot);
if (!types.isAssignable(t, types.erasure(paramType))) {
errorf(
param,
- "annotated allowed_type %s of parameter '%s' is not assignable to variable of type %s",
+ "annotated allowedTypes entry %s of parameter '%s' is not assignable to variable of "
+ + "type %s",
t,
paramAnnot.name(),
paramType);
}
+ if (types.isSameType(t, noneType)) {
+ allowsNoneType = true;
+ }
}
// Reject generic types C<T> other than C<?>,
@@ -391,6 +378,26 @@
}
}
+ // A "noneable" parameter variable must accept the value None.
+ // A parameter whose default is None must be noneable.
+ if (paramAnnot.noneable()) {
+ if (!types.isSameType(paramType, objectType)) {
+ errorf(
+ param,
+ "Expected type 'Object' but got type '%s' for noneable parameter '%s'. The argument"
+ + " for a noneable parameter may be None, so the java parameter must be"
+ + " compatible with the type of None as well as possible non-None values.",
+ paramType,
+ param.getSimpleName());
+ }
+ } else if (paramAnnot.defaultValue().equals("None") && !allowsNoneType) {
+ errorf(
+ param,
+ "Parameter '%s' has 'None' default value but is not noneable. (If this is intended"
+ + " as a mandatory parameter, leave the defaultValue field empty)",
+ paramAnnot.name());
+ }
+
// Check sense of flag-controlled parameters.
boolean hasFlag = false;
if (!paramAnnot.enableOnlyWithFlag().isEmpty()) {
diff --git a/src/main/java/net/starlark/java/eval/BuiltinCallable.java b/src/main/java/net/starlark/java/eval/BuiltinCallable.java
index a3f1342..5ed34ae 100644
--- a/src/main/java/net/starlark/java/eval/BuiltinCallable.java
+++ b/src/main/java/net/starlark/java/eval/BuiltinCallable.java
@@ -350,19 +350,6 @@
"in call to %s(), parameter '%s' got value of type '%s', want '%s'",
methodName, param.getName(), Starlark.type(value), param.getTypeErrorMessage());
}
-
- // None is valid if and only if the parameter is marked noneable,
- // in which case the above check passes as the list of classes will include NoneType.
- // The reason for this check is to ensure that merely having type=Object.class
- // does not allow None as an argument value; I'm not sure why, that but that's the
- // historical behavior.
- //
- // We do this check second because the first check prints a better error
- // that enumerates the allowed types.
- if (value == Starlark.NONE && !param.isNoneable()) {
- throw Starlark.errorf(
- "in call to %s(), parameter '%s' cannot be None", methodName, param.getName());
- }
}
// Returns a phrase meaning "disabled" appropriate to the specified flag.
diff --git a/src/main/java/net/starlark/java/eval/ParamDescriptor.java b/src/main/java/net/starlark/java/eval/ParamDescriptor.java
index 811e505..f62b221 100644
--- a/src/main/java/net/starlark/java/eval/ParamDescriptor.java
+++ b/src/main/java/net/starlark/java/eval/ParamDescriptor.java
@@ -31,7 +31,6 @@
private final String name;
@Nullable private final Object defaultValue;
- private final boolean noneable;
private final boolean named;
private final boolean positional;
private final List<Class<?>> allowedClasses; // non-empty
@@ -42,14 +41,14 @@
private ParamDescriptor(
String name,
String defaultExpr,
- boolean noneable,
boolean named,
boolean positional,
List<Class<?>> allowedClasses,
@Nullable String disabledByFlag) {
this.name = name;
+ // TODO(adonovan): apply the same validation logic to the default value
+ // as we do to caller-supplied values (see BuiltinCallable.checkParamValue).
this.defaultValue = defaultExpr.isEmpty() ? null : evalDefault(name, defaultExpr);
- this.noneable = noneable;
this.named = named;
this.positional = positional;
this.allowedClasses = allowedClasses;
@@ -87,17 +86,14 @@
} else {
allowedClasses.add(param.type());
}
- if (param.noneable()) {
- // A few annotations redundantly declare NoneType.
- if (!allowedClasses.contains(NoneType.class)) {
- allowedClasses.add(NoneType.class);
- }
+
+ if (param.noneable() && !allowedClasses.contains(NoneType.class)) {
+ allowedClasses.add(NoneType.class);
}
return new ParamDescriptor(
param.name(),
defaultExpr,
- param.noneable(),
param.named(),
param.positional(),
allowedClasses,
@@ -118,11 +114,6 @@
return allowedClasses;
}
- /** @see Param#noneable() */
- boolean isNoneable() {
- return noneable;
- }
-
/** @see Param#positional() */
boolean isPositional() {
return positional;
diff --git a/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java b/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java
index 4f3a9d3a..b5f5ccf 100644
--- a/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java
+++ b/src/test/java/net/starlark/java/eval/StarlarkEvaluationTest.java
@@ -237,7 +237,7 @@
positional = false,
named = true),
@Param(
- name = "nonNoneable",
+ name = "acceptsAny",
type = Object.class,
defaultValue = "\"a\"",
positional = false,
@@ -267,7 +267,7 @@
boolean posOrNamed,
boolean named,
boolean optionalNamed,
- Object nonNoneable,
+ Object acceptsAny,
Object noneable,
Object multi) {
return "with_params("
@@ -281,7 +281,7 @@
+ ", "
+ optionalNamed
+ ", "
- + nonNoneable
+ + acceptsAny
+ (noneable != Starlark.NONE ? ", " + noneable : "")
+ (multi != Starlark.NONE ? ", " + multi : "")
+ ")";
@@ -312,7 +312,7 @@
positional = false,
named = true),
@Param(
- name = "nonNoneable",
+ name = "acceptsAny",
type = Object.class,
defaultValue = "\"a\"",
positional = false,
@@ -343,7 +343,7 @@
boolean posOrNamed,
boolean named,
boolean optionalNamed,
- Object nonNoneable,
+ Object acceptsAny,
Object noneable,
Object multi,
StarlarkThread thread) {
@@ -358,7 +358,7 @@
+ ", "
+ optionalNamed
+ ", "
- + nonNoneable
+ + acceptsAny
+ (noneable != Starlark.NONE ? ", " + noneable : "")
+ (multi != Starlark.NONE ? ", " + multi : "")
+ ", "
@@ -1166,9 +1166,9 @@
ev.new Scenario()
.update("mock", new Mock())
.setUp("")
- .testIfExactError(
- "in call to with_params(), parameter 'nonNoneable' cannot be None",
- "mock.with_params(1, True, True, named=True, optionalNamed=False, nonNoneable=None)");
+ .testExpression(
+ "mock.with_params(1, True, True, named=True, optionalNamed=False, acceptsAny=None)",
+ "with_params(1, true, true, true, false, None)");
ev.new Scenario()
.update("mock", new Mock())
diff --git a/src/test/java/net/starlark/java/eval/testdata/int_constructor.sky b/src/test/java/net/starlark/java/eval/testdata/int_constructor.sky
index 840b430..2ca2584 100644
--- a/src/test/java/net/starlark/java/eval/testdata/int_constructor.sky
+++ b/src/test/java/net/starlark/java/eval/testdata/int_constructor.sky
@@ -14,7 +14,7 @@
assert_eq(int(False), 0)
# from other
-int(None) ### parameter 'x' cannot be None
+int(None) ### got NoneType, want string, int, or bool
---
# from string
