Accept args object in create_proto_compile_action.

This is a better design than type-checking list elements.

Only minor reordering of arguments is happening - additional parameters are added before `--proto_path`. Additional parameters are usually outputs related (--descriptor_set_out, --java_out).

Callee can decide if additional parameters are put into a params file or not. Native calles don't at the moment.
Alternative implementation would reuse additional_parameters, appending to it and creating a single params file.

Not using params file for additional parameters and using it for the standard ones seems like the best choice for the sake of caching (params file matches for proto_library and lang_proto_libraries).

PiperOrigin-RevId: 427112289

src/main/java/com/google/devtools/build/lib/rules/proto/BUILD

@@ -25,10 +25,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/docgen/annot",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
-        "//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
         "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity",
-        "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line",
-        "//src/main/java/com/google/devtools/build/lib/analysis:actions/symlink_action",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters",
@@ -37,18 +34,16 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
+        "//src/main/java/com/google/devtools/build/lib/analysis:starlark/args",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/packages",
-        "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
-        "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/proto",
-        "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/vfs",

src/main/java/com/google/devtools/build/lib/rules/proto/ProtoCompileActionBuilder.java

@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.actions.ResourceSetOrBuilder;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.starlark.Args;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.collect.nestedset.Depset.ElementType;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -34,6 +35,8 @@
 import java.util.HashSet;
 import java.util.List;
 import net.starlark.java.eval.Dict;
+import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkCallable;
 import net.starlark.java.eval.StarlarkFloat;
 import net.starlark.java.eval.StarlarkFunction;
@@ -138,24 +141,57 @@ public void maybeRegister(RuleContext ruleContext)
       return;
     }
 
-    ImmutableList.Builder<Object> additionalArgs = ImmutableList.builder();
-
-    if (langPlugin != null && langPlugin.getExecutable() != null) {
-      // We pass a separate langPlugin as there are plugins that cannot be overridden
-      // and thus we have to deal with "$xx_plugin" and "xx_plugin".
-      additionalArgs.add(Tuple.of(langPlugin.getExecutable(), langPluginFormat));
-    }
+    ruleContext.initStarlarkRuleContext();
+    StarlarkThread thread = ruleContext.getStarlarkThread();
+    Args additionalArgs = Args.newArgs(thread.mutability(), thread.getSemantics());
+
+    try {
+      if (langPlugin != null && langPlugin.getExecutable() != null) {
+        // We pass a separate langPlugin as there are plugins that cannot be overridden
+        // and thus we have to deal with "$xx_plugin" and "xx_plugin".
+        additionalArgs.addArgument(
+            langPlugin.getExecutable(), /*value=*/ Starlark.UNBOUND, langPluginFormat, thread);
+      }
 
-    if (langPluginParameter != null) {
-      additionalArgs.add(Tuple.of(langPluginParameter, langPluginParameterFormat));
-    }
+      if (langPluginParameter != null) {
+        additionalArgs.addJoined(
+            StarlarkList.immutableCopyOf(langPluginParameter),
+            /*values=*/ Starlark.UNBOUND,
+            /*joinWith=*/ "",
+            /*mapEach=*/ Starlark.NONE,
+            /*formatEach=*/ Starlark.NONE,
+            /*formatJoined=*/ langPluginParameterFormat,
+            /*omitIfEmpty=*/ true,
+            /*uniquify=*/ false,
+            /*expandDirectories=*/ true,
+            /*allowClosure=*/ false,
+            thread);
+      }
 
-    if (!hasServices) {
-      additionalArgs.add("--disallow_services");
-    }
+      if (!hasServices) {
+        additionalArgs.addArgument(
+            "--disallow_services",
+            /* value = */ Starlark.UNBOUND,
+            /* format = */ Starlark.NONE,
+            thread);
+      }
 
-    if (additionalCommandLineArguments != null) {
-      additionalArgs.addAll(additionalCommandLineArguments);
+      if (additionalCommandLineArguments != null) {
+        additionalArgs.addAll(
+            StarlarkList.immutableCopyOf(additionalCommandLineArguments),
+            /*values=*/ Starlark.UNBOUND,
+            /*mapEach=*/ Starlark.NONE,
+            /*formatEach=*/ Starlark.NONE,
+            /*beforeEach=*/ Starlark.NONE,
+            /*omitIfEmpty=*/ true,
+            /*uniquify=*/ false,
+            /*expandDirectories=*/ true,
+            /*terminateWith=*/ Starlark.NONE,
+            /*allowClosure=*/ false,
+            thread);
+      }
+    } catch (EvalException e) {
+      throw ruleContext.throwWithRuleError(e);
     }
 
     ImmutableList.Builder<FilesToRunProvider> plugins = new ImmutableList.Builder<>();
@@ -168,7 +204,7 @@ public void maybeRegister(RuleContext ruleContext)
 
     StarlarkFunction createProtoCompileAction =
         (StarlarkFunction) ruleContext.getStarlarkDefinedBuiltin("create_proto_compile_action");
-    ruleContext.initStarlarkRuleContext();
+
     ruleContext.callStarlarkOrThrowRuleError(
         createProtoCompileAction,
         ImmutableList.of(
@@ -177,7 +213,7 @@ public void maybeRegister(RuleContext ruleContext)
             /* proto_compiler */ protoCompiler,
             /* progress_message */ progressMessage,
             /* outputs */ StarlarkList.immutableCopyOf(outputs),
-            /* additional_args */ StarlarkList.immutableCopyOf(additionalArgs.build()),
+            /* additional_args */ additionalArgs,
             /* plugins */ StarlarkList.immutableCopyOf(plugins.build()),
             /* mnemonic */ mnemonic,
             /* strict_imports */ checkStrictImportPublic,
@@ -246,41 +282,63 @@ public static void registerActions(
       return;
     }
 
+    ruleContext.initStarlarkRuleContext();
+    StarlarkThread thread = ruleContext.getStarlarkThread();
+    Args additionalArgs = Args.newArgs(thread.mutability(), thread.getSemantics());
+
     // A set to check if there are multiple invocations with the same name.
     HashSet<String> invocationNames = new HashSet<>();
-    ImmutableList.Builder<Object> additionalArgs = ImmutableList.builder();
     ImmutableList.Builder<Object> plugins = ImmutableList.builder();
 
-    for (ToolchainInvocation invocation : toolchainInvocations) {
-      if (!invocationNames.add(invocation.name)) {
-        throw new IllegalStateException(
-            "Invocation name "
-                + invocation.name
-                + " appears more than once. "
-                + "This could lead to incorrect proto-compiler behavior");
+    try {
+      for (ToolchainInvocation invocation : toolchainInvocations) {
+        if (!invocationNames.add(invocation.name)) {
+          throw new IllegalStateException(
+              "Invocation name "
+                  + invocation.name
+                  + " appears more than once. "
+                  + "This could lead to incorrect proto-compiler behavior");
+        }
+
+        ProtoLangToolchainProvider toolchain = invocation.toolchain;
+
+        String format = toolchain.outReplacementFormatFlag();
+        additionalArgs.addArgument(
+            invocation.outReplacement, /*value=*/ Starlark.UNBOUND, format, thread);
+
+        if (toolchain.pluginExecutable() != null) {
+          additionalArgs.addArgument(
+              toolchain.pluginExecutable().getExecutable(),
+              /*value=*/ Starlark.UNBOUND,
+              toolchain.pluginFormatFlag(),
+              thread);
+          plugins.add(toolchain.pluginExecutable());
+        }
+
+        additionalArgs.addJoined(
+            StarlarkList.immutableCopyOf(invocation.protocOpts),
+            /*values=*/ Starlark.UNBOUND,
+            /*joinWith=*/ "",
+            /*mapEach=*/ Starlark.NONE,
+            /*formatEach=*/ Starlark.NONE,
+            /*formatJoined=*/ Starlark.NONE,
+            /*omitIfEmpty=*/ true,
+            /*uniquify=*/ false,
+            /*expandDirectories=*/ true,
+            /*allowClosure=*/ false,
+            thread);
       }
 
-      ProtoLangToolchainProvider toolchain = invocation.toolchain;
-
-      String format = toolchain.outReplacementFormatFlag();
-      additionalArgs.add(Tuple.of(invocation.outReplacement, format));
-
-      if (toolchain.pluginExecutable() != null) {
-        additionalArgs.add(
-            Tuple.of(toolchain.pluginExecutable().getExecutable(), toolchain.pluginFormatFlag()));
-        plugins.add(toolchain.pluginExecutable());
+      if (allowServices == Services.DISALLOW) {
+        additionalArgs.addArgument(
+            "--disallow_services", /*value=*/ Starlark.UNBOUND, /*format=*/ Starlark.NONE, thread);
       }
-
-      additionalArgs.addAll(invocation.protocOpts);
-    }
-
-    if (allowServices == Services.DISALLOW) {
-      additionalArgs.add("--disallow_services");
+    } catch (EvalException e) {
+      throw ruleContext.throwWithRuleError(e.getMessageWithStack());
     }
 
     StarlarkFunction createProtoCompileAction =
         (StarlarkFunction) ruleContext.getStarlarkDefinedBuiltin("create_proto_compile_action");
-    ruleContext.initStarlarkRuleContext();
     ruleContext.callStarlarkOrThrowRuleError(
         createProtoCompileAction,
         ImmutableList.of(
@@ -289,7 +347,7 @@ public static void registerActions(
             /* proto_compiler */ protoToolchain.getCompiler(),
             /* progress_message */ progressMessage,
             /* outputs */ StarlarkList.immutableCopyOf(outputs),
-            /* additional_args */ StarlarkList.immutableCopyOf(additionalArgs.build()),
+            /* additional_args */ additionalArgs,
             /* plugins */ StarlarkList.immutableCopyOf(plugins.build())),
         ImmutableMap.of(
             "strict_imports",

src/main/starlark/builtins_bzl/common/proto/proto_common.bzl

@@ -41,8 +41,9 @@ def _create_proto_compile_action(
       progress_message: The progress message to set on the action.
       outputs: The output files generated by the proto compiler.
       additional_args: Additional arguments to add to the action.
-        Accepts a list containing: a string, a pair of value and format string,
-        or a pair of list of strings and a format string.
+        Accepts an ctx.actions.args() object that is added at the beginning of command line.
+        Deprecated: Accepts a list containing: a string, a pair of value and
+        format string, or a pair of list of strings and a format string.
       plugins: Additional plugin executables used by proto compiler.
       mnemonic: The mnemonic to set on the action.
       strict_imports: Whether to check for strict imports.
@@ -58,14 +59,15 @@ def _create_proto_compile_action(
     args.add_all(proto_info.transitive_proto_path, map_each = _proto_path_flag)
     # Example: `--proto_path=--proto_path=bazel-bin/target/third_party/pkg/_virtual_imports/subpkg`
 
-    for arg in additional_args:
-        if type(arg) == type((None, None)):
-            if type(arg[0]) == type([]):
-                args.add_joined(arg[0], join_with = "", format_joined = arg[1])
+    if type(additional_args) == type([]):
+        for arg in additional_args:
+            if type(arg) == type((None, None)):
+                if type(arg[0]) == type([]):
+                    args.add_joined(arg[0], join_with = "", format_joined = arg[1])
+                else:
+                    args.add(arg[0], format = arg[1])
             else:
-                args.add(arg[0], format = arg[1])
-        else:
-            args.add(arg)
+                args.add(arg)
 
     args.add_all(ctx.fragments.proto.protoc_opts())
 
@@ -104,7 +106,7 @@ def _create_proto_compile_action(
         mnemonic = mnemonic,
         progress_message = progress_message,
         executable = proto_compiler,
-        arguments = [args],
+        arguments = [additional_args, args] if type(additional_args) == type(ctx.actions.args()) else [args],
         inputs = depset(transitive = [proto_info.transitive_sources, additional_inputs]),
         outputs = outputs,
         tools = plugins,

src/main/starlark/builtins_bzl/common/proto/proto_library.bzl

@@ -224,10 +224,10 @@ def _write_descriptor_set(ctx, deps, proto_info, descriptor_set):
 
     dependencies_descriptor_sets = depset(transitive = [dep.transitive_descriptor_sets for dep in deps])
 
-    args = []
+    args = ctx.actions.args()
     if ctx.fragments.proto.experimental_proto_descriptorsets_include_source_info():
-        args.append("--include_source_info")
-    args.append((descriptor_set, "--descriptor_set_out=%s"))
+        args.add("--include_source_info")
+    args.add(descriptor_set, format = "--descriptor_set_out=%s")
 
     proto_common.create_proto_compile_action(
         ctx,

src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java

@@ -833,6 +833,16 @@ protected ActionGraph getActionGraph() {
     return skyframeExecutor.getActionGraph(reporter);
   }
 
+  /** Returns all arguments used by the action. */
+  protected final ImmutableList<String> allArgsForAction(SpawnAction action) throws Exception {
+    ImmutableList.Builder<String> args = new ImmutableList.Builder<>();
+    List<CommandLineAndParamFileInfo> commandLines = action.getCommandLines().getCommandLines();
+    for (CommandLineAndParamFileInfo pair : commandLines.subList(1, commandLines.size())) {
+      args.addAll(pair.commandLine.arguments());
+    }
+    return args.build();
+  }
+
   /** Locates the first parameter file used by the action and returns its command line. */
   @Nullable
   protected final CommandLine paramFileCommandLineForAction(Action action) {

src/test/java/com/google/devtools/build/lib/rules/proto/BazelProtoLibraryTest.java

@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
+import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.util.MockProtoSupport;
@@ -1025,7 +1026,8 @@ public void testExperimentalProtoDescriptorSetsIncludeSourceInfo() throws Except
         "    srcs = ['a.proto'],",
         ")");
 
-    Iterable<String> commandLine = paramFileArgsForAction(getDescriptorWriteAction("//x:a_proto"));
+    Iterable<String> commandLine =
+        allArgsForAction((SpawnAction) getDescriptorWriteAction("//x:a_proto"));
     assertThat(commandLine).contains("--include_source_info");
   }