Allow structField callables to specify useSkylarkSemantics, useLocation, and useEnvironment
Unfortunately this doesn't work for all callers, namely NativeInfo objects, as they may have structField callables invoked from contexts that have no environment available.
RELNOTES[INC]: Skylark structs (using struct()) may no longer have to_json and to_proto overridden.
PiperOrigin-RevId: 201376969
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java b/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
index 21f8475..4396b43 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StructProvider.java
@@ -38,8 +38,14 @@
@Override
public Info createStruct(SkylarkDict<?, ?> kwargs, Location loc) throws EvalException {
- return SkylarkInfo.createSchemaless(
- this, kwargs.getContents(String.class, Object.class, "kwargs"), loc);
+ Map<String, Object> kwargsMap = kwargs.getContents(String.class, Object.class, "kwargs");
+ if (kwargsMap.containsKey("to_json")) {
+ throw new EvalException(loc, "cannot override built-in struct function 'to_json'");
+ }
+ if (kwargsMap.containsKey("to_proto")) {
+ throw new EvalException(loc, "cannot override built-in struct function 'to_proto'");
+ }
+ return SkylarkInfo.createSchemaless(this, kwargsMap, loc);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
index 71df625..2ccc6cc 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessor.java
@@ -158,14 +158,12 @@
throws SkylarkCallableProcessorException {
if (annotation.structField()) {
if (annotation.useAst()
- || annotation.useEnvironment()
- || annotation.useAst()
|| !annotation.extraPositionals().name().isEmpty()
|| !annotation.extraKeywords().name().isEmpty()) {
throw new SkylarkCallableProcessorException(
methodElement,
"@SkylarkCallable-annotated methods with structField=true may not also specify "
- + "useAst, useEnvironment, useLocation, extraPositionals, or extraKeywords");
+ + "useAst, extraPositionals, or extraKeywords");
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index ed45d60..b41f274c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -118,6 +118,32 @@
*/
public static Object eval(Object objValue, String name,
Location loc, Environment env) throws EvalException, InterruptedException {
+
+ Iterable<MethodDescriptor> methods =
+ objValue instanceof Class<?>
+ ? FuncallExpression.getMethods((Class<?>) objValue, name)
+ : FuncallExpression.getMethods(objValue.getClass(), name);
+
+ if (methods != null) {
+ Optional<MethodDescriptor> method =
+ Streams.stream(methods)
+ .filter(methodDescriptor -> methodDescriptor.getAnnotation().structField())
+ .findFirst();
+ if (method.isPresent() && method.get().getAnnotation().structField()) {
+ return FuncallExpression.callMethod(
+ method.get(),
+ name,
+ objValue,
+ FuncallExpression.extraInterpreterArgs(
+ method.get().getAnnotation(),
+ /* ast = */ null,
+ loc,
+ env).toArray(),
+ loc,
+ env);
+ }
+ }
+
if (objValue instanceof SkylarkClassObject) {
try {
return ((SkylarkClassObject) objValue).getValue(name);
@@ -142,22 +168,6 @@
}
}
- Iterable<MethodDescriptor> methods =
- objValue instanceof Class<?>
- ? FuncallExpression.getMethods((Class<?>) objValue, name)
- : FuncallExpression.getMethods(objValue.getClass(), name);
-
- if (methods != null) {
- Optional<MethodDescriptor> method =
- Streams.stream(methods)
- .filter(methodDescriptor -> methodDescriptor.getAnnotation().structField())
- .findFirst();
- if (method.isPresent() && method.get().getAnnotation().structField()) {
- return FuncallExpression.callMethod(
- method.get(), name, objValue, new Object[] {}, loc, env);
- }
- }
-
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 236bcc2..bf43266 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -411,6 +411,9 @@
/**
* Invokes the given structField=true method and returns the result.
*
+ * <p>The given method must <b>not</b> require extra-interpreter parameters, such as
+ * {@link Environment}. This method throws {@link IllegalArgumentException} for violations.</p>
+ *
* @param methodDescriptor the descriptor of the method to invoke
* @param fieldName the name of the struct field
* @param obj the object on which to invoke the method
@@ -420,7 +423,12 @@
public static Object invokeStructField(
MethodDescriptor methodDescriptor, String fieldName, Object obj)
throws EvalException, InterruptedException {
- Preconditions.checkArgument(methodDescriptor.getAnnotation().structField());
+ Preconditions.checkArgument(methodDescriptor.getAnnotation().structField(),
+ "Can only be invoked on structField callables");
+ Preconditions.checkArgument(!methodDescriptor.getAnnotation().useEnvironment()
+ || !methodDescriptor.getAnnotation().useSkylarkSemantics()
+ || !methodDescriptor.getAnnotation().useLocation(),
+ "Cannot be invoked on structField callables with extra interpreter params");
return callMethod(methodDescriptor, fieldName, obj, new Object[0], Location.BUILTIN, null);
}
@@ -494,8 +502,12 @@
if (methods != null) {
for (MethodDescriptor method : methods) {
if (method.getAnnotation().structField()) {
- // TODO(cparsons): Allow structField methods to accept interpreter-supplied arguments.
- return new Pair<>(method, null);
+ // This indicates a built-in structField which returns a function which may have
+ // one or more arguments itself. For example, foo.bar('baz'), where foo.bar is a
+ // structField returning a function. Calling the "bar" callable of foo should
+ // not have 'baz' propagated, though extra interpreter arguments should be supplied.
+ return new Pair<>(method,
+ extraInterpreterArgs(method.getAnnotation(), null, getLocation(), environment));
} else {
argumentListConversionResult = convertArgumentList(args, kwargs, method, environment);
if (argumentListConversionResult.getArguments() != null) {
@@ -567,6 +579,36 @@
}
/**
+ * Returns the extra interpreter arguments for the given {@link SkylarkCallable}, to be added
+ * at the end of the argument list for the callable.
+ *
+ * <p>This method accepts null {@code ast} only if {@code callable.useAst()} is false. It is
+ * up to the caller to validate this invariant.</p>
+ */
+ public static List<Object> extraInterpreterArgs(SkylarkCallable callable,
+ @Nullable FuncallExpression ast, Location loc, Environment env) {
+
+ ImmutableList.Builder<Object> builder = ImmutableList.builder();
+
+ if (callable.useLocation()) {
+ builder.add(loc);
+ }
+ if (callable.useAst()) {
+ if (ast == null) {
+ throw new IllegalArgumentException("Callable expects to receive ast: " + callable.name());
+ }
+ builder.add(ast);
+ }
+ if (callable.useEnvironment()) {
+ builder.add(env);
+ }
+ if (callable.useSkylarkSemantics()) {
+ builder.add(env.getSemantics());
+ }
+ return builder.build();
+ }
+
+ /**
* Constructs the parameters list to actually pass to the method, filling with default values if
* any. If there is a type or argument mismatch, returns a result containing an error message.
*/
@@ -702,18 +744,7 @@
if (acceptsExtraKwargs) {
builder.add(SkylarkDict.copyOf(environment, extraKwargsBuilder.build()));
}
- if (callable.useLocation()) {
- builder.add(getLocation());
- }
- if (callable.useAst()) {
- builder.add(this);
- }
- if (callable.useEnvironment()) {
- builder.add(environment);
- }
- if (callable.useSkylarkSemantics()) {
- builder.add(environment.getSemantics());
- }
+ builder.addAll(extraInterpreterArgs(callable, this, getLocation(), environment));
return ArgumentListConversionResult.fromArgumentList(builder.build());
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 23d9142..36c172b 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -820,6 +820,17 @@
}
@Test
+ public void testStructRestrictedOverrides() throws Exception {
+ checkErrorContains(
+ "cannot override built-in struct function 'to_json'",
+ "struct(to_json='foo')");
+
+ checkErrorContains(
+ "cannot override built-in struct function 'to_proto'",
+ "struct(to_proto='foo')");
+ }
+
+ @Test
public void testSimpleTextMessages() throws Exception {
checkTextMessage("struct(name='value').to_proto()", "name: \"value\"");
checkTextMessage("struct(name=['a', 'b']).to_proto()", "name: \"a\"", "name: \"b\"");
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
index 208e9a3..ae4a449 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/SkylarkCallableProcessorTest.java
@@ -74,7 +74,7 @@
.failsToCompile()
.withErrorContaining(
"@SkylarkCallable-annotated methods with structField=true may not also specify "
- + "useAst, useEnvironment, useLocation, extraPositionals, or extraKeywords");
+ + "useAst, extraPositionals, or extraKeywords");
}
@Test
@@ -85,7 +85,7 @@
.failsToCompile()
.withErrorContaining(
"@SkylarkCallable-annotated methods with structField=true may not also specify "
- + "useAst, useEnvironment, useLocation, extraPositionals, or extraKeywords");
+ + "useAst, extraPositionals, or extraKeywords");
}
@Test
@@ -96,7 +96,7 @@
.failsToCompile()
.withErrorContaining(
"@SkylarkCallable-annotated methods with structField=true may not also specify "
- + "useAst, useEnvironment, useLocation, extraPositionals, or extraKeywords");
+ + "useAst, extraPositionals, or extraKeywords");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
index 580be01..6df1d64 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/GoldenCase.java
@@ -211,4 +211,18 @@
public Integer selfCallMethod(String one, Integer two) {
return 0;
}
+
+ @SkylarkCallable(
+ name = "struct_field_method_with_extra_args",
+ documented = false,
+ structField = true,
+ useLocation = true,
+ useEnvironment = true,
+ useSkylarkSemantics = true
+ )
+ public String structFieldMethodWithInfo(Location location,
+ Environment environment,
+ SkylarkSemantics skylarkSemantics) {
+ return "dragon";
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/StructFieldWithInvalidInfo.java b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/StructFieldWithInvalidInfo.java
index 2843928..c66e44b 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/StructFieldWithInvalidInfo.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkinterface/processor/testsources/StructFieldWithInvalidInfo.java
@@ -15,7 +15,7 @@
package com.google.devtools.build.lib.skylarkinterface.processor.testsources;
import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
-import com.google.devtools.build.lib.syntax.Environment;
+import com.google.devtools.build.lib.syntax.FuncallExpression;
/** Test case which verifies a struct field method cannot specify useEnvironment. */
public class StructFieldWithInvalidInfo {
@@ -24,9 +24,9 @@
name = "struct_field_method_with_info",
documented = false,
structField = true,
- useEnvironment = true
+ useAst = true
)
- public String structFieldMethodWithInfo(Environment environment) {
+ public String structFieldMethodWithInfo(FuncallExpression ast) {
return "dragon";
}
}
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 e376a6d..68a6909 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
@@ -137,6 +137,15 @@
public String structField() {
return "a";
}
+ @SkylarkCallable(name = "struct_field_with_extra",
+ documented = false,
+ structField = true,
+ useSkylarkSemantics = true)
+ public String structFieldWithExtra(SkylarkSemantics sem) {
+ return "struct_field_with_extra("
+ + (sem != null)
+ + ")";
+ }
@SkylarkCallable(name = "struct_field_callable", documented = false, structField = true)
public BuiltinFunction structFieldCallable() {
return foobar;
@@ -1249,6 +1258,14 @@
}
@Test
+ public void testStructFieldWithExtraInterpreterParams() throws Exception {
+ new SkylarkTest()
+ .update("mock", new Mock())
+ .setUp("v = mock.struct_field_with_extra")
+ .testLookup("v", "struct_field_with_extra(true)");
+ }
+
+ @Test
public void testJavaFunctionWithParamsAndExtraInterpreterParams() throws Exception {
new SkylarkTest()
.update("mock", new Mock())
@@ -1851,6 +1868,7 @@
"string_list_dict",
"struct_field",
"struct_field_callable",
+ "struct_field_with_extra",
"value_of",
"voidfunc",
"with_args_and_env",
@@ -2064,7 +2082,7 @@
new SkylarkTest()
.update("val", new SkylarkClassObjectWithSkylarkCallables())
.setUp("v = val.collision_field")
- .testLookup("v", "fromValues");
+ .testLookup("v", "fromSkylarkCallable");
}
@Test
