bazel syntax: delete BuiltinFunction
PackageFactory's BuiltinRuleFunction (rule) and
WorkspaceFactory's newRuleFunction (repository_rule)
both now extend BaseFunction directly, without BuiltinFunction.
(We can further simplify by eliminating BaseFunction in a follow-up.)
Both functions now do their own argument checking, which is trivial:
they merely reject positional arguments.
The push/pop operations on the AllocationTracker Callstack are now
done by StarlarkThread.push/pop, so the stack contains all callables,
not just those that implement BaseFunction.
(The whole thing is redundant and will be eliminated.)
The BaseFunction constructor that accepts default values was
made package-private; only StarlarkFunction needs it.
Also, simplify the exception handling. Starlark.call is the sole place
that augments an EvalException with a location.
PiperOrigin-RevId: 285080959
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
index c909d9e..41a5b8a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
@@ -69,7 +69,7 @@
}
protected NativeProvider(Class<V> valueClass, String name) {
- super(FunctionSignature.KWARGS, /*defaultValues=*/ null);
+ super(FunctionSignature.KWARGS);
this.name = name;
this.key = new NativeKey(name, getClass());
this.valueClass = valueClass;
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 58a1f6e..e5446be 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
@@ -37,7 +37,6 @@
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.syntax.Argument;
import com.google.devtools.build.lib.syntax.BaseFunction;
-import com.google.devtools.build.lib.syntax.BuiltinFunction;
import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.DefStatement;
import com.google.devtools.build.lib.syntax.EvalException;
@@ -581,20 +580,31 @@
}
/**
- * {@link BuiltinFunction} adapter for creating {@link Rule}s for native {@link
+ * {@link BaseFunction} adapter for creating {@link Rule}s for native {@link
* com.google.devtools.build.lib.packages.RuleClass}es.
*/
- private static class BuiltinRuleFunction extends BuiltinFunction implements RuleFunction {
+ private static class BuiltinRuleFunction extends BaseFunction implements RuleFunction {
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);
}
- @SuppressWarnings("unused")
- public NoneType invoke(Map<String, Object> kwargs, Location loc, StarlarkThread thread)
+ @Override
+ public NoneType callImpl(
+ StarlarkThread thread,
+ @Nullable FuncallExpression call,
+ List<Object> args,
+ Map<String, Object> kwargs)
throws EvalException, InterruptedException {
+ if (!args.isEmpty()) {
+ throw new EvalException(null, "unexpected positional arguments");
+ }
+ Location loc = call != null ? call.getLocation() : Location.BUILTIN;
SkylarkUtils.checkLoadingOrWorkspacePhase(thread, ruleClass.getName(), loc);
try {
addRule(getContext(thread, loc), kwargs, loc, thread);
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 f8d6012..baa4cbe 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
@@ -127,7 +127,7 @@
*/
private SkylarkProvider(
@Nullable SkylarkKey key, @Nullable ImmutableList<String> fields, Location location) {
- super(buildSignature(fields), /*defaultValues=*/ null);
+ super(buildSignature(fields));
this.location = location;
this.fields = fields;
this.key = key; // possibly null
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 378623ec..af97687 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
@@ -14,8 +14,6 @@
package com.google.devtools.build.lib.packages;
-import static com.google.devtools.build.lib.syntax.Starlark.NONE;
-
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
@@ -30,10 +28,10 @@
import com.google.devtools.build.lib.packages.Package.NameConflictException;
import com.google.devtools.build.lib.packages.PackageFactory.EnvironmentExtension;
import com.google.devtools.build.lib.syntax.BaseFunction;
-import com.google.devtools.build.lib.syntax.BuiltinFunction;
import com.google.devtools.build.lib.syntax.ClassObject;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.FuncallExpression;
import com.google.devtools.build.lib.syntax.FunctionSignature;
import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Mutability;
@@ -51,6 +49,7 @@
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
@@ -259,16 +258,28 @@
* Returns a function-value implementing the build or workspace rule "ruleClass" (e.g. cc_library)
* in the specified package context.
*/
- private static BuiltinFunction newRuleFunction(
+ private static BaseFunction newRuleFunction(
final RuleFactory ruleFactory, final String ruleClassName, final boolean allowOverride) {
- return new BuiltinFunction(FunctionSignature.KWARGS) {
+ // 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) {
@Override
public String getName() {
return ruleClassName;
}
- public Object invoke(Map<String, Object> kwargs, Location loc, StarlarkThread thread)
+ @Override
+ public Object callImpl(
+ StarlarkThread thread,
+ @Nullable FuncallExpression call,
+ List<Object> args,
+ Map<String, Object> kwargs)
throws EvalException, InterruptedException {
+ if (!args.isEmpty()) {
+ throw new EvalException(null, "unexpected positional arguments");
+ }
+ Location loc = call != null ? call.getLocation() : Location.BUILTIN;
try {
Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder;
String externalRepoName = (String) kwargs.get("name");
@@ -308,7 +319,7 @@
| LabelSyntaxException e) {
throw new EvalException(loc, e.getMessage());
}
- return NONE;
+ return Starlark.NONE;
}
};
}
diff --git a/src/main/java/com/google/devtools/build/lib/profiler/memory/AllocationTracker.java b/src/main/java/com/google/devtools/build/lib/profiler/memory/AllocationTracker.java
index af51d73..d6bbdbf 100644
--- a/src/main/java/com/google/devtools/build/lib/profiler/memory/AllocationTracker.java
+++ b/src/main/java/com/google/devtools/build/lib/profiler/memory/AllocationTracker.java
@@ -25,9 +25,9 @@
import com.google.devtools.build.lib.packages.AspectClass;
import com.google.devtools.build.lib.packages.RuleClass;
import com.google.devtools.build.lib.packages.RuleFunction;
-import com.google.devtools.build.lib.syntax.BaseFunction;
import com.google.devtools.build.lib.syntax.Callstack;
import com.google.devtools.build.lib.syntax.Node;
+import com.google.devtools.build.lib.syntax.StarlarkCallable;
import com.google.monitoring.runtime.instrumentation.Sampler;
import com.google.perftools.profiles.ProfileProto.Function;
import com.google.perftools.profiles.ProfileProto.Line;
@@ -256,8 +256,8 @@
final Location location;
if (object instanceof Node) {
location = ((Node) object).getLocation();
- } else if (object instanceof BaseFunction) {
- location = ((BaseFunction) object).getLocation();
+ } else if (object instanceof StarlarkCallable) {
+ location = ((StarlarkCallable) object).getLocation();
} else {
throw new IllegalStateException(
"Unknown node type: " + object.getClass().getSimpleName());
@@ -269,12 +269,8 @@
file = "<native>";
}
}
- String function = null;
- if (object instanceof BaseFunction) {
- BaseFunction baseFunction = (BaseFunction) object;
- function = baseFunction.getName();
- }
- if (function != null) {
+ if (object instanceof StarlarkCallable) {
+ String function = ((StarlarkCallable) object).getName();
sample.addLocationId(
locationTable.get(Strings.nullToEmpty(file), Strings.nullToEmpty(function), line));
line = -1;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BUILD b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
index 348ab44..770e47a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
@@ -91,7 +91,6 @@
srcs = [
"BaseFunction.java",
"BuiltinCallable.java",
- "BuiltinFunction.java",
"CallUtils.java",
"Callstack.java",
"ClassObject.java",
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 307c485..c354797 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
@@ -26,32 +26,19 @@
import javax.annotation.Nullable;
/**
- * A base class for Skylark functions, whether builtin or user-defined.
- *
- * <p>Nomenclature: We call "Parameters" the formal parameters of a function definition. We call
- * "Arguments" the actual values supplied at the call site.
- *
- * <p>The outer calling convention is like that of python3, with named parameters that can be
- * mandatory or optional, and also be positional or named-only, and rest parameters for extra
- * positional and keyword arguments. Callers supply a {@code List<Object>} args for positional
- * arguments and a {@code Map<String, Object>} for keyword arguments, where positional arguments
- * will be resolved first, then keyword arguments, with errors for a clash between the two, for
- * missing mandatory parameter, or for unexpected extra positional or keyword argument in absence of
- * rest parameter.
- *
- * <p>The inner calling convention is to pass the underlying method an {@code Object[]} of the
- * type-checked argument values, one per expected parameter, parameters being sorted as documented
- * in {@link FunctionSignature}.
- *
- * <p>The function may provide default values for optional parameters not provided by the caller.
- * These default values can be null if there are no optional parameters or for builtin functions,
- * but not for user-defined functions that have optional parameters.
+ * 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.
*/
+// TODO(adonovan): express the processArguments functionality of this class as a standalone function
+// that takes signature and defaultValues as explicit parameters, and 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.
+// Only StarlarkFunction needs getDefaultValues().
public abstract class BaseFunction implements StarlarkCallable {
- // TODO(adonovan): Turn fields into abstract methods. Make processArguments a static function
- // with multiple parameters, instead of a "mix-in" that accesses instance fields.
-
private final FunctionSignature signature;
/**
@@ -77,7 +64,7 @@
}
/** Constructs a BaseFunction with a given signature and default values. */
- protected BaseFunction(FunctionSignature signature, @Nullable List<Object> defaultValues) {
+ BaseFunction(FunctionSignature signature, @Nullable List<Object> defaultValues) {
this.signature = Preconditions.checkNotNull(signature);
this.defaultValues = defaultValues;
if (defaultValues != null) {
@@ -91,23 +78,15 @@
}
/**
- * The size of the array required by the callee.
- */
- protected int getArgArraySize() {
- return signature.numParameters();
- }
-
- /**
* Process the caller-provided arguments into an array suitable for the callee (this function).
*/
- public Object[] processArguments(
+ private Object[] processArguments(
List<Object> args,
@Nullable Map<String, Object> kwargs,
@Nullable Location loc,
@Nullable StarlarkThread thread)
throws EvalException {
-
- Object[] arguments = new Object[getArgArraySize()];
+ Object[] arguments = new Object[signature.numParameters()];
ImmutableList<String> names = signature.getParameterNames();
@@ -276,20 +255,8 @@
throws EvalException, InterruptedException {
// 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);
-
- // 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();
- }
- }
+ return call(arguments, call, thread);
}
/**
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 879ed7e..b8ed052 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
@@ -70,7 +70,7 @@
Object[] javaArguments =
CallUtils.convertStarlarkArgumentsToJavaMethodArguments(
thread, call, desc, objValue.getClass(), args, kwargs);
- return desc.call(objValue, javaArguments, call.getLocation(), thread.mutability());
+ return desc.call(objValue, javaArguments, thread.mutability());
}
private MethodDescriptor getMethodDescriptor(StarlarkSemantics semantics) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
deleted file mode 100644
index 1a5a6f2..0000000
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ /dev/null
@@ -1,220 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.syntax;
-
-import com.google.common.base.Preconditions;
-import com.google.common.base.Throwables;
-import com.google.common.collect.Lists;
-import com.google.devtools.build.lib.events.Location;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
-import java.util.Arrays;
-import java.util.List;
-import java.util.NoSuchElementException;
-import javax.annotation.Nullable;
-
-/**
- * A class for Skylark functions provided as builtins by the Skylark implementation. Instances of
- * this class do not need to be serializable because they should effectively be treated as
- * constants.
- */
-public abstract class BuiltinFunction extends BaseFunction {
-
- // The underlying invoke() method.
- @Nullable private Method invokeMethod;
-
- // Classes of extra arguments required beside signature,
- // computed by configure from parameter types of invoke method.
- // TODO(adonovan): eliminate Location, FuncallExpression when they can be derived from the thread.
- private Class<?>[] extraParams; // ordered subset of {Location,FuncallExpression,StarlarkThread}
-
- // The returnType of the method.
- private Class<?> returnType;
-
- /** Creates a BuiltinFunction with the given signature. */
- protected BuiltinFunction(FunctionSignature signature) {
- super(signature);
- initialize();
- }
-
- @Override
- protected final int getArgArraySize() {
- return invokeMethod.getParameterCount();
- }
-
- @Override
- @Nullable
- public Object call(Object[] args, @Nullable FuncallExpression ast, StarlarkThread thread)
- throws EvalException, InterruptedException {
- Preconditions.checkNotNull(thread);
-
- // ast is null when called from Java (as there's no Skylark call site).
- Location loc = ast == null ? Location.BUILTIN : ast.getLocation();
-
- // Add extra arguments as needed.
- {
- int i = args.length - extraParams.length;
- for (Class<?> cls : extraParams) {
- if (cls == Location.class) {
- args[i] = loc;
- } else if (cls == FuncallExpression.class) {
- args[i] = ast;
- } else if (cls == StarlarkThread.class) {
- args[i] = thread;
- } else {
- throw new IllegalStateException("invalid extra argument: " + cls);
- }
- i++;
- }
- }
-
- // Last but not least, actually make an inner call to the function with the resolved arguments.
- try {
- return invokeMethod.invoke(this, args);
- } catch (InvocationTargetException x) {
- Throwable e = x.getCause();
-
- if (e instanceof EvalException) {
- throw ((EvalException) e).ensureLocation(loc);
- } else if (e instanceof IllegalArgumentException) {
- throw new EvalException(loc, "illegal argument in call to " + getName(), e);
- }
- Throwables.throwIfInstanceOf(e, InterruptedException.class);
- Throwables.throwIfUnchecked(e);
- throw badCallException(loc, e, args);
- } catch (IllegalArgumentException e) {
- // Either this was thrown by Java itself, or it's a bug
- // To cover the first case, let's manually check the arguments.
- final int len = args.length - extraParams.length;
- final Class<?>[] types = invokeMethod.getParameterTypes();
- for (int i = 0; i < args.length; i++) {
- if (args[i] != null && !types[i].isAssignableFrom(args[i].getClass())) {
- String paramName =
- i < len ? getSignature().getParameterNames().get(i) : extraParams[i - len].getName();
- throw new EvalException(
- loc,
- String.format(
- "argument '%s' has type '%s', but should be '%s'\nin call to %s",
- paramName,
- EvalUtils.getDataTypeName(args[i]),
- EvalUtils.getDataTypeNameFromClass(types[i]),
- getShortSignature()));
- }
- }
- throw badCallException(loc, e, args);
- } catch (IllegalAccessException e) {
- throw badCallException(loc, e, args);
- }
- }
-
- private static String stacktraceToString(StackTraceElement[] elts) {
- StringBuilder b = new StringBuilder();
- for (StackTraceElement e : elts) {
- b.append(e);
- b.append("\n");
- }
- return b.toString();
- }
-
- private IllegalStateException badCallException(Location loc, Throwable e, Object... args) {
- // If this happens, it's a bug in our code.
- return new IllegalStateException(
- String.format(
- "%s%s (%s)\n" + "while calling %s with args %s\n" + "Java parameter types: %s",
- (loc == null) ? "" : loc + ": ",
- Arrays.asList(args),
- e.getClass().getName(),
- stacktraceToString(e.getStackTrace()),
- this,
- Arrays.asList(invokeMethod.getParameterTypes())),
- e);
- }
-
- // Configures the reflection mechanism.
- private final void initialize() {
- this.invokeMethod = findMethod(this.getClass(), "invoke");
- Class<?>[] parameterTypes = invokeMethod.getParameterTypes();
- int numParameters = getSignature().numParameters();
- this.extraParams = extraParams(numParameters, parameterTypes);
-
- if (returnType != null) {
- Class<?> type = returnType;
- Class<?> methodReturnType = invokeMethod.getReturnType();
- Preconditions.checkArgument(
- type == methodReturnType,
- "signature for function %s says it returns %s but its invoke method returns %s",
- getName(),
- returnType,
- methodReturnType);
- }
- }
-
- /**
- * Returns the signature as "[className.]methodName(name1: paramType1, name2: paramType2, ...)"
- */
- private String getShortSignature() {
- StringBuilder builder = new StringBuilder();
- builder.append(getName()).append("(");
- getSignature().toStringBuilder(builder, /*defaultValuePrinter=*/ null);
- builder.append(")");
- return builder.toString();
- }
-
- // Returns the list of extra parameters beyond those in the signature.
- private Class<?>[] extraParams(int i, Class<?>[] parameterTypes) {
- List<Class<?>> extra = Lists.newArrayList();
- for (Class<?> cls : EXTRA_PARAM_CLASSES) {
- if (i < parameterTypes.length && parameterTypes[i] == cls) {
- extra.add(cls);
- i++;
- }
- }
- if (i != parameterTypes.length) {
- throw new IllegalStateException(
- String.format(
- "bad argument count for %s: method has %s arguments, type list has %s",
- getName(), i, parameterTypes.length));
- }
- return extra.toArray(new Class<?>[0]);
- }
-
- private static final Class<?>[] EXTRA_PARAM_CLASSES = {
- Location.class, FuncallExpression.class, StarlarkThread.class
- };
-
- // finds the method and makes it accessible (which is needed to find it, and later to use it)
- private static Method findMethod(Class<?> cls, String name) {
- Method found = null;
- for (Method method : cls.getDeclaredMethods()) {
- method.setAccessible(true);
- if (name.equals(method.getName())) {
- if (found != null) {
- throw new IllegalArgumentException(
- String.format("class %s has more than one method named %s", cls.getName(), name));
- }
- found = method;
- }
- }
- if (found == null) {
- throw new NoSuchElementException(
- String.format("class %s doesn't have a method named %s", cls.getName(), name));
- }
- return found;
- }
-
- @Override
- public void repr(Printer printer) {
- printer.append("<built-in function " + getName() + ">");
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java b/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java
index a314ab7..3c92f8b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Callstack.java
@@ -20,7 +20,7 @@
import java.util.List;
/**
- * Holds the Skylark callstack in thread-local storage. Contains all Expressions and BaseFunctions
+ * Holds the Skylark callstack in thread-local storage. Contains all Expressions and functions
* currently being evaluated.
*
* <p>This is needed for memory tracking, since the evaluator is not available in the context of
@@ -41,8 +41,8 @@
callstack.get().add(node);
}
- public static void push(BaseFunction function) {
- callstack.get().add(function);
+ public static void push(StarlarkCallable fn) {
+ callstack.get().add(fn);
}
public static void pop() {
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 3c020cb8..e17d302 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
@@ -136,7 +136,7 @@
if (useStarlarkSemantics) {
args[nargs - 1] = semantics;
}
- return call(obj, args, loc, mu);
+ return call(obj, args, mu);
}
/**
@@ -146,31 +146,31 @@
*
* <p>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, @Nullable Mutability mu)
+ Object call(Object obj, Object[] args, @Nullable Mutability mu)
throws EvalException, InterruptedException {
Preconditions.checkNotNull(obj);
Object result;
try {
result = method.invoke(obj, args);
- } catch (IllegalAccessException e) {
- // TODO(bazel-team): Print a nice error message. Maybe the method exists
- // and an argument is missing or has the wrong type.
- throw new EvalException(loc, "Method invocation failed: " + e);
- } catch (InvocationTargetException x) {
- Throwable e = x.getCause();
+ } catch (IllegalAccessException ex) {
+ // The annotated processor ensures that annotated methods are accessible.
+ throw new IllegalStateException(ex);
+
+ } catch (InvocationTargetException ex) {
+ Throwable e = ex.getCause();
if (e == null) {
- // This is unlikely to happen.
- throw new IllegalStateException(
- String.format(
- "causeless InvocationTargetException when calling %s with arguments %s at %s",
- obj, Arrays.toString(args), loc),
- x);
+ throw new IllegalStateException(e);
}
- Throwables.propagateIfPossible(e, InterruptedException.class);
+ // Don't intercept unchecked exceptions.
+ Throwables.throwIfUnchecked(e);
if (e instanceof EvalException) {
- throw ((EvalException) e).ensureLocation(loc);
+ throw (EvalException) e;
+ } else if (e instanceof InterruptedException) {
+ throw (InterruptedException) e;
+ } else {
+ // All other checked exceptions (e.g. LabelSyntaxException) are reported to Starlark.
+ throw new EvalException(null, null, e);
}
- throw new EvalException(loc, null, e);
}
if (method.getReturnType().equals(Void.TYPE)) {
return Starlark.NONE;
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 2f89636..00638b5 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
@@ -225,6 +225,8 @@
List<Object> args,
Map<String, Object> kwargs)
throws EvalException, InterruptedException {
+ Location loc = call != null ? call.getLocation() : null;
+
StarlarkCallable callable;
if (fn instanceof StarlarkCallable) {
callable = (StarlarkCallable) fn;
@@ -234,17 +236,16 @@
CallUtils.getSelfCallMethodDescriptor(thread.getSemantics(), fn.getClass());
if (desc == null) {
throw new EvalException(
- call != null ? call.getLocation() : null,
- "'" + EvalUtils.getDataTypeName(fn) + "' object is not callable");
+ loc, "'" + EvalUtils.getDataTypeName(fn) + "' object is not callable");
}
callable = new BuiltinCallable(fn, desc.getName(), desc);
}
- Location loc = call != null ? call.getLocation() : null;
thread.push(callable, loc, call);
try {
- // TODO(adonovan): unify exception handling here.
return callable.callImpl(thread, call, args, ImmutableMap.copyOf(kwargs));
+ } catch (EvalException ex) {
+ throw ex.ensureLocation(loc);
} finally {
thread.pop();
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
index 45a3168..04eefa9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
@@ -517,6 +517,12 @@
CallFrame fr = new CallFrame(fn, loc, call, this.lexicalFrame, this.globalFrame);
callstack.add(fr);
+ // Push the function onto the allocation tracker's stack.
+ // TODO(adonovan): optimize it out of existence.
+ if (Callstack.enabled) {
+ Callstack.push(fn);
+ }
+
ProfilerTask taskKind;
if (fn instanceof StarlarkFunction) {
StarlarkFunction sfn = (StarlarkFunction) fn;
@@ -554,6 +560,10 @@
if (top.profileSpan != null) {
top.profileSpan.close();
}
+
+ if (Callstack.enabled) {
+ Callstack.pop();
+ }
}
// Builtins cannot create or modify variable bindings,
@@ -1009,7 +1019,4 @@
public String getTransitiveContentHashCode() {
return transitiveHashCode;
}
-
- // legacy for copybara; to be inlined and deleted in Nov 2019.
- public static final Module SKYLARK = Module.createForBuiltins(Starlark.UNIVERSE);
}
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 c03a3a4..b698fba 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
@@ -62,7 +62,7 @@
private final Location location;
TestFunction(String file, String name, int line) {
- super(FunctionSignature.ANY, /*defaultValues=*/ null);
+ super(FunctionSignature.ANY);
this.name = name;
this.location = location(file, line);
}
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 a8cdca1..a258ec3 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
@@ -170,7 +170,7 @@
}
@SkylarkCallable(name = "interrupted_struct_field", documented = false, structField = true)
- public BuiltinFunction structFieldInterruptedCallable() throws InterruptedException {
+ public Object structFieldInterruptedCallable() throws InterruptedException {
throw new InterruptedException();
}
