From 40d1f454a345f92027910d513331045d5c9aca82 Mon Sep 17 00:00:00 2001 From: laurentlb Date: Fri, 8 Nov 2019 09:21:56 -0800 Subject: [PATCH] Remove flag incompatible_disallow_old_style_args_add https://github.com/bazelbuild/bazel/issues/5822 RELNOTES: None. PiperOrigin-RevId: 279328336 --- .../build/lib/analysis/skylark/Args.java | 61 +++------- .../skylark/SkylarkCustomCommandLine.java | 42 +------ .../packages/StarlarkSemanticsOptions.java | 15 --- .../build/lib/syntax/StarlarkSemantics.java | 5 - .../SkylarkSemanticsConsistencyTest.java | 2 - ...kylarkRuleImplementationFunctionsTest.java | 109 +----------------- 6 files changed, 23 insertions(+), 211 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java index 8e924d95764947..74ecff1dbd9ad6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/Args.java @@ -281,48 +281,26 @@ public CommandLineArgsApi addArgument( commandLine.add(argName); } if (value instanceof SkylarkNestedSet || value instanceof SkylarkList) { - if (starlarkSemantics.incompatibleDisallowOldStyleArgsAdd()) { - throw new EvalException( - loc, - "Args#add no longer accepts vectorized arguments when " - + "--incompatible_disallow_old_style_args_add is set. " - + "Please use Args#add_all or Args#add_joined."); - } - addVectorArg( - value, - /* argName= */ null, - mapFn != Runtime.NONE ? (BaseFunction) mapFn : null, - /* mapEach= */ null, - format != Runtime.NONE ? (String) format : null, - beforeEach != Runtime.NONE ? (String) beforeEach : null, - joinWith != Runtime.NONE ? (String) joinWith : null, - /* formatJoined= */ null, - /* omitIfEmpty= */ false, - /* uniquify= */ false, - /* expandDirectories= */ true, - /* terminateWith= */ null, - loc); - - } else { - if (mapFn != Runtime.NONE && starlarkSemantics.incompatibleDisallowOldStyleArgsAdd()) { - throw new EvalException( - loc, - "Args#add no longer accepts map_fn when" - + "--incompatible_disallow_old_style_args_add is set. " - + "Please eagerly map the value."); - } - if (beforeEach != Runtime.NONE) { - throw new EvalException(null, "'before_each' is not supported for scalar arguments"); - } - if (joinWith != Runtime.NONE) { - throw new EvalException(null, "'join_with' is not supported for scalar arguments"); - } - addScalarArg( - value, - format != Runtime.NONE ? (String) format : null, - mapFn != Runtime.NONE ? (BaseFunction) mapFn : null, - loc); + throw new EvalException( + loc, + "Args#add doesn't accept vectorized arguments. " + + "Please use Args#add_all or Args#add_joined."); + } + if (mapFn != Runtime.NONE) { + throw new EvalException( + loc, "Args#add doesn't accept map_fn. Please eagerly map the value."); } + if (beforeEach != Runtime.NONE) { + throw new EvalException(null, "'before_each' is not supported for scalar arguments"); + } + if (joinWith != Runtime.NONE) { + throw new EvalException(null, "'join_with' is not supported for scalar arguments"); + } + addScalarArg( + value, + format != Runtime.NONE ? (String) format : null, + mapFn != Runtime.NONE ? (BaseFunction) mapFn : null, + loc); return this; } @@ -500,7 +478,6 @@ private void validateMapEach(@Nullable BaseFunction mapEach, Location loc) private void validateFormatString(String argumentName, @Nullable String formatStr, Location loc) throws EvalException { if (formatStr != null - && starlarkSemantics.incompatibleDisallowOldStyleArgsAdd() && !SingleStringArgFormatter.isValid(formatStr)) { throw new EvalException( loc, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java index 03d961f2acd1a4..aeb9befdd9093e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java @@ -40,7 +40,6 @@ import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Mutability; -import com.google.devtools.build.lib.syntax.Printer; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkList; import com.google.devtools.build.lib.syntax.StarlarkSemantics; @@ -267,11 +266,10 @@ private int eval( } if ((features & HAS_FORMAT_EACH) != 0) { String formatStr = (String) arguments.get(argi++); - Formatter formatter = Formatter.get(location, starlarkSemantics); try { int count = stringValues.size(); for (int i = 0; i < count; ++i) { - stringValues.set(i, formatter.format(formatStr, stringValues.get(i))); + stringValues.set(i, SingleStringArgFormatter.format(formatStr, stringValues.get(i))); } } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); @@ -291,9 +289,8 @@ private int eval( if (!isEmptyAndShouldOmit) { String result = Joiner.on(joinWith).join(stringValues); if (formatJoined != null) { - Formatter formatter = Formatter.get(location, starlarkSemantics); try { - result = formatter.format(formatJoined, result); + result = SingleStringArgFormatter.format(formatJoined, result); } catch (IllegalFormatException e) { throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); } @@ -625,8 +622,7 @@ private int eval( String stringValue = CommandLineItem.expandToCommandLine(object); if (hasFormat) { String formatStr = (String) arguments.get(argi++); - Formatter formatter = Formatter.get(location, starlarkSemantics); - stringValue = formatter.format(formatStr, stringValue); + stringValue = SingleStringArgFormatter.format(formatStr, stringValue); } builder.add(stringValue); return argi; @@ -790,38 +786,6 @@ public void addToFingerprint(ActionKeyContext actionKeyContext, Fingerprint fing } } - private interface Formatter { - String format(String formatStr, String subject) throws CommandLineExpansionException; - - static Formatter get(Location location, StarlarkSemantics starlarkSemantics) { - return starlarkSemantics.incompatibleDisallowOldStyleArgsAdd() - ? SingleStringArgFormatter::format - : new LegacyFormatter(location); - } - } - - private static class LegacyFormatter implements Formatter { - @Nullable private final Location location; - private final ArrayList args; - - public LegacyFormatter(Location location) { - this.location = location; - this.args = new ArrayList<>(1); // Reused arg list to reduce GC - this.args.add(null); - } - - @Override - public String format(String formatStr, String subject) throws CommandLineExpansionException { - try { - args.set(0, subject); - SkylarkPrinter printer = Printer.getPrinter(); - return printer.formatWithList(formatStr, args).toString(); - } catch (IllegalFormatException e) { - throw new CommandLineExpansionException(errorMessage(e.getMessage(), location, null)); - } - } - } - private static Object applyMapFn( BaseFunction mapFn, Object arg, Location location, StarlarkSemantics starlarkSemantics) throws CommandLineExpansionException { 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 452cd2032579e1..ead4ea6dee1bc8 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 @@ -387,20 +387,6 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl + "to the rule definition, rather than the rule usage.") public boolean incompatibleVisibilityPrivateAttributesAtDefinition; - /** Controls legacy arguments to ctx.actions.Args#add. */ - @Option( - name = "incompatible_disallow_old_style_args_add", - defaultValue = "true", - category = "incompatible changes", - documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, - effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, - metadataTags = { - OptionMetadataTag.INCOMPATIBLE_CHANGE, - OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES - }, - help = "If set to true, vectorized calls to Args#add are disallowed.") - public boolean incompatibleDisallowOldStyleArgsAdd; - @Option( name = "incompatible_disallow_unverified_http_downloads", defaultValue = "true", @@ -645,7 +631,6 @@ public StarlarkSemantics toSkylarkSemantics() { .incompatibleDisableDeprecatedAttrParams(incompatibleDisableDeprecatedAttrParams) .incompatibleDisableDepsetItems(incompatibleDisableDepsetItems) .incompatibleDisallowEmptyGlob(incompatibleDisallowEmptyGlob) - .incompatibleDisallowOldStyleArgsAdd(incompatibleDisallowOldStyleArgsAdd) .incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax) .incompatibleDisallowUnverifiedHttpDownloads( incompatibleDisallowUnverifiedHttpDownloads) 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 3f0dac9c61ff41..dec278e0ea2db0 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 @@ -166,8 +166,6 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleDisallowEmptyGlob(); - public abstract boolean incompatibleDisallowOldStyleArgsAdd(); - public abstract boolean incompatibleDisallowStructProviderSyntax(); public abstract boolean incompatibleDisallowUnverifiedHttpDownloads(); @@ -263,7 +261,6 @@ public static Builder builderWithDefaults() { .incompatibleDisableDeprecatedAttrParams(true) .incompatibleDisableDepsetItems(false) .incompatibleDisallowEmptyGlob(false) - .incompatibleDisallowOldStyleArgsAdd(true) .incompatibleDisallowStructProviderSyntax(false) .incompatibleDisallowUnverifiedHttpDownloads(true) .incompatibleNewActionsApi(true) @@ -331,8 +328,6 @@ public abstract static class Builder { public abstract Builder incompatibleDisallowEmptyGlob(boolean value); - public abstract Builder incompatibleDisallowOldStyleArgsAdd(boolean value); - public abstract Builder incompatibleDisallowStructProviderSyntax(boolean value); public abstract Builder incompatibleDisallowUnverifiedHttpDownloads(boolean 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 70a51d320873b5..70126bcdbb8afe 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 @@ -142,7 +142,6 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_disable_depset_items=" + rand.nextBoolean(), "--incompatible_disable_third_party_license_checking=" + rand.nextBoolean(), "--incompatible_disallow_empty_glob=" + rand.nextBoolean(), - "--incompatible_disallow_old_style_args_add=" + rand.nextBoolean(), "--incompatible_disallow_struct_provider_syntax=" + rand.nextBoolean(), "--incompatible_disallow_unverified_http_downloads=" + rand.nextBoolean(), "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(), @@ -193,7 +192,6 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleDisableDepsetItems(rand.nextBoolean()) .incompatibleDisableThirdPartyLicenseChecking(rand.nextBoolean()) .incompatibleDisallowEmptyGlob(rand.nextBoolean()) - .incompatibleDisallowOldStyleArgsAdd(rand.nextBoolean()) .incompatibleDisallowStructProviderSyntax(rand.nextBoolean()) .incompatibleDisallowUnverifiedHttpDownloads(rand.nextBoolean()) .incompatibleDoNotSplitLinkingCmdline(rand.nextBoolean()) 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 ea382849a5b758..04b7b45e244499 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 @@ -2033,10 +2033,9 @@ public void testArgsScalarAdd() throws Exception { @Test public void testArgsScalarAddThrowsWithVectorArg() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=true"); setRuleContext(createRuleContext("//foo:foo")); checkEvalErrorContains( - "Args#add no longer accepts vectorized", + "Args#add doesn't accept vectorized", "args = ruleContext.actions.args()", "args.add([1, 2])", "ruleContext.actions.run(", @@ -2245,87 +2244,6 @@ public void testArgsAddJoined() throws Exception { .inOrder(); } - @Test - public void testLazyArgsLegacy() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=false"); - SkylarkRuleContext ruleContext = createRuleContext("//foo:foo"); - setRuleContext(ruleContext); - exec( - "def map_scalar(val): return 'mapped' + val", - "def map_vector(vals): return [x + 1 for x in vals]", - "args = ruleContext.actions.args()", - "args.add('--foo')", - "args.add('foo', format='format%s')", - "args.add('foo', map_fn=map_scalar)", - "args.add([1, 2])", - "args.add([1, 2], join_with=':')", - "args.add([1, 2], before_each='-before')", - "args.add([1, 2], format='format/%s')", - "args.add([1, 2], map_fn=map_vector)", - "args.add([1, 2], format='format/%s', join_with=':')", - "args.add(ruleContext.files.srcs)", - "args.add(ruleContext.files.srcs, format='format/%s')", - "ruleContext.actions.run(", - " inputs = depset(ruleContext.files.srcs),", - " outputs = ruleContext.files.srcs,", - " arguments = [args],", - " executable = ruleContext.files.tools[0],", - ")"); - SpawnAction action = - (SpawnAction) - Iterables.getOnlyElement( - ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); - assertThat(action.getArguments()) - .containsExactly( - "foo/t.exe", - "--foo", - "formatfoo", - "mappedfoo", - "1", - "2", - "1:2", - "-before", - "1", - "-before", - "2", - "format/1", - "format/2", - "2", - "3", - "format/1:format/2", - "foo/a.txt", - "foo/b.img", - "format/foo/a.txt", - "format/foo/b.img") - .inOrder(); - } - - @Test - public void testLegacyLazyArgMapFnReturnsWrongArgumentCount() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=false"); - SkylarkRuleContext ruleContext = createRuleContext("//foo:foo"); - setRuleContext(ruleContext); - exec( - "args = ruleContext.actions.args()", - "def bad_fn(args): return [0]", - "args.add([1, 2], map_fn=bad_fn)", - "ruleContext.actions.run(", - " inputs = depset(ruleContext.files.srcs),", - " outputs = ruleContext.files.srcs,", - " arguments = [args],", - " executable = ruleContext.files.tools[0],", - ")"); - SpawnAction action = - (SpawnAction) - Iterables.getOnlyElement( - ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); - CommandLineExpansionException e = - assertThrows(CommandLineExpansionException.class, () -> action.getArguments()); - assertThat(e) - .hasMessageThat() - .contains("map_fn must return a list of the same length as the input"); - } - @Test public void testMultipleLazyArgsMixedWithStrings() throws Exception { SkylarkRuleContext ruleContext = createRuleContext("//foo:foo"); @@ -2433,7 +2351,6 @@ public void testScalarBeforeEachErrorMessage() throws Exception { @Test public void testArgsAddInvalidTypesForArgAndValues() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=true"); setRuleContext(createRuleContext("//foo:foo")); checkEvalErrorContains( "expected value of type 'string' for arg name, got 'Integer'", @@ -2453,32 +2370,8 @@ public void testArgsAddInvalidTypesForArgAndValues() throws Exception { "args.add_all('--foo', 1)"); } - @Test - public void testLazyArgIllegalLegacyFormatString() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=false"); - SkylarkRuleContext ruleContext = createRuleContext("//foo:foo"); - setRuleContext(ruleContext); - exec( - "args = ruleContext.actions.args()", - "args.add('foo', format='format/%s%s')", // Expects two args, will only be given one - "ruleContext.actions.run(", - " inputs = depset(ruleContext.files.srcs),", - " outputs = ruleContext.files.srcs,", - " arguments = [args],", - " executable = ruleContext.files.tools[0],", - ")"); - SpawnAction action = - (SpawnAction) - Iterables.getOnlyElement( - ruleContext.getRuleContext().getAnalysisEnvironment().getRegisteredActions()); - CommandLineExpansionException e = - assertThrows(CommandLineExpansionException.class, () -> action.getArguments()); - assertThat(e.getMessage()).contains("not enough arguments"); - } - @Test public void testLazyArgIllegalFormatString() throws Exception { - setSkylarkSemanticsOptions("--incompatible_disallow_old_style_args_add=true"); setRuleContext(createRuleContext("//foo:foo")); checkEvalErrorContains( "Invalid value for parameter \"format\": Expected string with a single \"%s\"",