Automated rollback of commit 50a7e14e3df5513572f1ac3275786f243b51c930. *** Reason for rollback *** This is stacked on top of https://github.com/bazelbuild/bazel/commit/7c2174fea2b50af8fbf10367835b820af29700f6, which needs to be rolled back. *** Original change description *** bazel syntax: delete ExtraArgKind [...] RELNOTES: N/A PiperOrigin-RevId: 276732678
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 a6e3cef..96396b4 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.getLocation(), + ast, 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 a17fa4e..e2ca4ab 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,12 +65,17 @@ String getSkylarkName(); } - protected NativeProvider(Class<V> valueClass, String name) { - super(name, FunctionSignature.KWARGS, Location.BUILTIN); - this.key = new NativeKey(name, getClass()); + 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); this.valueClass = valueClass; - this.errorMessageFormatForUnknownField = - String.format("'%s' object has no attribute '%%s'", name); + 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 1e72875..89d4f1e 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); + super(ruleClassName, FunctionSignature.KWARGS, BuiltinFunction.USE_LOC_ENV); 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 d9ee924..0793459 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,7 +24,6 @@ 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; @@ -35,6 +34,7 @@ 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,17 +262,18 @@ */ private static BuiltinFunction newRuleFunction( final RuleFactory ruleFactory, final String ruleClassName, final boolean allowOverride) { - return new BuiltinFunction(ruleClassName, FunctionSignature.KWARGS) { - public Object invoke(Map<String, Object> kwargs, Location loc, StarlarkThread thread) + return new BuiltinFunction( + ruleClassName, FunctionSignature.KWARGS, BuiltinFunction.USE_AST_ENV) { + public Object invoke(Map<String, Object> kwargs, FuncallExpression ast, StarlarkThread thread) throws EvalException, InterruptedException { try { - Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder; + Package.Builder builder = PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder; String externalRepoName = (String) kwargs.get("name"); if (!allowOverride && externalRepoName != null && builder.getTarget(externalRepoName) != null) { throw new EvalException( - loc, + ast.getLocation(), "Cannot redefine repository after any load statement in the WORKSPACE file" + " (for repository '" + kwargs.get("name") @@ -282,7 +283,8 @@ // @<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, loc); + WorkspaceFactoryHelper.addRepoMappings( + builder, kwargs, externalRepoName, ast.getLocation()); RuleClass ruleClass = ruleFactory.getRuleClass(ruleClassName); RuleClass bindRuleClass = ruleFactory.getRuleClass("bind"); Rule rule = @@ -291,10 +293,10 @@ ruleClass, bindRuleClass, WorkspaceFactoryHelper.getFinalKwargs(kwargs), - loc); + ast); if (!WorkspaceGlobals.isLegalWorkspaceName(rule.getName())) { throw new EvalException( - loc, + ast.getLocation(), rule + "'s name field must be a legal workspace name;" + " workspace names may contain only A-Z, a-z, 0-9, '-', '_' and '.'"); @@ -302,7 +304,7 @@ } catch (RuleFactory.InvalidRuleException | Package.NameConflictException | LabelSyntaxException e) { - throw new EvalException(loc, e.getMessage()); + throw new EvalException(ast.getLocation(), 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 e519196..a7a37c3 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,6 +26,7 @@ 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; @@ -39,10 +40,10 @@ RuleClass ruleClass, RuleClass bindRuleClass, Map<String, Object> kwargs, - Location loc) + FuncallExpression ast) throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException, InterruptedException { - return createAndAddRepositoryRule(pkg, ruleClass, bindRuleClass, kwargs, loc, null); + return createAndAddRepositoryRule(pkg, ruleClass, bindRuleClass, kwargs, ast, null); } public static Rule createAndAddRepositoryRule( @@ -50,7 +51,7 @@ RuleClass ruleClass, RuleClass bindRuleClass, Map<String, Object> kwargs, - Location loc, + FuncallExpression ast, // just for getLocation(); TODO(adonovan): simplify String definitionInfo) throws RuleFactory.InvalidRuleException, Package.NameConflictException, LabelSyntaxException, InterruptedException { @@ -63,7 +64,7 @@ ruleClass, attributeValues, eventHandler, - loc, + ast.getLocation(), /* 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 ab44235..bacaf5f 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,6 +30,7 @@ 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; @@ -64,19 +65,19 @@ public NoneType workspace( String name, SkylarkDict<?, ?> managedDirectories, // <String, Object> - Location loc, + FuncallExpression ast, StarlarkThread thread) throws EvalException, InterruptedException { if (allowOverride) { if (!isLegalWorkspaceName(name)) { - throw new EvalException(loc, name + " is not a legal workspace name"); + throw new EvalException(ast.getLocation(), name + " is not a legal workspace name"); } String errorMessage = LabelValidator.validateTargetName(name); if (errorMessage != null) { - throw new EvalException(loc, errorMessage); + throw new EvalException(ast.getLocation(), errorMessage); } - PackageFactory.getContext(thread, loc).pkgBuilder.setWorkspaceName(name); - Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder; + PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder.setWorkspaceName(name); + Package.Builder builder = PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder; RuleClass localRepositoryRuleClass = ruleFactory.getRuleClass("local_repository"); RuleClass bindRuleClass = ruleFactory.getRuleClass("bind"); Map<String, Object> kwargs = ImmutableMap.<String, Object>of("name", name, "path", "."); @@ -84,9 +85,9 @@ // This effectively adds a "local_repository(name = "<ws>", path = ".")" // definition to the WORKSPACE file. WorkspaceFactoryHelper.createAndAddRepositoryRule( - builder, localRepositoryRuleClass, bindRuleClass, kwargs, loc); + builder, localRepositoryRuleClass, bindRuleClass, kwargs, ast); } catch (InvalidRuleException | NameConflictException | LabelSyntaxException e) { - throw new EvalException(loc, e.getMessage()); + throw new EvalException(ast.getLocation(), e.getMessage()); } // Add entry in repository map from "@name" --> "@" to avoid issue where bazel // treats references to @name as a separate external repo @@ -97,28 +98,29 @@ RepositoryName.MAIN); } parseManagedDirectories( - managedDirectories.getContents(String.class, Object.class, "managed_directories"), loc); + managedDirectories.getContents(String.class, Object.class, "managed_directories"), ast); return NONE; } else { throw new EvalException( - loc, "workspace() function should be used only at the top of the WORKSPACE file"); + ast.getLocation(), + "workspace() function should be used only at the top of the WORKSPACE file"); } } private void parseManagedDirectories( Map<String, ?> managedDirectories, // <String, SkylarkList<String>> - Location loc) + FuncallExpression ast) throws EvalException { Map<PathFragment, String> nonNormalizedPathsMap = Maps.newHashMap(); for (Map.Entry<String, ?> entry : managedDirectories.entrySet()) { - RepositoryName repositoryName = createRepositoryName(entry.getKey(), loc); + RepositoryName repositoryName = createRepositoryName(entry.getKey(), ast.getLocation()); List<PathFragment> paths = - getManagedDirectoriesPaths(entry.getValue(), loc, nonNormalizedPathsMap); + getManagedDirectoriesPaths(entry.getValue(), ast.getLocation(), nonNormalizedPathsMap); for (PathFragment dir : paths) { PathFragment floorKey = managedDirectoriesMap.floorKey(dir); if (dir.equals(floorKey)) { throw new EvalException( - loc, + ast.getLocation(), String.format( "managed_directories attribute should not contain multiple" + " (or duplicate) repository mappings for the same directory ('%s').", @@ -128,7 +130,7 @@ boolean isDescendant = floorKey != null && dir.startsWith(floorKey); if (isDescendant || (ceilingKey != null && ceilingKey.startsWith(dir))) { throw new EvalException( - loc, + ast.getLocation(), String.format( "managed_directories attribute value can not contain nested mappings." + " '%s' is a descendant of '%s'.", @@ -242,29 +244,29 @@ } @Override - public NoneType bind(String name, Object actual, Location loc, StarlarkThread thread) + public NoneType bind(String name, Object actual, FuncallExpression ast, StarlarkThread thread) throws EvalException, InterruptedException { Label nameLabel; try { nameLabel = Label.parseAbsolute("//external:" + name, ImmutableMap.of()); try { - Package.Builder builder = PackageFactory.getContext(thread, loc).pkgBuilder; + Package.Builder builder = PackageFactory.getContext(thread, ast.getLocation()).pkgBuilder; RuleClass ruleClass = ruleFactory.getRuleClass("bind"); WorkspaceFactoryHelper.addBindRule( builder, ruleClass, nameLabel, actual == NONE ? null : Label.parseAbsolute((String) actual, ImmutableMap.of()), - loc, + ast.getLocation(), new AttributeContainer(ruleClass)); } catch (RuleFactory.InvalidRuleException | Package.NameConflictException | LabelSyntaxException e) { - throw new EvalException(loc, e.getMessage()); + throw new EvalException(ast.getLocation(), e.getMessage()); } } catch (LabelSyntaxException e) { - throw new EvalException(loc, e.getMessage()); + throw new EvalException(ast.getLocation(), 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 0c0df8f..97d36cb 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,6 +20,7 @@ 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 +79,12 @@ + "\nManaged directories must be excluded from the source tree by listing" + " them (or their parent directories) in the .bazelignore file."), }, - useLocation = true, + useAst = true, useStarlarkThread = true) NoneType workspace( String name, SkylarkDict<?, ?> managedDirectories, // <String, SkylarkList<String>> - Location loc, + FuncallExpression ast, StarlarkThread thread) throws EvalException, InterruptedException; @@ -145,8 +146,8 @@ defaultValue = "None", doc = "The real label to be aliased") }, - useLocation = true, + useAst = true, useStarlarkThread = true) - NoneType bind(String name, Object actual, Location loc, StarlarkThread thread) + NoneType bind(String name, Object actual, FuncallExpression ast, 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 0346d45..6f139ae 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,6 +21,7 @@ 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; @@ -149,11 +150,11 @@ */ protected BaseFunction( @Nullable String name, - FunctionSignature signature, + @Nullable FunctionSignature signature, @Nullable List<Object> defaultValues, @Nullable Location location) { this(name); - this.signature = Preconditions.checkNotNull(signature); + this.signature = signature; this.defaultValues = defaultValues; this.location = location; @@ -364,7 +365,11 @@ } /** check types and convert as required */ - private void canonicalizeArguments(Object[] arguments, Location loc) throws EvalException { + 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. + List<SkylarkType> types = getEnforcedArgumentTypes(); // Check types, if supplied @@ -434,7 +439,6 @@ * @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 { @@ -479,16 +483,31 @@ 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. - void configure() { + protected void configure() { Preconditions.checkState(signature != null); - // 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. + // TODO(adonovan): this looks fishy. It clobbers this.enforcedArgumentTypes, populated as a + // side-effect of the getSignatureForCallable call in configure. 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 39de391..652bc24 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,7 +15,6 @@ 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; @@ -39,6 +38,18 @@ */ 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 = @@ -47,28 +58,37 @@ // 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} + // 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; // 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 final int getArgArraySize() { - return invokeMethod.getParameterCount(); + protected int getArgArraySize() { + return innerArgumentCount; } @Override @@ -80,18 +100,22 @@ // 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); + // 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; } i++; } @@ -117,12 +141,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 - extraParams.length; + final int len = args.length - ((extraArgs == null) ? 0 : extraArgs.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) : extraParams[i - len].getName(); + i < len ? signature.getParameterNames().get(i) : extraArgs[i - len].name(); throw new EvalException( loc, String.format( @@ -168,41 +192,34 @@ e); } - /** - * Configure the reflection mechanism. Called by signature processor for BuiltinFunctions already - * created. - */ - final void configureFromAnnotation(SkylarkSignature annotation) { + /** Configure the reflection mechanism */ + @Override + public void configure(SkylarkSignature annotation) { Preconditions.checkState(!isConfigured()); // must not be configured yet - this.enforcedArgumentTypes = new ArrayList<>(); - + enforcedArgumentTypes = new ArrayList<>(); + this.extraArgs = SkylarkSignatureProcessor.getExtraArgs(annotation); this.returnType = annotation.returnType(); - - // 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(); + super.configure(annotation); } - /** - * Configure the reflection mechanism. Called directly by constructor for BuiltinFunctions created - * with a signature, and called after annotation processing for other BuiltinFunctions. - */ + /** Configure the reflection mechanism */ @Override - final void configure() { - this.invokeMethod = findMethod(this.getClass(), "invoke"); + protected void configure() { + invokeMethod = findMethod("invoke"); + + int arguments = signature.numParameters(); + innerArgumentCount = arguments + (extraArgs == null ? 0 : extraArgs.length); Class<?>[] parameterTypes = invokeMethod.getParameterTypes(); - int numParameters = signature.numParameters(); - this.extraParams = extraParams(numParameters, parameterTypes); + 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)); + } if (enforcedArgumentTypes != null) { - for (int i = 0; i < numParameters; i++) { + for (int i = 0; i < arguments; i++) { SkylarkType enforcedType = enforcedArgumentTypes.get(i); if (enforcedType != null) { Class<?> parameterType = parameterTypes[i]; @@ -246,28 +263,6 @@ } } - // 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) { @@ -282,21 +277,21 @@ } // 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 findMethod(final String name) { Method found = null; - for (Method method : cls.getDeclaredMethods()) { + for (Method method : this.getClass().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)); + String.format("function %s has more than one method named %s", getName(), name)); } found = method; } } if (found == null) { throw new NoSuchElementException( - String.format("class %s doesn't have a method named %s", cls.getName(), name)); + String.format("function %s doesn't have a method named %s", 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 01bc0bc..db45a0b 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,9 +15,11 @@ 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; @@ -343,21 +345,43 @@ } } + /** 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 - * BuiltinFunction} instances, calling {@link BuiltinFunction#configure(SkylarkSignature)}. The - * fields will be picked up by reflection even if they are not public. + * BaseFunction} instances, calling {@link BaseFunction#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(); @@ -375,10 +399,10 @@ type, field); builtins.registerBuiltin(type, field.getName(), value); - if (BuiltinFunction.class.isAssignableFrom(field.getType())) { - BuiltinFunction function = (BuiltinFunction) value; + if (BaseFunction.class.isAssignableFrom(field.getType())) { + BaseFunction function = (BaseFunction) value; if (!function.isConfigured()) { - function.configureFromAnnotation(annotation); + function.configure(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 e720a6c..f861ad1 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.getLocation()); + packageBuilder, buildRuleClass(attributes), null, kwargs, ast); 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 32cd487..99425da 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
@@ -58,9 +58,8 @@ 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.SkylarkCallable; -import com.google.devtools.build.lib.skylarkinterface.SkylarkGlobalLibrary; -import com.google.devtools.build.lib.syntax.CallUtils; +import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature; +import com.google.devtools.build.lib.syntax.BuiltinFunction; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.EvalUtils; import com.google.devtools.build.lib.syntax.Printer; @@ -88,18 +87,15 @@ /** 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(); - // def mock(mandatory, optional=None, *, mandatory_key, optional_key='x') - @SkylarkCallable( + @SkylarkSignature( name = "mock", documented = false, parameters = { - @Param(name = "mandatory", doc = "", named = true), - @Param(name = "optional", doc = "", defaultValue = "None", noneable = true, named = true), + @Param(name = "mandatory", doc = ""), + @Param(name = "optional", doc = "", defaultValue = "None"), @Param(name = "mandatory_key", doc = "", positional = false, named = true), @Param( name = "optional_key", @@ -109,23 +105,14 @@ named = true) }, useStarlarkThread = true) - 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); - } + private BuiltinFunction mockFunc; + + /** + * Used for {@link #testStackTraceWithoutOriginalMessage()} and {@link + * #testNoStackTraceOnInterrupt}. + */ + @SkylarkSignature(name = "throw", documented = false) + BuiltinFunction throwFunction; @Before public final void createBuildFile() throws Exception { @@ -189,17 +176,41 @@ } private void setupSkylarkFunction(String line) throws Exception { - update("mock", CallUtils.getBuiltinCallable(this, "mock")); + 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); exec(line); } - private void checkSkylarkFunctionError(String errorSubstring, String line) throws Exception { + private void checkSkylarkFunctionError(String errorMsg, String line) throws Exception { EvalException e = assertThrows(EvalException.class, () -> setupSkylarkFunction(line)); - assertThat(e).hasMessageThat().contains(errorSubstring); + assertThat(e).hasMessageThat().isEqualTo(errorMsg); } - // 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')"); @@ -223,20 +234,25 @@ @Test public void testSkylarkFunctionTooFewArguments() throws Exception { checkSkylarkFunctionError( - "parameter 'mandatory' has no default value", "mock(mandatory_key='y')"); + "insufficient arguments received by mock(" + + "mandatory, optional = None, *, mandatory_key, optional_key = \"x\") " + + "(got 0, expected at least 1)", + "mock()"); } @Test public void testSkylarkFunctionTooManyArguments() throws Exception { checkSkylarkFunctionError( - "expected no more than 2 positional arguments, but got 3", - "mock('a', 'b', 'c', mandatory_key='y')"); + "too many (3) positional arguments in call to " + + "mock(mandatory, optional = None, *, mandatory_key, optional_key = \"x\")", + "mock('a', 'b', 'c')"); } @Test public void testSkylarkFunctionAmbiguousArguments() throws Exception { checkSkylarkFunctionError( - "got multiple values for keyword argument 'mandatory'", + "argument 'mandatory' passed both by position and by name " + + "in call to mock(mandatory, optional = None, *, mandatory_key, optional_key = \"x\")", "mock('by position', mandatory='by_key', mandatory_key='c')"); } @@ -1878,37 +1894,30 @@ } } - @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 { - update("throw", CallUtils.getBuiltinCallable(this, "throw1")); + setupThrowFunction( + new BuiltinFunction("throw") { + @SuppressWarnings("unused") + public Object invoke() throws Exception { + throw new ThereIsNoMessageException(); + } + }); + 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 { - update("throw", CallUtils.getBuiltinCallable(this, "throw2")); + setupThrowFunction( + new BuiltinFunction("throw") { + @SuppressWarnings("unused") + public Object invoke() throws Exception { + throw new InterruptedException(); + } + }); assertThrows(InterruptedException.class, () -> eval("throw()")); } @@ -3124,4 +3133,22 @@ 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 3287dcc..c34f2d0 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,8 +1298,7 @@ @Test public void testStructAccessAsFuncall() throws Exception { - foobar.configureFromAnnotation( - getClass().getDeclaredField("foobar").getAnnotation(SkylarkSignature.class)); + foobar.configure(getClass().getDeclaredField("foobar").getAnnotation(SkylarkSignature.class)); new SkylarkTest() .update("mock", new Mock()) .setUp("v = mock.struct_field_callable()") @@ -1314,7 +1313,7 @@ @Test public void testCallingInterruptedFunction() throws Exception { - interruptedFunction.configureFromAnnotation( + interruptedFunction.configure( getClass().getDeclaredField("interruptedFunction").getAnnotation(SkylarkSignature.class)); update("interrupted_function", interruptedFunction); assertThrows(InterruptedException.class, () -> eval("interrupted_function()"));