Create flag to turn down named-ness of certain Starlark params
This change creates --experimental_restrict_named_params to make all builtin Starlark parameters which are named via legacyNamed = true to instead be treated as positional-only.
This is a step on the path to cleaning up namedness of builtin Starlark parameters.
When we finalize the list of parameters which should truly be positional-only, we can change this flag to be prefixed with --incompatible instead of --experimental, as per the incompatible change policies.
Progress toward #5010.
RELNOTES: None.
PiperOrigin-RevId: 236898018
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
index 4231d1f..1ef10cb 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -126,6 +126,19 @@
+ "debugging.")
public boolean experimentalPlatformsApi;
+ // TODO(cparsons): Change this flag to --incompatible instead of --experimental when it is
+ // fully implemented.
+ @Option(
+ name = "experimental_restrict_named_params",
+ defaultValue = "false",
+ documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+ effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+ metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+ help =
+ "If set to true, restricts a number of Starlark built-in function parameters to be "
+ + "only specifiable positionally (and not by keyword).")
+ public boolean experimentalRestrictNamedParams;
+
// TODO(cparsons): Resolve and finalize the transition() API. The transition implementation
// function should accept two mandatory parameters, 'settings' and 'attr'.
@Option(
@@ -551,6 +564,7 @@
.experimentalJavaCommonCreateProviderEnabledPackages(
experimentalJavaCommonCreateProviderEnabledPackages)
.experimentalPlatformsApi(experimentalPlatformsApi)
+ .experimentalRestrictNamedParams(experimentalRestrictNamedParams)
.experimentalStarlarkConfigTransitions(experimentalStarlarkConfigTransitions)
.experimentalTransitionWhitelistLocation(experimentalTransitionWhitelistLocation)
.incompatibleBzlDisallowLoadAfterStatement(incompatibleBzlDisallowLoadAfterStatement)
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 6cff955..344ba77 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
@@ -221,7 +221,7 @@
boolean allowNonDefaultPositionalNext = true;
for (Param parameter : annotation.parameters()) {
- if ((!parameter.positional()) && (!isParamNamed(parameter))) {
+ if (!parameter.positional() && !parameter.named()) {
throw new SkylarkCallableProcessorException(
methodElement,
String.format("Parameter '%s' must be either positional or named",
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 33104a3..3785595 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
@@ -553,10 +553,7 @@
}
} else { // Param not specified by user. Use default value.
if (param.getDefaultValue().isEmpty()) {
- throw argumentMismatchException(
- String.format("parameter '%s' has no default value", param.getName()),
- method,
- objClass);
+ throw unspecifiedParameterException(param, method, objClass, kwargs);
}
value =
SkylarkSignatureProcessor.getDefaultValue(
@@ -613,6 +610,22 @@
return builder.build().toArray();
}
+ private EvalException unspecifiedParameterException(
+ ParamDescriptor param,
+ MethodDescriptor method,
+ Class<?> objClass,
+ Map<String, Object> kwargs) {
+ if (kwargs.containsKey(param.getName())) {
+ return argumentMismatchException(
+ String.format("parameter '%s' may not be specified by name", param.getName()),
+ method,
+ objClass);
+ } else {
+ return argumentMismatchException(
+ String.format("parameter '%s' has no default value", param.getName()), method, objClass);
+ }
+ }
+
private EvalException unexpectedKeywordArgumentException(
Set<String> unexpectedKeywords,
MethodDescriptor method,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java b/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java
index ec10c19..7a47cc1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ParamDescriptor.java
@@ -56,7 +56,6 @@
Class<?> generic1,
boolean noneable,
boolean named,
- boolean legacyNamed,
boolean positional,
SkylarkType skylarkType,
@Nullable String valueOverride,
@@ -67,7 +66,7 @@
this.allowedTypes = allowedTypes;
this.generic1 = generic1;
this.noneable = noneable;
- this.named = named || legacyNamed;
+ this.named = named;
this.positional = positional;
this.skylarkType = skylarkType;
this.valueOverride = valueOverride;
@@ -103,14 +102,20 @@
allowedTypes,
generic,
noneable,
- param.named(),
- param.legacyNamed(),
+ isNamed(param, starlarkSemantics),
param.positional(),
getType(type, generic, allowedTypes, noneable),
valueOverride,
flagResponsibleForDisable);
}
+ private static boolean isNamed(Param param, StarlarkSemantics starlarkSemantics) {
+ if (param.named()) {
+ return true;
+ }
+ return param.legacyNamed() && !starlarkSemantics.experimentalRestrictNamedParams();
+ }
+
/** @see Param#name() */
public String getName() {
return name;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
index b2a879c..857d7b9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -127,6 +127,8 @@
public abstract boolean experimentalPlatformsApi();
+ public abstract boolean experimentalRestrictNamedParams();
+
public abstract boolean experimentalStarlarkConfigTransitions();
public abstract String experimentalTransitionWhitelistLocation();
@@ -208,6 +210,7 @@
.experimentalEnableAndroidMigrationApis(false)
.experimentalJavaCommonCreateProviderEnabledPackages(ImmutableList.of())
.experimentalPlatformsApi(false)
+ .experimentalRestrictNamedParams(false)
.experimentalStarlarkConfigTransitions(false)
.experimentalTransitionWhitelistLocation("")
.incompatibleUseToolchainProvidersInJavaCommon(false)
@@ -257,6 +260,8 @@
public abstract Builder experimentalPlatformsApi(boolean value);
+ public abstract Builder experimentalRestrictNamedParams(boolean value);
+
public abstract Builder experimentalStarlarkConfigTransitions(boolean value);
public abstract Builder experimentalTransitionWhitelistLocation(String value);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index 9ac3941..563c864 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -130,6 +130,7 @@
+ ","
+ rand.nextDouble(),
"--experimental_platforms_api=" + rand.nextBoolean(),
+ "--experimental_restrict_named_params=" + rand.nextBoolean(),
"--experimental_starlark_config_transitions=" + rand.nextBoolean(),
"--experimental_transition_whitelist_location=" + rand.nextDouble(),
"--incompatible_bzl_disallow_load_after_statement=" + rand.nextBoolean(),
@@ -177,6 +178,7 @@
.experimentalJavaCommonCreateProviderEnabledPackages(
ImmutableList.of(String.valueOf(rand.nextDouble()), String.valueOf(rand.nextDouble())))
.experimentalPlatformsApi(rand.nextBoolean())
+ .experimentalRestrictNamedParams(rand.nextBoolean())
.experimentalStarlarkConfigTransitions(rand.nextBoolean())
.experimentalTransitionWhitelistLocation(String.valueOf(rand.nextDouble()))
.incompatibleBzlDisallowLoadAfterStatement(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
index a7a74fc..af019ff 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/MethodLibraryTest.java
@@ -745,4 +745,13 @@
.testIfErrorContains("None", "fail(msg=None)")
.testEval("type(None)", "'NoneType'");
}
+
+ @Test
+ public void testExperimentalStarlarkConfig() throws Exception {
+ new SkylarkTest("--experimental_restrict_named_params")
+ .testIfErrorContains(
+ "parameter 'elements' may not be specified by name, "
+ + "for call to method join(elements) of 'string'",
+ "','.join(elements=['foo', 'bar'])");
+ }
}
