bazel syntax: optimize calls to built-in methods
This change rewrites CallUtils.convertStarlarkArgumentsToJavaMethodArguments,
now called BuiltinCallable.getArgumentVector, to process arguments in three
steps---positional, named, defaults---avoiding all memory allocation
except for the argument vector itself. Evaluation of parameter default
value expressions has been moved into ParamDescriptor, so that concurrent
hash map lookups are avoided.
Also, the error messages have been improved in many ways
- they are more concise, and don't dump the whole signature.
- they state facts before expectations.
- they are better phrased (subject first, emphasis last).
- the called function is always named, but simply, not in fussy detail.
- the numbers of accepted parameters are stated, when relevant.
- the incorrect type is shown after a failed type check.
- unexpected keywords are spell-checked.
- plurals are correct.
The logic is somewhat simpler.
Except in lib.syntax itself, test expectations of these error messages
have been edited down to the crucial parts, for robustness.
This is a breaking API change for copybara.
BEGIN_PUBLIC
bazel syntax: optimize calls to built-in methods
END_PUBLIC
PiperOrigin-RevId: 288725155
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
index 7b3ff5d..2b7a08e 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkCallable.java
@@ -127,11 +127,15 @@
* than are explicitly allowed by the method signature. If this is defined, all additional
* positional arguments are passed as elements of a {@link Tuple<Object>} to the method.
*
- * <p>See python's <code>*args</code> (http://thepythonguru.com/python-args-and-kwargs/).
+ * <p>See Python's <code>*args</code> (http://thepythonguru.com/python-args-and-kwargs/).
*
* <p>If defined, the annotated method must declare a corresponding parameter to which a {@code
* Tuple<Object>} may be assigned. See the interface-level javadoc for details.
*/
+ // TODO(adonovan): consider using a simpler type than Param here. All that's needed at run-time
+ // is a boolean. The doc tools want a name and doc string, but the rest is irrelevant and
+ // distracting.
+ // Ditto extraKeywords.
Param extraPositionals() default @Param(name = "");
/**
@@ -141,7 +145,7 @@
* explicitly declared by the method signature. If this is defined, all additional named arguments
* are passed as elements of a {@link Dict<String, Object>} to the method.
*
- * <p>See python's <code>**kwargs</code> (http://thepythonguru.com/python-args-and-kwargs/).
+ * <p>See Python's <code>**kwargs</code> (http://thepythonguru.com/python-args-and-kwargs/).
*
* <p>If defined, the annotated method must declare a corresponding parameter to which a {@code
* Dict<String, Object>} may be assigned. See the interface-level javadoc for details.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
index 7a3f247..a95d20f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
@@ -13,8 +13,16 @@
// limitations under the License.
package com.google.devtools.build.lib.syntax;
+import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.util.SpellChecker;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.LinkedHashMap;
+import java.util.List;
import javax.annotation.Nullable;
/**
@@ -33,6 +41,7 @@
* Constructs a BuiltinCallable for a StarlarkCallable-annotated method of the given name (as seen
* by Starlark, not Java).
*/
+ // TODO(adonovan): eliminate calls to this constructor from tests; use getattr instead.
BuiltinCallable(Object obj, String methodName) {
this(obj, methodName, /*desc=*/ null);
}
@@ -58,22 +67,12 @@
throws EvalException, InterruptedException {
MethodDescriptor desc =
this.desc != null ? this.desc : getMethodDescriptor(thread.getSemantics());
- Object objValue = obj;
-
- if (obj instanceof String) {
- // Prepend string receiver to argument list.
- // TODO(adonovan): move this into convertStarlarkArgumentsToJavaMethodArguments.
- Object[] arr = new Object[positional.length + 1];
- arr[0] = obj;
- System.arraycopy(positional, 0, arr, 1, positional.length);
- positional = arr;
- objValue = StringModule.INSTANCE;
- }
-
- Object[] javaArguments =
- CallUtils.convertStarlarkArgumentsToJavaMethodArguments(
- thread, methodName, desc, objValue.getClass(), positional, named);
- return desc.call(objValue, javaArguments, thread.mutability());
+ Preconditions.checkArgument(
+ !desc.isStructField(),
+ "struct field methods should be handled by DotExpression separately");
+ Object[] vector = getArgumentVector(thread, loc, desc, positional, named);
+ return desc.call(
+ obj instanceof String ? StringModule.INSTANCE : obj, vector, thread.mutability());
}
private MethodDescriptor getMethodDescriptor(StarlarkSemantics semantics) {
@@ -99,4 +98,272 @@
public void repr(Printer printer) {
printer.append("<built-in function " + methodName + ">");
}
+
+ @Override
+ public String toString() {
+ return methodName;
+ }
+
+ /**
+ * Converts the arguments of a Starlark call into the argument vector for a reflective call to a
+ * SkylarkCallable-annotated Java method.
+ *
+ * @param thread the Starlark thread for the call
+ * @param loc the location of the call expression, or BUILTIN for calls from Java
+ * @param desc descriptor for the StarlarkCallable-annotated method
+ * @param positional a list of positional arguments
+ * @param named a list of named arguments, as alternating Strings/Objects. May contain dups.
+ * @return the array of arguments which may be passed to {@link MethodDescriptor#call}
+ * @throws EvalException if the given set of arguments are invalid for the given method. For
+ * example, if any arguments are of unexpected type, or not all mandatory parameters are
+ * specified by the user
+ */
+ private Object[] getArgumentVector(
+ StarlarkThread thread,
+ Location loc,
+ MethodDescriptor desc, // intentionally shadows this.desc
+ Object[] positional,
+ Object[] named)
+ throws EvalException {
+
+ // Overview of steps:
+ // - allocate vector of actual arguments of correct size.
+ // - process positional arguments, accumulating surplus ones into *args.
+ // - process named arguments, accumulating surplus ones into **kwargs.
+ // - set default values for missing optionals, and report missing mandatory parameters.
+ // - set special parameters.
+ // The static checks ensure that positional parameters appear before named,
+ // and mandatory positionals appear before optional.
+ // No additional memory allocation occurs in the common (success) case.
+ // Flag-disabled parameters are skipped during argument matching, as if they do not exist. They
+ // are instead assigned their flag-disabled values.
+
+ ParamDescriptor[] parameters = desc.getParameters();
+
+ // Allocate argument vector.
+ int n = parameters.length;
+ if (desc.acceptsExtraArgs()) {
+ n++;
+ }
+ if (desc.acceptsExtraKwargs()) {
+ n++;
+ }
+ if (desc.isUseLocation()) {
+ n++;
+ }
+ if (desc.isUseStarlarkThread()) {
+ n++;
+ }
+ Object[] vector = new Object[n];
+
+ // positional arguments
+ int paramIndex = 0;
+ int argIndex = 0;
+ if (obj instanceof String) {
+ // String methods get the string as an extra argument
+ // because their true receiver is StringModule.INSTANCE.
+ vector[paramIndex++] = obj;
+ }
+ for (; argIndex < positional.length && paramIndex < parameters.length; paramIndex++) {
+ ParamDescriptor param = parameters[paramIndex];
+ if (!param.isPositional()) {
+ break;
+ }
+
+ // disabled?
+ StarlarkSemantics.FlagIdentifier flag = param.disabledByFlag();
+ if (flag != null) {
+ // Skip disabled parameter as if not present at all.
+ // The default value will be filled in below.
+ continue;
+ }
+
+ Object value = positional[argIndex++];
+ checkParamValue(param, value);
+ vector[paramIndex] = value;
+ }
+
+ // *args
+ Tuple<Object> varargs = null;
+ if (desc.acceptsExtraArgs()) {
+ varargs = Tuple.wrap(Arrays.copyOfRange(positional, argIndex, positional.length));
+ } else if (argIndex < positional.length) {
+ if (argIndex == 0) {
+ throw Starlark.errorf("%s() got unexpected positional argument", methodName);
+ } else {
+ throw Starlark.errorf(
+ "%s() accepts no more than %d positional argument%s but got %d",
+ methodName, argIndex, plural(argIndex), positional.length);
+ }
+ }
+
+ // named arguments
+ LinkedHashMap<String, Object> kwargs = desc.acceptsExtraKwargs() ? new LinkedHashMap<>() : null;
+ for (int i = 0; i < named.length; i += 2) {
+ String name = (String) named[i]; // safe
+ Object value = named[i + 1];
+
+ // look up parameter
+ int index = desc.getParameterIndex(name);
+ // unknown parameter?
+ if (index < 0) {
+ // spill to **kwargs
+ if (kwargs == null) {
+ List<String> allNames =
+ Arrays.stream(parameters)
+ .map(ParamDescriptor::getName)
+ .collect(ImmutableList.toImmutableList());
+ throw Starlark.errorf(
+ "%s() got unexpected keyword argument '%s'%s",
+ methodName, name, SpellChecker.didYouMean(name, allNames));
+ }
+
+ // duplicate named argument?
+ if (kwargs.put(name, value) != null) {
+ throw Starlark.errorf(
+ "%s() got multiple values for keyword argument '%s'", methodName, name);
+ }
+ continue;
+ }
+ ParamDescriptor param = parameters[index];
+
+ // positional-only param?
+ if (!param.isNamed()) {
+ // spill to **kwargs
+ if (kwargs == null) {
+ throw Starlark.errorf(
+ "%s() got named argument for positional-only parameter '%s'", methodName, name);
+ }
+
+ // duplicate named argument?
+ if (kwargs.put(name, value) != null) {
+ throw Starlark.errorf(
+ "%s() got multiple values for keyword argument '%s'", methodName, name);
+ }
+ continue;
+ }
+
+ // disabled?
+ StarlarkSemantics.FlagIdentifier flag = param.disabledByFlag();
+ if (flag != null) {
+ // spill to **kwargs
+ if (kwargs == null) {
+ throw Starlark.errorf(
+ "in call to %s(), parameter '%s' is %s",
+ methodName, param.getName(), disabled(flag, thread.getSemantics()));
+ }
+
+ // duplicate named argument?
+ if (kwargs.put(name, value) != null) {
+ throw Starlark.errorf(
+ "%s() got multiple values for keyword argument '%s'", methodName, name);
+ }
+ continue;
+ }
+
+ checkParamValue(param, value);
+
+ // duplicate?
+ if (vector[index] != null) {
+ throw Starlark.errorf("%s() got multiple values for argument '%s'", methodName, name);
+ }
+
+ vector[index] = value;
+ }
+
+ // Set default values for missing parameters,
+ // and report any that are still missing.
+ List<String> missingPositional = null;
+ List<String> missingNamed = null;
+ for (int i = 0; i < parameters.length; i++) {
+ if (vector[i] == null) {
+ ParamDescriptor param = parameters[i];
+ vector[i] = param.getDefaultValue();
+ if (vector[i] == null) {
+ if (param.isPositional()) {
+ if (missingPositional == null) {
+ missingPositional = new ArrayList<>();
+ }
+ missingPositional.add(param.getName());
+ } else {
+ if (missingNamed == null) {
+ missingNamed = new ArrayList<>();
+ }
+ missingNamed.add(param.getName());
+ }
+ }
+ }
+ }
+ if (missingPositional != null) {
+ throw Starlark.errorf(
+ "%s() missing %d required positional argument%s: %s",
+ methodName,
+ missingPositional.size(),
+ plural(missingPositional.size()),
+ Joiner.on(", ").join(missingPositional));
+ }
+ if (missingNamed != null) {
+ throw Starlark.errorf(
+ "%s() missing %d required named argument%s: %s",
+ methodName,
+ missingNamed.size(),
+ plural(missingNamed.size()),
+ Joiner.on(", ").join(missingNamed));
+ }
+
+ // special parameters
+ int i = parameters.length;
+ if (desc.acceptsExtraArgs()) {
+ vector[i++] = varargs;
+ }
+ if (desc.acceptsExtraKwargs()) {
+ vector[i++] = Dict.wrap(thread.mutability(), kwargs);
+ }
+ if (desc.isUseLocation()) {
+ vector[i++] = loc;
+ }
+ if (desc.isUseStarlarkThread()) {
+ vector[i++] = thread;
+ }
+
+ return vector;
+ }
+
+ private static String plural(int n) {
+ return n == 1 ? "" : "s";
+ }
+
+ private void checkParamValue(ParamDescriptor param, Object value) throws EvalException {
+ // invalid argument?
+ SkylarkType type = param.getSkylarkType();
+ if (!type.contains(value)) {
+ throw Starlark.errorf(
+ "in call to %s(), parameter '%s' got value of type '%s', want '%s'",
+ methodName, param.getName(), EvalUtils.getDataTypeName(value), type);
+ }
+
+ // unexpected None?
+ 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.
+ private static String disabled(
+ StarlarkSemantics.FlagIdentifier flag, StarlarkSemantics semantics) {
+ // If the flag is True, it must be a deprecation flag. Otherwise it's an experimental flag.
+ // TODO(adonovan): is that assumption sound?
+ if (semantics.flagValue(flag)) {
+ return String.format(
+ "deprecated and will be removed soon. It may be temporarily re-enabled by setting"
+ + " --%s=false",
+ flag.getFlagName());
+ } else {
+ return String.format(
+ "experimental and thus unavailable with the current flags. It may be enabled by setting"
+ + " --%s",
+ flag.getFlagName());
+ }
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
index cc52b63..0360c6a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/CallUtils.java
@@ -14,30 +14,19 @@
package com.google.devtools.build.lib.syntax;
-import com.google.common.base.Joiner;
-import com.google.common.base.Preconditions;
import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
-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.Maps;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
-import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier;
import com.google.devtools.build.lib.util.Pair;
import java.lang.reflect.Method;
-import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.HashMap;
-import java.util.LinkedHashMap;
-import java.util.List;
import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import javax.annotation.Nullable;
@@ -207,276 +196,4 @@
getSelfCallMethodDescriptor(StarlarkSemantics.DEFAULT_SEMANTICS, objClass);
return selfCall == null ? null : selfCall.getAnnotation();
}
-
- /**
- * Converts Starlark-defined arguments to an array of argument {@link Object}s that may be passed
- * to a given callable-from-Starlark Java method.
- *
- * @param thread the Starlark thread for the call
- * @param methodName the named of the called method
- * @param call the syntax tree of the call expression
- * @param method a descriptor for a java method callable from Starlark
- * @param objClass the class of the java object on which to invoke this method
- * @param positional a list of positional arguments
- * @param named a list of named arguments, as alternating Strings/Objects. May contain dups.
- * @return the array of arguments which may be passed to {@link MethodDescriptor#call}
- * @throws EvalException if the given set of arguments are invalid for the given method. For
- * example, if any arguments are of unexpected type, or not all mandatory parameters are
- * specified by the user
- */
- // TODO(adonovan): move to BuiltinCallable
- static Object[] convertStarlarkArgumentsToJavaMethodArguments(
- StarlarkThread thread,
- String methodName,
- MethodDescriptor method,
- Class<?> objClass,
- Object[] positional,
- Object[] named)
- throws EvalException {
- Preconditions.checkArgument(!method.isStructField(),
- "struct field methods should be handled by DotExpression separately");
-
- // TODO(adonovan): optimize and simplify this function and improve the error messages.
- // In particular, don't build a map unless isAcceptsExtraArgs();
- // instead, make two passes, the first over positional+named,
- // the second over the vacant parameters.
-
- LinkedHashMap<String, Object> kwargs = Maps.newLinkedHashMapWithExpectedSize(named.length / 2);
- for (int i = 0; i < named.length; i += 2) {
- String name = (String) named[i]; // safe
- Object value = named[i + 1];
- if (kwargs.put(name, value) != null) {
- throw Starlark.errorf("duplicate argument '%s' in call to '%s'", name, methodName);
- }
- }
-
- ImmutableList<ParamDescriptor> parameters = method.getParameters();
- // TODO(adonovan): opt: compute correct size and use an array.
- final int extraArgsCount = 4; // *args, **kwargs, Location, StarlarkThread
- List<Object> builder = new ArrayList<>(parameters.size() + extraArgsCount);
-
- int argIndex = 0;
-
- // Process parameters specified in callable.parameters()
- // Positional parameters are always enumerated before non-positional parameters,
- // And default-valued positional parameters are always enumerated after other positional
- // parameters. These invariants are validated by the SkylarkCallable annotation processor.
- // Index is used deliberately, since usage of iterators adds a significant overhead
- for (int i = 0; i < parameters.size(); ++i) {
- ParamDescriptor param = parameters.get(i);
- SkylarkType type = param.getSkylarkType();
- Object value;
-
- if (param.isDisabledInCurrentSemantics()) {
- value = evalDefault(param.getName(), param.getValueOverride());
- builder.add(value);
- continue;
- }
-
- Object namedValue = param.isNamed() ? kwargs.remove(param.getName()) : null;
-
- if (argIndex < positional.length
- && param.isPositional()) { // Positional args and params remain.
- value = positional[argIndex];
- if (!type.contains(value)) {
- throw argumentMismatchException(
- String.format(
- "expected value of type '%s' for parameter '%s'", type, param.getName()),
- method,
- objClass);
- }
- if (namedValue != null) {
- throw argumentMismatchException(
- String.format("got multiple values for keyword argument '%s'", param.getName()),
- method,
- objClass);
- }
- argIndex++;
- } else { // No more positional arguments, or no more positional parameters.
- if (namedValue != null) {
- // Param specified by keyword argument.
- value = namedValue;
- if (!type.contains(value)) {
- throw argumentMismatchException(
- String.format(
- "expected value of type '%s' for parameter '%s'", type, param.getName()),
- method,
- objClass);
- }
- } else { // Param not specified by user. Use default value.
- if (param.getDefaultValue().isEmpty()) {
- throw unspecifiedParameterException(param, method, objClass, kwargs);
- }
- value = evalDefault(param.getName(), param.getDefaultValue());
- }
- }
- if (value == Starlark.NONE && !param.isNoneable()) {
- throw argumentMismatchException(
- String.format("parameter '%s' cannot be None", param.getName()), method, objClass);
- }
- builder.add(value);
- }
-
- // *args
- if (method.isAcceptsExtraArgs()) {
- builder.add(Tuple.wrap(Arrays.copyOfRange(positional, argIndex, positional.length)));
- } else if (argIndex < positional.length) {
- throw argumentMismatchException(
- String.format(
- "expected no more than %s positional arguments, but got %s",
- argIndex, positional.length),
- method,
- objClass);
- }
-
- // **kwargs
- if (method.isAcceptsExtraKwargs()) {
- builder.add(Dict.wrap(thread.mutability(), kwargs));
- } else if (!kwargs.isEmpty()) {
- throw unexpectedKeywordArgumentException(kwargs.keySet(), method, objClass, thread);
- }
-
- if (method.isUseLocation()) {
- builder.add(thread.getCallerLocation());
- }
- if (method.isUseStarlarkThread()) {
- builder.add(thread);
- }
- return builder.toArray();
- }
-
- private static EvalException unspecifiedParameterException(
- ParamDescriptor param,
- MethodDescriptor method,
- Class<?> objClass,
- Map<String, Object> kwargs) {
- if (kwargs.containsKey(param.getName())) {
- return argumentMismatchException(
- String.format("parameter '%s' may not be specified by name", param.getName()),
- method,
- objClass);
- } else {
- return argumentMismatchException(
- String.format("parameter '%s' has no default value", param.getName()),
- method,
- objClass);
- }
- }
-
- private static EvalException unexpectedKeywordArgumentException(
- Set<String> unexpectedKeywords,
- MethodDescriptor method,
- Class<?> objClass,
- StarlarkThread thread) {
- // Check if any of the unexpected keywords are for parameters which are disabled by the
- // current semantic flags. Throwing an error with information about the misconfigured
- // semantic flag is likely far more helpful.
- for (ParamDescriptor param : method.getParameters()) {
- if (param.isDisabledInCurrentSemantics() && unexpectedKeywords.contains(param.getName())) {
- FlagIdentifier flagIdentifier = param.getFlagResponsibleForDisable();
- // If the flag is True, it must be a deprecation flag. Otherwise it's an experimental flag.
- if (thread.getSemantics().flagValue(flagIdentifier)) {
- return new EvalException(
- null,
- String.format(
- "parameter '%s' is deprecated and will be removed soon. It may be "
- + "temporarily re-enabled by setting --%s=false",
- param.getName(), flagIdentifier.getFlagName()));
- } else {
- return new EvalException(
- null,
- String.format(
- "parameter '%s' is experimental and thus unavailable with the current "
- + "flags. It may be enabled by setting --%s",
- param.getName(), flagIdentifier.getFlagName()));
- }
- }
- }
-
- return argumentMismatchException(
- String.format(
- "unexpected keyword%s %s",
- unexpectedKeywords.size() > 1 ? "s" : "",
- Joiner.on(", ").join(Iterables.transform(unexpectedKeywords, s -> "'" + s + "'"))),
- method,
- objClass);
- }
-
- private static EvalException argumentMismatchException(
- String errorDescription, MethodDescriptor methodDescriptor, Class<?> objClass) {
- if (methodDescriptor.isSelfCall() || SkylarkInterfaceUtils.hasSkylarkGlobalLibrary(objClass)) {
- return Starlark.errorf(
- "%s, for call to function %s",
- errorDescription, formatMethod(objClass, methodDescriptor));
- } else {
- return Starlark.errorf(
- "%s, for call to method %s of '%s'",
- errorDescription,
- formatMethod(objClass, methodDescriptor),
- EvalUtils.getDataTypeNameFromClass(objClass));
- }
- }
-
- private static String formatMethod(Class<?> objClass, MethodDescriptor methodDescriptor) {
- ImmutableList.Builder<String> argTokens = ImmutableList.builder();
- // Skip first parameter ('self') for StringModule, as its a special case.
- Iterable<ParamDescriptor> parameters =
- objClass == StringModule.class
- ? Iterables.skip(methodDescriptor.getParameters(), 1)
- : methodDescriptor.getParameters();
-
- for (ParamDescriptor paramDescriptor : parameters) {
- if (!paramDescriptor.isDisabledInCurrentSemantics()) {
- if (paramDescriptor.getDefaultValue().isEmpty()) {
- argTokens.add(paramDescriptor.getName());
- } else {
- argTokens.add(paramDescriptor.getName() + " = " + paramDescriptor.getDefaultValue());
- }
- }
- }
- if (methodDescriptor.isAcceptsExtraArgs()) {
- argTokens.add("*args");
- }
- if (methodDescriptor.isAcceptsExtraKwargs()) {
- argTokens.add("**kwargs");
- }
- return methodDescriptor.getName() + "(" + Joiner.on(", ").join(argTokens.build()) + ")";
- }
-
- // A memoization of evalDefault, keyed by expression.
- // This cache is manually maintained (instead of using LoadingCache),
- // as default values may sometimes be recursively requested.
- private static final ConcurrentHashMap<String, Object> defaultValueCache =
- new ConcurrentHashMap<>();
-
- // Evaluates the default value expression for a parameter.
- private static Object evalDefault(String name, String expr) {
- if (expr.isEmpty()) {
- return Starlark.NONE;
- }
- Object x = defaultValueCache.get(expr);
- if (x != null) {
- return x;
- }
- try (Mutability mutability = Mutability.create("initialization")) {
- // Note that this Starlark thread ignores command line flags.
- StarlarkThread thread =
- StarlarkThread.builder(mutability)
- .useDefaultSemantics()
- .setGlobals(Module.createForBuiltins(Starlark.UNIVERSE))
- .build();
- thread.getGlobals().put("unbound", Starlark.UNBOUND);
- x = EvalUtils.eval(ParserInput.fromLines(expr), thread);
- defaultValueCache.put(expr, x);
- return x;
- } catch (Exception ex) {
- if (ex instanceof InterruptedException) {
- Thread.currentThread().interrupt();
- }
- throw new IllegalArgumentException(
- String.format(
- "while evaluating default value %s of parameter %s: %s", expr, name, ex.getMessage()),
- ex);
- }
- }
}
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 02e101f..b8997b4 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
@@ -16,7 +16,6 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
-import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
@@ -28,7 +27,7 @@
* metadata. This is needed because the annotation is sometimes in a superclass.
*
* <p>The annotation metadata is duplicated in this class to avoid usage of Java dynamic proxies
- * which are ~7X slower.
+ * which are ~7× slower.
*/
final class MethodDescriptor {
private final Method method;
@@ -38,9 +37,9 @@
private final String doc;
private final boolean documented;
private final boolean structField;
- private final ImmutableList<ParamDescriptor> parameters;
- private final ParamDescriptor extraPositionals;
- private final ParamDescriptor extraKeywords;
+ private final ParamDescriptor[] parameters;
+ private final boolean extraPositionals;
+ private final boolean extraKeywords;
private final boolean selfCall;
private final boolean allowReturnNones;
private final boolean useLocation;
@@ -54,9 +53,9 @@
String doc,
boolean documented,
boolean structField,
- ImmutableList<ParamDescriptor> parameters,
- ParamDescriptor extraPositionals,
- ParamDescriptor extraKeywords,
+ ParamDescriptor[] parameters,
+ boolean extraPositionals,
+ boolean extraKeywords,
boolean selfCall,
boolean allowReturnNones,
boolean useLocation,
@@ -98,9 +97,9 @@
annotation.structField(),
Arrays.stream(annotation.parameters())
.map(param -> ParamDescriptor.of(param, semantics))
- .collect(ImmutableList.toImmutableList()),
- ParamDescriptor.of(annotation.extraPositionals(), semantics),
- ParamDescriptor.of(annotation.extraKeywords(), semantics),
+ .toArray(ParamDescriptor[]::new),
+ !annotation.extraPositionals().name().isEmpty(),
+ !annotation.extraKeywords().name().isEmpty(),
annotation.selfCall(),
annotation.allowReturnNones(),
annotation.useLocation(),
@@ -205,30 +204,31 @@
return allowReturnNones;
}
- /** @see SkylarkCallable#extraPositionals() */
- ParamDescriptor getExtraPositionals() {
+ /** @return {@code true} if this method accepts extra arguments ({@code *args}) */
+ boolean acceptsExtraArgs() {
return extraPositionals;
}
- ParamDescriptor getExtraKeywords() {
+ /** @see SkylarkCallable#extraKeywords() */
+ boolean acceptsExtraKwargs() {
return extraKeywords;
}
- /** @return {@code true} if this method accepts extra arguments ({@code *args}) */
- boolean isAcceptsExtraArgs() {
- return !getExtraPositionals().getName().isEmpty();
- }
-
- /** @see SkylarkCallable#extraKeywords() */
- boolean isAcceptsExtraKwargs() {
- return !getExtraKeywords().getName().isEmpty();
- }
-
/** @see SkylarkCallable#parameters() */
- ImmutableList<ParamDescriptor> getParameters() {
+ ParamDescriptor[] getParameters() {
return parameters;
}
+ /** Returns the index of the named parameter or -1 if not found. */
+ int getParameterIndex(String name) {
+ for (int i = 0; i < parameters.length; i++) {
+ if (parameters[i].getName().equals(name)) {
+ return i;
+ }
+ }
+ return -1;
+ }
+
/** @see SkylarkCallable#documented() */
boolean isDocumented() {
return documented;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java b/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java
index cf16770..f669219 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java
@@ -18,13 +18,14 @@
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.ParamType;
import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier;
+import java.util.concurrent.ConcurrentHashMap;
import javax.annotation.Nullable;
/** A value class for storing {@link Param} metadata to avoid using Java proxies. */
final class ParamDescriptor {
private final String name;
- private final String defaultValue;
+ @Nullable private final Object defaultValue;
private final Class<?> type;
private final Class<?> generic1;
private final boolean noneable;
@@ -33,40 +34,29 @@
// While the type can be inferred completely by the Param annotation, this tuple allows for the
// type of a given parameter to be determined only once, as it is an expensive operation.
private final SkylarkType skylarkType;
-
- // The next two fields relate to toggling this parameter via semantic flag -- they will
- // be null if and only if this parameter is enabled, and will otherwise contain information
- // about what to do with the disabled parameter. (If the parameter is 'disabled', it will be
- // treated as unusable from Starlark.)
-
- // The value of this disabled parameter (as interpreted in Starlark) will be passed to the Java
- // method.
- @Nullable private final String valueOverride;
- // The flag responsible for disabling this parameter. If a user attempts to use this disabled
- // parameter from Starlark, this identifier can be used to create the appropriate error message.
- @Nullable private final FlagIdentifier flagResponsibleForDisable;
+ // The semantics flag responsible for disabling this parameter, or null if enabled.
+ // It is an error for Starlark code to supply a value to a disabled parameter.
+ @Nullable private final FlagIdentifier disabledByFlag;
private ParamDescriptor(
String name,
- String defaultValue,
+ @Nullable String defaultExpr,
Class<?> type,
Class<?> generic1,
boolean noneable,
boolean named,
boolean positional,
SkylarkType skylarkType,
- @Nullable String valueOverride,
- @Nullable FlagIdentifier flagResponsibleForDisable) {
+ @Nullable FlagIdentifier disabledByFlag) {
this.name = name;
- this.defaultValue = defaultValue;
+ this.defaultValue = defaultExpr.isEmpty() ? null : evalDefault(name, defaultExpr);
this.type = type;
this.generic1 = generic1;
this.noneable = noneable;
this.named = named;
this.positional = positional;
this.skylarkType = skylarkType;
- this.valueOverride = valueOverride;
- this.flagResponsibleForDisable = flagResponsibleForDisable;
+ this.disabledByFlag = disabledByFlag;
}
/**
@@ -78,22 +68,20 @@
Class<?> generic = param.generic1();
boolean noneable = param.noneable();
- boolean isParamEnabledWithCurrentSemantics =
- starlarkSemantics.isFeatureEnabledBasedOnTogglingFlags(
- param.enableOnlyWithFlag(), param.disableWithFlag());
-
- String valueOverride = null;
- FlagIdentifier flagResponsibleForDisable = FlagIdentifier.NONE;
- if (!isParamEnabledWithCurrentSemantics) {
- valueOverride = param.valueWhenDisabled();
- flagResponsibleForDisable =
+ String defaultExpr = param.defaultValue();
+ FlagIdentifier disabledByFlag = null;
+ if (!starlarkSemantics.isFeatureEnabledBasedOnTogglingFlags(
+ param.enableOnlyWithFlag(), param.disableWithFlag())) {
+ defaultExpr = param.valueWhenDisabled();
+ disabledByFlag =
param.enableOnlyWithFlag() != FlagIdentifier.NONE
? param.enableOnlyWithFlag()
: param.disableWithFlag();
}
+
return new ParamDescriptor(
param.name(),
- param.defaultValue(),
+ defaultExpr,
type,
generic,
noneable,
@@ -101,8 +89,7 @@
|| (param.legacyNamed() && !starlarkSemantics.incompatibleRestrictNamedParams()),
param.positional(),
getType(type, generic, param.allowedTypes(), noneable),
- valueOverride,
- flagResponsibleForDisable);
+ disabledByFlag);
}
/** @see Param#name() */
@@ -158,8 +145,9 @@
return named;
}
- /** @see Param#defaultValue() */
- String getDefaultValue() {
+ /** Returns the effective default value of this parameter, or null if mandatory. */
+ @Nullable
+ Object getDefaultValue() {
return defaultValue;
}
@@ -167,36 +155,55 @@
return skylarkType;
}
- /** Returns true if this parameter is disabled under the current skylark semantic flags. */
- boolean isDisabledInCurrentSemantics() {
- return valueOverride != null;
+ /** Returns the flag responsible for disabling this parameter, or null if it is enabled. */
+ @Nullable
+ FlagIdentifier disabledByFlag() {
+ return disabledByFlag;
}
- /**
- * Returns the value the parameter should take, given that the parameter is disabled under the
- * current skylark semantics.
- *
- * @throws IllegalStateException if invoked when {@link #isDisabledInCurrentSemantics()} is false
- */
- String getValueOverride() {
- Preconditions.checkState(
- isDisabledInCurrentSemantics(),
- "parameter is not disabled under the current semantic flags. getValueOverride should be "
- + "called only if isParameterDisabled is true");
- return valueOverride;
+ // A memoization of evalDefault, keyed by expression.
+ // This cache is manually maintained (instead of using LoadingCache),
+ // as default values may sometimes be recursively requested.
+ private static final ConcurrentHashMap<String, Object> defaultValueCache =
+ new ConcurrentHashMap<>();
+
+ // Evaluates the default value expression for a parameter.
+ private static Object evalDefault(String name, String expr) {
+ // Common cases; also needed for bootstrapping UNIVERSE.
+ if (expr.equals("None")) {
+ return Starlark.NONE;
+ } else if (expr.equals("True")) {
+ return true;
+ } else if (expr.equals("False")) {
+ return false;
+ } else if (expr.equals("unbound")) {
+ return Starlark.UNBOUND;
+ }
+
+ Object x = defaultValueCache.get(expr);
+ if (x != null) {
+ return x;
+ }
+ try (Mutability mutability = Mutability.create("initialization")) {
+ // Note that this Starlark thread ignores command line flags.
+ StarlarkThread thread =
+ StarlarkThread.builder(mutability)
+ .useDefaultSemantics()
+ .setGlobals(Module.createForBuiltins(Starlark.UNIVERSE))
+ .build();
+ thread.getGlobals().put("unbound", Starlark.UNBOUND);
+ x = EvalUtils.eval(ParserInput.fromLines(expr), thread);
+ defaultValueCache.put(expr, x);
+ return x;
+ } catch (Exception ex) {
+ if (ex instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
+ throw new IllegalArgumentException(
+ String.format(
+ "while evaluating default value '%s' of parameter '%s': %s",
+ expr, name, ex.getMessage()));
+ }
}
- /**
- * Returns the flag responsible for disabling this parameter, given that the parameter is disabled
- * under the current skylark semantics.
- *
- * @throws IllegalStateException if invoked when {@link #isDisabledInCurrentSemantics()} is false
- */
- FlagIdentifier getFlagResponsibleForDisable() {
- Preconditions.checkState(
- isDisabledInCurrentSemantics(),
- "parameter is not disabled under the current semantic flags. getFlagResponsibleForDisable "
- + " should be called only if isParameterDisabled is true");
- return flagResponsibleForDisable;
- }
}
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 ea925d3..c74d721 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
@@ -288,11 +288,12 @@
// TODO(adonovan): logically this should be a parameter.
StarlarkSemantics semantics = StarlarkSemantics.DEFAULT_SEMANTICS;
for (String name : CallUtils.getMethodNames(semantics, v.getClass())) {
- // We pass desc=null instead of the descriptor that CallUtils.getMethod would
- // return because DEFAULT_SEMANTICS is probably incorrect for the call.
+ // We use the 2-arg (desc=null) BuiltinCallable constructor instead of passing
+ // the descriptor that CallUtils.getMethod would return,
+ // because DEFAULT_SEMANTICS is probably incorrect for the call.
// The effect is that the default semantics determine which methods appear in
// env, but the thread's semantics determine which method calls succeed.
- env.put(name, new BuiltinCallable(v, name, /*desc=*/ null));
+ env.put(name, new BuiltinCallable(v, name));
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 4e5bf23..3b85a63 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -142,16 +142,16 @@
@Test
public void testExportsFilesVisibilityMustBeSequence() throws Exception {
expectEvalError(
- "expected value of type 'sequence or NoneType' for parameter 'visibility', "
- + "for call to method exports_files",
+ "in call to exports_files(), parameter 'visibility' got value of type 'depset', want"
+ + " 'sequence or NoneType'",
"exports_files(srcs=[], visibility=depset(['notice']))");
}
@Test
public void testExportsFilesLicensesMustBeSequence() throws Exception {
expectEvalError(
- "expected value of type 'sequence of strings or NoneType' for parameter 'licenses', "
- + "for call to method exports_files",
+ "in call to exports_files(), parameter 'licenses' got value of type 'depset', want"
+ + " 'sequence of strings or NoneType'",
"exports_files(srcs=[], licenses=depset(['notice']))");
}
@@ -647,7 +647,7 @@
events.setFailFast(false);
assertGlobFails(
"glob(['incl'],['excl'],3,True,'extraarg')",
- "expected no more than 4 positional arguments, but got 5, for call to method glob");
+ "glob() accepts no more than 4 positional arguments but got 5");
}
@Test
@@ -655,8 +655,8 @@
events.setFailFast(false);
assertGlobFails(
"glob(1, exclude=2)",
- "expected value of type 'sequence of strings' for parameter 'include', "
- + "for call to method glob");
+ "in call to glob(), parameter 'include' got value of type 'int', want 'sequence of"
+ + " strings'");
}
@Test
@@ -820,8 +820,7 @@
@Test
public void testPackageGroupNamedArguments() throws Exception {
expectEvalError(
- "expected no more than 0 positional arguments, but got 1,",
- "package_group('skin', name = 'x')");
+ "package_group() got unexpected positional argument", "package_group('skin', name = 'x')");
}
@Test
@@ -1042,7 +1041,7 @@
@Test
public void testIncompleteEnvironmentGroup() throws Exception {
expectEvalError(
- "parameter 'defaults' has no default value, for call to function environment_group",
+ "environment_group() missing 1 required named argument: defaults",
"environment(name = 'foo')",
"environment_group(name='group', environments = [':foo'])");
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
index ac0c181..ee3be76 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java
@@ -4588,56 +4588,58 @@
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
assertThat(e)
.hasMessageThat()
- .contains("parameter 'toolchain_identifier' has no default value");
+ .contains("missing 1 required named argument: toolchain_identifier");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredHostSystemName() throws Exception {
setupSkylarkRuleForStringFieldsTesting("host_system_name");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'host_system_name' has no default value");
+ assertThat(e).hasMessageThat().contains("missing 1 required named argument: host_system_name");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredTargetSystemName() throws Exception {
setupSkylarkRuleForStringFieldsTesting("target_system_name");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'target_system_name' has no default value");
+ assertThat(e)
+ .hasMessageThat()
+ .contains("missing 1 required named argument: target_system_name");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredTargetCpu() throws Exception {
setupSkylarkRuleForStringFieldsTesting("target_cpu");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'target_cpu' has no default value");
+ assertThat(e).hasMessageThat().contains("missing 1 required named argument: target_cpu");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredTargetLibc() throws Exception {
setupSkylarkRuleForStringFieldsTesting("target_libc");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'target_libc' has no default value");
+ assertThat(e).hasMessageThat().contains("missing 1 required named argument: target_libc");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredCompiler() throws Exception {
setupSkylarkRuleForStringFieldsTesting("compiler");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'compiler' has no default value");
+ assertThat(e).hasMessageThat().contains("missing 1 required named argument: compiler");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredAbiVersion() throws Exception {
setupSkylarkRuleForStringFieldsTesting("abi_version");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'abi_version' has no default value");
+ assertThat(e).hasMessageThat().contains("missing 1 required named argument: abi_version");
}
@Test
public void testCcToolchainInfoFromSkylarkRequiredAbiLibcVersion() throws Exception {
setupSkylarkRuleForStringFieldsTesting("abi_libc_version");
AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget("//foo:r"));
- assertThat(e).hasMessageThat().contains("parameter 'abi_libc_version' has no default value");
+ assertThat(e).hasMessageThat().contains("missing 1 required named argument: abi_libc_version");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiTest.java
index 9d102cf..c19f77c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiTest.java
@@ -277,7 +277,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a:r");
- assertContainsEvent("expected value of type 'JavaRuntimeInfo' for parameter 'host_javabase'");
+ assertContainsEvent("got value of type 'Target', want 'JavaRuntimeInfo'");
}
@Test
@@ -2096,7 +2096,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a:r");
- assertContainsEvent("expected value of type 'JavaRuntimeInfo' for parameter 'host_javabase'");
+ assertContainsEvent("got value of type 'Target', want 'JavaRuntimeInfo'");
}
@Test
@@ -2135,8 +2135,7 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//a:r");
- assertContainsEvent(
- "expected value of type 'JavaToolchainInfo' for parameter 'java_toolchain'");
+ assertContainsEvent("got value of type 'Target', want 'JavaToolchainInfo'");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibraryTest.java
index 17f54f9..ed4c9f3 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibraryTest.java
@@ -713,9 +713,7 @@
AssertionError expected =
assertThrows(AssertionError.class, () ->
getConfiguredTarget("//examples/apple_skylark:my_target"));
- assertThat(expected)
- .hasMessageThat()
- .contains("unexpected keyword 'foo', for call to function AppleStaticLibrary");
+ assertThat(expected).hasMessageThat().contains("unexpected keyword argument 'foo'");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java
index 1a4b234..5613823 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyInfoTest.java
@@ -122,13 +122,12 @@
@Test
public void starlarkConstructorErrors_TransitiveSources() throws Exception {
checkEvalErrorContains(
- "'transitive_sources' has no default value", //
+ "missing 1 required named argument: transitive_sources", //
"PyInfo()");
checkEvalErrorContains(
- "expected value of type 'depset of Files' for parameter 'transitive_sources'",
- "PyInfo(transitive_sources = 'abc')");
+ "got value of type 'string', want 'depset of Files'", "PyInfo(transitive_sources = 'abc')");
checkEvalErrorContains(
- "expected value of type 'depset of Files' for parameter 'transitive_sources'",
+ "got value of type 'depset', want 'depset of Files'",
"PyInfo(transitive_sources = depset(direct=['abc']))");
checkEvalErrorContains(
"'transitive_sources' field should be a postorder-compatible depset of Files",
@@ -138,31 +137,31 @@
@Test
public void starlarkConstructorErrors_UsesSharedLibraries() throws Exception {
checkEvalErrorContains(
- "expected value of type 'bool' for parameter 'uses_shared_libraries'",
+ "got value of type 'string', want 'bool'",
"PyInfo(transitive_sources = depset([]), uses_shared_libraries = 'abc')");
}
@Test
public void starlarkConstructorErrors_Imports() throws Exception {
checkEvalErrorContains(
- "expected value of type 'depset of strings' for parameter 'imports'",
+ "got value of type 'string', want 'depset of strings'",
"PyInfo(transitive_sources = depset([]), imports = 'abc')");
checkEvalErrorContains(
- "expected value of type 'depset of strings' for parameter 'imports'",
+ "got value of type 'depset', want 'depset of strings'",
"PyInfo(transitive_sources = depset([]), imports = depset(direct=[123]))");
}
@Test
public void starlarkConstructorErrors_HasPy2OnlySources() throws Exception {
checkEvalErrorContains(
- "expected value of type 'bool' for parameter 'has_py2_only_sources'",
+ "got value of type 'string', want 'bool'",
"PyInfo(transitive_sources = depset([]), has_py2_only_sources = 'abc')");
}
@Test
public void starlarkConstructorErrors_HasPy3OnlySources() throws Exception {
checkEvalErrorContains(
- "expected value of type 'bool' for parameter 'has_py3_only_sources'",
+ "got value of type 'string', want 'bool'",
"PyInfo(transitive_sources = depset([]), has_py3_only_sources = 'abc')");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
index b4c293c..882b043 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/python/PyRuntimeInfoTest.java
@@ -143,14 +143,14 @@
@Test
public void starlarkConstructorErrors_Files() throws Exception {
checkEvalErrorContains(
- "expected value of type 'depset of Files or NoneType' for parameter 'files'",
+ "got value of type 'string', want 'depset of Files or NoneType'",
"PyRuntimeInfo(",
" interpreter = dummy_interpreter,",
" files = 'abc',",
" python_version = 'PY2',",
")");
checkEvalErrorContains(
- "expected value of type 'depset of Files or NoneType' for parameter 'files'",
+ "got value of type 'depset', want 'depset of Files or NoneType'",
"PyRuntimeInfo(",
" interpreter = dummy_interpreter,",
" files = depset(['abc']),",
@@ -168,7 +168,7 @@
@Test
public void starlarkConstructorErrors_PythonVersion() throws Exception {
checkEvalErrorContains(
- "parameter 'python_version' has no default value",
+ "missing 1 required named argument: python_version",
"PyRuntimeInfo(",
" interpreter_path = '/system/interpreter',",
")");
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 8b9dd80..7841fdb 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -394,8 +394,7 @@
"str",
"\t\tstr.index(1)"
+ System.lineSeparator()
- + "expected value of type 'string' for parameter 'sub', for call to method "
- + "index(sub, start = 0, end = None) of 'string'");
+ + "in call to index(), parameter 'sub' got value of type 'int', want 'string'");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 2301288..b9c9be4 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -458,10 +458,7 @@
@Test
public void testNonLabelAttrWithProviders() throws Exception {
checkEvalErrorContains(
- "unexpected keyword 'providers', for call to method "
- + "string(default = '', doc = '', mandatory = False, values = []) "
- + "of 'attr (a language module)'",
- "attr.string(providers = ['a'])");
+ "unexpected keyword argument 'providers'", "attr.string(providers = ['a'])");
}
private static final RuleClass.ConfiguredTargetFactory<Object, Object, Exception>
@@ -546,11 +543,7 @@
@Test
public void testAttrDefaultValueBadType() throws Exception {
- checkEvalErrorContains(
- "expected value of type 'string' for parameter 'default', for call to method "
- + "string(default = '', doc = '', mandatory = False, values = []) "
- + "of 'attr (a language module)'",
- "attr.string(default = 1)");
+ checkEvalErrorContains("got value of type 'int', want 'string'", "attr.string(default = 1)");
}
@Test
@@ -580,10 +573,7 @@
@Test
public void testAttrBadKeywordArguments() throws Exception {
checkEvalErrorContains(
- "unexpected keyword 'bad_keyword', for call to method "
- + "string(default = '', doc = '', mandatory = False, values = []) of "
- + "'attr (a language module)'",
- "attr.string(bad_keyword = '')");
+ "string() got unexpected keyword argument 'bad_keyword'", "attr.string(bad_keyword = '')");
}
@Test
@@ -650,11 +640,7 @@
@Test
public void testAttrDocValueBadType() throws Exception {
- checkEvalErrorContains(
- "expected value of type 'string' for parameter 'doc', for call to method "
- + "string(default = '', doc = '', mandatory = False, values = []) "
- + "of 'attr (a language module)'",
- "attr.string(doc = 1)");
+ checkEvalErrorContains("got value of type 'int', want 'string'", "attr.string(doc = 1)");
}
@Test
@@ -670,7 +656,9 @@
}
@Test
- public void testLateBoundAttrWorksWithOnlyLabel() throws Exception {
+ public void testFunctionAsAttrDefault() throws Exception {
+ exec("def f(): pass");
+
// Late-bound attributes, which are computed during analysis as a function
// of the configuration, are only available for attributes involving labels:
// attr.label
@@ -678,48 +666,34 @@
// attr.label_keyed_string_dict
// attr.output,
// attr.output_list
- checkEvalErrorContains(
- "expected value of type 'string' for parameter 'default', for call to method "
- + "string(default = '', doc = '', mandatory = False, values = []) "
- + "of 'attr (a language module)'",
- "def attr_value(cfg): return 'a'",
- "attr.string(default=attr_value)");
- }
+ // (See testRuleClassImplicitOutputFunctionDependingOnComputedAttribute
+ // for a more detailed positive test.)
+ evalAndExport(
+ "attr.label(default=f)",
+ "attr.label_list(default=f)",
+ "attr.label_keyed_string_dict(default=f)");
+ // Note: the default parameter of attr.output{,_list} is deprecated
+ // (see --incompatible_no_output_attr_default)
- @Test
- public void testNoComputedAttrDefaults() throws Exception {
- // This is a regression test for github.com/bazelbuild/bazel/issues/9463.
- // The loading-phase feature, computed attribute defaults, is not exposed
- // to Starlark.
- // (Not to be confused with "late-bound defaults", an analysis-phase
- // mechanism only for attributes involving labels; see test above.)
+ // For all other attribute types, the default value may not be a function.
//
- // Most attributes like attr.string should not accept a function as
- // their default value. (The bug was that the @SkylarkCallable
+ // (This is a regression test for github.com/bazelbuild/bazel/issues/9463.
+ // The loading-phase feature of "computed attribute defaults" is not exposed
+ // to Starlark; the bug was that the @SkylarkCallable
// annotation was more permissive than the method declaration.)
- exec("def f(): pass");
+ checkEvalErrorContains("got value of type 'function', want 'string'", "attr.string(default=f)");
checkEvalErrorContains(
- "expected value of type 'string' for parameter 'default'", "attr.string(default=f)");
+ "got value of type 'function', want 'sequence of strings'", "attr.string_list(default=f)");
+ checkEvalErrorContains("got value of type 'function', want 'int'", "attr.int(default=f)");
checkEvalErrorContains(
- "expected value of type 'sequence of strings' for parameter 'default'",
- "attr.string_list(default=f)");
+ "got value of type 'function', want 'sequence of ints'", "attr.int_list(default=f)");
+ checkEvalErrorContains("got value of type 'function', want 'bool'", "attr.bool(default=f)");
checkEvalErrorContains(
- "expected value of type 'int' for parameter 'default'", "attr.int(default=f)");
+ "got value of type 'function', want 'dict'", "attr.string_dict(default=f)");
checkEvalErrorContains(
- "expected value of type 'sequence of ints' for parameter 'default'",
- "attr.int_list(default=f)");
- checkEvalErrorContains(
- "expected value of type 'bool' for parameter 'default'", "attr.bool(default=f)");
- checkEvalErrorContains(
- "expected value of type 'dict' for parameter 'default'", "attr.string_dict(default=f)");
- checkEvalErrorContains(
- "expected value of type 'dict' for parameter 'default'",
- "attr.string_list_dict(default=f)");
- // Also:
- // - The attr.output(default=...) parameter is deprecated
- // (see --incompatible_no_output_attr_default)
- // - attr.license appears to be disabled already.
- // (see --incompatible_no_attr_license)
+ "got value of type 'function', want 'dict'", "attr.string_list_dict(default=f)");
+ // Note: attr.license appears to be disabled already.
+ // (see --incompatible_no_attr_license)
}
private static final Label FAKE_LABEL = Label.parseAbsoluteUnchecked("//fake/label.bzl");
@@ -812,23 +786,20 @@
public void testRuleUnknownKeyword() throws Exception {
registerDummyStarlarkFunction();
checkEvalErrorContains(
- "unexpected keyword 'bad_keyword', for call to function rule(",
- "rule(impl, bad_keyword = 'some text')");
+ "unexpected keyword argument 'bad_keyword'", "rule(impl, bad_keyword = 'some text')");
}
@Test
public void testRuleImplementationMissing() throws Exception {
checkEvalErrorContains(
- "parameter 'implementation' has no default value, for call to function rule(",
- "rule(attrs = {})");
+ "rule() missing 1 required positional argument: implementation", "rule(attrs = {})");
}
@Test
public void testRuleBadTypeForAdd() throws Exception {
registerDummyStarlarkFunction();
checkEvalErrorContains(
- "expected value of type 'dict or NoneType' for parameter 'attrs', "
- + "for call to function rule(",
+ "in call to rule(), parameter 'attrs' got value of type 'string', want 'dict or NoneType'",
"rule(impl, attrs = 'some text')");
}
@@ -843,9 +814,7 @@
@Test
public void testRuleBadTypeForDoc() throws Exception {
registerDummyStarlarkFunction();
- checkEvalErrorContains(
- "expected value of type 'string' for parameter 'doc', for call to function rule(",
- "rule(impl, doc = 1)");
+ checkEvalErrorContains("got value of type 'int', want 'string'", "rule(impl, doc = 1)");
}
@Test
@@ -1093,8 +1062,8 @@
@Test
public void testLabelAttrWrongDefault() throws Exception {
checkEvalErrorContains(
- "expected value of type 'Label or string or LateBoundDefault or function or NoneType' "
- + "for parameter 'default', for call to method label(",
+ "got value of type 'int', want 'Label or string or LateBoundDefault or function or"
+ + " NoneType'",
"attr.label(default = 123)");
}
@@ -1191,8 +1160,7 @@
@Test
public void testStructPosArgs() throws Exception {
- checkEvalErrorContains(
- "expected no more than 0 positional arguments, but got 1", "x = struct(1, b = 2)");
+ checkEvalErrorContains("struct() got unexpected positional argument", "x = struct(1, b = 2)");
}
@Test
@@ -1452,10 +1420,7 @@
@Test
public void declaredProvidersBadTypeForDoc() throws Exception {
- checkEvalErrorContains(
- "expected value of type 'string' for parameter 'doc', for call to function "
- + "provider(doc = '', fields = None)",
- "provider(doc = 1)");
+ checkEvalErrorContains("got value of type 'int', want 'string'", "provider(doc = 1)");
}
@Test
@@ -1594,9 +1559,7 @@
@Test
public void aspectBadTypeForDoc() throws Exception {
registerDummyStarlarkFunction();
- checkEvalErrorContains(
- "expected value of type 'string' for parameter 'doc', for call to function aspect(",
- "aspect(impl, doc = 1)");
+ checkEvalErrorContains("got value of type 'int', want 'string'", "aspect(impl, doc = 1)");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index d881afa..34d8df9 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -222,20 +222,20 @@
@Test
public void testSkylarkFunctionTooFewArguments() throws Exception {
checkSkylarkFunctionError(
- "parameter 'mandatory' has no default value", "mock(mandatory_key='y')");
+ "missing 1 required positional argument: mandatory", "mock(mandatory_key='y')");
}
@Test
public void testSkylarkFunctionTooManyArguments() throws Exception {
checkSkylarkFunctionError(
- "expected no more than 2 positional arguments, but got 3",
+ "mock() accepts no more than 2 positional arguments but got 3",
"mock('a', 'b', 'c', mandatory_key='y')");
}
@Test
public void testSkylarkFunctionAmbiguousArguments() throws Exception {
checkSkylarkFunctionError(
- "got multiple values for keyword argument 'mandatory'",
+ "mock() got multiple values for argument 'mandatory'",
"mock('by position', mandatory='by_key', mandatory_key='c')");
}
@@ -325,8 +325,7 @@
public void testCreateSpawnActionArgumentsBadExecutable() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "expected value of type 'File or string or FilesToRunProvider' for parameter 'executable', "
- + "for call to method run(",
+ "got value of type 'int', want 'File or string or FilesToRunProvider'",
"ruleContext.actions.run(",
" inputs = ruleContext.files.srcs,",
" outputs = ruleContext.files.srcs,",
@@ -380,7 +379,7 @@
public void testCreateSpawnActionUnknownParam() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "unexpected keyword 'bad_param', for call to method run(",
+ "run() got unexpected keyword argument 'bad_param'",
"f = ruleContext.actions.declare_file('foo.sh')",
"ruleContext.actions.run(outputs=[], bad_param = 'some text', executable = f)");
}
@@ -541,8 +540,7 @@
checkEmptyAction("mnemonic = 'test', inputs = depset(ruleContext.files.srcs)");
checkEvalErrorContains(
- "parameter 'mnemonic' has no default value, for call to method "
- + "do_nothing(mnemonic, inputs = []) of 'actions'",
+ "do_nothing() missing 1 required named argument: mnemonic",
"ruleContext.actions.do_nothing(inputs = ruleContext.files.srcs)");
}
@@ -771,7 +769,7 @@
public void testBadParamTypeErrorMessage() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "expected value of type 'string or Args' for parameter 'content'",
+ "got value of type 'int', want 'string or Args'",
"ruleContext.actions.write(",
" output = ruleContext.files.srcs[0],",
" content = 1,",
@@ -852,8 +850,7 @@
public void testRunfilesBadSetGenericType() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "expected value of type 'depset of Files or NoneType' for parameter 'transitive_files', "
- + "for call to method runfiles(",
+ "got value of type 'depset', want 'depset of Files or NoneType'",
"ruleContext.runfiles(transitive_files=depset([1, 2, 3]))");
}
@@ -950,7 +947,7 @@
public void testRunfilesBadKeywordArguments() throws Exception {
setRuleContext(createRuleContext("//foo:foo"));
checkEvalErrorContains(
- "unexpected keyword 'bad_keyword', for call to method runfiles(",
+ "runfiles() got unexpected keyword argument 'bad_keyword'",
"ruleContext.runfiles(bad_keyword = '')");
}
@@ -1307,7 +1304,7 @@
assertThrows(AssertionError.class, () -> getConfiguredTarget("//test:my_rule"));
assertThat(expected)
.hasMessageThat()
- .contains("unexpected keyword 'foo', for call to function DefaultInfo(");
+ .contains("DefaultInfo() got unexpected keyword argument 'foo'");
}
@Test
@@ -1999,9 +1996,11 @@
"silly_rule(name = 'silly')");
thrown.handleAssertionErrors(); // Compatibility with JUnit 4.11
thrown.expect(AssertionError.class);
+ // This confusing message shows why we should distinguish
+ // built-ins and Starlark functions in their repr strings.
thrown.expectMessage(
- "expected value of type 'function' for parameter 'implementation', "
- + "for call to function rule");
+ "in call to rule(), parameter 'implementation' got value of type 'function', want"
+ + " 'function'");
getConfiguredTarget("//test:silly");
}
@@ -2365,7 +2364,8 @@
"args = ruleContext.actions.args()",
"args.add_all(1)");
checkEvalErrorContains(
- "expected value of type 'sequence or depset' for parameter 'values'",
+ "in call to add_all(), parameter 'values' got value of type 'int', want 'sequence or"
+ + " depset'",
"args = ruleContext.actions.args()",
"args.add_all('--foo', 1)");
}
@@ -2787,7 +2787,8 @@
assertThat(expected)
.hasMessageThat()
.contains(
- "expected value of type 'int' for parameter 'default', " + "for call to method int(");
+ "in call to int(), parameter 'default' got value of type 'LateBoundDefault', want"
+ + " 'int'");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java b/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
index d38652f..affb7ee 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/DepsetTest.java
@@ -239,7 +239,8 @@
public void testTooManyPositionals() throws Exception {
new BothModesTest()
.testIfErrorContains(
- "expected no more than 2 positional arguments, but got 3", "depset([], 'default', [])");
+ "depset() accepts no more than 2 positional arguments but got 3",
+ "depset([], 'default', [])");
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 615c613..03f12f9 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -799,19 +799,14 @@
@Test
public void testDictKeysTooManyArgs() throws Exception {
- newTest()
- .testIfExactError(
- "expected no more than 0 positional arguments, but got 1, "
- + "for call to method keys() of 'dict'",
- "{'a': 1}.keys('abc')");
+ newTest().testIfExactError("keys() got unexpected positional argument", "{'a': 1}.keys('abc')");
}
@Test
public void testDictKeysTooManyKeyArgs() throws Exception {
newTest()
.testIfExactError(
- "unexpected keyword 'arg', for call to method keys() of 'dict'",
- "{'a': 1}.keys(arg='abc')");
+ "keys() got unexpected keyword argument 'arg'", "{'a': 1}.keys(arg='abc')");
}
@Test
@@ -819,17 +814,17 @@
// TODO(adonovan): when the duplication is literal, this should be caught by a static check.
newTest()
.testIfExactError(
- "duplicate argument 'arg' in call to 'keys'",
- "{'a': 1}.keys(arg='abc', arg='def', k=1, k=2)");
+ "int() got multiple values for argument 'base'", "int('1', base=10, base=16)");
+ new SkylarkTest()
+ .testIfExactError(
+ "int() got multiple values for argument 'base'", "int('1', base=10, **dict(base=16))");
}
@Test
public void testArgBothPosKey() throws Exception {
newTest()
.testIfErrorContains(
- "got multiple values for keyword argument 'base', "
- + "for call to function int(x, base = unbound)",
- "int('2', 3, base=3)");
+ "int() got multiple values for argument 'base'", "int('2', 3, base=3)");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index 56762cc..b8af7b4 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -110,8 +110,7 @@
// only one built-in function.
new BothModesTest()
.testIfExactError(
- "expected value of type 'string' for parameter 'sub', "
- + "for call to method index(sub, start = 0, end = None) of 'string'",
+ "in call to index(), parameter 'sub' got value of type 'int', want 'string'",
"'test'.index(1)");
}
@@ -135,8 +134,7 @@
+ LINE_SEPARATOR
+ "\t\t\"test\".index(x)"
+ LINE_SEPARATOR
- + "expected value of type 'string' for parameter 'sub', "
- + "for call to method index(sub, start = 0, end = None) of 'string'",
+ + "in call to index(), parameter 'sub' got value of type 'int', want 'string'",
"def foo():",
" bar(1)",
"def bar(x):",
@@ -150,8 +148,8 @@
new BothModesTest()
.testIfErrorContains("substring \"z\" not found in \"abc\"", "'abc'.index('z')")
.testIfErrorContains(
- "expected value of type 'string or tuple of strings' for parameter 'sub', "
- + "for call to method startswith(sub, start = 0, end = None) of 'string'",
+ "in call to startswith(), parameter 'sub' got value of type 'int', want 'string or"
+ + " tuple of strings'",
"'test'.startswith(1)")
.testIfErrorContains("in dict, got string, want iterable", "dict('a')");
}
@@ -334,7 +332,8 @@
"in dict, dictionary update sequence element #0 is not iterable (string)",
"dict([('a')])")
.testIfErrorContains(
- "expected no more than 1 positional arguments, but got 3", "dict((3,4), (3,2), (1,2))")
+ "dict() accepts no more than 1 positional argument but got 3",
+ "dict((3,4), (3,2), (1,2))")
.testIfErrorContains(
"item #0 has length 3, but exactly two elements are required",
"dict([('a', 'b', 'c')])");
@@ -461,8 +460,7 @@
.testExpression("hash('skylark')", "skylark".hashCode())
.testExpression("hash('google')", "google".hashCode())
.testIfErrorContains(
- "expected value of type 'string' for parameter 'value', "
- + "for call to function hash(value)",
+ "in call to hash(), parameter 'value' got value of type 'NoneType', want 'string'",
"hash(None)");
}
@@ -759,8 +757,7 @@
public void testExperimentalStarlarkConfig() throws Exception {
new SkylarkTest("--incompatible_restrict_named_params")
.testIfErrorContains(
- "parameter 'elements' may not be specified by name, "
- + "for call to method join(elements) of 'string'",
+ "join() got named argument for positional-only parameter 'elements'",
"','.join(elements=['foo', 'bar'])");
}
@@ -796,7 +793,7 @@
"parameter 'direct' cannot be specified both positionally and by keyword",
"depset([0, 1], 'default', direct=[0,1])")
.testIfErrorContains(
- "parameter 'items' is deprecated and will be removed soon. "
+ "in call to depset(), parameter 'items' is deprecated and will be removed soon. "
+ "It may be temporarily re-enabled by setting "
+ "--incompatible_disable_depset_inputs=false",
"depset(items=[0,1])");
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 e3b7693..10f8408 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
@@ -1115,7 +1115,8 @@
// posOrNamed by name and three positional parameters, there is a conflict.
new SkylarkTest()
.update("mock", new Mock())
- .testIfErrorContains("got multiple values for keyword argument 'posOrNamed'",
+ .testIfErrorContains(
+ "with_params() got multiple values for argument 'posOrNamed'",
"mock.with_params(1, True, True, posOrNamed=True, named=True)");
}
@@ -1123,17 +1124,20 @@
public void testTooManyPositionalArgs() throws Exception {
new SkylarkTest()
.update("mock", new Mock())
- .testIfErrorContains("expected no more than 3 positional arguments, but got 4",
+ .testIfErrorContains(
+ "with_params() accepts no more than 3 positional arguments but got 4",
"mock.with_params(1, True, True, 'toomany', named=True)");
new SkylarkTest()
.update("mock", new Mock())
- .testIfErrorContains("expected no more than 3 positional arguments, but got 5",
+ .testIfErrorContains(
+ "with_params() accepts no more than 3 positional arguments but got 5",
"mock.with_params(1, True, True, 'toomany', 'alsotoomany', named=True)");
new SkylarkTest()
.update("mock", new Mock())
- .testIfErrorContains("expected no more than 1 positional arguments, but got 2",
+ .testIfErrorContains(
+ "is_empty() accepts no more than 1 positional argument but got 2",
"mock.is_empty('a', 'b')");
}
@@ -1161,19 +1165,12 @@
.update("mock", new Mock())
.setUp("")
.testIfExactError(
- "parameter 'named' has no default value, for call to "
- + "method with_params(pos1, pos2 = False, posOrNamed = False, named, "
- + "optionalNamed = False, nonNoneable = \"a\", noneable = None, multi = None) "
- + "of 'Mock'",
- "mock.with_params(1, True)");
+ "with_params() missing 1 required named argument: named", "mock.with_params(1, True)");
new SkylarkTest()
.update("mock", new Mock())
.setUp("")
.testIfExactError(
- "parameter 'named' has no default value, for call to "
- + "method with_params(pos1, pos2 = False, posOrNamed = False, named, "
- + "optionalNamed = False, nonNoneable = \"a\", noneable = None, multi = None) "
- + "of 'Mock'",
+ "with_params() missing 1 required named argument: named",
"mock.with_params(1, True, True)");
new SkylarkTest()
.update("mock", new Mock())
@@ -1191,30 +1188,28 @@
.update("mock", new Mock())
.setUp("")
.testIfExactError(
- "unexpected keyword 'n', for call to "
- + "method with_params(pos1, pos2 = False, posOrNamed = False, named, "
- + "optionalNamed = False, nonNoneable = \"a\", noneable = None, multi = None) "
- + "of 'Mock'",
+ "with_params() got unexpected keyword argument 'posornamed' (did you mean"
+ + " 'posOrNamed'?)",
+ "mock.with_params(1, True, named=True, posornamed=True)");
+ new SkylarkTest()
+ .update("mock", new Mock())
+ .setUp("")
+ .testIfExactError(
+ "with_params() got unexpected keyword argument 'n'",
"mock.with_params(1, True, named=True, posOrNamed=True, n=2)");
new SkylarkTest()
.update("mock", new Mock())
.setUp("")
.testIfExactError(
- "parameter 'nonNoneable' cannot be None, for call to method "
- + "with_params(pos1, pos2 = False, posOrNamed = False, named, "
- + "optionalNamed = False, nonNoneable = \"a\", noneable = None, multi = None) "
- + "of 'Mock'",
+ "in call to with_params(), parameter 'nonNoneable' cannot be None",
"mock.with_params(1, True, True, named=True, optionalNamed=False, nonNoneable=None)");
new SkylarkTest()
.update("mock", new Mock())
.setUp("")
.testIfExactError(
- "expected value of type 'string or int or sequence of ints or NoneType' for parameter"
- + " 'multi', for call to method "
- + "with_params(pos1, pos2 = False, posOrNamed = False, named, "
- + "optionalNamed = False, nonNoneable = \"a\", noneable = None, multi = None) "
- + "of 'Mock'",
+ "in call to with_params(), parameter 'multi' got value of type 'bool', want 'string or"
+ + " int or sequence of ints or NoneType'",
"mock.with_params(1, True, named=True, multi=False)");
// We do not enforce list item parameter type constraints.
@@ -1261,7 +1256,7 @@
new SkylarkTest()
.update("mock", new Mock())
.testIfErrorContains(
- "expected value of type 'string' for parameter 'pos', for call to function MockFn(pos)",
+ "in call to MockFn(), parameter 'pos' got value of type 'int', want 'string'",
"v = mock(1)");
}
@@ -1966,8 +1961,7 @@
public void testPrintBadKwargs() throws Exception {
new SkylarkTest()
.testIfErrorContains(
- "unexpected keywords 'end', 'other', for call to function print(sep = \" \", *args)",
- "print(end='x', other='y')");
+ "print() got unexpected keyword argument 'end'", "print(end='x', other='y')");
}
// Override tests in EvaluationTest incompatible with Skylark
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFlagGuardingTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFlagGuardingTest.java
index 263aac4..34eac92 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFlagGuardingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkFlagGuardingTest.java
@@ -141,8 +141,8 @@
new SkylarkTest("--experimental_build_setting_api=true")
.update("mock", new Mock())
.testIfErrorContains(
- "expected value of type 'bool' for parameter 'b', "
- + "for call to method positionals_only_method(a, b, c) of 'Mock'",
+ "in call to positionals_only_method(), parameter 'b' got value of type 'int', want"
+ + " 'bool'",
"mock.positionals_only_method(1, 3)");
new SkylarkTest("--experimental_build_setting_api=false")
@@ -152,8 +152,8 @@
new SkylarkTest("--experimental_build_setting_api=false")
.update("mock", new Mock())
.testIfErrorContains(
- "expected value of type 'int' for parameter 'c', for call to method "
- + "positionals_only_method(a, c) of 'Mock'",
+ "in call to positionals_only_method(), parameter 'c' got value of type 'bool', want"
+ + " 'int'",
"mock.positionals_only_method(1, True, 3)");
}
@@ -167,8 +167,7 @@
new SkylarkTest("--experimental_build_setting_api=true")
.update("mock", new Mock())
.testIfErrorContains(
- "parameter 'b' has no default value, "
- + "for call to method keywords_only_method(a, b, c) of 'Mock'",
+ "keywords_only_method() missing 1 required named argument: b",
"mock.keywords_only_method(a=1, c=3)");
new SkylarkTest("--experimental_build_setting_api=false")
@@ -186,6 +185,7 @@
@Test
public void testMixedParamsMethod() throws Exception {
+ // def mixed_params_method(a, b, c = ?, d = ?)
new SkylarkTest("--experimental_build_setting_api=true")
.update("mock", new Mock())
.testEval(
@@ -195,10 +195,12 @@
new SkylarkTest("--experimental_build_setting_api=true")
.update("mock", new Mock())
.testIfErrorContains(
- "parameter 'b' has no default value, "
- + "for call to method mixed_params_method(a, b, c, d) of 'Mock'",
+ // Missing named arguments (d) are not reported
+ // if there are missing positional arguments.
+ "mixed_params_method() missing 1 required positional argument: b",
"mock.mixed_params_method(1, c=3)");
+ // def mixed_params_method(a, b disabled = False, c disabled = 3, d = ?)
new SkylarkTest("--experimental_build_setting_api=false")
.update("mock", new Mock())
.testEval(
@@ -207,8 +209,13 @@
new SkylarkTest("--experimental_build_setting_api=false")
.update("mock", new Mock())
.testIfErrorContains(
- "expected no more than 1 positional arguments, but got 2, "
- + "for call to method mixed_params_method(a, d) of 'Mock'",
+ "mixed_params_method() accepts no more than 1 positional argument but got 2",
+ "mock.mixed_params_method(1, True, d=True)");
+
+ new SkylarkTest("--experimental_build_setting_api=false")
+ .update("mock", new Mock())
+ .testIfErrorContains(
+ "mixed_params_method() accepts no more than 1 positional argument but got 2",
"mock.mixed_params_method(1, True, c=3, d=True)");
}
@@ -223,8 +230,7 @@
new SkylarkTest("--experimental_build_setting_api=true", "--incompatible_no_attr_license=false")
.update("mock", new Mock())
.testIfErrorContains(
- "parameter 'b' has no default value, "
- + "for call to method keywords_multiple_flags(a, b, c) of 'Mock'",
+ "keywords_multiple_flags() missing 2 required named arguments: b, c",
"mock.keywords_multiple_flags(a=42)");
new SkylarkTest("--experimental_build_setting_api=false", "--incompatible_no_attr_license=true")
diff --git a/src/test/shell/integration/outputs_test.sh b/src/test/shell/integration/outputs_test.sh
index 2863318..e2f523a 100755
--- a/src/test/shell/integration/outputs_test.sh
+++ b/src/test/shell/integration/outputs_test.sh
@@ -170,7 +170,7 @@
if bazel build //$pkg:demo &> $TEST_log; then
fail "Build expected to fail"
fi
- expect_log "expected value of type 'dict or NoneType or function' for parameter 'outputs', for call to function rule"
+ expect_log "got.* 'select', want 'dict or NoneType or function'" # (outputs)
}
function test_configurable_output_error() {
diff --git a/src/test/starlark/testdata/function.sky b/src/test/starlark/testdata/function.sky
index a8fe7bb..2d8e908 100644
--- a/src/test/starlark/testdata/function.sky
+++ b/src/test/starlark/testdata/function.sky
@@ -38,7 +38,7 @@
---
x = {}.pop
-x() ### parameter 'key' has no default value
+x() ### missing 1 required positional argument: key
---
# getattr
diff --git a/src/test/starlark/testdata/int_constructor.sky b/src/test/starlark/testdata/int_constructor.sky
index f6ee91e..17eb1f8 100644
--- a/src/test/starlark/testdata/int_constructor.sky
+++ b/src/test/starlark/testdata/int_constructor.sky
@@ -23,7 +23,7 @@
---
int('ab') ### invalid literal for int\(\) with base 10: "ab"
---
-int(None) ### parameter 'x' cannot be None, for call to function int(x, base = unbound)
+int(None) ### parameter 'x' cannot be None
---
int('123', 3) ### invalid literal for int\(\) with base 3: "123"
---
diff --git a/src/test/starlark/testdata/int_function.sky b/src/test/starlark/testdata/int_function.sky
index 29d066f..ff72397 100644
--- a/src/test/starlark/testdata/int_function.sky
+++ b/src/test/starlark/testdata/int_function.sky
@@ -10,10 +10,10 @@
assert_eq(int(False), 0)
---
-int(None) ### parameter 'x' cannot be None, for call to function int(x, base = unbound)
+int(None) ### parameter 'x' cannot be None
---
# This case is allowed in Python but not Skylark
-int() ### parameter 'x' has no default value, for call to function int(x, base = unbound)
+int() ### missing 1 required positional argument: x
---
# string, no base
diff --git a/src/test/starlark/testdata/reversed.sky b/src/test/starlark/testdata/reversed.sky
index 9d07c40..da208a8 100644
--- a/src/test/starlark/testdata/reversed.sky
+++ b/src/test/starlark/testdata/reversed.sky
@@ -7,11 +7,11 @@
assert_eq(reversed('bbb'.elems()), ['b', 'b', 'b'])
---
-reversed(None) ### expected value of type 'sequence'
+reversed(None) ### got .* 'NoneType', want 'sequence'
---
-reversed(1) ### expected value of type 'sequence'
+reversed(1) ### got .* 'int', want 'sequence'
---
-reversed({1: 3}) ### expected value of type 'sequence'
+reversed({1: 3}) ### got .* 'dict', want 'sequence'
---
x = ['a', 'b']
diff --git a/src/test/starlark/testdata/string_misc.sky b/src/test/starlark/testdata/string_misc.sky
index edd1f07..fd909ed 100644
--- a/src/test/starlark/testdata/string_misc.sky
+++ b/src/test/starlark/testdata/string_misc.sky
@@ -89,7 +89,7 @@
assert_eq('a'.endswith(()), False)
assert_eq(''.endswith(()), False)
---
-'a'.endswith(['a']) ### expected value of type 'string or tuple of strings' for parameter 'sub', for call to method endswith(sub, start = 0, end = None) of 'string'
+'a'.endswith(['a']) ### got .* 'list', want 'string or tuple of strings
---
'1'.endswith((1,)) ### expected type 'string' for 'string' element but got type 'int' instead
---
@@ -117,7 +117,7 @@
assert_eq('a'.startswith(()), False)
assert_eq(''.startswith(()), False)
---
-'a'.startswith(['a']) ### expected value of type 'string or tuple of strings' for parameter 'sub', for call to method startswith(sub, start = 0, end = None) of 'string'
+'a'.startswith(['a']) ### got .* 'list', want 'string or tuple of strings'
---
'1'.startswith((1,)) ### expected type 'string' for 'string' element but got type 'int' instead
---
