From 770606a77e147406e3b2bfd9d707f04a66890a4c Mon Sep 17 00:00:00 2001 From: adonovan Date: Wed, 11 Dec 2019 11:52:31 -0800 Subject: [PATCH] bazel syntax: define Starlark.call and use it everywhere All calls of Starlark callable values go through this function, whether from the interpreter or Java code. A follow-up change will make it responsible for doing the push/pop of the call stack. Also, rename StarlarkCallable.call to callImpl, since no-one should use it directly. (And reorder its parameters.) Also, remove sole external call to BaseFunction.callWithArgArray, and inline it into sole remaining internal caller. MethodDescriptor.callField now handles the two places that call a @SkylarkCallable(structField)-annotated Java method. Previously they were inconsistent in which "extra parameters" they permitted. Now they allow Location and StarlarkSemantics, but not StarlarkThread or FuncallExpression. The annotation processor rejects useStarlarkThreadfor a structField. (One instance was downgraded to useStarlarkSemantics.) This is a breaking API change for copybara. PiperOrigin-RevId: 285031076 --- .../StarlarkDefinedConfigTransition.java | 3 +- .../skylark/SkylarkCustomCommandLine.java | 12 ++-- .../SkylarkRuleConfiguredTargetUtil.java | 9 +-- .../skylark/SkylarkRepositoryFunction.java | 14 +++-- .../lib/packages/StarlarkCallbackHelper.java | 7 ++- .../build/lib/rules/cpp/CcLinkingContext.java | 8 +-- .../lib/skyframe/SkylarkAspectFactory.java | 17 +++-- .../cpp/CcLinkingContextApi.java | 6 +- .../skylarkbuildapi/cpp/LinkerInputApi.java | 6 +- .../processor/SkylarkCallableProcessor.java | 10 ++- .../build/lib/syntax/BaseFunction.java | 60 ++++++------------ .../build/lib/syntax/BuiltinCallable.java | 12 ++-- .../devtools/build/lib/syntax/CallUtils.java | 63 ++----------------- .../devtools/build/lib/syntax/Eval.java | 2 +- .../devtools/build/lib/syntax/EvalUtils.java | 6 +- .../build/lib/syntax/MethodDescriptor.java | 39 ++++++++++-- .../build/lib/syntax/MethodLibrary.java | 5 +- .../devtools/build/lib/syntax/Starlark.java | 32 +++++++++- .../build/lib/syntax/StarlarkCallable.java | 27 ++++---- .../FakeSkylarkNativeModuleApi.java | 8 +-- .../lib/packages/SkylarkProviderTest.java | 8 ++- .../SkylarkCallableProcessorTest.java | 6 +- .../processor/testsources/GoldenCase.java | 4 +- .../build/lib/syntax/EvaluationTest.java | 16 ++--- .../build/lib/syntax/FunctionTest.java | 8 +-- .../lib/syntax/SkylarkEvaluationTest.java | 8 +-- 26 files changed, 195 insertions(+), 201 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java index a239a2281b96f0..b12ce93ae0564b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/StarlarkDefinedConfigTransition.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.Printer; import com.google.devtools.build.lib.syntax.Sequence; +import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.syntax.StarlarkThread; import java.util.List; @@ -282,7 +283,7 @@ private Object evalFunction(BaseFunction function, ImmutableList args) .setEventHandler(getEventHandler()) .build(); starlarkContext.storeInThread(thread); - return function.call(args, ImmutableMap.of(), null, thread); + return Starlark.call(thread, function, /*call=*/ null, args, /*kwargs=*/ ImmutableMap.of()); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java index 059ed3c6d0d670..31be6b03c60d58 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java @@ -796,7 +796,7 @@ private static Object applyMapFn( .setSemantics(starlarkSemantics) .setEventHandler(NullEventHandler.INSTANCE) .build(); - return mapFn.call(args, ImmutableMap.of(), null, thread); + return Starlark.call(thread, mapFn, /*call=*/ null, args, /*kwargs=*/ ImmutableMap.of()); } catch (EvalException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, e.getCause())); } catch (InterruptedException e) { @@ -820,11 +820,15 @@ private static void applyMapEach( // TODO(b/77140311): Error if we issue print statements .setEventHandler(NullEventHandler.INSTANCE) .build(); - Object[] args = new Object[1]; int count = originalValues.size(); for (int i = 0; i < count; ++i) { - args[0] = originalValues.get(i); - Object ret = mapFn.callWithArgArray(args, null, thread, location); + Object ret = + Starlark.call( + thread, + mapFn, + /*call=*/ null, + originalValues.subList(i, i + 1), + /*kwargs=*/ ImmutableMap.of()); if (ret instanceof String) { consumer.accept((String) ret); } else if (ret instanceof Sequence) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java index d73876089d3cd2..b3459a7b4a38b2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java @@ -131,11 +131,12 @@ public static ConfiguredTarget buildRule( } Object target = - ruleImplementation.call( + Starlark.call( + thread, + ruleImplementation, + /*call=*/ null, /*args=*/ ImmutableList.of(skylarkRuleContext), - /*kwargs*/ ImmutableMap.of(), - /*ast=*/ null, - thread); + /*kwargs=*/ ImmutableMap.of()); if (ruleContext.hasErrors()) { return null; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java index 904e1770fb9646..a1b8a0ab4541e7 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Mutability; +import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkFunction; import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.syntax.StarlarkThread; @@ -178,15 +179,16 @@ public RepositoryDirectoryValue.Builder fetch( // Also we do a lot of stuff in there, maybe blocking operations and we should certainly make // it possible to return null and not block but it doesn't seem to be easy with Skylark // structure as it is. - Object retValue = - function.call( + Object result = + Starlark.call( + thread, + function, + /*call=*/ null, /*args=*/ ImmutableList.of(skylarkRepositoryContext), - /*kwargs=*/ ImmutableMap.of(), - null, - thread); + /*kwargs=*/ ImmutableMap.of()); RepositoryResolvedEvent resolved = new RepositoryResolvedEvent( - rule, skylarkRepositoryContext.getAttr(), outputDirectory, retValue); + rule, skylarkRepositoryContext.getAttr(), outputDirectory, result); if (resolved.isNewInformationReturned()) { env.getListener().handle(Event.debug(resolved.getMessage())); env.getListener().handle(Event.debug(rule.getDefinitionInformation())); diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkCallbackHelper.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkCallbackHelper.java index 0428d69677434b..64c5c613b7be3c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkCallbackHelper.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkCallbackHelper.java @@ -14,12 +14,14 @@ package com.google.devtools.build.lib.packages; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.syntax.ClassObject; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.FuncallExpression; import com.google.devtools.build.lib.syntax.Mutability; +import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkFunction; import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.syntax.StarlarkThread; @@ -77,8 +79,9 @@ public Object call(EventHandler eventHandler, ClassObject ctx, Object... argumen .setEventHandler(eventHandler) .build(); context.storeInThread(thread); - return callback.call(buildArgumentList(ctx, arguments), null, ast, thread); - } catch (ClassCastException | IllegalArgumentException e) { + return Starlark.call( + thread, callback, ast, buildArgumentList(ctx, arguments), /*kwargs=*/ ImmutableMap.of()); + } catch (ClassCastException | IllegalArgumentException e) { // TODO(adonovan): investigate throw new EvalException(ast.getLocation(), e.getMessage()); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java index 9665070bb011df..4423c8ee8e5de6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingContext.java @@ -35,7 +35,7 @@ import com.google.devtools.build.lib.syntax.Sequence; import com.google.devtools.build.lib.syntax.SkylarkType; import com.google.devtools.build.lib.syntax.StarlarkList; -import com.google.devtools.build.lib.syntax.StarlarkThread; +import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.util.Fingerprint; import java.util.Arrays; import java.util.Collection; @@ -205,7 +205,7 @@ public List getLibraries() { } @Override - public Sequence getSkylarkLibrariesToLink(StarlarkThread thread) { + public Sequence getSkylarkLibrariesToLink(StarlarkSemantics semantics) { return StarlarkList.immutableCopyOf(getLibraries()); } @@ -404,9 +404,9 @@ public Sequence getSkylarkUserLinkFlags() { } @Override - public Object getSkylarkLibrariesToLink(StarlarkThread thread) { + public Object getSkylarkLibrariesToLink(StarlarkSemantics semantics) { // TODO(plf): Flag can be removed already. - if (thread.getSemantics().incompatibleDepsetForLibrariesToLinkGetter()) { + if (semantics.incompatibleDepsetForLibrariesToLinkGetter()) { return Depset.of(LibraryToLink.TYPE, getLibraries()); } else { return StarlarkList.immutableCopyOf(getLibraries().toList()); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java index b02766e096c9f1..db8479df6f98a5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java @@ -38,6 +38,7 @@ import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.Mutability; import com.google.devtools.build.lib.syntax.SkylarkType; +import com.google.devtools.build.lib.syntax.Starlark; import com.google.devtools.build.lib.syntax.StarlarkThread; import com.google.devtools.build.lib.syntax.StarlarkValue; import java.util.Map; @@ -87,16 +88,14 @@ public ConfiguredAspect create( ruleContext.getLabel()) .storeInThread(thread); - Object aspectSkylarkObject; try { - aspectSkylarkObject = - skylarkAspect - .getImplementation() - .call( - /*args=*/ ImmutableList.of(ctadBase.getConfiguredTarget(), skylarkRuleContext), - /* kwargs= */ ImmutableMap.of(), - /*ast=*/ null, - thread); + Object aspectSkylarkObject = + Starlark.call( + thread, + skylarkAspect.getImplementation(), + /*call=*/ null, + /*args=*/ ImmutableList.of(ctadBase.getConfiguredTarget(), skylarkRuleContext), + /*kwargs=*/ ImmutableMap.of()); // If allowing analysis failures, targets should be created somewhat normally, and errors // will be propagated via a hook elsewhere as AnalysisFailureInfo. diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcLinkingContextApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcLinkingContextApi.java index e0381e7d9442dd..f9f1f16f4d6c72 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcLinkingContextApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcLinkingContextApi.java @@ -20,8 +20,8 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.Depset; import com.google.devtools.build.lib.syntax.Sequence; +import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier; -import com.google.devtools.build.lib.syntax.StarlarkThread; import com.google.devtools.build.lib.syntax.StarlarkValue; /** Wrapper for every C++ linking provider. */ @@ -44,8 +44,8 @@ public interface CcLinkingContextApi extends StarlarkValu "Returns the depset of LibraryToLink. May return a list but this is" + "deprecated. See #8118.", structField = true, - useStarlarkThread = true) - Object getSkylarkLibrariesToLink(StarlarkThread thread); + useStarlarkSemantics = true) + Object getSkylarkLibrariesToLink(StarlarkSemantics semantics); @SkylarkCallable( name = "additional_inputs", diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/LinkerInputApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/LinkerInputApi.java index 88280983b64ac2..29e6ca0dcda2a7 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/LinkerInputApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/LinkerInputApi.java @@ -22,8 +22,8 @@ import com.google.devtools.build.lib.skylarkinterface.SkylarkModuleCategory; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Sequence; +import com.google.devtools.build.lib.syntax.StarlarkSemantics; import com.google.devtools.build.lib.syntax.StarlarkSemantics.FlagIdentifier; -import com.google.devtools.build.lib.syntax.StarlarkThread; import com.google.devtools.build.lib.syntax.StarlarkValue; /** Either libraries, flags or other files that may be passed to the linker as inputs. */ @@ -56,8 +56,8 @@ public interface LinkerInputApi< + "deprecated. See #8118.", structField = true, enableOnlyWithFlag = FlagIdentifier.EXPERIMENTAL_CC_SHARED_LIBRARY, - useStarlarkThread = true) - Sequence getSkylarkLibrariesToLink(StarlarkThread thread); + useStarlarkSemantics = true) + Sequence getSkylarkLibrariesToLink(StarlarkSemantics semantics); @SkylarkCallable( name = "additional_inputs", diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java index f0da506b3f5980..8d619945ba2d49 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java @@ -289,12 +289,20 @@ private void verifyNotStructFieldWithParams( throws SkylarkCallableProcessorException { if (annotation.structField()) { if (annotation.useAst() + || annotation.useStarlarkThread() || !annotation.extraPositionals().name().isEmpty() || !annotation.extraKeywords().name().isEmpty()) { + // TODO(adonovan): decide on the restrictions. + // - useLocation is needed only by repository_ctx.os. Abolish? + // - useStarlarkSemantics is needed only by getSkylarkLibrariesToLink. + // - banning useStarlarkThread has not been a problem so far, + // and avoids many tricky problems (especially in StructImpl.equal), + // but it forces implementations to assume Mutability=null, + // which is not quite right. throw new SkylarkCallableProcessorException( methodElement, "@SkylarkCallable-annotated methods with structField=true may not also specify " - + "useAst, extraPositionals, or extraKeywords"); + + "useAst, useStarlarkThread, extraPositionals, or extraKeywords"); } } } 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 a248503a50ee90..307c485cbdd399 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 @@ -267,28 +267,29 @@ public Object[] processArguments( return arguments; } - /** - * The outer calling convention to a BaseFunction. - * - * @param args a list of all positional arguments (as in *args) - * @param kwargs a map for key arguments (as in **kwargs) - * @param ast the expression for this function's definition - * @param thread the StarlarkThread in the function is called - * @return the value resulting from evaluating the function with the given arguments - * @throws EvalException-s containing source information. - */ @Override - public Object call( + public Object callImpl( + StarlarkThread thread, + @Nullable FuncallExpression call, List args, - @Nullable Map kwargs, - @Nullable FuncallExpression ast, - StarlarkThread thread) + Map kwargs) throws EvalException, InterruptedException { - // ast is null when called from Java (as there's no Skylark call site). - Location loc = ast == null ? Location.BUILTIN : ast.getLocation(); + // call is null when called from Java (as there's no Skylark call site). + Location loc = call == null ? Location.BUILTIN : call.getLocation(); Object[] arguments = processArguments(args, kwargs, loc, thread); - return callWithArgArray(arguments, ast, thread, getLocation()); + + // TODO(adonovan): move Callstack.push/pop into StarlarkThread.push/pop. + try { + if (Callstack.enabled) { + Callstack.push(this); + } + return call(arguments, call, thread); + } finally { + if (Callstack.enabled) { + Callstack.pop(); + } + } } /** @@ -307,31 +308,6 @@ protected Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkTh String.format("function %s not implemented", getName())); } - /** - * The outer calling convention to a BaseFunction. This function expects all arguments to have - * been resolved into positional ones. - * - * @param ast the expression for this function's definition - * @param thread the StarlarkThread in the function is called - * @return the value resulting from evaluating the function with the given arguments - * @throws EvalException-s containing source information. - */ - // TODO(adonovan): make this private. The sole external caller has a location but no ast. - public Object callWithArgArray( - Object[] arguments, @Nullable FuncallExpression ast, StarlarkThread thread, Location loc) - throws EvalException, InterruptedException { - try { - if (Callstack.enabled) { - Callstack.push(this); - } - return call(arguments, ast, thread); - } finally { - if (Callstack.enabled) { - Callstack.pop(); - } - } - } - /** * Render this object in the form of an equivalent Python function signature. */ 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 0cdd4614553b4d..c91f56cf3aa06c 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 @@ -58,11 +58,8 @@ public final class BuiltinCallable implements StarlarkCallable { } @Override - public Object call( - List args, - @Nullable Map kwargs, - FuncallExpression ast, - StarlarkThread thread) + public Object callImpl( + StarlarkThread thread, FuncallExpression call, List args, Map kwargs) throws EvalException, InterruptedException { MethodDescriptor methodDescriptor = desc != null ? desc : getMethodDescriptor(thread.getSemantics()); @@ -83,8 +80,9 @@ public Object call( Profiler.instance().profile(ProfilerTask.STARLARK_BUILTIN_FN, methodName)) { Object[] javaArguments = CallUtils.convertStarlarkArgumentsToJavaMethodArguments( - thread, ast, methodDescriptor, clazz, args, kwargs); - return methodDescriptor.call(objValue, javaArguments, ast.getLocation(), thread); + thread, call, methodDescriptor, clazz, args, kwargs); + return methodDescriptor.call( + objValue, javaArguments, call.getLocation(), 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 7cb4ac59c09e7a..540ff89994ab64 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 @@ -167,11 +167,7 @@ public static Object getField(StarlarkSemantics semantics, Object x, String fiel String.format( "value of type %s has no .%s field", EvalUtils.getDataTypeName(x), fieldName)); } - // This condition is enforced statically by the annotation processor. - Preconditions.checkArgument( - !(desc.isUseStarlarkThread() || desc.isUseStarlarkSemantics() || desc.isUseLocation()), - "Cannot be invoked on structField callables with extra interpreter params"); - return desc.call(x, new Object[0], Location.BUILTIN, /*thread=*/ null); + return desc.callField(x, Location.BUILTIN, semantics, /*mu=*/ null); } /** Returns the names of the Starlark fields of {@code x} under the specified semantics. */ @@ -241,7 +237,7 @@ static Object[] convertStarlarkArgumentsToJavaMethodArguments( "struct field methods should be handled by DotExpression separately"); ImmutableList parameters = method.getParameters(); - // *args, **kwargs, location, ast, thread, skylark semantics + // *args, **kwargs, location, call, thread, skylark semantics final int extraArgsCount = 6; List builder = new ArrayList<>(parameters.size() + extraArgsCount); @@ -426,44 +422,20 @@ private static EvalException argumentMismatchException( } } - /** - * Returns the extra interpreter arguments for the given {@link SkylarkCallable}, to be added at - * the end of the argument list for the callable. - * - *

This method accepts null {@code ast} only if {@code callable.useAst()} is false. It is up to - * the caller to validate this invariant. - */ - static List extraInterpreterArgs( - MethodDescriptor method, - @Nullable FuncallExpression ast, - Location loc, - StarlarkThread thread) { - List builder = new ArrayList<>(); - appendExtraInterpreterArgs(builder, method, ast, loc, thread); - return ImmutableList.copyOf(builder); - } - - /** - * Same as {@link #extraInterpreterArgs(MethodDescriptor, FuncallExpression, Location, - * StarlarkThread)} but appends args to a passed {@code builder} to avoid unnecessary allocations - * of intermediate instances. - * - * @see #extraInterpreterArgs(MethodDescriptor, FuncallExpression, Location, StarlarkThread) - */ private static void appendExtraInterpreterArgs( List builder, MethodDescriptor method, - @Nullable FuncallExpression ast, + @Nullable FuncallExpression call, Location loc, StarlarkThread thread) { if (method.isUseLocation()) { builder.add(loc); } if (method.isUseAst()) { - if (ast == null) { + if (call == null) { throw new IllegalArgumentException("Callable expects to receive ast: " + method.getName()); } - builder.add(ast); + builder.add(call); } if (method.isUseStarlarkThread()) { builder.add(thread); @@ -499,31 +471,6 @@ private static String formatMethod(Class objClass, MethodDescriptor methodDes return methodDescriptor.getName() + "(" + Joiner.on(", ").join(argTokens.build()) + ")"; } - static Object call( - StarlarkThread thread, - FuncallExpression call, - Object fn, - ArrayList posargs, - Map kwargs) - throws EvalException, InterruptedException { - - if (fn instanceof StarlarkCallable) { - StarlarkCallable callable = (StarlarkCallable) fn; - return callable.call(posargs, ImmutableMap.copyOf(kwargs), call, thread); - } - - MethodDescriptor selfCall = getSelfCallMethodDescriptor(thread.getSemantics(), fn.getClass()); - if (selfCall != null) { - Object[] javaArguments = - convertStarlarkArgumentsToJavaMethodArguments( - thread, call, selfCall, fn.getClass(), posargs, kwargs); - return selfCall.call(fn, javaArguments, call.getLocation(), thread); - } - - throw new EvalException( - call.getLocation(), "'" + EvalUtils.getDataTypeName(fn) + "' object is not callable"); - } - // A memoization of evalDefault, keyed by expression. // This cache is manually maintained (instead of using LoadingCache), // as default values may sometimes be recursively requested. 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 4800180d79809a..c8d0d50351a1ae 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 @@ -490,7 +490,7 @@ private static Object doEval(StarlarkThread thread, Expression expr) // CallUtils.convertStarlarkArgumentsToJavaMethodArguments---and this avoids constructing // another hash table in nearly every call evalArguments(thread, call, posargs, kwargs); - return CallUtils.call(thread, call, fn, posargs, kwargs); + return Starlark.call(thread, fn, call, posargs, kwargs); } case IDENTIFIER: diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java index 5c905fecc63504..69d6ae3615a797 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java @@ -454,11 +454,7 @@ static Object getAttr(StarlarkThread thread, Location loc, Object x, String name MethodDescriptor method = CallUtils.getMethod(thread.getSemantics(), x.getClass(), name); if (method != null) { if (method.isStructField()) { - return method.call( - x, - CallUtils.extraInterpreterArgs(method, /*ast=*/ null, loc, thread).toArray(), - loc, - thread); + return method.callField(x, loc, thread.getSemantics(), thread.mutability()); } else { return new BuiltinCallable(x, name, method); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java index 2e31842faa6923..3c020cb81cb1cd 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java @@ -22,6 +22,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Arrays; +import javax.annotation.Nullable; /** * A value class to store Methods with their corresponding {@link SkylarkCallable} annotation @@ -112,13 +113,40 @@ static MethodDescriptor of( annotation.useStarlarkSemantics()); } + private static final Object[] EMPTY = {}; + + /** Calls this method, which must have {@code structField=true}. */ + Object callField(Object obj, Location loc, StarlarkSemantics semantics, @Nullable Mutability mu) + throws EvalException, InterruptedException { + if (!structField) { + throw new IllegalStateException("not a struct field: " + name); + } + // Enforced by annotation processor. + if (useAst || useStarlarkThread) { + throw new IllegalStateException( + "SkylarkCallable has structField and useAST/useStarlarkThread: " + name); + } + int nargs = (useLocation ? 1 : 0) + (useStarlarkSemantics ? 1 : 0); + Object[] args = nargs == 0 ? EMPTY : new Object[nargs]; + // TODO(adonovan): used only once, repository_ctx.os. Abolish? + if (useLocation) { + args[0] = loc; + } + // TODO(adonovan): used only once, in getSkylarkLibrariesToLink. Abolish? + if (useStarlarkSemantics) { + args[nargs - 1] = semantics; + } + return call(obj, args, loc, mu); + } + /** - * Invokes this method using {@code obj} as a target and {@code args} as arguments. + * Invokes this method using {@code obj} as a target and {@code args} as Java arguments. + * + *

Methods with {@code void} return type return {@code None} following Python convention. * - *

{@code obj} may be {@code null} in case this method is static. Methods with {@code void} - * return type return {@code None} following Python convention. + *

The Mutability is used if it is necessary to allocate a Starlark copy of a Java result. */ - Object call(Object obj, Object[] args, Location loc, StarlarkThread thread) + Object call(Object obj, Object[] args, Location loc, @Nullable Mutability mu) throws EvalException, InterruptedException { Preconditions.checkNotNull(obj); Object result; @@ -159,8 +187,7 @@ Object call(Object obj, Object[] args, Location loc, StarlarkThread thread) } } - // Careful: thread may be null when we are called by invokeStructField. - return Starlark.fromJava(result, thread != null ? thread.mutability() : null); + return Starlark.fromJava(result, mu); } /** @see SkylarkCallable#name() */ diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java index 3293e8fb04d639..549fbc56d3d8ca 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java @@ -192,7 +192,7 @@ public StarlarkList sorted( } } else if (key instanceof StarlarkCallable) { final StarlarkCallable keyfn = (StarlarkCallable) key; - final FuncallExpression ast = new FuncallExpression(Identifier.of(""), ImmutableList.of()); + final FuncallExpression call = new FuncallExpression(Identifier.of(""), ImmutableList.of()); class KeyComparator implements Comparator { Exception e; @@ -210,7 +210,8 @@ public int compare(Object x, Object y) { } Object callKeyFunc(Object x) throws EvalException, InterruptedException { - return keyfn.call(Collections.singletonList(x), ImmutableMap.of(), ast, thread); + return Starlark.call( + thread, keyfn, call, Collections.singletonList(x), ImmutableMap.of()); } } 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 4abb74e3a521c5..dd3fbe1c76d086 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 @@ -29,7 +29,7 @@ * all clients of the Starlark interpreter. */ // TODO(adonovan): move these here: -// len, str, iterate, equal, compare, getattr, index, slice, parse, exec, eval, and so on. +// equal, compare, getattr, index, slice, parse, exec, eval, and so on. public final class Starlark { private Starlark() {} // uninstantiable @@ -213,6 +213,36 @@ public static String formatWithList(String pattern, List arguments) { return Printer.getPrinter().formatWithList(pattern, arguments).toString(); } + /** + * 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)}. + */ + public static Object call( + StarlarkThread thread, + Object fn, + @Nullable FuncallExpression call, + List args, + Map kwargs) + throws EvalException, InterruptedException { + StarlarkCallable callable; + if (fn instanceof StarlarkCallable) { + callable = (StarlarkCallable) fn; + } else { + // @SkylarkCallable(selfCall)? + MethodDescriptor desc = + CallUtils.getSelfCallMethodDescriptor(thread.getSemantics(), fn.getClass()); + if (desc == null) { + throw new EvalException( + call != null ? call.getLocation() : null, + "'" + EvalUtils.getDataTypeName(fn) + "' object is not callable"); + } + callable = new BuiltinCallable(fn, desc.getName(), desc); + } + + // TODO(adonovan): do push/pop, profiling, and exception handling here. + return callable.callImpl(thread, call, args, ImmutableMap.copyOf(kwargs)); + } + /** * Adds to the environment {@code env} all {@code StarlarkCallable}-annotated fields and methods * of value {@code v}. The class of {@code v} must have or inherit a {@code SkylarkModule} or 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 a8372d15f96084..d2056f94994329 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 @@ -24,28 +24,27 @@ * Starlark like a function, including built-in functions and methods, Starlark functions, and * application-defined objects (such as rules, aspects, and providers in Bazel). */ +// TODO(adonovan): rename to just "Callable", since it's unambiguous. public interface StarlarkCallable extends StarlarkValue { /** - * Call this function with the given arguments. + * Defines the implementation of function calling for a callable value. * - *

Neither the callee nor the caller may modify the args List or kwargs Map. + *

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. * - * @param args the list of positional arguments - * @param kwargs the mapping of named arguments - * @param call the syntax tree of the function call * @param thread the StarlarkThread in which the function is called - * @return the result of the call - * @throws EvalException if there was an error invoking this function + * @param call the function call expression (going away) + * @param args positional arguments + * @param kwargs named arguments */ - // TODO(adonovan): - // - make StarlarkThread the first parameter. - // - eliminate the FuncallExpression parameter (which can be accessed through thread). - Object call( + // TODO(adonovan): optimize the calling convention; see FUNCALL in Eval.java. + Object callImpl( + StarlarkThread thread, + @Nullable FuncallExpression call, // TODO(adonovan): eliminate List args, - @Nullable Map kwargs, - @Nullable FuncallExpression call, - StarlarkThread thread) + Map kwargs) throws EvalException, InterruptedException; /** Returns the form this callable value should take in a stack trace. */ 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 bbb9f339c89a20..2ccaba842a8cab 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 @@ -93,11 +93,11 @@ public Object getValue(String name) throws EvalException { // "native". return new StarlarkCallable() { @Override - public Object call( - List args, - @Nullable Map kwargs, + public Object callImpl( + StarlarkThread thread, @Nullable FuncallExpression call, - StarlarkThread thread) { + List args, + Map kwargs) { return Starlark.NONE; } diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java index 90caa5a902e1e1..ad2f46b78728e2 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java @@ -137,8 +137,12 @@ private static SkylarkInfo instantiateWithA1B2C3(SkylarkProvider provider) throw StarlarkThread thread = StarlarkThread.builder(Mutability.create("test")).useDefaultSemantics().build(); Object result = - provider.call( - ImmutableList.of(), ImmutableMap.of("a", 1, "b", 2, "c", 3), /*ast=*/ null, thread); + Starlark.call( + thread, + provider, + /*call=*/ null, + /*args=*/ ImmutableList.of(), + /*kwargs=*/ ImmutableMap.of("a", 1, "b", 2, "c", 3)); assertThat(result).isInstanceOf(SkylarkInfo.class); return (SkylarkInfo) result; } diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java index 9942e79d4b2062..b878fd2b1e2954 100644 --- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java @@ -84,7 +84,7 @@ public void testStructFieldWithInvalidInfo() throws Exception { .failsToCompile() .withErrorContaining( "@SkylarkCallable-annotated methods with structField=true may not also specify " - + "useAst, extraPositionals, or extraKeywords"); + + "useAst, useStarlarkThread, extraPositionals, or extraKeywords"); } @Test @@ -95,7 +95,7 @@ public void testStructFieldWithExtraArgs() throws Exception { .failsToCompile() .withErrorContaining( "@SkylarkCallable-annotated methods with structField=true may not also specify " - + "useAst, extraPositionals, or extraKeywords"); + + "useAst, useStarlarkThread, extraPositionals, or extraKeywords"); } @Test @@ -106,7 +106,7 @@ public void testStructFieldWithExtraKeywords() throws Exception { .failsToCompile() .withErrorContaining( "@SkylarkCallable-annotated methods with structField=true may not also specify " - + "useAst, extraPositionals, or extraKeywords"); + + "useAst, useStarlarkThread, extraPositionals, or extraKeywords"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java index 7fd64572323205..144602f9963d5c 100644 --- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java +++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java @@ -215,10 +215,8 @@ public Integer selfCallMethod(String one, Integer two) { documented = false, structField = true, useLocation = true, - useStarlarkThread = true, useStarlarkSemantics = true) - public String structFieldMethodWithInfo( - Location location, StarlarkThread thread, StarlarkSemantics starlarkSemantics) { + public String structFieldMethodWithInfo(Location location, StarlarkSemantics starlarkSemantics) { return "dragon"; } 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 f4bd8322eaaa77..8950fa387a812d 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 @@ -141,11 +141,11 @@ public String getName() { } @Override - public Object call( - List args, - Map kwargs, + public Object callImpl( + StarlarkThread thread, FuncallExpression ast, - StarlarkThread thread) { + List args, + Map kwargs) { int sum = 0; for (Object arg : args) { sum += (Integer) arg; @@ -188,11 +188,11 @@ public String getName() { } @Override - public Object call( + public Object callImpl( + StarlarkThread thread, + FuncallExpression call, List args, - final Map kwargs, - FuncallExpression ast, - StarlarkThread thread) { + Map kwargs) { return Dict.copyOf(thread.mutability(), kwargs); } }; 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 fc4e9a7b176eb9..22002839f0dc69 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 @@ -76,11 +76,11 @@ public String getName() { } @Override - public Object call( + public Object callImpl( + StarlarkThread thread, + FuncallExpression call, List args, - Map kwargs, - FuncallExpression ast, - StarlarkThread thread) + Map kwargs) throws EvalException, InterruptedException { params.addAll(args); return Starlark.NONE; 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 9fb50eb832d4d6..10b5472cc851db 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 @@ -2097,11 +2097,11 @@ public String getName() { } @Override - public Object call( - List args, - Map kwargs, + public Object callImpl( + StarlarkThread thread, FuncallExpression call, - StarlarkThread thread) { + List args, + Map kwargs) { return "fromValues"; } };