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'])"); + } }