Remove flag incompatible_disallow_old_style_args_add

#5822 RELNOTES: None. PiperOrigin-RevId: 279328336
bazelbuild · Nov 8, 2019 · 40d1f45 · 40d1f45
1 parent dd1e091
commit 40d1f45
Show file tree

Hide file tree

Showing 6 changed files with 23 additions and 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
@@ -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
@@ -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<Object> 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
@@ -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
@@ -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
@@ -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/...st/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/...st/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\"",