From 3b08beb18939c2372c8c4913a2b71da861b834b9 Mon Sep 17 00:00:00 2001 From: cparsons Date: Tue, 5 Mar 2019 12:25:34 -0800 Subject: [PATCH] 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 --- .../packages/StarlarkSemanticsOptions.java | 14 +++++++++++++ .../processor/SkylarkCallableProcessor.java | 2 +- .../build/lib/syntax/FuncallExpression.java | 21 +++++++++++++++---- .../build/lib/syntax/ParamDescriptor.java | 13 ++++++++---- .../build/lib/syntax/StarlarkSemantics.java | 5 +++++ .../SkylarkSemanticsConsistencyTest.java | 2 ++ .../build/lib/syntax/MethodLibraryTest.java | 9 ++++++++ 7 files changed, 57 insertions(+), 9 deletions(-) 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 4231d1f3c2c54c..1ef10cbfdb934c 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 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "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 @@ public StarlarkSemantics toSkylarkSemantics() { .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 6cff955fb30aca..344ba777202ec4 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 @@ private void verifyParamSemantics(ExecutableElement methodElement, SkylarkCallab 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 33104a3dd0c15d..37855955ee9f7c 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 @@ public Object[] convertStarlarkArgumentsToJavaMethodArguments( } } 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 @@ public Object[] convertStarlarkArgumentsToJavaMethodArguments( return builder.build().toArray(); } + private EvalException unspecifiedParameterException( + ParamDescriptor param, + MethodDescriptor method, + Class objClass, + Map 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 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 ec10c190a773a5..7a47cc17bdd5cd 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 @@ private ParamDescriptor( Class generic1, boolean noneable, boolean named, - boolean legacyNamed, boolean positional, SkylarkType skylarkType, @Nullable String valueOverride, @@ -67,7 +66,7 @@ private ParamDescriptor( 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 @@ static ParamDescriptor of(Param param, StarlarkSemantics starlarkSemantics) { 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 b2a879c7987bdb..857d7b94379be0 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 boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean experimentalPlatformsApi(); + public abstract boolean experimentalRestrictNamedParams(); + public abstract boolean experimentalStarlarkConfigTransitions(); public abstract String experimentalTransitionWhitelistLocation(); @@ -208,6 +210,7 @@ public static Builder builderWithDefaults() { .experimentalEnableAndroidMigrationApis(false) .experimentalJavaCommonCreateProviderEnabledPackages(ImmutableList.of()) .experimentalPlatformsApi(false) + .experimentalRestrictNamedParams(false) .experimentalStarlarkConfigTransitions(false) .experimentalTransitionWhitelistLocation("") .incompatibleUseToolchainProvidersInJavaCommon(false) @@ -257,6 +260,8 @@ public abstract static class Builder { 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 9ac394166e02ae..563c8647782b4a 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 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E + "," + 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 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .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 a7a74fc3f8a415..af019ff3d25c2a 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 @@ public void testLegacyNamed() throws Exception { .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'])"); + } }