Clean up FuncallExpression and document it better With this change, FuncallExpression defers to DotExpression in many cases instead of re-inventing dot expresssion evaluation. This removes some of the code duplication between these two classes. Additionally, this avoids creating a BuiltinCallable for selfCall methods of objects. This is a benefit in that it cleans up the code (it removes the need for redundant BuiltinCallable code, to be removed in a future change), and it also prevents creating a superfluous object which is to be immediately discarded. RELNOTES: None. PiperOrigin-RevId: 219877592
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 b1d314e..fa31f08 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
@@ -17,7 +17,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.Preconditions; -import com.google.common.base.Throwables; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; @@ -29,13 +28,11 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; -import com.google.devtools.build.lib.syntax.EvalException.EvalExceptionWithJavaCause; import com.google.devtools.build.lib.syntax.Runtime.NoneType; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; -import com.google.devtools.build.lib.util.Pair; import java.io.IOException; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; @@ -219,32 +216,6 @@ return methods; } - private static class ArgumentListConversionResult { - private final ImmutableList<Object> arguments; - private final String error; - - private ArgumentListConversionResult(ImmutableList<Object> arguments, String error) { - this.arguments = arguments; - this.error = error; - } - - public static ArgumentListConversionResult fromArgumentList(ImmutableList<Object> arguments) { - return new ArgumentListConversionResult(arguments, null); - } - - public static ArgumentListConversionResult fromError(String error) { - return new ArgumentListConversionResult(null, error); - } - - public String getError() { - return error; - } - - public ImmutableList<Object> getArguments() { - return arguments; - } - } - /** * An exception class to handle exceptions in direct Java API calls. */ @@ -366,7 +337,7 @@ public static Set<String> getStructFieldNames(Class<?> objClass) { return getStructFieldNames(SkylarkSemantics.DEFAULT_SEMANTICS, objClass); } - + /** Returns the list of names of Skylark callable Methods of objClass with structField=true. */ public static Set<String> getStructFieldNames(SkylarkSemantics semantics, Class<?> objClass) { try { @@ -438,25 +409,38 @@ } /** - * Returns a {@link BuiltinCallable} object representing a function which calls the selfCall java + * Returns a {@link MethodDescriptor} object representing a function which calls the selfCall java * method of the given object (the {@link SkylarkCallable} method with {@link * SkylarkCallable#selfCall()} set to true). * * @throws IllegalStateException if no such method exists for the object */ - public static BuiltinCallable getSelfCallMethod(SkylarkSemantics semantics, Object obj) { + public static MethodDescriptor getSelfCallMethodDescriptor( + SkylarkSemantics semantics, Object obj) { try { Optional<MethodDescriptor> selfCallDescriptor = selfCallCache.get(new MethodDescriptorKey(obj.getClass(), semantics)); if (!selfCallDescriptor.isPresent()) { throw new IllegalStateException("Class " + obj.getClass() + " has no selfCall method"); } - MethodDescriptor descriptor = selfCallDescriptor.get(); - return new BuiltinCallable(descriptor.getName(), obj, descriptor); + return selfCallDescriptor.get(); } catch (ExecutionException e) { throw new IllegalStateException("Method loading failed: " + e); } } + /** + * Returns a {@link BuiltinCallable} object representing a function which calls the selfCall java + * method of the given object (the {@link SkylarkCallable} method with {@link + * SkylarkCallable#selfCall()} set to true). + * + * @deprecated use {@link #getSelfCallMethodDescriptor} instead + * @throws IllegalStateException if no such method exists for the object + */ + @Deprecated + public static BuiltinCallable getSelfCallMethod(SkylarkSemantics semantics, Object obj) { + MethodDescriptor descriptor = getSelfCallMethodDescriptor(semantics, obj); + return new BuiltinCallable(descriptor.getName(), obj, descriptor); + } /** * Returns a {@link BuiltinCallable} representing a {@link SkylarkCallable}-annotated instance @@ -498,44 +482,156 @@ return methodDescriptor.call(obj, new Object[0], Location.BUILTIN, null); } - // Throws an EvalException when there is no function matching the given name and arguments. - private Pair<MethodDescriptor, List<Object>> findJavaMethod( + // Throws an EvalException when the arguments do not match the method signature. + private Object[] convertStarlarkArgumentsToJavaMethodArguments( + MethodDescriptor method, Class<?> objClass, - String methodName, List<Object> args, Map<String, Object> kwargs, Environment environment) throws EvalException { - MethodDescriptor method = getMethod(environment.getSemantics(), objClass, methodName); - ArgumentListConversionResult argumentListConversionResult = null; - if (method != null) { - if (method.isStructField()) { - // This indicates a built-in structField which returns a function which may have - // one or more arguments itself. For example, foo.bar('baz'), where foo.bar is a - // structField returning a function. Calling the "bar" callable of foo should - // not have 'baz' propagated, though extra interpreter arguments should be supplied. - return new Pair<>(method, extraInterpreterArgs(method, null, getLocation(), environment)); - } else { - argumentListConversionResult = convertArgumentList(args, kwargs, method, environment); + Preconditions.checkArgument(!method.isStructField(), + "struct field methods should be handled by DotExpression separately"); - if (argumentListConversionResult.getArguments() != null) { - return new Pair<>(method, argumentListConversionResult.getArguments()); - } else { - throw new EvalException(getLocation(), + ImmutableList<ParamDescriptor> parameters = method.getParameters(); + ImmutableList.Builder<Object> builder = + ImmutableList.builderWithExpectedSize(parameters.size() + EXTRA_ARGS_COUNT); + boolean acceptsExtraArgs = method.isAcceptsExtraArgs(); + boolean acceptsExtraKwargs = method.isAcceptsExtraKwargs(); + + int argIndex = 0; + + // Process parameters specified in callable.parameters() + Set<String> keys = new LinkedHashSet<>(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. + // 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 (argIndex < args.size() && param.isPositional()) { // Positional args and params remain. + value = args.get(argIndex); + if (!type.contains(value)) { + throw argumentMismatchException( String.format( - "%s, in method call %s of '%s'", - argumentListConversionResult.getError(), - formatMethod(objClass, methodName, args, kwargs), - EvalUtils.getDataTypeNameFromClass(objClass))); + "expected value of type '%s' for parameter '%s'", type, param.getName()), + method, objClass, args, kwargs); + } + if (param.isNamed() && keys.contains(param.getName())) { + throw argumentMismatchException( + String.format("got multiple values for keyword argument '%s'", param.getName()), + method, objClass, args, kwargs); + } + argIndex++; + } else { // No more positional arguments, or no more positional parameters. + if (param.isNamed() && keys.remove(param.getName())) { + // Param specified by keyword argument. + value = kwargs.get(param.getName()); + if (!type.contains(value)) { + throw argumentMismatchException( + String.format( + "expected value of type '%s' for parameter '%s'", type, param.getName()), + method, objClass, args, kwargs); + } + } else { // Param not specified by user. Use default value. + if (param.getDefaultValue().isEmpty()) { + throw argumentMismatchException( + String.format("parameter '%s' has no default value", param.getName()), + method, objClass, args, kwargs); + } + value = + SkylarkSignatureProcessor.getDefaultValue( + param.getName(), param.getDefaultValue(), null); } } - } else { // method == null - throw new EvalException(getLocation(), - String.format( - "type '%s' has no method %s", - EvalUtils.getDataTypeNameFromClass(objClass), - formatMethod(objClass, methodName, args, kwargs))); + if (!param.isNoneable() && value instanceof NoneType) { + throw argumentMismatchException( + String.format("parameter '%s' cannot be None", param.getName()), + method, objClass, args, kwargs); + } + builder.add(value); } + + ImmutableList<Object> extraArgs = ImmutableList.of(); + if (argIndex < args.size()) { + if (acceptsExtraArgs) { + ImmutableList.Builder<Object> extraArgsBuilder = + ImmutableList.builderWithExpectedSize(args.size() - argIndex); + for (; argIndex < args.size(); argIndex++) { + extraArgsBuilder.add(args.get(argIndex)); + } + extraArgs = extraArgsBuilder.build(); + } else { + throw argumentMismatchException( + String.format( + "expected no more than %s positional arguments, but got %s", + argIndex, args.size()), + method, objClass, args, kwargs); + } + } + ImmutableMap<String, Object> extraKwargs = ImmutableMap.of(); + if (!keys.isEmpty()) { + if (acceptsExtraKwargs) { + ImmutableMap.Builder<String, Object> extraKwargsBuilder = + ImmutableMap.builderWithExpectedSize(keys.size()); + for (String key : keys) { + extraKwargsBuilder.put(key, kwargs.get(key)); + } + extraKwargs = extraKwargsBuilder.build(); + } else { + throw argumentMismatchException( + String.format( + "unexpected keyword%s %s", + keys.size() > 1 ? "s" : "", + Joiner.on(",").join(Iterables.transform(keys, s -> "'" + s + "'"))), + method, objClass, args, kwargs); + } + } + + // Then add any skylark-interpreter arguments (for example kwargs or the Environment). + if (acceptsExtraArgs) { + builder.add(Tuple.copyOf(extraArgs)); + } + if (acceptsExtraKwargs) { + builder.add(SkylarkDict.copyOf(environment, extraKwargs)); + } + appendExtraInterpreterArgs(builder, method, this, getLocation(), environment); + + return builder.build().toArray(); + } + + private EvalException argumentMismatchException(String errorDescription, + MethodDescriptor methodDescriptor, Class<?> objClass, List<Object> args, + Map<String, Object> kwargs) { + if (methodDescriptor.isSelfCall()) { + return new EvalException( + getLocation(), + String.format( + "%s, in call to %s", + errorDescription, + formatMethod(objClass, methodDescriptor.getName(), args, kwargs))); + } else { + return new EvalException( + getLocation(), + String.format( + "%s, in method call %s of '%s'", + errorDescription, + formatMethod(objClass, methodDescriptor.getName(), args, kwargs), + EvalUtils.getDataTypeNameFromClass(objClass))); + } + } + + private EvalException missingMethodException( + Class<?> objClass, String methodName, List<Object> args, Map<String, Object> kwargs) { + return new EvalException( + getLocation(), + String.format( + "type '%s' has no method %s", + EvalUtils.getDataTypeNameFromClass(objClass), + formatMethod(objClass, methodName, args, kwargs))); } /** @@ -582,118 +678,6 @@ } } - /** - * Constructs the parameters list to actually pass to the method, filling with default values if - * any. If there is a type or argument mismatch, returns a result containing an error message. - */ - private ArgumentListConversionResult convertArgumentList( - List<Object> args, - Map<String, Object> kwargs, - MethodDescriptor method, - Environment environment) { - ImmutableList<ParamDescriptor> parameters = method.getParameters(); - ImmutableList.Builder<Object> builder = - ImmutableList.builderWithExpectedSize(parameters.size() + EXTRA_ARGS_COUNT); - boolean acceptsExtraArgs = method.isAcceptsExtraArgs(); - boolean acceptsExtraKwargs = method.isAcceptsExtraKwargs(); - - int argIndex = 0; - - // Process parameters specified in callable.parameters() - Set<String> keys = new LinkedHashSet<>(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. - // 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 (argIndex < args.size() && param.isPositional()) { // Positional args and params remain. - value = args.get(argIndex); - if (!type.contains(value)) { - return ArgumentListConversionResult.fromError( - String.format( - "expected value of type '%s' for parameter '%s'", type, param.getName())); - } - if (param.isNamed() && keys.contains(param.getName())) { - return ArgumentListConversionResult.fromError( - String.format("got multiple values for keyword argument '%s'", param.getName())); - } - argIndex++; - } else { // No more positional arguments, or no more positional parameters. - if (param.isNamed() && keys.remove(param.getName())) { - // Param specified by keyword argument. - value = kwargs.get(param.getName()); - if (!type.contains(value)) { - return ArgumentListConversionResult.fromError( - String.format( - "expected value of type '%s' for parameter '%s'", type, param.getName())); - } - } else { // Param not specified by user. Use default value. - if (param.getDefaultValue().isEmpty()) { - return ArgumentListConversionResult.fromError( - String.format("parameter '%s' has no default value", param.getName())); - } - value = - SkylarkSignatureProcessor.getDefaultValue( - param.getName(), param.getDefaultValue(), null); - } - } - if (!param.isNoneable() && value instanceof NoneType) { - return ArgumentListConversionResult.fromError( - String.format("parameter '%s' cannot be None", param.getName())); - } - builder.add(value); - } - - ImmutableList<Object> extraArgs = ImmutableList.of(); - if (argIndex < args.size()) { - if (acceptsExtraArgs) { - ImmutableList.Builder<Object> extraArgsBuilder = - ImmutableList.builderWithExpectedSize(args.size() - argIndex); - for (; argIndex < args.size(); argIndex++) { - extraArgsBuilder.add(args.get(argIndex)); - } - extraArgs = extraArgsBuilder.build(); - } else { - return ArgumentListConversionResult.fromError( - String.format( - "expected no more than %s positional arguments, but got %s", - argIndex, args.size())); - } - } - ImmutableMap<String, Object> extraKwargs = ImmutableMap.of(); - if (!keys.isEmpty()) { - if (acceptsExtraKwargs) { - ImmutableMap.Builder<String, Object> extraKwargsBuilder = - ImmutableMap.builderWithExpectedSize(keys.size()); - for (String key : keys) { - extraKwargsBuilder.put(key, kwargs.get(key)); - } - extraKwargs = extraKwargsBuilder.build(); - } else { - return ArgumentListConversionResult.fromError( - String.format( - "unexpected keyword%s %s", - keys.size() > 1 ? "s" : "", - Joiner.on(",").join(Iterables.transform(keys, s -> "'" + s + "'")))); - } - } - - // Then add any skylark-interpreter arguments (for example kwargs or the Environment). - if (acceptsExtraArgs) { - builder.add(Tuple.copyOf(extraArgs)); - } - if (acceptsExtraKwargs) { - builder.add(SkylarkDict.copyOf(environment, extraKwargs)); - } - appendExtraInterpreterArgs(builder, method, this, getLocation(), environment); - - return ArgumentListConversionResult.fromArgumentList(builder.build()); - } - private static String formatMethod( Class<?> objClass, String name, List<Object> args, Map<String, Object> kwargs) { if (objClass == StringModule.class) { @@ -768,118 +752,20 @@ } /** - * Checks whether the given object is callable, either by being a {@link BaseFunction} or having a - * {@link SkylarkCallable}-annotated method with selfCall = true. + * Evaluate this FuncallExpression's arguments, and put the resulting evaluated expressions + * into the given {@code posargs} and {@code kwargs} collections. * - * @return a BaseFunction object representing the callable function this object represents - * @throws EvalException if the object is not callable. + * @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 env the current environment */ - private static BaseFunction checkCallable( - SkylarkSemantics semantics, Object functionValue, Location location) throws EvalException { - if (functionValue instanceof BaseFunction) { - return (BaseFunction) functionValue; - } else if (hasSelfCallMethod(semantics, functionValue.getClass())) { - return getSelfCallMethod(semantics, functionValue); - } else { - throw new EvalException( - location, "'" + EvalUtils.getDataTypeName(functionValue) + "' object is not callable"); - } - } - - private boolean includeSelfAsArg( - Object value, @Nullable BaseFunction globalBuiltinRegistryFunction) { - if (value instanceof String) { - // String is a special case, as it is treated like a skylark value but cannot be subclassed - // in java. Callable functions which represent methods on string objects must thus be given - // the string 'self' object. - return true; - } - if (globalBuiltinRegistryFunction != null && !isNamespace(value.getClass())) { - // Non-namespace objects which have registered static functions in the global builtin registry - // need to have the instance object passed to the method invocation. - // TODO(cparsons): Global builtin registry functions are going away, so this use-case is - // deprecated. - return true; - } - return false; - } - - /** - * Call a method depending on the type of an object it is called on. - * - * @param positionalArgs positional arguments to pass to the method - * @param call the original expression that caused this call, needed for rules especially - */ - private Object invokeObjectMethod( - Object value, - String method, - @Nullable BaseFunction function, - ImmutableList<Object> positionalArgs, - ImmutableMap<String, Object> keyWordArgs, - FuncallExpression call, - Environment env) - throws EvalException, InterruptedException { - Location location = call.getLocation(); - if (function != null) { - return function.call(positionalArgs, keyWordArgs, call, env); - } - Object fieldValue = - (value instanceof ClassObject) ? ((ClassObject) value).getValue(method) : null; - if (fieldValue != null) { - if (!(fieldValue instanceof BaseFunction)) { - throw new EvalException( - location, String.format("struct field '%s' is not a function", method)); - } - function = (BaseFunction) fieldValue; - return function.call(positionalArgs, keyWordArgs, call, env); - } else { - // When calling a Java method, the name is not in the Environment, - // so evaluating 'function' would fail. - Class<?> objClass; - Object obj; - if (value instanceof Class<?>) { - // Static call - obj = null; - objClass = (Class<?>) value; - } else if (value instanceof String) { - // String is special-cased, since it can't be subclassed. Methods on strings defer - // to StringModule. - obj = StringModule.INSTANCE; - objClass = StringModule.class; - } else { - obj = value; - objClass = value.getClass(); - } - Pair<MethodDescriptor, List<Object>> javaMethod = - call.findJavaMethod(objClass, method, positionalArgs, keyWordArgs, env); - if (javaMethod.first.isStructField()) { - // Not a method but a callable attribute - Object func; - try { - func = javaMethod.first.invoke(obj); - } catch (IllegalAccessException e) { - throw new EvalException(getLocation(), "method invocation failed: " + e); - } catch (InvocationTargetException e) { - if (e.getCause() instanceof FuncallException) { - throw new EvalException(getLocation(), e.getCause().getMessage()); - } else if (e.getCause() != null) { - Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class); - throw new EvalExceptionWithJavaCause(getLocation(), e.getCause()); - } else { - // This is unlikely to happen - throw new EvalException(getLocation(), "method invocation failed: " + e); - } - } - return callFunction(func, env); - } - return javaMethod.first.call(obj, javaMethod.second.toArray(), location, env); - } - } - @SuppressWarnings("unchecked") - private void evalArguments(ImmutableList.Builder<Object> posargs, Map<String, Object> kwargs, - Environment env) + private void evalArguments( + List<Object> posargs, Map<String, Object> kwargs, Environment env) 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 @@ -899,7 +785,9 @@ getLocation(), "argument after * must be an iterable, not " + EvalUtils.getDataTypeName(value)); } - posargs.addAll((Iterable<Object>) value); + for (Object starArgUnit : (Iterable<Object>) value) { + posargs.add(starArgUnit); + } } else if (arg.isStarStar()) { // expand the kwargs ImmutableList<String> duplicates = addKeywordArgsAndReturnDuplicates(kwargs, value, getLocation()); @@ -939,46 +827,118 @@ @Override Object doEval(Environment env) throws EvalException, InterruptedException { - // TODO: Remove this special case once method resolution and invocation are supported as - // separate steps. + // This is a hack which provides some performance improvement over the alternative: + // Consider 'foo.bar()'. Without this hack, the parser would evaluate the DotExpression + // 'foo.bar' first, determine that 'foo.bar' is a callable object, and then invoke the + // callable object. If 'foo' is a java object, however, the parser would need to create a + // new function object to represent 'foo.bar' *just* so it could invoke method 'bar' of 'foo'. + // Constructing throwaway function objects would be a performance hit, so instead this code + // effectively 'looks ahead' to invoke an object method directly. if (function instanceof DotExpression) { return invokeObjectMethod(env, (DotExpression) function); } Object funcValue = function.eval(env); - return callFunction(funcValue, env); + ArrayList<Object> posargs = new ArrayList<>(); + Map<String, Object> kwargs = new LinkedHashMap<>(); + evalArguments(posargs, kwargs, env); + return callFunction(funcValue, posargs, kwargs, env); } /** Invokes object.function() and returns the result. */ private Object invokeObjectMethod(Environment env, DotExpression dot) throws EvalException, InterruptedException { Object objValue = dot.getObject().eval(env); - String method = dot.getField().getName(); - @Nullable - BaseFunction function = Runtime.getBuiltinRegistry().getFunction(objValue.getClass(), method); - ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>(); - if (includeSelfAsArg(objValue, function)) { - posargs.add(objValue); - } - // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or - // we'd still have to have a HashMap on the side for the sake of properly handling duplicates. + String methodName = dot.getField().getName(); + ArrayList<Object> posargs = new ArrayList<>(); Map<String, Object> kwargs = new LinkedHashMap<>(); evalArguments(posargs, kwargs, env); - return invokeObjectMethod( - objValue, method, function, posargs.build(), ImmutableMap.copyOf(kwargs), this, env); + + // Case 1: Object is a String. String is an unusual special case. + if (objValue instanceof String) { + return callStringMethod((String) objValue, methodName, posargs, kwargs, env); + } + + // Case 2: Object is a Java object with a matching @SkylarkCallable method. + // This is an optimization. For 'foo.bar()' where 'foo' is a java object with a callable + // java method 'bar()', this avoids evaluating 'foo.bar' in isolation (which would require + // creating a throwaway function-like object). + MethodDescriptor methodDescriptor = + FuncallExpression.getMethod(env.getSemantics(), objValue.getClass(), methodName); + if (methodDescriptor != null && !methodDescriptor.isStructField()) { + Object[] javaArguments = convertStarlarkArgumentsToJavaMethodArguments( + methodDescriptor, objValue.getClass(), posargs, kwargs, env); + return methodDescriptor.call(objValue, javaArguments, getLocation(), env); + } + + // Case 3: Object is a function registered with the BuiltinRegistry. + // TODO(cparsons): The runtime builtin registry is deprecated and only used by non-Bazel users + // of the Starlark interpreter. Remove its use. + BaseFunction legacyRuntimeFunction = + Runtime.getBuiltinRegistry().getFunction(objValue.getClass(), methodName); + if (legacyRuntimeFunction != null) { + return callLegacyBuiltinRegistryFunction( + legacyRuntimeFunction, objValue, posargs, kwargs, env); + } + + // Case 4: All other cases. Evaluate "foo.bar" as a dot expression, then try to invoke it + // as a callable. + Object functionObject = DotExpression.eval(objValue, methodName, dot.getLocation(), env); + if (functionObject == null) { + throw missingMethodException(objValue.getClass(), methodName, posargs, kwargs); + } else { + return callFunction(functionObject, posargs, kwargs, env); + } + } + + private Object callLegacyBuiltinRegistryFunction(BaseFunction legacyRuntimeFunction, + Object objValue, ArrayList<Object> posargs, Map<String, Object> kwargs, Environment env) + throws EvalException, InterruptedException { + if (!isNamespace(objValue.getClass())) { + posargs.add(0, objValue); + } + return legacyRuntimeFunction.call(posargs, kwargs, this, env); + } + + private Object callStringMethod(String objValue, String methodName, + ArrayList<Object> posargs, Map<String, Object> kwargs, Environment env) + throws InterruptedException, EvalException { + // String is a special case, since it can't be subclassed. Methods on strings defer + // to StringModule, and thus need to include the actual string as a 'self' parameter. + posargs.add(0, objValue); + + MethodDescriptor method = getMethod(env.getSemantics(), StringModule.class, methodName); + if (method == null) { + throw missingMethodException(StringModule.class, methodName, posargs, kwargs); + } + + Object[] javaArguments = convertStarlarkArgumentsToJavaMethodArguments( + method, StringModule.class, posargs, kwargs, env); + return method.call( + StringModule.INSTANCE, javaArguments, getLocation(), env); } /** - * Calls a function object + * Calls a callable object {@code funcValue}. + * + * @throws EvalException if funcValue is not a callable object or if invalid arguments are + * given */ - private Object callFunction(Object funcValue, Environment env) + private Object callFunction(Object funcValue, + ArrayList<Object> posargs, Map<String, Object> kwargs, Environment env) throws EvalException, InterruptedException { - ImmutableList.Builder<Object> posargs = new ImmutableList.Builder<>(); - // We copy this into an ImmutableMap in the end, but we can't use an ImmutableMap.Builder, or - // we'd still have to have a HashMap on the side for the sake of properly handling duplicates. - Map<String, Object> kwargs = new LinkedHashMap<>(); - BaseFunction function = checkCallable(env.getSemantics(), funcValue, getLocation()); - evalArguments(posargs, kwargs, env); - return function.call(posargs.build(), ImmutableMap.copyOf(kwargs), this, env); + + if (funcValue instanceof BaseFunction) { + BaseFunction function = (BaseFunction) funcValue; + return function.call(posargs, ImmutableMap.copyOf(kwargs), this, env); + } else if (hasSelfCallMethod(env.getSemantics(), funcValue.getClass())) { + MethodDescriptor descriptor = getSelfCallMethodDescriptor(env.getSemantics(), funcValue); + Object[] javaArguments = convertStarlarkArgumentsToJavaMethodArguments( + descriptor, funcValue.getClass(), posargs, kwargs, env); + return descriptor.call(funcValue, javaArguments, getLocation(), env); + } else { + throw new EvalException( + getLocation(), "'" + EvalUtils.getDataTypeName(funcValue) + "' object is not callable"); + } } /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java index 83ff7d0..ea378ae 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
@@ -324,7 +324,13 @@ */ private static final BuiltinRegistry builtins = new BuiltinRegistry(); - /** Retrieve the static instance containing information on all known Skylark builtins. */ + /** + * Retrieve the static instance containing information on all known Skylark builtins. + * + * @deprecated do not use a static singleton registry -- instead set up the Skylark environment + * with 'global' objects + */ + @Deprecated public static BuiltinRegistry getBuiltinRegistry() { return builtins; }
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 2809f04..932ce26 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
@@ -734,6 +734,6 @@ assertThrows(AssertionError.class, () -> getConfiguredTarget("//examples/apple_skylark:my_target")); assertThat(expected).hasMessageThat() - .contains("unexpected keyword 'foo' in call"); + .contains("unexpected keyword 'foo', in call to AppleStaticLibrary"); } }
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 69511a0..60dc76f 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
@@ -1127,8 +1127,7 @@ @Test public void testStructAccessingNonFunctionFieldWithArgs() throws Exception { - checkErrorContains( - "struct field 'a' is not a function", "x = struct(a = 1, b = 2)", "x1 = x.a(1)"); + checkErrorContains("'int' object is not callable", "x = struct(a = 1, b = 2)", "x1 = x.a(1)"); } @Test @@ -1140,7 +1139,7 @@ @Test public void testStructPosArgs() throws Exception { checkErrorContains( - "struct(**kwargs) does not accept positional arguments, but got 1", "x = struct(1, b = 2)"); + "expected no more than 0 positional arguments, but got 1", "x = struct(1, b = 2)"); } @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 4bf5203..fe87346 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
@@ -1292,7 +1292,7 @@ assertThrows(AssertionError.class, () -> getConfiguredTarget("//test:my_rule")); assertThat(expected) .hasMessageThat() - .contains("unexpected keyword 'foo' in call to DefaultInfo"); + .contains("unexpected keyword 'foo', in call to DefaultInfo"); } @Test
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 732c4e8..682a8a4 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
@@ -1272,7 +1272,7 @@ new SkylarkTest() .update("mock", new Mock()) .testIfErrorContains( - "expected string for 'pos' while calling MockFn but got int instead: 1", + "expected value of type 'string' for parameter 'pos', in call to MockFn(int)", "v = mock(1)"); } @@ -2175,19 +2175,11 @@ } @Test - public void testStructFieldDefinedInValuesAndSkylarkCallable() throws Exception { - new SkylarkTest() - .update("val", new SkylarkClassObjectWithSkylarkCallables()) - .setUp("v = val.collision_field") - .testLookup("v", "fromSkylarkCallable"); - } - - @Test public void testStructMethodDefinedInValuesAndSkylarkCallable() throws Exception { new SkylarkTest() .update("val", new SkylarkClassObjectWithSkylarkCallables()) .setUp("v = val.collision_method()") - .testLookup("v", "fromValues"); + .testLookup("v", "fromSkylarkCallable"); } @Test