bazel syntax: fast calling convention This CL changes the calling convention so that the caller provides the callee with two arrays, one of positional and one of named arguments, the latter represented as alternating String/Object values. The array of named arguments seen by the callee may contain duplicates. This reduces allocation, especially of hash tables, as the callee must already check for duplicates implied by the combination of positional and named. BaseFunction historically adapted an "outer" call protocol to an "inner" one, interpreting the positional and named arguments with respect to a FunctionSignature. The change moves the task of adaptation into the base interface, StarlarkCallable, so that subclasses may implement either the outer protocol (fastcall) or the inner one (call)---or neither for and "unimplemented" test or fake function. The inner protocol now simply collects all the positional and named arguments and presents them as a tuple and a dict, as if by the signature def f(*args, **kwargs). BaseFunction's role is reduced to implementing methods like toString and repr based on the signature and default values, and exposing this information to the documentation tools. (The default behaviors of these methods could be improved.) The signature and default values are not implicitly used for any kind of validation, though subclasses may use them explicitly for that purpose. BaseFunction.processArguments has moved to Starlark.checkSignature, making Signature-based argument processing available to classes unrelated to BaseFunction. This function could be much improved in clarity and UI, but in this CL I have limited edits to performance concerns: - eliminating the 'bothPosKey' pass and allocation (which means we don't distinguish the causes of repeated argument). - case 2b deleted (it's no longer a faster version of 2c) - avoid duplicate hashing when inserting to kwargs. I plan to improve the code and the UI (error messages) in a follow-up. Also - turn BaseFunction.signature field into an abstract method; for most subclasses it is a constant. - PackageFactory.newPackageFunction uses the inner protocol and does its own parameter validation. Simpler than forcing it to use FunctionSignature. - avoid dubious list mutation in BuiltinCallable The performance impacts have been hard to measure, with benchmarks showing high variance. A few ad-hoc tests of particularly expensive single BUILD files have shown consistent reductions in real time of around 15%. PiperOrigin-RevId: 286259755
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java index 5efa23d..705699e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -92,10 +92,10 @@ import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkFunction; import com.google.devtools.build.lib.syntax.StarlarkThread; +import com.google.devtools.build.lib.syntax.Tuple; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.Pair; import java.util.Collection; -import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; import javax.annotation.Nullable; @@ -613,7 +613,6 @@ RuleClassType type, ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes, Location definitionLocation) { - super(FunctionSignature.KWARGS); this.builder = builder; this.type = type; this.attributes = attributes; @@ -623,7 +622,6 @@ /** This is for post-export reconstruction for serialization. */ private SkylarkRuleFunction( RuleClass ruleClass, RuleClassType type, Location definitionLocation, Label skylarkLabel) { - super(FunctionSignature.KWARGS); Preconditions.checkNotNull( ruleClass, "RuleClass must be non-null as this SkylarkRuleFunction should have been exported."); @@ -642,11 +640,16 @@ } @Override - public Object callImpl( + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; // just for documentation + } + + @Override + public Object call( StarlarkThread thread, @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) + Tuple<Object> args, + Dict<String, Object> kwargs) throws EvalException, InterruptedException, ConversionException { Location loc = call != null ? call.getLocation() : Location.BUILTIN; if (!args.isEmpty()) { @@ -686,10 +689,10 @@ } RuleFactory.createAndAddRule( pkgContext, ruleClass, attributeValues, loc, thread, new AttributeContainer(ruleClass)); - return Starlark.NONE; } catch (InvalidRuleException | NameConflictException e) { throw new EvalException(loc, e.getMessage()); } + return Starlark.NONE; } /**
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java index 8d43210..4998d70 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -40,6 +40,7 @@ import com.google.devtools.build.lib.skylarkbuildapi.repository.RepositoryModuleApi; import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.DebugFrame; +import com.google.devtools.build.lib.syntax.Dict; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.FunctionSignature; @@ -48,7 +49,7 @@ import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkFunction; import com.google.devtools.build.lib.syntax.StarlarkThread; -import java.util.List; +import com.google.devtools.build.lib.syntax.Tuple; import java.util.Map; import javax.annotation.Nullable; @@ -122,7 +123,6 @@ RuleClass.Builder builder, Location ruleClassDefinitionLocation, BaseFunction implementation) { - super(FunctionSignature.KWARGS); this.builder = builder; this.ruleClassDefinitionLocation = ruleClassDefinitionLocation; this.implementation = implementation; @@ -134,6 +134,11 @@ } @Override + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; + } + + @Override public void export(Label extensionLabel, String exportedName) { this.extensionLabel = extensionLabel; this.exportedName = exportedName; @@ -154,11 +159,11 @@ } @Override - public Object callImpl( + public Object call( StarlarkThread thread, @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) + Tuple<Object> args, + Dict<String, Object> kwargs) throws EvalException, InterruptedException { if (!args.isEmpty()) { throw new EvalException(null, "unexpected positional arguments");
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index b5826a9..2bbca30 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.DefStatement; +import com.google.devtools.build.lib.syntax.Dict; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.Expression; @@ -63,6 +64,7 @@ import com.google.devtools.build.lib.syntax.StarlarkThread.Extension; import com.google.devtools.build.lib.syntax.Statement; import com.google.devtools.build.lib.syntax.StringLiteral; +import com.google.devtools.build.lib.syntax.Tuple; import com.google.devtools.build.lib.syntax.ValidationEnvironment; import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -494,56 +496,58 @@ /** Returns a function-value implementing "package" in the specified package context. */ // TODO(cparsons): Migrate this function to be defined with @SkylarkCallable. + // TODO(adonovan): don't call this function twice (once for BUILD files and + // once for the native module) as it results in distinct objects. (Using + // @SkylarkCallable may accomplish that.) private static BaseFunction newPackageFunction( final ImmutableMap<String, PackageArgument<?>> packageArguments) { - // Flatten the map of argument name of PackageArgument specifier in two co-indexed arrays: - // one for the argument names, to create a FunctionSignature when we create the function, - // one of the PackageArgument specifiers, over which to iterate at every function invocation - // at the same time that we iterate over the function arguments. - final int numArgs = packageArguments.size(); - final String[] argumentNames = new String[numArgs]; - final PackageArgument<?>[] argumentSpecifiers = new PackageArgument<?>[numArgs]; - int i = 0; - for (Map.Entry<String, PackageArgument<?>> entry : packageArguments.entrySet()) { - argumentNames[i] = entry.getKey(); - argumentSpecifiers[i++] = entry.getValue(); - } + FunctionSignature signature = + FunctionSignature.namedOnly(0, packageArguments.keySet().toArray(new String[0])); - return new BaseFunction(FunctionSignature.namedOnly(0, argumentNames)) { + return new BaseFunction() { @Override public String getName() { return "package"; } @Override - public Object call(Object[] arguments, FuncallExpression ast, StarlarkThread thread) + public FunctionSignature getSignature() { + return signature; // (only for documentation) + } + + @Override + public Object call( + StarlarkThread thread, + @Nullable FuncallExpression call, + Tuple<Object> args, + Dict<String, Object> kwargs) throws EvalException { - Location loc = ast.getLocation(); + if (!args.isEmpty()) { + throw new EvalException(null, "unexpected positional arguments"); + } + Location loc = call != null ? call.getLocation() : Location.BUILTIN; Package.Builder pkgBuilder = getContext(thread, loc).pkgBuilder; // Validate parameter list if (pkgBuilder.isPackageFunctionUsed()) { - throw new EvalException(loc, "'package' can only be used once per BUILD file"); + throw new EvalException(null, "'package' can only be used once per BUILD file"); } pkgBuilder.setPackageFunctionUsed(); - // Parse params - boolean foundParameter = false; - - for (int i = 0; i < numArgs; i++) { - Object value = arguments[i]; - if (value != null) { - foundParameter = true; - argumentSpecifiers[i].convertAndProcess(pkgBuilder, loc, value); - } - } - - if (!foundParameter) { + // Each supplied argument must name a PackageArgument. + if (kwargs.isEmpty()) { throw new EvalException( - loc, "at least one argument must be given to the 'package' function"); + null, "at least one argument must be given to the 'package' function"); } - + for (Map.Entry<String, Object> kwarg : kwargs.entrySet()) { + String name = kwarg.getKey(); + PackageArgument<?> pkgarg = packageArguments.get(name); + if (pkgarg == null) { + throw new EvalException(null, "unexpected keyword argument: " + name); + } + pkgarg.convertAndProcess(pkgBuilder, loc, kwarg.getValue()); + } return Starlark.NONE; } }; @@ -585,19 +589,20 @@ private final RuleClass ruleClass; BuiltinRuleFunction(RuleClass ruleClass) { - // TODO(adonovan): the only thing BaseFunction is doing for us is holding - // an (uninteresting) FunctionSignature. Can we extend StarlarkCallable directly? - // Only docgen appears to depend on BaseFunction. - super(FunctionSignature.KWARGS); this.ruleClass = Preconditions.checkNotNull(ruleClass); } @Override - public NoneType callImpl( + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; // just for documentation + } + + @Override + public NoneType call( StarlarkThread thread, @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) + Tuple<Object> args, + Dict<String, Object> kwargs) throws EvalException, InterruptedException { if (!args.isEmpty()) { throw new EvalException(null, "unexpected positional arguments");
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java index baa4cbe..0021ddc 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.FunctionSignature; import com.google.devtools.build.lib.syntax.Printer; +import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkThread; import java.util.Collection; import java.util.Map; @@ -51,6 +52,7 @@ private static final String DEFAULT_ERROR_MESSAGE_FORMAT = "Object has no '%s' attribute."; private final Location location; + private final FunctionSignature signature; /** For schemaful providers, the sorted list of allowed field names. */ // (The requirement for sortedness comes from SkylarkInfo.fromSortedFieldList, @@ -127,7 +129,7 @@ */ private SkylarkProvider( @Nullable SkylarkKey key, @Nullable ImmutableList<String> fields, Location location) { - super(buildSignature(fields)); + this.signature = buildSignature(fields); this.location = location; this.fields = fields; this.key = key; // possibly null @@ -136,6 +138,11 @@ : makeErrorMessageFormatForUnknownField(key.getExportedName()); } + @Override + public FunctionSignature getSignature() { + return signature; + } + private static FunctionSignature buildSignature(@Nullable ImmutableList<String> fields) { return fields == null ? FunctionSignature.KWARGS // schemaless @@ -143,18 +150,24 @@ } @Override - protected Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkThread thread) + public Object fastcall( + StarlarkThread thread, @Nullable FuncallExpression call, Object[] positional, Object[] named) throws EvalException, InterruptedException { - Location loc = ast != null ? ast.getLocation() : Location.BUILTIN; + // TODO(adonovan): we can likely come up with a more efficient implementation + // ...then make matchSignature private again? + Object[] arguments = + Starlark.matchSignature( + signature, this, /*defaults=*/ null, thread.mutability(), positional, named); + Location loc = call != null ? call.getLocation() : Location.BUILTIN; if (fields == null) { // provider(**kwargs) @SuppressWarnings("unchecked") - Map<String, Object> kwargs = (Map<String, Object>) args[0]; + Map<String, Object> kwargs = (Map<String, Object>) arguments[0]; return SkylarkInfo.create(this, kwargs, loc); } else { // provider(a=..., b=..., ...) // The order of args is determined by the signature: that is, fields, which is sorted. - return SkylarkInfo.fromSortedFieldList(this, fields, args, loc); + return SkylarkInfo.fromSortedFieldList(this, fields, arguments, loc); } }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java index 317c523..9125ae2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension; import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.ClassObject; +import com.google.devtools.build.lib.syntax.Dict; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.FuncallExpression; @@ -41,6 +42,7 @@ import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.syntax.StarlarkThread; import com.google.devtools.build.lib.syntax.StarlarkThread.Extension; +import com.google.devtools.build.lib.syntax.Tuple; import com.google.devtools.build.lib.syntax.ValidationEnvironment; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -261,21 +263,23 @@ */ private static BaseFunction newRuleFunction( final RuleFactory ruleFactory, final String ruleClassName, final boolean allowOverride) { - // TODO(adonovan): the only thing BaseFunction is doing for us is holding - // an (uninteresting) FunctionSignature. Can we extend StarlarkCallable directly? - // Only docgen appears to depend on BaseFunction. - return new BaseFunction(FunctionSignature.KWARGS) { + return new BaseFunction() { @Override public String getName() { return ruleClassName; } @Override - public Object callImpl( + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; // just for documentation + } + + @Override + public Object call( StarlarkThread thread, @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) + Tuple<Object> args, + Dict<String, Object> kwargs) throws EvalException, InterruptedException { if (!args.isEmpty()) { throw new EvalException(null, "unexpected positional arguments"); @@ -283,6 +287,7 @@ Location loc = call != null ? call.getLocation() : Location.BUILTIN; try { Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder; + // TODO(adonovan): this doesn't look safe! String externalRepoName = (String) kwargs.get("name"); if (!allowOverride && externalRepoName != null
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java index 4160d1c..39a974a 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -14,65 +14,59 @@ 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.common.collect.ImmutableSet; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; -import com.google.devtools.build.lib.events.Location; -import java.util.ArrayList; -import java.util.List; -import java.util.Map; +import java.util.Arrays; +import java.util.Set; import javax.annotation.Nullable; /** - * BaseFunction is a base class for functions that have a FunctionSignature and optional default - * values. Its {@link #callImpl} method converts the positional and named arguments into an array of - * values corresponding to the parameters of the FunctionSignature, then calls the subclass's {@link - * #call} method with this array. + * BaseFunction is an abstract base class to simplify the task of defining a subclass of + * StarlarkCallable. + * + * <p>Subclasses of BaseFunction provide a {@code FunctionSignature} and an optional tuple of + * default values for optional parameters, and BaseFunction implements the functionality of {@link + * #toString}, {@link #repr}, and {@link #isImmutable}. The signature may also be used by + * documentation tools. */ -// TODO(adonovan): do away with this class. There is no real need for a concept of "StarlarkCallable -// with Signature", though a few places in Bazel rely on this concept. For example, the -// Args.add_all(map_all=..., map_each=...) parameters have their signatures checked to discover -// errors eagerly. public abstract class BaseFunction implements StarlarkCallable { - // TODO(adonovan): don't materialise this. - private final FunctionSignature signature; - - /** Returns the signature of this function. */ - public FunctionSignature getSignature() { - return signature; - } + /** + * Returns the signature of this function. This does not affect argument validation; it is only + * for documentation and error messages. However, subclasses may choose to pass it to {@link + * Starlark#matchSignature}. + */ + public abstract FunctionSignature getSignature(); /** * Returns the optional tuple of default values for optional parameters. For example, the defaults * for {@code def f(a, b=1, *, c, d=2)} would be {@code (1, 2)}. May return null if the function * has no optional parameters. + * + * <p>As with {@code getSignature}, this method does not affect argument validation; it is only + * for documentation and error messages. However, subclasses may choose to pass it to {@link + * Starlark#matchSignature}. */ @Nullable public Tuple<Object> getDefaultValues() { return null; } - /** Constructs a BaseFunction with a given signature. */ - protected BaseFunction(FunctionSignature signature) { - this.signature = Preconditions.checkNotNull(signature); - } - - /** - * Process the caller-provided arguments into an array suitable for the callee (this function). - */ - private static Object[] processArguments( - StarlarkCallable func, // only for use in error messages + // TODO(adonovan): This has nothing to do with BaseFunction. + // Move it to Starlark.matchSignature (in a follow-up, for ease of review). + static Object[] matchSignature( FunctionSignature signature, + StarlarkCallable func, // only for use in error messages Tuple<Object> defaults, @Nullable Mutability mu, - List<Object> args, - Map<String, Object> kwargs) + Object[] positional, + Object[] named) throws EvalException { - // TODO(adonovan): simplify this function when we implement the "vector" calling convention. + // TODO(adonovan): simplify this function. Combine cases 1 and 2 without loss of efficiency. // TODO(adonovan): reduce the verbosity of errors. Printing func.toString is often excessive. + // Make the error messages consistent in form. + // TODO(adonovan): report an error if there were missing values in 'defaults'. Object[] arguments = new Object[signature.numParameters()]; @@ -80,7 +74,7 @@ // Note that this variable will be adjusted down if there are extra positionals, // after these extra positionals are dumped into starParam. - int numPositionalArgs = args.size(); + int numPositionalArgs = positional.length; int numMandatoryPositionalParams = signature.numMandatoryPositionals(); int numOptionalPositionalParams = signature.numOptionalPositionals(); @@ -97,14 +91,15 @@ if (hasVarargs) { // Nota Bene: we collect extra positional arguments in a (tuple,) rather than a [list], // and this is actually the same as in Python. - int starParamIndex = numNamedParams; + Object varargs; if (numPositionalArgs > numPositionalParams) { - arguments[starParamIndex] = - Tuple.copyOf(args.subList(numPositionalParams, numPositionalArgs)); + varargs = + Tuple.wrap(Arrays.copyOfRange(positional, numPositionalParams, numPositionalArgs)); numPositionalArgs = numPositionalParams; // clip numPositionalArgs } else { - arguments[starParamIndex] = Tuple.empty(); + varargs = Tuple.empty(); } + arguments[numNamedParams] = varargs; } else if (numPositionalArgs > numPositionalParams) { throw new EvalException( null, @@ -114,11 +109,11 @@ } for (int i = 0; i < numPositionalArgs; i++) { - arguments[i] = args.get(i); + arguments[i] = positional[i]; } // (2) handle keyword arguments - if (kwargs == null || kwargs.isEmpty()) { + if (named.length == 0) { // Easy case (2a): there are no keyword arguments. // All arguments were positional, so check we had enough to fill all mandatory positionals. if (numPositionalArgs < numMandatoryPositionalParams) { @@ -145,65 +140,51 @@ if (hasKwargs) { arguments[kwargIndex] = Dict.of(mu); } - } else if (hasKwargs && numNamedParams == 0) { - // Easy case (2b): there are no named parameters, but there is a **kwargs. - // Therefore all keyword arguments go directly to the kwarg. - // Note that *args and **kwargs themselves don't count as named. - // Also note that no named parameters means no mandatory parameters that weren't passed, - // and no missing optional parameters for which to use a default. Thus, no loops. - // NB: not 2a means kwarg isn't null - arguments[kwargIndex] = Dict.copyOf(mu, kwargs); } else { - // Hard general case (2c): some keyword arguments may correspond to named parameters - Dict<String, Object> kwArg = hasKwargs ? Dict.of(mu) : Dict.empty(); + // general case (2b): some keyword arguments may correspond to named parameters + Dict<String, Object> kwargs = hasKwargs ? Dict.of(mu) : null; - // For nicer stabler error messages, start by checking against - // an argument being provided both as positional argument and as keyword argument. - ArrayList<String> bothPosKey = new ArrayList<>(); - for (int i = 0; i < numPositionalArgs; i++) { - String name = names.get(i); - if (kwargs.containsKey(name)) { - bothPosKey.add(name); - } - } - if (!bothPosKey.isEmpty()) { - throw new EvalException( - null, - String.format( - "argument%s '%s' passed both by position and by name in call to %s", - (bothPosKey.size() > 1 ? "s" : ""), Joiner.on("', '").join(bothPosKey), func)); - } - - // Accept the arguments that were passed. - for (Map.Entry<String, Object> entry : kwargs.entrySet()) { - String keyword = entry.getKey(); - Object value = entry.getValue(); + // Accept the named arguments that were passed. + for (int i = 0; i < named.length; i += 2) { + String keyword = (String) named[i]; // safe + Object value = named[i + 1]; int pos = names.indexOf(keyword); // the list should be short, so linear scan is OK. if (0 <= pos && pos < numNamedParams) { + if (arguments[pos] != null) { + throw new EvalException( + null, String.format("%s got multiple values for parameter '%s'", func, keyword)); + } arguments[pos] = value; } else { if (!hasKwargs) { - List<String> unexpected = Ordering.natural().sortedCopy(Sets.difference( - kwargs.keySet(), ImmutableSet.copyOf(names.subList(0, numNamedParams)))); + Set<String> unexpected = Sets.newHashSet(); + for (int j = 0; j < named.length; j += 2) { + unexpected.add((String) named[j]); + } + unexpected.removeAll(names.subList(0, numNamedParams)); + // TODO(adonovan): do spelling check. throw new EvalException( null, String.format( "unexpected keyword%s '%s' in call to %s", - unexpected.size() > 1 ? "s" : "", Joiner.on("', '").join(unexpected), func)); + unexpected.size() > 1 ? "s" : "", + Joiner.on("', '").join(Ordering.natural().sortedCopy(unexpected)), + func)); } - if (kwArg.containsKey(keyword)) { + int sz = kwargs.size(); + kwargs.put(keyword, value, null); + if (kwargs.size() == sz) { throw new EvalException( null, String.format("%s got multiple values for keyword argument '%s'", func, keyword)); } - kwArg.put(keyword, value, null); } } if (hasKwargs) { - arguments[kwargIndex] = Dict.copyOf(mu, kwArg); + arguments[kwargIndex] = kwargs; } - // Check that all mandatory parameters were filled in general case 2c. + // Check that all mandatory parameters were filled in general case 2b. // Note: it's possible that numPositionalArgs > numMandatoryPositionalParams but that's OK. for (int i = numPositionalArgs; i < numMandatoryPositionalParams; i++) { if (arguments[i] == null) { @@ -241,45 +222,11 @@ } } } - } // End of general case 2c for argument passing. + } // End of general case 2b for argument passing. return arguments; } - @Override - public Object callImpl( - StarlarkThread thread, - @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) - throws EvalException, InterruptedException { - Object[] arguments = - processArguments(this, signature, getDefaultValues(), thread.mutability(), args, kwargs); - return call(arguments, call, thread); - } - - /** - * Inner call to a BaseFunction subclasses need to @Override this method. - * - * @param args an array of argument values sorted as per the signature. - * @param ast the source code for the function if user-defined - * @param thread the Starlark thread for the call - * @throws InterruptedException may be thrown in the function implementations. - * @deprecated override the {@code callImpl} method directly. - */ - // TODO(adonovan): the only remaining users of the "inner" protocol are: - // - StarlarkFunction. - // - PackageFactory.newPackageFunction, which goes to heroic lengths to reinvent the wheel. - // - SkylarkProvider, which can be optimized by using callImpl directly once - // we've implemented the "vector" calling convention described in Eval. - @Deprecated - protected Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkThread thread) - throws EvalException, InterruptedException { - throw new EvalException( - (ast == null) ? Location.BUILTIN : ast.getLocation(), - String.format("function %s not implemented", getName())); - } - /** * Render this object in the form of an equivalent Python function signature. */ @@ -288,7 +235,7 @@ StringBuilder sb = new StringBuilder(); sb.append(getName()); sb.append('('); - signature.toStringBuilder(sb, this::printDefaultValue); + getSignature().toStringBuilder(sb, this::printDefaultValue); sb.append(')'); return sb.toString(); }
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 b8ed052..a1d13d6 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
@@ -14,8 +14,6 @@ package com.google.devtools.build.lib.syntax; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; -import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** @@ -55,21 +53,26 @@ } @Override - public Object callImpl( - StarlarkThread thread, FuncallExpression call, List<Object> args, Map<String, Object> kwargs) + public Object fastcall( + StarlarkThread thread, @Nullable FuncallExpression call, Object[] positional, Object[] named) throws EvalException, InterruptedException { MethodDescriptor desc = this.desc != null ? this.desc : getMethodDescriptor(thread.getSemantics()); Object objValue = obj; if (obj instanceof String) { - args.add(0, obj); // TODO(adonovan): this mutation looks dubious + // 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, call, desc, objValue.getClass(), args, kwargs); + thread, methodName, call, desc, objValue.getClass(), positional, named); return desc.call(objValue, javaArguments, thread.mutability()); }
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 540ff89..f1753a3 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
@@ -23,7 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils; @@ -34,6 +34,7 @@ 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; @@ -214,39 +215,55 @@ * 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 args a list of positional Starlark arguments - * @param kwargs a map of keyword Starlark arguments; keys are the used keyword, and values are - * their corresponding values in the method call - * @param thread the Starlark thread for the call + * @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, FuncallExpression call, MethodDescriptor method, Class<?> objClass, - List<Object> args, - Map<String, Object> kwargs) + 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 new EvalException( + null, String.format("duplicate argument '%s' in call to '%s'", name, methodName)); + } + } + ImmutableList<ParamDescriptor> parameters = method.getParameters(); // *args, **kwargs, location, call, thread, skylark semantics + // TODO(adonovan): opt: compute correct size and use an array. final int extraArgsCount = 6; List<Object> builder = new ArrayList<>(parameters.size() + extraArgsCount); int argIndex = 0; // Process parameters specified in callable.parameters() - // Many methods don't have any kwargs, so don't allocate a new hash set in that case. - Set<String> keys = - kwargs.isEmpty() ? ImmutableSet.of() : CompactHashSet.create(kwargs.keySet()); // 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. @@ -262,8 +279,11 @@ continue; } - if (argIndex < args.size() && param.isPositional()) { // Positional args and params remain. - value = args.get(argIndex); + 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( call, @@ -272,7 +292,7 @@ method, objClass); } - if (param.isNamed() && keys.contains(param.getName())) { + if (namedValue != null) { throw argumentMismatchException( call, String.format("got multiple values for keyword argument '%s'", param.getName()), @@ -281,9 +301,9 @@ } argIndex++; } else { // No more positional arguments, or no more positional parameters. - if (param.isNamed() && !keys.isEmpty() && keys.remove(param.getName())) { + if (namedValue != null) { // Param specified by keyword argument. - value = kwargs.get(param.getName()); + value = namedValue; if (!type.contains(value)) { throw argumentMismatchException( call, @@ -299,7 +319,7 @@ value = evalDefault(param.getName(), param.getDefaultValue()); } } - if (!param.isNoneable() && value instanceof NoneType) { + if (value == Starlark.NONE && !param.isNoneable()) { throw argumentMismatchException( call, String.format("parameter '%s' cannot be None", param.getName()), @@ -311,25 +331,22 @@ // *args if (method.isAcceptsExtraArgs()) { - builder.add(Tuple.copyOf(args.subList(argIndex, args.size()))); - } else if (argIndex < args.size()) { + builder.add(Tuple.wrap(Arrays.copyOfRange(positional, argIndex, positional.length))); + } else if (argIndex < positional.length) { throw argumentMismatchException( call, String.format( - "expected no more than %s positional arguments, but got %s", argIndex, args.size()), + "expected no more than %s positional arguments, but got %s", + argIndex, positional.length), method, objClass); } // **kwargs if (method.isAcceptsExtraKwargs()) { - Dict<String, Object> dict = Dict.of(thread.mutability()); - for (String k : keys) { - dict.put(k, kwargs.get(k), (Location) null); - } - builder.add(dict); - } else if (!keys.isEmpty()) { - throw unexpectedKeywordArgumentException(call, keys, method, objClass, thread); + builder.add(Dict.wrap(thread.mutability(), kwargs)); + } else if (!kwargs.isEmpty()) { + throw unexpectedKeywordArgumentException(call, kwargs.keySet(), method, objClass, thread); } // Add Location, FuncallExpression, and/or StarlarkThread.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Dict.java b/src/main/java/com/google/devtools/build/lib/syntax/Dict.java index 0a1e2e8..a9bb500 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Dict.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Dict.java
@@ -79,13 +79,26 @@ public final class Dict<K, V> implements Map<K, V>, StarlarkMutable, SkylarkIndexable, StarlarkIterable<K> { - private final LinkedHashMap<K, V> contents = new LinkedHashMap<>(); + private final LinkedHashMap<K, V> contents; /** Final except for {@link #unsafeShallowFreeze}; must not be modified any other way. */ private Mutability mutability; - private Dict(@Nullable Mutability mutability) { + private Dict(@Nullable Mutability mutability, LinkedHashMap<K, V> contents) { this.mutability = mutability == null ? Mutability.IMMUTABLE : mutability; + this.contents = contents; + } + + private Dict(@Nullable Mutability mutability) { + this(mutability, new LinkedHashMap<>()); + } + + /** + * Takes ownership of the supplied LinkedHashMap and returns a new Dict that wraps it. The caller + * must not subsequently modify the map, but the Dict may do so. + */ + static <K, V> Dict<K, V> wrap(@Nullable Mutability mutability, LinkedHashMap<K, V> contents) { + return new Dict<>(mutability, contents); } @Override @@ -216,6 +229,7 @@ useLocation = true) @SuppressWarnings("unchecked") // Cast of value to V public Object setdefault(K key, Object defaultValue, Location loc) throws EvalException { + // TODO(adonovan): opt: use putIfAbsent to avoid hashing twice. Object value = get(key); if (value != null) { return value; @@ -347,6 +361,7 @@ @SuppressWarnings("unchecked") private <KK extends K, VV extends V> Dict<K, V> putAllUnsafe(Map<KK, VV> m) { for (Map.Entry<KK, VV> e : m.entrySet()) { + // TODO(adonovan): the fromJava call here is suspicious and inconsistent. contents.put(e.getKey(), (VV) Starlark.fromJava(e.getValue(), mutability)); } return this;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java index 9ea97f1..d6c5caf 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -14,17 +14,16 @@ package com.google.devtools.build.lib.syntax; -import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.events.Location; import com.google.devtools.build.lib.util.SpellChecker; import java.util.ArrayList; -import java.util.LinkedHashMap; +import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; -import javax.annotation.Nullable; /** A syntax-tree-walking evaluator. */ // TODO(adonovan): make this class the sole locus of tree-based evaluation logic. @@ -134,7 +133,7 @@ // They may be discontinuous: // def f(a, b=1, *, c, d=2) has a defaults tuple of (1, 2). // TODO(adonovan): record the gaps (e.g. c) with a sentinel - // to simplify processArguments. + // to simplify Starlark.matchSignature. Tuple<Object> defaults = Tuple.empty(); int ndefaults = node.getSignature().numOptionals(); if (ndefaults > 0) { @@ -514,18 +513,81 @@ FuncallExpression call = (FuncallExpression) expr; Object fn = eval(thread, call.getFunction()); - ArrayList<Object> posargs = new ArrayList<>(); - Map<String, Object> kwargs = new LinkedHashMap<>(); - // TODO(adonovan): optimize the calling convention to pass a contiguous array of - // positional and named arguments with an array of Strings for the names (which is - // constant for all non-**kwargs call sites). The caller would remain responsible for - // flattening f(*args) and f(**kwargs), but the callee would become responsible for - // duplicate name checking. - // Callees already need to do this work anyway---see BaseFunction.processArguments and - // CallUtils.convertStarlarkArgumentsToJavaMethodArguments---and this avoids constructing - // another hash table in nearly every call - evalArguments(thread, call, posargs, kwargs); - return Starlark.call(thread, fn, call, posargs, kwargs); + + // StarStar and Star args are guaranteed to be last, if they occur. + ImmutableList<Argument> arguments = call.getArguments(); + int n = arguments.size(); + Argument.StarStar starstar = null; + if (n > 0 && arguments.get(n - 1) instanceof Argument.StarStar) { + starstar = (Argument.StarStar) arguments.get(n - 1); + n--; + } + Argument.Star star = null; + if (n > 0 && arguments.get(n - 1) instanceof Argument.Star) { + star = (Argument.Star) arguments.get(n - 1); + n--; + } + // Inv: n = |positional| + |named| + + // Allocate assuming no *args/**kwargs. + int npos = call.getNumPositionalArguments(); + int i; + + // f(expr) -- positional args + Object[] positional = npos == 0 ? EMPTY : new Object[npos]; + for (i = 0; i < npos; i++) { + Argument arg = arguments.get(i); + Object value = eval(thread, arg.getValue()); + positional[i] = value; + } + + // f(id=expr) -- named args + Object[] named = n == npos ? EMPTY : new Object[2 * (n - npos)]; + for (int j = 0; i < n; i++) { + Argument.Keyword arg = (Argument.Keyword) arguments.get(i); + Object value = eval(thread, arg.getValue()); + named[j++] = arg.getName(); + named[j++] = value; + } + + // f(*args) -- varargs + if (star != null) { + Object value = eval(thread, star.getValue()); + if (!(value instanceof StarlarkIterable)) { + throw new EvalException( + call.getLocation(), + "argument after * must be an iterable, not " + EvalUtils.getDataTypeName(value)); + } + // TODO(adonovan): opt: if value.size is known, preallocate (and skip if empty). + ArrayList<Object> list = new ArrayList<>(); + Collections.addAll(list, positional); + Iterables.addAll(list, ((Iterable<?>) value)); + positional = list.toArray(); + } + + // f(**kwargs) + if (starstar != null) { + Object value = eval(thread, starstar.getValue()); + if (!(value instanceof Dict)) { + throw new EvalException( + call.getLocation(), + "argument after ** must be a dict, not " + EvalUtils.getDataTypeName(value)); + } + Dict<?, ?> kwargs = (Dict<?, ?>) value; + int j = named.length; + named = Arrays.copyOf(named, j + 2 * kwargs.size()); + for (Map.Entry<?, ?> e : kwargs.entrySet()) { + if (!(e.getKey() instanceof String)) { + throw new EvalException( + call.getLocation(), + "keywords must be strings, not " + EvalUtils.getDataTypeName(e.getKey())); + } + named[j++] = e.getKey(); + named[j++] = e.getValue(); + } + } + + return Starlark.fastcall(thread, fn, call, positional, named); } case IDENTIFIER: @@ -730,6 +792,8 @@ return comp.isDict() ? dict : StarlarkList.copyOf(thread.mutability(), list); } + private static final Object[] EMPTY = {}; + /** Returns an exception which should be thrown instead of the original one. */ private static EvalException maybeTransformException(Node node, EvalException original) { // If there is already a non-empty stack trace, we only add this node iff it describes a @@ -749,117 +813,6 @@ } } - /** - * Add one named argument to the keyword map, and returns whether that name has been encountered - * before. - */ - private static boolean addKeywordArgAndCheckIfDuplicate( - Map<String, Object> kwargs, String name, Object value) { - return kwargs.put(name, value) != null; - } - - /** - * Add multiple arguments to the keyword map (**kwargs), and returns all the names of those - * arguments that have been encountered before or {@code null} if there are no such names. - */ - @Nullable - private static ImmutableList<String> addKeywordArgsAndReturnDuplicates( - Map<String, Object> kwargs, Object items, Location location) throws EvalException { - if (!(items instanceof Map<?, ?>)) { - throw new EvalException( - location, - "argument after ** must be a dictionary, not '" + EvalUtils.getDataTypeName(items) + "'"); - } - ImmutableList.Builder<String> duplicatesBuilder = null; - for (Map.Entry<?, ?> entry : ((Map<?, ?>) items).entrySet()) { - if (!(entry.getKey() instanceof String)) { - throw new EvalException( - location, - "keywords must be strings, not '" + EvalUtils.getDataTypeName(entry.getKey()) + "'"); - } - String argName = (String) entry.getKey(); - if (addKeywordArgAndCheckIfDuplicate(kwargs, argName, entry.getValue())) { - if (duplicatesBuilder == null) { - duplicatesBuilder = ImmutableList.builder(); - } - duplicatesBuilder.add(argName); - } - } - return duplicatesBuilder == null ? null : duplicatesBuilder.build(); - } - - /** - * Evaluate this FuncallExpression's arguments, and put the resulting evaluated expressions into - * the given {@code posargs} and {@code kwargs} collections. - * - * @param posargs a list to which all positional arguments will be added - * @param kwargs a mutable map to which all keyword arguments will be added. A mutable map is used - * here instead of an immutable map builder to deal with duplicates without memory overhead - * @param thread the Starlark thread for the call - */ - private static void evalArguments( - StarlarkThread thread, - FuncallExpression call, - List<Object> posargs, - Map<String, Object> kwargs) - throws EvalException, InterruptedException { - - // Optimize allocations for the common case where they are no duplicates. - ImmutableList.Builder<String> duplicatesBuilder = null; - // Iterate over the arguments. We assume all positional arguments come before any keyword - // or star arguments, because the argument list was already validated by the Parser, - // which should be the only place that build FuncallExpressions. - // Argument lists are typically short and functions are frequently called, so go by index - // (O(1) for ImmutableList) to avoid the iterator overhead. - List<Argument> args = call.getArguments(); - for (int i = 0; i < args.size(); i++) { - Argument arg = args.get(i); - Object value = eval(thread, arg.getValue()); - if (arg instanceof Argument.Positional) { - // f(expr) - posargs.add(value); - } else if (arg instanceof Argument.Keyword) { - // f(id=expr) - String name = arg.getName(); - if (addKeywordArgAndCheckIfDuplicate(kwargs, name, value)) { - if (duplicatesBuilder == null) { - duplicatesBuilder = ImmutableList.builder(); - } - duplicatesBuilder.add(name); - } - } else if (arg instanceof Argument.Star) { - // f(*args): expand args - if (!(value instanceof StarlarkIterable)) { - throw new EvalException( - call.getLocation(), - "argument after * must be an iterable, not " + EvalUtils.getDataTypeName(value)); - } - Iterables.addAll(posargs, ((Iterable<?>) value)); - } else { - // f(**kwargs): expand kwargs - ImmutableList<String> duplicates = - addKeywordArgsAndReturnDuplicates(kwargs, value, call.getLocation()); - if (duplicates != null) { - if (duplicatesBuilder == null) { - duplicatesBuilder = ImmutableList.builder(); - } - duplicatesBuilder.addAll(duplicates); - } - } - } - if (duplicatesBuilder != null) { - ImmutableList<String> dups = duplicatesBuilder.build(); - throw new EvalException( - call.getLocation(), - "duplicate keyword" - + (dups.size() > 1 ? "s" : "") - + " '" - + Joiner.on("', '").join(dups) - + "' in call to " - + call.getFunction()); - } - } - private static void checkInterrupt() throws InterruptedException { if (Thread.interrupted()) { throw new InterruptedException();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java index 08f278c..20f4924 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -16,8 +16,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import java.util.Collections; -import java.util.List; /** Syntax node for a function call expression. */ // TODO(adonovan): rename CallExpression. @@ -29,8 +27,15 @@ FuncallExpression(Expression function, ImmutableList<Argument> arguments) { this.function = Preconditions.checkNotNull(function); - this.arguments = Preconditions.checkNotNull(arguments); - this.numPositionalArgs = countPositionalArguments(); + this.arguments = arguments; + + int n = 0; + for (Argument arg : arguments) { + if (arg instanceof Argument.Positional) { + n++; + } + } + this.numPositionalArgs = n; } /** Returns the function that is called. */ @@ -38,35 +43,16 @@ return this.function; } - /** - * Returns the number of positional arguments. - */ - private int countPositionalArguments() { - int num = 0; - for (Argument arg : arguments) { - if (arg instanceof Argument.Positional) { - num++; - } - } - return num; - } - - /** - * Returns an (immutable, ordered) list of function arguments. The first n are positional and the - * remaining ones are keyword args, where n = getNumPositionalArguments(). - */ - public List<Argument> getArguments() { - return Collections.unmodifiableList(arguments); - } - - /** - * Returns the number of arguments which are positional; the remainder are - * keyword arguments. - */ + /** Returns the number of arguments of type {@code Argument.Positional}. */ public int getNumPositionalArguments() { return numPositionalArgs; } + /** Returns the function arguments. */ + public ImmutableList<Argument> getArguments() { + return arguments; + } + @Override public String toString() { StringBuilder buf = new StringBuilder();
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 bdf61c5..6f8bb37 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
@@ -215,8 +215,8 @@ } /** - * Calls the function-like value {@code fn} in the specified thread, passing the given positional - * and named arguments, as if by the Starlark expression {@code fn(*args, **kwargs)}. + * Calls the function-like value {@code fn} in the specified thread, passing it the given + * positional and named arguments, as if by the Starlark expression {@code fn(*args, **kwargs)}. */ public static Object call( StarlarkThread thread, @@ -225,6 +225,26 @@ List<Object> args, Map<String, Object> kwargs) throws EvalException, InterruptedException { + Object[] named = new Object[2 * kwargs.size()]; + int i = 0; + for (Map.Entry<String, Object> e : kwargs.entrySet()) { + named[i++] = e.getKey(); + named[i++] = e.getValue(); + } + return fastcall(thread, fn, call, args.toArray(), named); + } + + /** + * Calls the function-like value {@code fn} in the specified thread, passing it the given + * positional and named arguments in the "fastcall" array representation. + */ + public static Object fastcall( + StarlarkThread thread, + Object fn, + @Nullable FuncallExpression call, + Object[] positional, + Object[] named) + throws EvalException, InterruptedException { Location loc = call != null ? call.getLocation() : null; StarlarkCallable callable; @@ -243,7 +263,7 @@ thread.push(callable, loc); try { - return callable.callImpl(thread, call, args, ImmutableMap.copyOf(kwargs)); + return callable.fastcall(thread, call, positional, named); } catch (EvalException ex) { throw ex.ensureLocation(loc); } finally { @@ -287,6 +307,32 @@ env.put(annot.name(), v); } + /** + * Checks the {@code positional} and {@code named} arguments supplied to an implementation of + * {@link StarlarkCallable#fastcall} to ensure they match the {@code signature}. It returns an + * array of effective parameter values corresponding to the parameters of the signature. Newly + * allocated values (e.g. a {@code **kwargs} dict) use the Mutability {@code mu}. + * + * <p>If the function has optional parameters, their default values must be supplied by {@code + * defaults}; see {@link BaseFunction#getDefaultValues} for details. + * + * <p>The caller is responsible for accessing the correct element and casting to an appropriate + * type. + * + * <p>On failure, it throws an EvalException incorporating {@code func.toString()}. + */ + public static Object[] matchSignature( + FunctionSignature signature, + StarlarkCallable func, // only used in error messages + @Nullable Tuple<Object> defaults, + @Nullable Mutability mu, + Object[] positional, + Object[] named) + throws EvalException { + // TODO(adonovan): move implementation here. + return BaseFunction.matchSignature(signature, func, defaults, mu, positional, named); + } + // TODO(adonovan): // // The code below shows the API that is the destination toward which all of the recent
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkCallable.java index d2056f9..960296d 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkCallable.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkCallable.java
@@ -15,37 +15,85 @@ package com.google.devtools.build.lib.syntax; import com.google.devtools.build.lib.events.Location; -import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** * The StarlarkCallable interface is implemented by all Starlark values that may be called from * Starlark like a function, including built-in functions and methods, Starlark functions, and * application-defined objects (such as rules, aspects, and providers in Bazel). + * + * <p>It defines two methods: {@code fastcall}, for performance, or {@code call} for convenience. By + * default, {@code fastcall} delegates to {@code call}, and call throws an exception, so an + * implementer may override either one. */ // TODO(adonovan): rename to just "Callable", since it's unambiguous. public interface StarlarkCallable extends StarlarkValue { /** - * Defines the implementation of function calling for a callable value. + * Defines the "convenient" implementation of function calling for a callable value. * * <p>Do not call this function directly. Use the {@link Starlark#call} function to make a call, * as it handles necessary book-keeping such as maintenance of the call stack, exception handling, * and so on. * + * <p>The default implementation throws an exception. + * * @param thread the StarlarkThread in which the function is called - * @param call the function call expression (going away) - * @param args positional arguments - * @param kwargs named arguments + * @param call the function call expression (this is going away) + * @param args a tuple of the arguments passed by position + * @param kwargs a new, mutable dict of the arguments passed by keyword. Iteration order is + * determined by keyword order in the call expression. */ - // TODO(adonovan): optimize the calling convention; see FUNCALL in Eval.java. - Object callImpl( + default Object call( + StarlarkThread thread, + @Nullable FuncallExpression call, + Tuple<Object> args, + Dict<String, Object> kwargs) + throws EvalException, InterruptedException { + throw new EvalException(null, String.format("function %s not implemented", getName())); + } + + /** + * Defines the "fast" implementation of function calling for a callable value. + * + * <p>Do not call this function directly. Use the {@link Starlark#call} or {@link + * Starlark#fastcall} function to make a call, as it handles necessary book-keeping such as + * maintenance of the call stack, exception handling, and so on. + * + * <p>This method defines the low-level or "fast" calling convention. A more convenient interface + * is provided by the {@link #call} method, which provides a signature analogous to {@code def + * f(*args, **kwargs)}, or possibly the "self-call" feature of the {@link + * SkylarkCallable#selfCall} annotation mechanism. Implementations may elect to use {@code + * Starlark.matchSignature} to assist with argument processing. + * + * <p>The default implementation forwards the call to {@code call}, after rejecting any duplicate + * named arguments. Other implementations of this method should similarly reject duplicates. + * + * @param thread the StarlarkThread in which the function is called + * @param call the function call expression (this is going away) + * @param positional a list of positional arguments + * @param named a list of named arguments, as alternating Strings/Objects. May contain dups. + */ + default Object fastcall( StarlarkThread thread, @Nullable FuncallExpression call, // TODO(adonovan): eliminate - List<Object> args, - Map<String, Object> kwargs) - throws EvalException, InterruptedException; + Object[] positional, + Object[] named) + throws EvalException, InterruptedException { + Object[] arguments = + Starlark.matchSignature( + FunctionSignature.ANY, // def f(*args, **kwargs) + this, + /*defaults=*/ null, + thread.mutability(), + positional, + named); + @SuppressWarnings("unchecked") + Tuple<Object> args = (Tuple<Object>) arguments[0]; + @SuppressWarnings("unchecked") + Dict<String, Object> kwargs = (Dict<String, Object>) arguments[1]; + return call(thread, call, args, kwargs); + } /** Returns the form this callable value should take in a stack trace. */ String getName();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java index c4fc3b4..425c0da 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFunction.java
@@ -15,11 +15,13 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.events.Location; +import javax.annotation.Nullable; /** A StarlarkFunction is the function value created by a Starlark {@code def} statement. */ public final class StarlarkFunction extends BaseFunction { private final String name; + private final FunctionSignature signature; private final Location location; private final ImmutableList<Statement> statements; private final Module module; // a function closes over its defining module @@ -34,8 +36,8 @@ Tuple<Object> defaultValues, ImmutableList<Statement> statements, Module module) { - super(signature); this.name = name; + this.signature = signature; this.location = location; this.statements = statements; this.module = module; @@ -48,6 +50,11 @@ } @Override + public FunctionSignature getSignature() { + return signature; + } + + @Override public Location getLocation() { return location; } @@ -69,7 +76,8 @@ } @Override - protected Object call(Object[] arguments, FuncallExpression ast, StarlarkThread thread) + public Object fastcall( + StarlarkThread thread, @Nullable FuncallExpression call, Object[] positional, Object[] named) throws EvalException, InterruptedException { if (thread.mutability().isFrozen()) { throw new EvalException(null, "Trying to call in frozen environment"); @@ -78,8 +86,11 @@ throw new EvalException(null, String.format("function '%s' called recursively", name)); } - // Registering the functions's arguments as variables in the local StarlarkThread - // foreach loop is not used to avoid iterator overhead + // Compute the effective parameter values + // and update the corresponding variables. + Object[] arguments = + Starlark.matchSignature( + getSignature(), this, getDefaultValues(), thread.mutability(), positional, named); ImmutableList<String> names = getSignature().getParameterNames(); for (int i = 0; i < names.size(); ++i) { thread.update(names.get(i), arguments[i]);
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeProviderApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeProviderApi.java index 488ffbe..298428c 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeProviderApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeProviderApi.java
@@ -16,12 +16,12 @@ import com.google.devtools.build.lib.skylarkbuildapi.core.ProviderApi; import com.google.devtools.build.lib.syntax.BaseFunction; +import com.google.devtools.build.lib.syntax.Dict; import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.FunctionSignature; import com.google.devtools.build.lib.syntax.Printer; import com.google.devtools.build.lib.syntax.StarlarkThread; -import java.util.List; -import java.util.Map; +import com.google.devtools.build.lib.syntax.Tuple; import javax.annotation.Nullable; /** Fake callable implementation of {@link ProviderApi}. */ @@ -34,16 +34,12 @@ private final String name = "ProviderIdentifier" + idCounter++; - public FakeProviderApi() { - super(FunctionSignature.KWARGS); - } - @Override - public Object callImpl( + public Object call( StarlarkThread thread, - @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) { + @Nullable FuncallExpression ast, + Tuple<Object> args, + Dict<String, Object> kwargs) { return new FakeStructApi(); } @@ -53,5 +49,10 @@ } @Override + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; + } + + @Override public void repr(Printer printer) {} }
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAspect.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAspect.java index eca98d5..0fb1577 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAspect.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAspect.java
@@ -16,14 +16,8 @@ import com.google.devtools.build.lib.skylarkbuildapi.SkylarkAspectApi; import com.google.devtools.build.lib.syntax.BaseFunction; -import com.google.devtools.build.lib.syntax.EvalException; -import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.FunctionSignature; import com.google.devtools.build.lib.syntax.Printer; -import com.google.devtools.build.lib.syntax.StarlarkThread; -import java.util.List; -import java.util.Map; -import javax.annotation.Nullable; /** Fake implementation of {@link SkylarkAspectApi}. */ public class FakeSkylarkAspect extends BaseFunction implements SkylarkAspectApi { @@ -35,25 +29,16 @@ private final String name = "AspectIdentifier" + idCounter++; - public FakeSkylarkAspect() { - super(FunctionSignature.KWARGS); - } - - @Override - public Object callImpl( - StarlarkThread thread, - @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) - throws EvalException { - throw new EvalException("not implemented"); - } - @Override public String getName() { return name; } @Override + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; + } + + @Override public void repr(Printer printer) {} }
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkNativeModuleApi.java index 2ccaba8..1093f0e 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkNativeModuleApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkNativeModuleApi.java
@@ -29,8 +29,6 @@ import com.google.devtools.build.lib.syntax.StarlarkCallable; import com.google.devtools.build.lib.syntax.StarlarkList; import com.google.devtools.build.lib.syntax.StarlarkThread; -import java.util.List; -import java.util.Map; import javax.annotation.Nullable; /** Fake implementation of {@link SkylarkNativeModuleApi}. */ @@ -93,11 +91,11 @@ // "native". return new StarlarkCallable() { @Override - public Object callImpl( + public Object fastcall( StarlarkThread thread, @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) { + Object[] positional, + Object[] named) { return Starlark.NONE; }
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java index 23ee2d3..189430b 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
@@ -250,8 +250,9 @@ private static int idCounter = 0; private final String name = "RuleDefinitionIdentifier" + idCounter++; - public RuleDefinitionIdentifier() { - super(FunctionSignature.KWARGS); + @Override + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; } @Override
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java index 75ea775..c48a96a 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
@@ -34,9 +34,7 @@ import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.AttributeType; import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.RuleInfo; import java.util.List; -import java.util.Map; import java.util.stream.Collectors; -import javax.annotation.Nullable; /** * Fake implementation of {@link RepositoryModuleApi}. @@ -101,24 +99,15 @@ private static int idCounter = 0; private final String name = "RepositoryRuleDefinitionIdentifier" + idCounter++; - public RepositoryRuleDefinitionIdentifier() { - super(FunctionSignature.KWARGS); - } - - @Override - public Object callImpl( - StarlarkThread thread, - @Nullable FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) - throws EvalException { - throw new EvalException("not implemented"); - } - @Override public String getName() { return name; } + + @Override + public FunctionSignature getSignature() { + return FunctionSignature.KWARGS; + } } @Override
diff --git a/src/test/java/com/google/devtools/build/lib/profiler/memory/AllocationTrackerTest.java b/src/test/java/com/google/devtools/build/lib/profiler/memory/AllocationTrackerTest.java index b698fba..23ef4a6 100644 --- a/src/test/java/com/google/devtools/build/lib/profiler/memory/AllocationTrackerTest.java +++ b/src/test/java/com/google/devtools/build/lib/profiler/memory/AllocationTrackerTest.java
@@ -22,11 +22,10 @@ import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleFunction; import com.google.devtools.build.lib.profiler.memory.AllocationTracker.RuleBytes; -import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.Callstack; import com.google.devtools.build.lib.syntax.Expression; -import com.google.devtools.build.lib.syntax.FunctionSignature; import com.google.devtools.build.lib.syntax.ParserInput; +import com.google.devtools.build.lib.syntax.StarlarkCallable; import com.google.devtools.build.lib.syntax.SyntaxError; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.perftools.profiles.ProfileProto.Function; @@ -57,12 +56,11 @@ return Expression.parse(input); } - private static class TestFunction extends BaseFunction { + private static class TestFunction implements StarlarkCallable { private final String name; private final Location location; TestFunction(String file, String name, int line) { - super(FunctionSignature.ANY); this.name = name; this.location = location(file, line); } @@ -76,8 +74,6 @@ public Location getLocation() { return location; } - - public void invoke() {} } static class TestRuleFunction extends TestFunction implements RuleFunction {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java index 8c69e45..4f5a96f 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/BaseFunctionTest.java
@@ -25,37 +25,15 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Tests for {@link BaseFunction}. This tests the argument processing by BaseFunction between the - * outer call(posargs, kwargs, ast, thread) and the inner call(args, ast, thread). - */ +/** Tests of the argument processing of {@code Starlark.matchSignature}. */ +// TODO(adonovan): rename. @RunWith(JUnit4.class) public class BaseFunctionTest extends EvaluationTestCase { - /** - * Handy implementation of {@link BaseFunction} that returns all its args as a list. (We'd use - * Tuple, but it can't handle null.) - */ - private static class TestingBaseFunction extends BaseFunction { - TestingBaseFunction(FunctionSignature signature) { - super(signature); - } - - @Override - public String getName() { - return "mixed"; - } - - @Override - public Object call(Object[] arguments, FuncallExpression ast, StarlarkThread thread) { - return Arrays.asList(arguments); - } - } - - private void checkBaseFunction(BaseFunction func, String callExpression, String expectedOutput) + private void checkFunction(StarlarkCallable fn, String callExpression, String expectedOutput) throws Exception { initialize(); - update(func.getName(), func); + update(fn.getName(), fn); if (expectedOutput.charAt(0) == '[') { // a tuple => expected to pass assertWithMessage("Wrong output for " + callExpression) @@ -70,6 +48,7 @@ } } + // TODO(adonovan): redesign this test so that inputs and expected outputs are adjacent. private static final String[] BASE_FUNCTION_EXPRESSIONS = { "mixed()", "mixed(1)", @@ -84,25 +63,53 @@ "mixed(bar=2, foo=1, wiz=3)", }; - public void checkBaseFunctions(boolean onlyNamedArguments, - String expectedSignature, String... expectedResults) throws Exception { - BaseFunction func = new TestingBaseFunction( + public void checkFunctions( + boolean onlyNamedArguments, String expectedSignature, String... expectedResults) + throws Exception { + FunctionSignature sig = onlyNamedArguments - ? FunctionSignature.namedOnly(1, "foo", "bar") - : FunctionSignature.of(1, "foo", "bar")); + ? FunctionSignature.namedOnly(1, "foo", "bar") + : FunctionSignature.of(1, "foo", "bar"); + // This test uses BaseFunction only for its 'repr' implementation. + // The meat of this test exercises only StarlarkCallable. + // TODO(adonovan): make it easier to get repr correct and eliminate BaseFunction here. + BaseFunction func = + new BaseFunction() { + @Override + public String getName() { + return "mixed"; + } + + @Override + public FunctionSignature getSignature() { + return sig; + } + + @Override + public Object fastcall( + StarlarkThread thread, FuncallExpression call, Object[] positional, Object[] named) + throws EvalException { + Object[] arguments = + Starlark.matchSignature( + sig, this, /*defaults=*/ null, thread.mutability(), positional, named); + return Arrays.asList(arguments); + } + }; assertThat(func.toString()).isEqualTo(expectedSignature); for (int i = 0; i < BASE_FUNCTION_EXPRESSIONS.length; i++) { String expr = BASE_FUNCTION_EXPRESSIONS[i]; String expected = expectedResults[i]; - checkBaseFunction(func, expr, expected); + checkFunction(func, expr, expected); } } @Test public void testNoSurplusArguments() throws Exception { - checkBaseFunctions(false, "mixed(foo, bar = ?)", + checkFunctions( + false, + "mixed(foo, bar = ?)", "insufficient arguments received by mixed(foo, bar = ?) (got 0, expected at least 1)", "[1, null]", "[1, 2]", @@ -112,13 +119,15 @@ "missing mandatory positional argument 'foo' while calling mixed(foo, bar = ?)", "[1, 2]", "[1, 2]", - "argument 'foo' passed both by position and by name in call to mixed(foo, bar = ?)", + "mixed(foo, bar = ?) got multiple values for parameter 'foo'", "unexpected keyword 'wiz' in call to mixed(foo, bar = ?)"); } @Test public void testOnlyNamedArguments() throws Exception { - checkBaseFunctions(true, "mixed(*, foo, bar = ?)", + checkFunctions( + true, + "mixed(*, foo, bar = ?)", "missing mandatory keyword arguments in call to mixed(*, foo, bar = ?)", "mixed(*, foo, bar = ?) does not accept positional arguments, but got 1", "mixed(*, foo, bar = ?) does not accept positional arguments, but got 2",
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 49d4c3f..5c16309 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
@@ -24,7 +24,6 @@ import com.google.devtools.build.lib.testutil.TestMode; import java.util.Collections; import java.util.List; -import java.util.Map; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -142,27 +141,20 @@ assertThat(interruptFunction.callCount).isEqualTo(2); } - private static class InterruptFunction extends BaseFunction { + private static class InterruptFunction implements StarlarkCallable { private int callCount = 0; - private InterruptFunction() { - super(FunctionSignature.ANY); - } - @Override public String getName() { return "interrupt"; } @Override - public Object callImpl( - StarlarkThread thread, - FuncallExpression ast, - List<Object> args, - Map<String, Object> kwargs) { + public Object fastcall( + StarlarkThread thread, FuncallExpression ast, Object[] positional, Object[] named) { callCount++; - if (!args.isEmpty() && (Boolean) args.get(0)) { + if (positional.length > 0 && Starlark.truth(positional[0])) { Thread.currentThread().interrupt(); } return Starlark.NONE; @@ -239,21 +231,18 @@ @Test public void testSumFunction() throws Exception { - BaseFunction sum = - new BaseFunction(FunctionSignature.ANY) { + StarlarkCallable sum = + new StarlarkCallable() { @Override public String getName() { return "sum"; } @Override - public Object callImpl( - StarlarkThread thread, - FuncallExpression ast, - List<Object> args, - Map<String, Object> kwargs) { + public Object fastcall( + StarlarkThread thread, FuncallExpression call, Object[] positional, Object[] named) { int sum = 0; - for (Object arg : args) { + for (Object arg : positional) { sum += (Integer) arg; } return sum; @@ -284,22 +273,21 @@ @Test public void testKeywordArgs() throws Exception { - // This function returns the map of keyword arguments passed to it. - BaseFunction kwargs = - new BaseFunction(FunctionSignature.KWARGS) { + StarlarkCallable kwargs = + new StarlarkCallable() { @Override public String getName() { return "kwargs"; } @Override - public Object callImpl( + public Object call( StarlarkThread thread, FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) { - return Dict.copyOf(thread.mutability(), kwargs); + Tuple<Object> args, + Dict<String, Object> kwargs) { + return kwargs; } }; @@ -827,8 +815,11 @@ @Test public void testDictKeysDuplicateKeyArgs() throws Exception { - newTest().testIfExactError("duplicate keywords 'arg', 'k' in call to {\"a\": 1}.keys", - "{'a': 1}.keys(arg='abc', arg='def', k=1, k=2)"); + // 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)"); } @Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java index 2200283..1d653d1 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java
@@ -19,7 +19,6 @@ import com.google.devtools.build.lib.testutil.MoreAsserts; import java.util.ArrayList; import java.util.List; -import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -68,20 +67,20 @@ } private void createOuterFunction(final List<Object> params) throws Exception { - BaseFunction outerFunc = - new BaseFunction(FunctionSignature.ANY) { + StarlarkCallable outerFunc = + new StarlarkCallable() { @Override public String getName() { return "outer_func"; } @Override - public Object callImpl( + public NoneType call( StarlarkThread thread, FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) - throws EvalException, InterruptedException { + Tuple<Object> args, + Dict<String, Object> kwargs) + throws EvalException { params.addAll(args); return Starlark.NONE; } @@ -358,7 +357,7 @@ @Test public void testKwargsBadKey() throws Exception { checkEvalError( - "keywords must be strings, not 'int'", + "keywords must be strings, not int", // "def func(a, b): return a + b", "func('a', **{3: 1})"); } @@ -366,21 +365,23 @@ @Test public void testKwargsIsNotDict() throws Exception { checkEvalError( - "argument after ** must be a dictionary, not 'int'", + "argument after ** must be a dict, not int", "def func(a, b): return a + b", "func('a', **42)"); } @Test public void testKwargsCollision() throws Exception { - checkEvalError("argument 'b' passed both by position and by name in call to func(a, b)", + checkEvalError( + "func(a, b) got multiple values for parameter 'b'", "def func(a, b): return a + b", "func('a', 'b', **{'b': 'foo'})"); } @Test public void testKwargsCollisionWithNamed() throws Exception { - checkEvalError("duplicate keyword 'b' in call to func", + checkEvalError( + "func(a, b) got multiple values for parameter 'b'", "def func(a, b): return a + b", "func('a', b = 'b', **{'b': 'foo'})"); }
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 a258ec3..1f47542 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
@@ -2093,11 +2093,8 @@ } @Override - public Object callImpl( - StarlarkThread thread, - FuncallExpression call, - List<Object> args, - Map<String, Object> kwargs) { + public Object fastcall( + StarlarkThread thread, FuncallExpression call, Object[] positional, Object[] named) { return "fromValues"; } };