Add arguments parameter to RunEnvironmentInfo

Executable Starlark rules can use the new `arguments` parameter on
`RunEnvironmentInfo` to specify the arguments that Bazel should pass
on the command line with `test` or `run`.

If set to a non-`None` value, this parameter overrides the value of
the `args` attribute that is implicitly defined for all rules. This
allows Starlark rules to implement their own version of this attribute
which isn't bound to its proprietary processing (data label expansion
and tokenization).

Along the way, this commit adds test coverage and documentation for the
interplay between `RunEnvironmentInfo`'s `environment` and `--test_env`.

The value of the `arguments` field of `RunEnvironmentInfo` is
intentionally not exposed to Starlark yet: It is not clear how these
arguments should be represented and whether rules relying on the magic
`args` attribute should also provide this field.

Fixes bazelbuild#16076
Work towards bazelbuild#12313

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -1025,6 +1025,7 @@ java_library(
     name = "run_environment_info",
     srcs = ["RunEnvironmentInfo.java"],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions",
         "//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/starlarkbuildapi",

src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java

@@ -474,6 +474,7 @@ private TestProvider initializeTestProvider(FilesToRunProvider filesToRunProvide
     if (environmentProvider != null) {
       testActionBuilder.addExtraEnv(environmentProvider.getEnvironment());
       testActionBuilder.addExtraInheritedEnv(environmentProvider.getInheritedEnvironment());
+      testActionBuilder.setStarlarkTargetArgs(environmentProvider.getArguments());
     }
 
     TestParams testParams =

src/main/java/com/google/devtools/build/lib/analysis/RunEnvironmentInfo.java

@@ -17,15 +17,20 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.BuiltinProvider;
 import com.google.devtools.build.lib.packages.NativeInfo;
 import com.google.devtools.build.lib.starlarkbuildapi.RunEnvironmentInfoApi;
+
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Sequence;
+import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkList;
 
 /**
@@ -40,16 +45,19 @@ public final class RunEnvironmentInfo extends NativeInfo implements RunEnvironme
 
   private final ImmutableMap<String, String> environment;
   private final ImmutableList<String> inheritedEnvironment;
+  @Nullable private final CommandLine arguments;
   private final boolean shouldErrorOnNonExecutableRule;
 
   /** Constructs a new provider with the given fixed and inherited environment variables. */
   public RunEnvironmentInfo(
       Map<String, String> environment,
       List<String> inheritedEnvironment,
+      @Nullable CommandLine arguments,
       boolean shouldErrorOnNonExecutableRule) {
     this.environment = ImmutableMap.copyOf(Preconditions.checkNotNull(environment));
     this.inheritedEnvironment =
         ImmutableList.copyOf(Preconditions.checkNotNull(inheritedEnvironment));
+    this.arguments = arguments;
     this.shouldErrorOnNonExecutableRule = shouldErrorOnNonExecutableRule;
   }
 
@@ -76,6 +84,11 @@ public ImmutableList<String> getInheritedEnvironment() {
     return inheritedEnvironment;
   }
 
+  @Nullable
+  public CommandLine getArguments() {
+    return arguments;
+  }
+
   /**
    * Returns whether advertising this provider on a non-executable (and thus non-test) rule should
    * result in an error or a warning. The latter is required to not break testing.TestEnvironment,
@@ -95,11 +108,12 @@ private RunEnvironmentInfoProvider() {
 
     @Override
     public RunEnvironmentInfoApi constructor(
-        Dict<?, ?> environment, Sequence<?> inheritedEnvironment) throws EvalException {
+        Dict<?, ?> environment, Sequence<?> inheritedEnvironment, Object arguments) throws EvalException {
       return new RunEnvironmentInfo(
           Dict.cast(environment, String.class, String.class, "environment"),
           StarlarkList.immutableCopyOf(
               Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")),
+          arguments == Starlark.NONE ? null : CommandLine.of(Sequence.cast(arguments, String.class, "arguments")),
           /* shouldErrorOnNonExecutableRule= */ true);
     }
   }

src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java

@@ -420,6 +420,20 @@ public static RunfilesSupport withExecutable(
         computeActionEnvironment(ruleContext));
   }
 
+  /**
+   * Creates and returns a {@link RunfilesSupport} object for the given rule and executable without
+   * computing arguments based on the value of the "args" attribute.
+   */
+  public static RunfilesSupport withExecutableNoArgs(
+      RuleContext ruleContext, Runfiles runfiles, Artifact executable) {
+    return RunfilesSupport.create(
+        ruleContext,
+        executable,
+        runfiles,
+        CommandLine.EMPTY,
+        computeActionEnvironment(ruleContext));
+  }
+
   /**
    * Creates and returns a {@link RunfilesSupport} object for the given rule and executable. Note
    * that this method calls back into the passed in rule to obtain the runfiles.

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleConfiguredTargetUtil.java

@@ -343,30 +343,40 @@ private static void addProviders(
       }
     }
 
+    DefaultInfo defaultInfo = null;
     boolean defaultProviderProvidedExplicitly = false;
+    boolean starlarkArgsProvided = false;
 
     for (Info declaredProvider : declaredProviders.values()) {
       if (getProviderKey(declaredProvider).equals(DefaultInfo.PROVIDER.getKey())) {
-        parseDefaultProviderFields((DefaultInfo) declaredProvider, context, builder);
+        defaultInfo = (DefaultInfo) declaredProvider;
         defaultProviderProvidedExplicitly = true;
-      } else if (getProviderKey(declaredProvider).equals(RunEnvironmentInfo.PROVIDER.getKey())
-          && !(context.isExecutable() || context.getRuleContext().isTestTarget())) {
-        String message =
-            "Returning RunEnvironmentInfo from a non-executable, non-test target has no effect";
-        RunEnvironmentInfo runEnvironmentInfo = (RunEnvironmentInfo) declaredProvider;
-        if (runEnvironmentInfo.shouldErrorOnNonExecutableRule()) {
-          context.getRuleContext().ruleError(message);
-        } else {
-          context.getRuleContext().ruleWarning(message);
-          builder.addStarlarkDeclaredProvider(declaredProvider);
+      } else if (getProviderKey(declaredProvider).equals(RunEnvironmentInfo.PROVIDER.getKey())) {
+        if (((RunEnvironmentInfo) declaredProvider).getArguments() != null) {
+          // Ensure that RunfilesSupport does not parse the "args" attribute, the rule handles it in
+          // a custom way.
+          starlarkArgsProvided = true;
+        }
+        if (!(context.isExecutable() || context.getRuleContext().isTestTarget())) {
+          String message =
+              "Returning RunEnvironmentInfo from a non-executable, non-test target has no effect";
+          RunEnvironmentInfo runEnvironmentInfo = (RunEnvironmentInfo) declaredProvider;
+          if (runEnvironmentInfo.shouldErrorOnNonExecutableRule()) {
+            context.getRuleContext().ruleError(message);
+          } else {
+            context.getRuleContext().ruleWarning(message);
+          }
         }
+        builder.addStarlarkDeclaredProvider(declaredProvider);
       } else {
         builder.addStarlarkDeclaredProvider(declaredProvider);
       }
     }
 
-    if (!defaultProviderProvidedExplicitly) {
-      parseDefaultProviderFields(oldStyleProviders, context, builder);
+    if (defaultProviderProvidedExplicitly) {
+      parseDefaultProviderFields(defaultInfo, context, builder, starlarkArgsProvided);
+    } else {
+      parseDefaultProviderFields(oldStyleProviders, context, builder, starlarkArgsProvided);
     }
 
     for (String field : oldStyleProviders.getFieldNames()) {
@@ -491,7 +501,10 @@ private static Provider.Key getProviderKey(Info info) throws EvalException {
    * throws an {@link EvalException} if there are unknown fields.
    */
   private static void parseDefaultProviderFields(
-      StructImpl info, StarlarkRuleContext context, RuleConfiguredTargetBuilder builder)
+      StructImpl info,
+      StarlarkRuleContext context,
+      RuleConfiguredTargetBuilder builder,
+      boolean starlarkArgsProvided)
       throws EvalException {
     Depset files = null;
     Runfiles statelessRunfiles = null;
@@ -591,7 +604,8 @@ private static void parseDefaultProviderFields(
         files,
         statelessRunfiles,
         dataRunfiles,
-        defaultRunfiles);
+        defaultRunfiles,
+        starlarkArgsProvided);
   }
 
   private static void addSimpleProviders(
@@ -601,7 +615,8 @@ private static void addSimpleProviders(
       @Nullable Depset files,
       Runfiles statelessRunfiles,
       Runfiles dataRunfiles,
-      Runfiles defaultRunfiles)
+      Runfiles defaultRunfiles,
+      boolean starlarkArgsProvided)
       throws EvalException {
 
     // TODO(bazel-team) if both 'files' and 'executable' are provided, 'files' overrides
@@ -643,8 +658,13 @@ private static void addSimpleProviders(
       RunfilesSupport runfilesSupport = null;
       if (!computedDefaultRunfiles.isEmpty()) {
         Preconditions.checkNotNull(executable, "executable must not be null");
-        runfilesSupport =
-            RunfilesSupport.withExecutable(ruleContext, computedDefaultRunfiles, executable);
+        if (starlarkArgsProvided) {
+          runfilesSupport = RunfilesSupport.withExecutableNoArgs(
+              ruleContext, computedDefaultRunfiles, executable);
+        } else {
+          runfilesSupport =
+              RunfilesSupport.withExecutable(ruleContext, computedDefaultRunfiles, executable);
+        }
         assertExecutableSymlinkPresent(runfilesSupport.getRunfiles(), executable);
       }
       builder.setRunfilesSupport(runfilesSupport, executable);

src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java

@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.actions.RunfilesSupplier;
 import com.google.devtools.build.lib.analysis.Allowlist;
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
@@ -98,12 +99,14 @@ public NestedSet<PackageGroupContents> getPackageSpecifications() {
   private int explicitShardCount;
   private final Map<String, String> extraEnv;
   private final Set<String> extraInheritedEnv;
+  @Nullable private CommandLine starlarkTargetArgs;
 
   public TestActionBuilder(RuleContext ruleContext) {
     this.ruleContext = ruleContext;
     this.extraEnv = new TreeMap<>();
     this.extraInheritedEnv = new TreeSet<>();
     this.additionalTools = new ImmutableList.Builder<>();
+    this.starlarkTargetArgs = null;
   }
 
   /**
@@ -180,6 +183,12 @@ public TestActionBuilder addExtraInheritedEnv(List<String> extraInheritedEnv) {
     return this;
   }
 
+  @CanIgnoreReturnValue
+  public TestActionBuilder setStarlarkTargetArgs(@Nullable CommandLine starlarkTargetArgs) {
+    this.starlarkTargetArgs = starlarkTargetArgs;
+    return this;
+  }
+
   /** Set the explicit shard count. Note that this may be overridden by the sharding strategy. */
   @CanIgnoreReturnValue
   public TestActionBuilder setShardCount(int explicitShardCount) {
@@ -340,6 +349,7 @@ private TestParams createTestAction(int shards)
               ruleContext,
               runfilesSupport,
               executable,
+              starlarkTargetArgs,
               instrumentedFileManifest,
               shards,
               runsPerTest);
@@ -352,7 +362,13 @@ private TestParams createTestAction(int shards)
     } else {
       executionSettings =
           new TestTargetExecutionSettings(
-              ruleContext, runfilesSupport, executable, null, shards, runsPerTest);
+              ruleContext,
+              runfilesSupport,
+              executable,
+              starlarkTargetArgs,
+              null,
+              shards,
+              runsPerTest);
     }
 
     extraTestEnv.putAll(extraEnv);

src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java

@@ -53,6 +53,7 @@ public final class TestTargetExecutionSettings {
       RuleContext ruleContext,
       RunfilesSupport runfilesSupport,
       Artifact executable,
+      @Nullable CommandLine starlarkTargetArgs,
       Artifact instrumentedFileManifest,
       int shards,
       int runs)
@@ -62,7 +63,8 @@ public final class TestTargetExecutionSettings {
     BuildConfigurationValue config = ruleContext.getConfiguration();
     TestConfiguration testConfig = config.getFragment(TestConfiguration.class);
 
-    CommandLine targetArgs = runfilesSupport.getArgs();
+    CommandLine targetArgs =
+        starlarkTargetArgs != null ? starlarkTargetArgs : runfilesSupport.getArgs();
     testArguments =
         CommandLine.concat(targetArgs, ImmutableList.copyOf(testConfig.getTestArguments()));
 

src/main/java/com/google/devtools/build/lib/rules/test/StarlarkTestingModule.java

@@ -49,6 +49,7 @@ public RunEnvironmentInfo testEnvironment(
         Dict.cast(environment, String.class, String.class, "environment"),
         StarlarkList.immutableCopyOf(
             Sequence.cast(inheritedEnvironment, String.class, "inherited_environment")),
+        /* arguments= */null,
         /* shouldErrorOnNonExecutableRule= */ false);
   }
 

src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java

@@ -176,19 +176,28 @@ protected BuildResult processRequest(final CommandEnvironment env, BuildRequest
   public void editOptions(OptionsParser optionsParser) { }
 
   /**
-   * Compute the arguments the binary should be run with by concatenating the arguments in its
-   * {@code args} attribute and the arguments on the Blaze command line.
+   * Compute the arguments the binary should be run with by concatenating the arguments provided by
+   * its {@link RunEnvironmentInfo} or, if not set, in its {@code args} attribute, and the arguments
+   * on the Blaze command line.
    */
-  @Nullable
   private List<String> computeArgs(ConfiguredTarget targetToRun, List<String> commandLineArgs)
       throws InterruptedException, CommandLineExpansionException {
     List<String> args = Lists.newArrayList();
 
-    FilesToRunProvider provider = targetToRun.getProvider(FilesToRunProvider.class);
-    RunfilesSupport runfilesSupport = provider == null ? null : provider.getRunfilesSupport();
-    if (runfilesSupport != null && runfilesSupport.getArgs() != null) {
-      CommandLine targetArgs = runfilesSupport.getArgs();
-      Iterables.addAll(args, targetArgs.arguments());
+    // If a Starlark rule explicitly sets arguments on RunEnvironmentInfo, these arguments take
+    // precedence over those obtained from the implicit "args" attribute on all rules so that
+    // Starlark rules can implement their own logic for this attribute.
+    RunEnvironmentInfo runEnvironmentInfo =
+        (RunEnvironmentInfo) targetToRun.get(RunEnvironmentInfo.PROVIDER.getKey());
+    if (runEnvironmentInfo != null && runEnvironmentInfo.getArguments() != null) {
+      Iterables.addAll(args, runEnvironmentInfo.getArguments().arguments());
+    } else {
+      FilesToRunProvider provider = targetToRun.getProvider(FilesToRunProvider.class);
+      RunfilesSupport runfilesSupport = provider == null ? null : provider.getRunfilesSupport();
+      if (runfilesSupport != null && runfilesSupport.getArgs() != null) {
+        CommandLine targetArgs = runfilesSupport.getArgs();
+        Iterables.addAll(args, targetArgs.arguments());
+      }
     }
     args.addAll(commandLineArgs);
     return args;

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/RunEnvironmentInfoApi.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.starlarkbuildapi;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.docgen.annot.StarlarkConstructor;
 import com.google.devtools.build.lib.starlarkbuildapi.core.ProviderApi;
@@ -26,11 +27,12 @@
 import net.starlark.java.annot.StarlarkMethod;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.NoneType;
 import net.starlark.java.eval.Sequence;
 
 /**
- * Provider containing any additional environment variables for use when running executables, either
- * in test actions or when executed via the run command.
+ * Provider containing any additional environment variables and arguments for use when running
+ * executables, either in test actions or when executed via the run command.
  */
 @StarlarkBuiltin(
     name = "RunEnvironmentInfo",
@@ -78,7 +80,9 @@ interface RunEnvironmentInfoApiProvider extends ProviderApi {
               doc =
                   "A map of string keys and values that represent environment variables and their"
                       + " values. These will be made available when the target that returns this"
-                      + " provider is executed, either as a test or via the run command."),
+                      + " provider is executed, either as a test or via the run command. In the"
+                      + " case of a test, environment variables specified on the command line via"
+                      + " <code>--test_env</code> take precedence over values specified here."),
           @Param(
               name = "inherited_environment",
               allowedTypes = {@ParamType(type = Sequence.class, generic1 = String.class)},
@@ -91,13 +95,29 @@ interface RunEnvironmentInfoApiProvider extends ProviderApi {
                       + " when the target that returns this provider is executed, either as a"
                       + " test or via the run command. If a variable is contained in both <code>"
                       + "environment</code> and <code>inherited_environment</code>, the value"
-                      + " inherited from the shell environment will take precedence if set.")
+                      + " inherited from the shell environment will take precedence if set."),
+          @Param(
+              name = "arguments",
+              allowedTypes = {
+                  @ParamType(type = Sequence.class, generic1 = String.class),
+                  @ParamType(type = NoneType.class)
+              },
+              defaultValue = "None",
+              named = true,
+              positional = false,
+              doc = "A list of arguments. If set to a value other than <code>None</code>, this"
+                        + " list will be appended to the command line when the target that returns"
+                        + " this provider is executed, either as a test or via the run command. In"
+                        + " this case, the arguments specified in the <code>args</code> attribute"
+                        + " implicitly defined for all rules are <strong>not</code> appended"
+                        + " automatically.")
         },
         selfCall = true)
     @StarlarkConstructor
     RunEnvironmentInfoApi constructor(
         Dict<?, ?> environment, // <String, String> expected
-        Sequence<?> inheritedEnvironment /* <String> expected */)
+        Sequence<?> inheritedEnvironment, /* <String> expected */
+        Object arguments /* Sequence<String> or NoneType expected */)
         throws EvalException;
   }
 }