bazel syntax: delete ExtraArgKind
(Second attempt at commit 50a7e14e3df5513572f1ac3275786f243b51c930, rolled back in commit 8ce9d84aa054cfd126da16190f5ad5dcb268a827.
The first attempt was sound but was collateral damage in another rollback.)
The extra arguments are now computed from the invoke
method's parameter types. We use Class instead of the enum.
Delete SkylarkSignatureProcessor.getExtraArgs, no longer needed.
The use{Location,Ast,StarlarkThread} annotation flags are redundant,
though the build-time processor ensures they are consistent.
Also:
- rename overloaded BaseFunction.configure(SkylarkSignature)
to configureFromAnnotation, and move it down into BuiltinFunction.
- replace FuncallExpression with Location in various places.
- NativeProvider: simplify ctor
RELNOTES: N/A
PiperOrigin-RevId: 277563210
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index 96396b4..a6e3cef 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -187,7 +187,7 @@
ruleClass,
null,
WorkspaceFactoryHelper.getFinalKwargs(attributeValues),
- ast,
+ ast.getLocation(),
callStack.toString());
return rule;
} catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
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 e2ca4ab..a17fa4e 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
@@ -65,17 +65,12 @@
String getSkylarkName();
}
- protected NativeProvider(Class<V> clazz, String name) {
- this(clazz, name, FunctionSignature.KWARGS);
- }
-
- @SuppressWarnings("unchecked")
- private NativeProvider(Class<V> valueClass, String name, FunctionSignature signature) {
- super(name, signature, Location.BUILTIN);
- Class<? extends NativeProvider<?>> clazz = (Class<? extends NativeProvider<?>>) getClass();
- key = new NativeKey(name, clazz);
+ protected NativeProvider(Class<V> valueClass, String name) {
+ super(name, FunctionSignature.KWARGS, Location.BUILTIN);
+ this.key = new NativeKey(name, getClass());
this.valueClass = valueClass;
- errorMessageFormatForUnknownField = String.format("'%s' object has no attribute '%%s'", name);
+ this.errorMessageFormatForUnknownField =
+ String.format("'%s' object has no attribute '%%s'", name);
}
/**
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 89d4f1e..1e72875 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
@@ -1289,7 +1289,7 @@
private final RuleClass ruleClass;
BuiltinRuleFunction(String ruleClassName, RuleFactory ruleFactory) {
- super(ruleClassName, FunctionSignature.KWARGS, BuiltinFunction.USE_LOC_ENV);
+ super(ruleClassName, FunctionSignature.KWARGS);
this.ruleClassName = ruleClassName;
Preconditions.checkNotNull(ruleFactory, "ruleFactory was null");
this.ruleClass = Preconditions.checkNotNull(
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 0793459..d9ee924 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
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.events.NullEventHandler;
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
@@ -34,7 +35,6 @@
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;
@@ -262,18 +262,17 @@
*/
private static BuiltinFunction newRuleFunction(
final RuleFactory ruleFactory, final String ruleClassName, final boolean allowOverride) {
- return new BuiltinFunction(
- ruleClassName, FunctionSignature.KWARGS, BuiltinFunction.USE_AST_ENV) {
- public Object invoke(Map<String, Object> kwargs, FuncallExpression ast, StarlarkThread thread)
+ return new BuiltinFunction(ruleClassName, FunctionSignature.KWARGS) {
+ public Object invoke(Map<String, Object> kwargs, Location loc, StarlarkThread thread)
throws EvalException, InterruptedException {
try {
- Package.Builder builder = PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder;
+ Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder;
String externalRepoName = (String) kwargs.get("name");
if (!allowOverride
&& externalRepoName != null
&& builder.getTarget(externalRepoName) != null) {
throw new EvalException(
- ast.getLocation(),
+ loc,
"Cannot redefine repository after any load statement in the WORKSPACE file"
+ " (for repository '"
+ kwargs.get("name")
@@ -283,8 +282,7 @@
// @<mainRepoName> as a separate repository. This will be overridden if the main
// repository has a repo_mapping entry from <mainRepoName> to something.
WorkspaceFactoryHelper.addMainRepoEntry(builder, externalRepoName, thread.getSemantics());
- WorkspaceFactoryHelper.addRepoMappings(
- builder, kwargs, externalRepoName, ast.getLocation());
+ WorkspaceFactoryHelper.addRepoMappings(builder, kwargs, externalRepoName, loc);
RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName);
RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
Rule rule =
@@ -293,10 +291,10 @@
ruleClass,
bindRuleClass,
WorkspaceFactoryHelper.getFinalKwargs(kwargs),
- ast);
+ loc);
if (!WorkspaceGlobals.isLegalWorkspaceName(rule.getName())) {
throw new EvalException(
- ast.getLocation(),
+ loc,
rule
+ "'s name field must be a legal workspace name;"
+ " workspace names may contain only A-Z, a-z, 0-9, '-', '_' and '.'");
@@ -304,7 +302,7 @@
} catch (RuleFactory.InvalidRuleException
| Package.NameConflictException
| LabelSyntaxException e) {
- throw new EvalException(ast.getLocation(), e.getMessage());
+ throw new EvalException(loc, e.getMessage());
}
return NONE;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
index a7a37c3..e519196 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java
@@ -26,7 +26,6 @@
import com.google.devtools.build.lib.events.StoredEventHandler;
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import java.util.Map;
@@ -40,10 +39,10 @@
RuleClass ruleClass,
RuleClass bindRuleClass,
Map<String, Object> kwargs,
- FuncallExpression ast)
+ Location loc)
throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException,
InterruptedException {
- return createAndAddRepositoryRule(pkg, ruleClass, bindRuleClass, kwargs, ast, null);
+ return createAndAddRepositoryRule(pkg, ruleClass, bindRuleClass, kwargs, loc, null);
}
public static Rule createAndAddRepositoryRule(
@@ -51,7 +50,7 @@
RuleClass ruleClass,
RuleClass bindRuleClass,
Map<String, Object> kwargs,
- FuncallExpression ast, // just for getLocation(); TODO(adonovan): simplify
+ Location loc,
String definitionInfo)
throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException,
InterruptedException {
@@ -64,7 +63,7 @@
ruleClass,
attributeValues,
eventHandler,
- ast.getLocation(),
+ loc,
/* thread= */ null,
new AttributeContainer(ruleClass));
pkg.addEvents(eventHandler.getEvents());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
index bacaf5f..ab44235 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
@@ -30,7 +30,6 @@
import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
import com.google.devtools.build.lib.skylarkbuildapi.WorkspaceGlobalsApi;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
import com.google.devtools.build.lib.syntax.Runtime.NoneType;
import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -65,19 +64,19 @@
public NoneType workspace(
String name,
SkylarkDict<?, ?> managedDirectories, // <String, Object>
- FuncallExpression ast,
+ Location loc,
StarlarkThread thread)
throws EvalException, InterruptedException {
if (allowOverride) {
if (!isLegalWorkspaceName(name)) {
- throw new EvalException(ast.getLocation(), name + " is not a legal workspace name");
+ throw new EvalException(loc, name + " is not a legal workspace name");
}
String errorMessage = LabelValidator.validateTargetName(name);
if (errorMessage != null) {
- throw new EvalException(ast.getLocation(), errorMessage);
+ throw new EvalException(loc, errorMessage);
}
- PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder.setWorkspaceName(name);
- Package.Builder builder = PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder;
+ PackageFactory.getContext(thread, loc).pkgBuilder.setWorkspaceName(name);
+ Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder;
RuleClass localRepositoryRuleClass = ruleFactory.getRuleClass("local_repository");
RuleClass bindRuleClass = ruleFactory.getRuleClass("bind");
Map<String, Object> kwargs = ImmutableMap.<String, Object>of("name", name, "path", ".");
@@ -85,9 +84,9 @@
// This effectively adds a "local_repository(name = "<ws>", path = ".")"
// definition to the WORKSPACE file.
WorkspaceFactoryHelper.createAndAddRepositoryRule(
- builder, localRepositoryRuleClass, bindRuleClass, kwargs, ast);
+ builder, localRepositoryRuleClass, bindRuleClass, kwargs, loc);
} catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) {
- throw new EvalException(ast.getLocation(), e.getMessage());
+ throw new EvalException(loc, e.getMessage());
}
// Add entry in repository map from "@name" --> "@" to avoid issue where bazel
// treats references to @name as a separate external repo
@@ -98,29 +97,28 @@
RepositoryName.MAIN);
}
parseManagedDirectories(
- managedDirectories.getContents(String.class, Object.class, "managed_directories"), ast);
+ managedDirectories.getContents(String.class, Object.class, "managed_directories"), loc);
return NONE;
} else {
throw new EvalException(
- ast.getLocation(),
- "workspace() function should be used only at the top of the WORKSPACE file");
+ loc, "workspace() function should be used only at the top of the WORKSPACE file");
}
}
private void parseManagedDirectories(
Map<String, ?> managedDirectories, // <String, SkylarkList<String>>
- FuncallExpression ast)
+ Location loc)
throws EvalException {
Map<PathFragment, String> nonNormalizedPathsMap = Maps.newHashMap();
for (Map.Entry<String, ?> entry : managedDirectories.entrySet()) {
- RepositoryName repositoryName = createRepositoryName(entry.getKey(), ast.getLocation());
+ RepositoryName repositoryName = createRepositoryName(entry.getKey(), loc);
List<PathFragment> paths =
- getManagedDirectoriesPaths(entry.getValue(), ast.getLocation(), nonNormalizedPathsMap);
+ getManagedDirectoriesPaths(entry.getValue(), loc, nonNormalizedPathsMap);
for (PathFragment dir : paths) {
PathFragment floorKey = managedDirectoriesMap.floorKey(dir);
if (dir.equals(floorKey)) {
throw new EvalException(
- ast.getLocation(),
+ loc,
String.format(
"managed_directories attribute should not contain multiple"
+ " (or duplicate) repository mappings for the same directory ('%s').",
@@ -130,7 +128,7 @@
boolean isDescendant = floorKey != null && dir.startsWith(floorKey);
if (isDescendant || (ceilingKey != null && ceilingKey.startsWith(dir))) {
throw new EvalException(
- ast.getLocation(),
+ loc,
String.format(
"managed_directories attribute value can not contain nested mappings."
+ " '%s' is a descendant of '%s'.",
@@ -244,29 +242,29 @@
}
@Override
- public NoneType bind(String name, Object actual, FuncallExpression ast, StarlarkThread thread)
+ public NoneType bind(String name, Object actual, Location loc, StarlarkThread thread)
throws EvalException, InterruptedException {
Label nameLabel;
try {
nameLabel = Label.parseAbsolute("//external:" + name, ImmutableMap.of());
try {
- Package.Builder builder = PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder;
+ Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder;
RuleClass ruleClass = ruleFactory.getRuleClass("bind");
WorkspaceFactoryHelper.addBindRule(
builder,
ruleClass,
nameLabel,
actual == NONE ? null : Label.parseAbsolute((String) actual, ImmutableMap.of()),
- ast.getLocation(),
+ loc,
new AttributeContainer(ruleClass));
} catch (RuleFactory.InvalidRuleException
| Package.NameConflictException
| LabelSyntaxException e) {
- throw new EvalException(ast.getLocation(), e.getMessage());
+ throw new EvalException(loc, e.getMessage());
}
} catch (LabelSyntaxException e) {
- throw new EvalException(ast.getLocation(), e.getMessage());
+ throw new EvalException(loc, e.getMessage());
}
return NONE;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java
index 5a00bde..50cb6b8 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/WorkspaceGlobalsApi.java
@@ -20,7 +20,6 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkGlobalLibrary;
import com.google.devtools.build.lib.syntax.EvalException;
-import com.google.devtools.build.lib.syntax.FuncallExpression;
import com.google.devtools.build.lib.syntax.Runtime.NoneType;
import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.syntax.SkylarkList;
@@ -78,12 +77,12 @@
+ "\nManaged directories must be excluded from the source tree by listing"
+ " them (or their parent directories) in the .bazelignore file."),
},
- useAst = true,
+ useLocation = true,
useStarlarkThread = true)
NoneType workspace(
String name,
SkylarkDict<?, ?> managedDirectories, // <String, SkylarkList<String>>
- FuncallExpression ast,
+ Location loc,
StarlarkThread thread)
throws EvalException, InterruptedException;
@@ -145,8 +144,8 @@
defaultValue = "None",
doc = "The real label to be aliased")
},
- useAst = true,
+ useLocation = true,
useStarlarkThread = true)
- NoneType bind(String name, Object actual, FuncallExpression ast, StarlarkThread thread)
+ NoneType bind(String name, Object actual, Location loc, StarlarkThread thread)
throws EvalException, InterruptedException;
}
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 6f139ae..0346d45 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
@@ -21,7 +21,6 @@
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
import java.util.ArrayList;
import java.util.List;
@@ -150,11 +149,11 @@
*/
protected BaseFunction(
@Nullable String name,
- @Nullable FunctionSignature signature,
+ FunctionSignature signature,
@Nullable List<Object> defaultValues,
@Nullable Location location) {
this(name);
- this.signature = signature;
+ this.signature = Preconditions.checkNotNull(signature);
this.defaultValues = defaultValues;
this.location = location;
@@ -365,11 +364,7 @@
}
/** check types and convert as required */
- protected void canonicalizeArguments(Object[] arguments, Location loc) throws EvalException {
- // TODO(bazel-team): maybe link syntax.SkylarkType and package.Type,
- // so we can simultaneously typecheck and convert?
- // Note that a BuiltinFunction already does typechecking of simple types.
-
+ private void canonicalizeArguments(Object[] arguments, Location loc) throws EvalException {
List<SkylarkType> types = getEnforcedArgumentTypes();
// Check types, if supplied
@@ -439,6 +434,7 @@
* @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 {
@@ -483,31 +479,16 @@
return t != null ? t.toString() : null;
}
- /** Configure a BaseFunction from a @SkylarkSignature annotation */
- // TODO(adonovan): this does not belong here. Move down into BuiltinFunction.
- public void configure(SkylarkSignature annotation) {
- Preconditions.checkState(!isConfigured()); // must not be configured yet
-
- // side effect: appends to getEnforcedArgumentTypes()
- SkylarkSignatureProcessor.SignatureInfo info =
- SkylarkSignatureProcessor.getSignatureForCallable(
- getName(), annotation, /*paramDoc=*/ new ArrayList<>(), getEnforcedArgumentTypes());
- this.signature = info.signature;
- this.paramTypes = info.types;
- this.defaultValues = info.defaultValues;
-
- this.objectType = annotation.objectType().equals(Object.class)
- ? null : annotation.objectType();
- configure();
- }
-
/** Configure a function based on its signature */
// This function is called after the signature is initialized.
- protected void configure() {
+ void configure() {
Preconditions.checkState(signature != null);
- // TODO(adonovan): this looks fishy. It clobbers this.enforcedArgumentTypes, populated as a
- // side-effect of the getSignatureForCallable call in configure.
+ // BuiltinFunction overrides this method without calling this
+ // implementation, so this statement does not clobber the
+ // enforcedArgumentTypes computed by getSignatureForCallable.
+ // Still it is hard to explain what the configure method does.
+ // TODO(adonovan): eliminate SkylarkSignature then simplify.
this.enforcedArgumentTypes = this.paramTypes;
}
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
index 652bc24..39de391 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -15,6 +15,7 @@
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 com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
@@ -38,18 +39,6 @@
*/
public class BuiltinFunction extends BaseFunction {
- /** ExtraArgKind so you can tweek your function's own calling convention */
- public enum ExtraArgKind {
- LOCATION,
- SYNTAX_TREE,
- ENVIRONMENT;
- }
- // Predefined system add-ons to function signatures
- public static final ExtraArgKind[] USE_LOC_ENV =
- new ExtraArgKind[] {ExtraArgKind.LOCATION, ExtraArgKind.ENVIRONMENT};
- public static final ExtraArgKind[] USE_AST_ENV =
- new ExtraArgKind[] {ExtraArgKind.SYNTAX_TREE, ExtraArgKind.ENVIRONMENT};
-
// Builtins cannot create or modify variable bindings. So it's sufficient to use a shared
// instance.
private static final LexicalFrame SHARED_LEXICAL_FRAME_FOR_BUILTIN_FUNCTION_CALLS =
@@ -58,37 +47,28 @@
// The underlying invoke() method.
@Nullable private Method invokeMethod;
- // extra arguments required beside signature.
- @Nullable private ExtraArgKind[] extraArgs;
-
- // The count of arguments in the inner invoke method,
- // to be used as size of argument array by the outer call method.
- private int innerArgumentCount;
+ // 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;
- /** Create unconfigured (signature-less) function from its name */
+ /** Create unconfigured (signature-less) function from its name. */
protected BuiltinFunction(String name) {
super(name);
}
- /** Creates a BuiltinFunction with the given name and signature */
+ /** Creates a BuiltinFunction with the given name and signature. */
protected BuiltinFunction(String name, FunctionSignature signature) {
super(name, signature);
configure();
}
- /** Creates a BuiltinFunction with the given name and signature and extra arguments */
- protected BuiltinFunction(String name, FunctionSignature signature, ExtraArgKind[] extraArgs) {
- super(name, signature);
- this.extraArgs = extraArgs;
- configure();
- }
-
@Override
- protected int getArgArraySize() {
- return innerArgumentCount;
+ protected final int getArgArraySize() {
+ return invokeMethod.getParameterCount();
}
@Override
@@ -100,22 +80,18 @@
// 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, if needed
- if (extraArgs != null) {
- int i = args.length - extraArgs.length;
- for (BuiltinFunction.ExtraArgKind extraArg : extraArgs) {
- switch(extraArg) {
- case LOCATION:
- args[i] = loc;
- break;
-
- case SYNTAX_TREE:
- args[i] = ast;
- break;
-
- case ENVIRONMENT:
- args[i] = thread;
- break;
+ // 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++;
}
@@ -141,12 +117,12 @@
} 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 - ((extraArgs == null) ? 0 : extraArgs.length);
+ 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 ? signature.getParameterNames().get(i) : extraArgs[i - len].name();
+ i < len ? signature.getParameterNames().get(i) : extraParams[i - len].getName();
throw new EvalException(
loc,
String.format(
@@ -192,34 +168,41 @@
e);
}
- /** Configure the reflection mechanism */
- @Override
- public void configure(SkylarkSignature annotation) {
+ /**
+ * Configure the reflection mechanism. Called by signature processor for BuiltinFunctions already
+ * created.
+ */
+ final void configureFromAnnotation(SkylarkSignature annotation) {
Preconditions.checkState(!isConfigured()); // must not be configured yet
- enforcedArgumentTypes = new ArrayList<>();
- this.extraArgs = SkylarkSignatureProcessor.getExtraArgs(annotation);
+ this.enforcedArgumentTypes = new ArrayList<>();
+
this.returnType = annotation.returnType();
- super.configure(annotation);
+
+ // Appends to getEnforcedArgumentTypes() and paramDoc as a side effect.
+ SkylarkSignatureProcessor.SignatureInfo info =
+ SkylarkSignatureProcessor.getSignatureForCallable(
+ getName(), annotation, /*paramDoc=*/ new ArrayList<>(), this.enforcedArgumentTypes);
+ this.signature = info.signature;
+ this.paramTypes = info.types;
+ this.defaultValues = info.defaultValues;
+
+ this.objectType = annotation.objectType() == Object.class ? null : annotation.objectType();
+ configure();
}
- /** Configure the reflection mechanism */
+ /**
+ * Configure the reflection mechanism. Called directly by constructor for BuiltinFunctions created
+ * with a signature, and called after annotation processing for other BuiltinFunctions.
+ */
@Override
- protected void configure() {
- invokeMethod = findMethod("invoke");
-
- int arguments = signature.numParameters();
- innerArgumentCount = arguments + (extraArgs == null ? 0 : extraArgs.length);
+ final void configure() {
+ this.invokeMethod = findMethod(this.getClass(), "invoke");
Class<?>[] parameterTypes = invokeMethod.getParameterTypes();
- if (innerArgumentCount != parameterTypes.length) {
- // Guard message construction by check to avoid autoboxing two integers.
- throw new IllegalStateException(
- String.format(
- "bad argument count for %s: method has %s arguments, type list has %s",
- getName(), innerArgumentCount, parameterTypes.length));
- }
+ int numParameters = signature.numParameters();
+ this.extraParams = extraParams(numParameters, parameterTypes);
if (enforcedArgumentTypes != null) {
- for (int i = 0; i < arguments; i++) {
+ for (int i = 0; i < numParameters; i++) {
SkylarkType enforcedType = enforcedArgumentTypes.get(i);
if (enforcedType != null) {
Class<?> parameterType = parameterTypes[i];
@@ -263,6 +246,28 @@
}
}
+ // 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
+ };
+
/** Returns list, or null if all its elements are null. */
@Nullable
private static <E> List<E> valueListOrNull(List<E> list) {
@@ -277,21 +282,21 @@
}
// finds the method and makes it accessible (which is needed to find it, and later to use it)
- Method findMethod(final String name) {
+ private static Method findMethod(Class<?> cls, String name) {
Method found = null;
- for (Method method : this.getClass().getDeclaredMethods()) {
+ for (Method method : cls.getDeclaredMethods()) {
method.setAccessible(true);
if (name.equals(method.getName())) {
if (found != null) {
throw new IllegalArgumentException(
- String.format("function %s has more than one method named %s", getName(), name));
+ String.format("class %s has more than one method named %s", cls.getName(), name));
}
found = method;
}
}
if (found == null) {
throw new NoSuchElementException(
- String.format("function %s doesn't have a method named %s", getName(), name));
+ String.format("class %s doesn't have a method named %s", cls.getName(), name));
}
return found;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
index db45a0b..01bc0bc 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSignatureProcessor.java
@@ -15,11 +15,9 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
-import com.google.common.primitives.Booleans;
import com.google.devtools.build.lib.skylarkinterface.Param;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
-import com.google.devtools.build.lib.syntax.BuiltinFunction.ExtraArgKind;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.HashMap;
@@ -345,43 +343,21 @@
}
}
- /** Extract additional signature information for BuiltinFunction-s */
- static ExtraArgKind[] getExtraArgs(SkylarkSignature annotation) {
- final int numExtraArgs =
- Booleans.countTrue(
- annotation.useLocation(), annotation.useAst(), annotation.useStarlarkThread());
- if (numExtraArgs == 0) {
- return null;
- }
- final ExtraArgKind[] extraArgs = new ExtraArgKind[numExtraArgs];
- int i = 0;
- if (annotation.useLocation()) {
- extraArgs[i++] = ExtraArgKind.LOCATION;
- }
- if (annotation.useAst()) {
- extraArgs[i++] = ExtraArgKind.SYNTAX_TREE;
- }
- if (annotation.useStarlarkThread()) {
- extraArgs[i++] = ExtraArgKind.ENVIRONMENT;
- }
- return extraArgs;
- }
-
/**
* Processes all {@link SkylarkSignature}-annotated fields in a class.
*
* <p>This includes registering these fields as builtins using {@link Runtime}, and for {@link
- * BaseFunction} instances, calling {@link BaseFunction#configure(SkylarkSignature)}. The fields
- * will be picked up by reflection even if they are not public.
+ * BuiltinFunction} instances, calling {@link BuiltinFunction#configure(SkylarkSignature)}. The
+ * fields will be picked up by reflection even if they are not public.
*
* <p>This function should be called once per class, before the builtins registry is frozen. In
* practice, this is usually called from the class's own static initializer block. E.g., a class
- * {@code Foo} containing {@code @SkylarkSignature} annotations would end with
- * {@code static { SkylarkSignatureProcessor.configureSkylarkFunctions(Foo.class); }}.
+ * {@code Foo} containing {@code @SkylarkSignature} annotations would end with {@code static {
+ * SkylarkSignatureProcessor.configureSkylarkFunctions(Foo.class); }}.
*
* <p><b>If you see exceptions from {@link Runtime.BuiltinRegistry} here:</b> Be sure the class's
- * static initializer has in fact been called before the registry was frozen. In Bazel, see
- * {@link com.google.devtools.build.lib.runtime.BlazeRuntime#initSkylarkBuiltinsRegistry}.
+ * static initializer has in fact been called before the registry was frozen. In Bazel, see {@link
+ * com.google.devtools.build.lib.runtime.BlazeRuntime#initSkylarkBuiltinsRegistry}.
*/
public static void configureSkylarkFunctions(Class<?> type) {
Runtime.BuiltinRegistry builtins = Runtime.getBuiltinRegistry();
@@ -399,10 +375,10 @@
type,
field);
builtins.registerBuiltin(type, field.getName(), value);
- if (BaseFunction.class.isAssignableFrom(field.getType())) {
- BaseFunction function = (BaseFunction) value;
+ if (BuiltinFunction.class.isAssignableFrom(field.getType())) {
+ BuiltinFunction function = (BuiltinFunction) value;
if (!function.isConfigured()) {
- function.configure(annotation);
+ function.configureFromAnnotation(annotation);
}
Class<?> nameSpace = function.getObjectType();
if (nameSpace != null) {
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
index f861ad1..e720a6c 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContextTest.java
@@ -103,7 +103,7 @@
FuncallExpression ast = (FuncallExpression) Expression.parse(input);
Rule rule =
WorkspaceFactoryHelper.createAndAddRepositoryRule(
- packageBuilder, buildRuleClass(attributes), null, kwargs, ast);
+ packageBuilder, buildRuleClass(attributes), null, kwargs, ast.getLocation());
HttpDownloader downloader = Mockito.mock(HttpDownloader.class);
SkyFunction.Environment environment = Mockito.mock(SkyFunction.Environment.class);
Mockito.when(environment.getListener()).thenReturn(listener);
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 4b76420..da06293 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
@@ -56,8 +56,9 @@
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
import com.google.devtools.build.lib.skylarkinterface.Param;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
-import com.google.devtools.build.lib.syntax.BuiltinFunction;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkGlobalLibrary;
+import com.google.devtools.build.lib.syntax.CallUtils;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.Printer;
@@ -85,15 +86,18 @@
/** Tests for skylark functions relating to rule implemenetation. */
@RunWith(JUnit4.class)
+@SkylarkGlobalLibrary // needed for CallUtils.getBuiltinCallable, sadly
public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
+
@Rule public ExpectedException thrown = ExpectedException.none();
- @SkylarkSignature(
+ // def mock(mandatory, optional=None, *, mandatory_key, optional_key='x')
+ @SkylarkCallable(
name = "mock",
documented = false,
parameters = {
- @Param(name = "mandatory", doc = ""),
- @Param(name = "optional", doc = "", defaultValue = "None"),
+ @Param(name = "mandatory", doc = "", named = true),
+ @Param(name = "optional", doc = "", defaultValue = "None", noneable = true, named = true),
@Param(name = "mandatory_key", doc = "", positional = false, named = true),
@Param(
name = "optional_key",
@@ -103,14 +107,23 @@
named = true)
},
useStarlarkThread = true)
- private BuiltinFunction mockFunc;
-
- /**
- * Used for {@link #testStackTraceWithoutOriginalMessage()} and {@link
- * #testNoStackTraceOnInterrupt}.
- */
- @SkylarkSignature(name = "throw", documented = false)
- BuiltinFunction throwFunction;
+ public Object mock(
+ Object mandatory,
+ Object optional,
+ Object mandatoryKey,
+ Object optionalKey,
+ StarlarkThread thread) {
+ return EvalUtils.optionMap(
+ thread,
+ "mandatory",
+ mandatory,
+ "optional",
+ optional,
+ "mandatory_key",
+ mandatoryKey,
+ "optional_key",
+ optionalKey);
+ }
@Before
public final void createBuildFile() throws Exception {
@@ -174,41 +187,17 @@
}
private void setupSkylarkFunction(String line) throws Exception {
- mockFunc =
- new BuiltinFunction("mock") {
- @SuppressWarnings("unused")
- public Object invoke(
- Object mandatory,
- Object optional,
- Object mandatoryKey,
- Object optionalKey,
- StarlarkThread thread) {
- return EvalUtils.optionMap(
- thread,
- "mandatory",
- mandatory,
- "optional",
- optional,
- "mandatory_key",
- mandatoryKey,
- "optional_key",
- optionalKey);
- }
- };
- assertThat(mockFunc.isConfigured()).isFalse();
- mockFunc.configure(
- SkylarkRuleImplementationFunctionsTest.class
- .getDeclaredField("mockFunc")
- .getAnnotation(SkylarkSignature.class));
- update("mock", mockFunc);
+ update("mock", CallUtils.getBuiltinCallable(this, "mock"));
exec(line);
}
- private void checkSkylarkFunctionError(String errorMsg, String line) throws Exception {
+ private void checkSkylarkFunctionError(String errorSubstring, String line) throws Exception {
EvalException e = assertThrows(EvalException.class, () -> setupSkylarkFunction(line));
- assertThat(e).hasMessageThat().isEqualTo(errorMsg);
+ assertThat(e).hasMessageThat().contains(errorSubstring);
}
+ // TODO(adonovan): move these tests of the interpreter core into lib.syntax.
+
@Test
public void testSkylarkFunctionPosArgs() throws Exception {
setupSkylarkFunction("a = mock('a', 'b', mandatory_key='c')");
@@ -232,25 +221,20 @@
@Test
public void testSkylarkFunctionTooFewArguments() throws Exception {
checkSkylarkFunctionError(
- "insufficient arguments received by mock("
- + "mandatory, optional = None, *, mandatory_key, optional_key = \"x\") "
- + "(got 0, expected at least 1)",
- "mock()");
+ "parameter 'mandatory' has no default value", "mock(mandatory_key='y')");
}
@Test
public void testSkylarkFunctionTooManyArguments() throws Exception {
checkSkylarkFunctionError(
- "too many (3) positional arguments in call to "
- + "mock(mandatory, optional = None, *, mandatory_key, optional_key = \"x\")",
- "mock('a', 'b', 'c')");
+ "expected no more than 2 positional arguments, but got 3",
+ "mock('a', 'b', 'c', mandatory_key='y')");
}
@Test
public void testSkylarkFunctionAmbiguousArguments() throws Exception {
checkSkylarkFunctionError(
- "argument 'mandatory' passed both by position and by name "
- + "in call to mock(mandatory, optional = None, *, mandatory_key, optional_key = \"x\")",
+ "got multiple values for keyword argument 'mandatory'",
"mock('by position', mandatory='by_key', mandatory_key='c')");
}
@@ -1892,30 +1876,37 @@
}
}
+ @SkylarkCallable(name = "throw1", documented = false)
+ public Object throw1() throws Exception {
+ class ThereIsNoMessageException extends EvalException {
+ ThereIsNoMessageException() {
+ super(null, "This is not the message you are looking for."); // Unused dummy message
+ }
+
+ @Override
+ public String getMessage() {
+ return "";
+ }
+ }
+ throw new ThereIsNoMessageException();
+ }
+
@Test
public void testStackTraceWithoutOriginalMessage() throws Exception {
- setupThrowFunction(
- new BuiltinFunction("throw") {
- @SuppressWarnings("unused")
- public Object invoke() throws Exception {
- throw new ThereIsNoMessageException();
- }
- });
-
+ update("throw", CallUtils.getBuiltinCallable(this, "throw1"));
checkEvalErrorContains(
"There Is No Message: SkylarkRuleImplementationFunctionsTest",
"throw()");
}
+ @SkylarkCallable(name = "throw2", documented = false)
+ public Object throw2() throws Exception {
+ throw new InterruptedException();
+ }
+
@Test
public void testNoStackTraceOnInterrupt() throws Exception {
- setupThrowFunction(
- new BuiltinFunction("throw") {
- @SuppressWarnings("unused")
- public Object invoke() throws Exception {
- throw new InterruptedException();
- }
- });
+ update("throw", CallUtils.getBuiltinCallable(this, "throw2"));
assertThrows(InterruptedException.class, () -> eval("throw()"));
}
@@ -3131,22 +3122,4 @@
ctx.getRuleContext().getAnalysisEnvironment().getLocalGeneratingAction(params);
assertThat(action.getInputs()).contains(directory);
}
-
- private void setupThrowFunction(BuiltinFunction func) throws Exception {
- throwFunction = func;
- throwFunction.configure(
- getClass().getDeclaredField("throwFunction").getAnnotation(SkylarkSignature.class));
- update("throw", throwFunction);
- }
-
- private static class ThereIsNoMessageException extends EvalException {
- public ThereIsNoMessageException() {
- super(null, "This is not the message you are looking for."); // Unused dummy message
- }
-
- @Override
- public String getMessage() {
- return "";
- }
- }
}
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 c34f2d0..3287dcc 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
@@ -1298,7 +1298,8 @@
@Test
public void testStructAccessAsFuncall() throws Exception {
- foobar.configure(getClass().getDeclaredField("foobar").getAnnotation(SkylarkSignature.class));
+ foobar.configureFromAnnotation(
+ getClass().getDeclaredField("foobar").getAnnotation(SkylarkSignature.class));
new SkylarkTest()
.update("mock", new Mock())
.setUp("v = mock.struct_field_callable()")
@@ -1313,7 +1314,7 @@
@Test
public void testCallingInterruptedFunction() throws Exception {
- interruptedFunction.configure(
+ interruptedFunction.configureFromAnnotation(
getClass().getDeclaredField("interruptedFunction").getAnnotation(SkylarkSignature.class));
update("interrupted_function", interruptedFunction);
assertThrows(InterruptedException.class, () -> eval("interrupted_function()"));
