Stringify Labels in display form in Args

`Label`s added to `Args` are now formatted in display form, which means
that they will be repo-relative if referring to the main repo and use an
apparent repository name if possible.

Previously, labels were formatted in repo-relative form if referring to
the main repo and using canonical repository names exclusively for
external repos. At the same time, `Args` docs explicitly stated that
the exact stringification of any type other than `File` is unspecified.
The previous behavior was problematic since it neither always used
canonical labels (which could be useful for writing tests that check
these labels against an allowlist) nor provided labels that could be
included in BUILD files (canonical names are explicitly unstable).
Furthermore, whether a label resulted in a string prefixed with a single
or two `@`s already dependended on a user choice, namely the value of
`--enable_bzlmod`.

Thus, this change is not considered to be breaking. It makes it so that
existing rulesets relying on default stringification to return a BUILD-
file compatible label in WORKSPACE continue to do so with Bzlmod with no
code changes.

This change aims to be as memory efficient as possible:
* Single labels or sequences of labels that reference targets in the
  main repo incur no memory overhead.
* Labels referring to external repos as well as `NestedSet`s of labels
  result in an additional Skyframe dependency for each target using the
  command line as well as one additional reference (4 bytes) stored in
  the command line's `arguments`.

Author: fmeum

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

@@ -168,5 +168,8 @@ Artifact.DerivedArtifact getDerivedArtifact(
 
   ActionKeyContext getActionKeyContext();
 
+  /**
+   * Returns and registers a Skyframe dependency on the {@link RepositoryMapping} of the main repo.
+   */
   RepositoryMapping getMainRepoMapping() throws InterruptedException;
 }

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

@@ -411,6 +411,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_value_creation_exception",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:starlark_builtins_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:workspace_status_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe/config",
@@ -2442,6 +2443,7 @@ java_library(
         "//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/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/starlarkbuildapi",

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

@@ -23,6 +23,8 @@
 import com.google.devtools.build.lib.actions.ParamFileInfo;
 import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.collect.nestedset.Depset;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -70,7 +72,8 @@ public void repr(Printer printer) {
   @Override
   public void debugPrint(Printer printer, StarlarkSemantics semantics) {
     try {
-      printer.append(Joiner.on(" ").join(build().arguments()));
+      printer.append(
+          Joiner.on(" ").join(build(/* mainRepoMappingSupplier */ () -> null).arguments()));
     } catch (CommandLineExpansionException e) {
       printer.append("Cannot expand command line: " + e.getMessage());
     } catch (InterruptedException e) {
@@ -101,8 +104,13 @@ public void debugPrint(Printer printer, StarlarkSemantics semantics) {
    */
   public abstract ImmutableSet<Artifact> getDirectoryArtifacts();
 
+  public interface InterruptibleRepoMappingSupplier {
+    RepositoryMapping get() throws InterruptedException;
+  }
+
   /** Returns the command line built by this {@link Args} object. */
-  public abstract CommandLine build();
+  public abstract CommandLine build(InterruptibleRepoMappingSupplier mainRepoMappingSupplier)
+      throws InterruptedException;
 
   /**
    * Returns a frozen {@link Args} representation corresponding to an already-registered action.
@@ -157,7 +165,7 @@ public ImmutableSet<Artifact> getDirectoryArtifacts() {
     }
 
     @Override
-    public CommandLine build() {
+    public CommandLine build(InterruptibleRepoMappingSupplier mainRepoMappingSupplier) {
       return commandLine;
     }
 
@@ -259,6 +267,12 @@ private static class MutableArgs extends Args implements StarlarkValue, Mutabili
      */
     private boolean flagPerLine = false;
 
+    /**
+     * True if the command line needs to stringify any {@link Label}s without an explicit 'map_each'
+     * function.
+     */
+    private boolean mayStringifyExternalLabel = false;
+
     // May be set explicitly once -- if unset defaults to ParameterFileType.SHELL_QUOTED.
     private ParameterFileType parameterFileType = null;
     private String flagFormatString;
@@ -307,6 +321,9 @@ public CommandLineArgsApi addArgument(
             "Args.add() doesn't accept vectorized arguments. Please use Args.add_all() or"
                 + " Args.add_joined() instead.");
       }
+      if (value instanceof Label label && !label.getRepository().isMain()) {
+        mayStringifyExternalLabel = true;
+      }
       addSingleArg(value, format != Starlark.NONE ? (String) format : null);
       return this;
     }
@@ -436,8 +453,12 @@ private void addVectorArg(
       validateFormatString("format_each", formatEach);
       validateFormatString("format_joined", formatJoined);
       StarlarkCustomCommandLine.VectorArg.Builder vectorArg;
-      if (value instanceof Depset) {
-        Depset starlarkNestedSet = (Depset) value;
+      if (value instanceof Depset starlarkNestedSet) {
+        if (mapEach == null && Label.class.equals(starlarkNestedSet.getElementClass())) {
+          // We don't want to eagerly check whether all labels reference targets in the main repo,
+          // so just assume they might not. Nested sets of labels should be rare.
+          mayStringifyExternalLabel = true;
+        }
         NestedSet<?> nestedSet = starlarkNestedSet.getSet();
         if (nestedSet.isEmpty() && omitIfEmpty) {
           return;
@@ -454,6 +475,13 @@ private void addVectorArg(
         if (expandDirectories) {
           scanForDirectories(starlarkList);
         }
+        // Labels referencing targets in the main repo are stringified as //pkg:name and thus
+        // don't require a RepositoryMapping. If a map_each function is provided, default
+        // stringification via Label#toString() is not used.
+        mayStringifyExternalLabel |=
+            (mapEach != null)
+                && starlarkList.stream()
+                    .anyMatch(o -> o instanceof Label label && !label.getRepository().isMain());
         vectorArg = new StarlarkCustomCommandLine.VectorArg.Builder(starlarkList);
       }
       commandLine.recordArgStart();
@@ -573,8 +601,10 @@ private MutableArgs(@Nullable Mutability mutability, StarlarkSemantics starlarkS
     }
 
     @Override
-    public CommandLine build() {
-      return commandLine.build(flagPerLine);
+    public CommandLine build(InterruptibleRepoMappingSupplier mainRepoMappingSupplier)
+        throws InterruptedException {
+      return commandLine.build(
+          flagPerLine, mayStringifyExternalLabel ? mainRepoMappingSupplier.get() : null);
     }
 
     @Override

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

@@ -332,7 +332,8 @@ public void symlink(
   }
 
   @Override
-  public void write(FileApi output, Object content, Boolean isExecutable) throws EvalException {
+  public void write(FileApi output, Object content, Boolean isExecutable)
+      throws EvalException, InterruptedException {
     context.checkMutable("actions.write");
     RuleContext ruleContext = getRuleContext();
 
@@ -347,7 +348,7 @@ public void write(FileApi output, Object content, Boolean isExecutable) throws E
               ruleContext.getActionOwner(),
               NestedSetBuilder.wrap(Order.STABLE_ORDER, args.getDirectoryArtifacts()),
               (Artifact) output,
-              args.build(),
+              args.build(context.getRuleContext().getAnalysisEnvironment()::getMainRepoMapping),
               args.getParameterFileType());
     } else {
       throw new AssertionError("Unexpected type: " + content.getClass().getSimpleName());
@@ -373,7 +374,7 @@ public void run(
       Object shadowedActionUnchecked,
       Object resourceSetUnchecked,
       Object toolchainUnchecked)
-      throws EvalException {
+      throws EvalException, InterruptedException {
     context.checkMutable("actions.run");
     execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked);
     toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked);
@@ -382,7 +383,7 @@ public void run(
     boolean useAutoExecGroups = ruleContext.useAutoExecGroups();
 
     StarlarkAction.Builder builder = new StarlarkAction.Builder();
-    buildCommandLine(builder, arguments);
+    buildCommandLine(builder, arguments, ruleContext.getAnalysisEnvironment()::getMainRepoMapping);
     if (executableUnchecked instanceof Artifact) {
       Artifact executable = (Artifact) executableUnchecked;
       FilesToRunProvider provider = context.getExecutableRunfiles(executable, "executable");
@@ -560,15 +561,15 @@ public void runShell(
       Object shadowedActionUnchecked,
       Object resourceSetUnchecked,
       Object toolchainUnchecked)
-      throws EvalException {
+      throws EvalException, InterruptedException {
     context.checkMutable("actions.run_shell");
     execGroupUnchecked = context.maybeOverrideExecGroup(execGroupUnchecked);
     toolchainUnchecked = context.maybeOverrideToolchain(toolchainUnchecked);
 
     RuleContext ruleContext = getRuleContext();
 
     StarlarkAction.Builder builder = new StarlarkAction.Builder();
-    buildCommandLine(builder, arguments);
+    buildCommandLine(builder, arguments, ruleContext.getAnalysisEnvironment()::getMainRepoMapping);
 
     // When we use a shell command, add an empty argument before other arguments.
     //   e.g.  bash -c "cmd" '' 'arg1' 'arg2'
@@ -631,8 +632,11 @@ public void runShell(
         builder);
   }
 
-  private static void buildCommandLine(SpawnAction.Builder builder, Sequence<?> argumentsList)
-      throws EvalException {
+  private static void buildCommandLine(
+      SpawnAction.Builder builder,
+      Sequence<?> argumentsList,
+      Args.InterruptibleRepoMappingSupplier repoMappingSupplier)
+      throws EvalException, InterruptedException {
     ImmutableList.Builder<String> stringArgs = null;
     for (Object value : argumentsList) {
       if (value instanceof String) {
@@ -647,7 +651,7 @@ private static void buildCommandLine(SpawnAction.Builder builder, Sequence<?> ar
         }
         Args args = (Args) value;
         ParamFileInfo paramFileInfo = args.getParamFileInfo();
-        builder.addCommandLine(args.build(), paramFileInfo);
+        builder.addCommandLine(args.build(repoMappingSupplier), paramFileInfo);
       } else {
         throw Starlark.errorf(
             "expected list of strings or ctx.actions.args() for arguments instead of %s",

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

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
+import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.AbstractCommandLine;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
@@ -36,6 +37,8 @@
 import com.google.devtools.build.lib.actions.PathMapper;
 import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.serialization.VisibleForSerialization;
@@ -182,7 +185,8 @@ private int eval(
         int argi,
         List<String> builder,
         @Nullable ArtifactExpander artifactExpander,
-        PathMapper pathMapper)
+        PathMapper pathMapper,
+        @Nullable RepositoryMapping mainRepoMapping)
         throws CommandLineExpansionException, InterruptedException {
       StarlarkCallable mapEach = null;
       StarlarkSemantics starlarkSemantics = null;
@@ -219,7 +223,7 @@ private int eval(
         int count = expandedValues.size();
         stringValues = new ArrayList<>(expandedValues.size());
         for (int i = 0; i < count; ++i) {
-          stringValues.add(expandToCommandLine(expandedValues.get(i), pathMapper));
+          stringValues.add(expandToCommandLine(expandedValues.get(i), pathMapper, mainRepoMapping));
         }
       }
       // It's safe to uniquify at this stage, any transformations after this
@@ -607,9 +611,15 @@ static void push(List<Object> arguments, Object object, String format) {
       arguments.add(format);
     }
 
-    static int eval(List<Object> arguments, int argi, List<String> builder, PathMapper pathMapper) {
+    static int eval(
+        List<Object> arguments,
+        int argi,
+        List<String> builder,
+        PathMapper pathMapper,
+        @Nullable RepositoryMapping mainRepoMapping) {
       Object object = arguments.get(argi++);
-      String stringValue = StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper);
+      String stringValue =
+          StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper, mainRepoMapping);
       String formatStr = (String) arguments.get(argi++);
       builder.add(SingleStringArgFormatter.format(formatStr, stringValue));
       return argi;
@@ -662,10 +672,13 @@ Builder addFormatted(Object object, String format) {
       return this;
     }
 
-    CommandLine build(boolean flagPerLine) {
+    CommandLine build(boolean flagPerLine, @Nullable RepositoryMapping mainRepoMapping) {
       if (arguments.isEmpty()) {
         return CommandLine.empty();
       }
+      if (mainRepoMapping != null) {
+        arguments.add(mainRepoMapping);
+      }
       Object[] args = arguments.toArray();
       return flagPerLine
           ? new StarlarkCustomCommandLineWithIndexes(args, argStartIndexes.build())
@@ -700,20 +713,39 @@ public Iterable<String> arguments(
     List<String> result = new ArrayList<>();
     List<Object> arguments = rawArgsAsList();
 
-    for (int argi = 0; argi < arguments.size(); ) {
+    RepositoryMapping mainRepoMapping;
+    int size;
+    // Added in #build() if any labels in the command line require this to be formatted with an
+    // apparent repository name.
+    if (arguments.getLast() instanceof RepositoryMapping) {
+      mainRepoMapping = (RepositoryMapping) arguments.getLast();
+      size = arguments.size() - 1;
+    } else {
+      mainRepoMapping = null;
+      size = arguments.size();
+    }
+
+    for (int argi = 0; argi < size; ) {
       Object arg = arguments.get(argi++);
       if (arg instanceof VectorArg) {
-        argi = ((VectorArg) arg).eval(arguments, argi, result, artifactExpander, pathMapper);
+        argi =
+            ((VectorArg) arg)
+                .eval(arguments, argi, result, artifactExpander, pathMapper, mainRepoMapping);
       } else if (arg == SingleFormattedArg.MARKER) {
-        argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper);
+        argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper, mainRepoMapping);
       } else {
-        result.add(expandToCommandLine(arg, pathMapper));
+        result.add(expandToCommandLine(arg, pathMapper, mainRepoMapping));
       }
     }
     return ImmutableList.copyOf(pathMapper.mapCustomStarlarkArgs(result));
   }
 
-  private static String expandToCommandLine(Object object, PathMapper pathMapper) {
+  private static String expandToCommandLine(
+      Object object, PathMapper pathMapper, @Nullable RepositoryMapping mainRepoMapping) {
+    if (mainRepoMapping != null && object instanceof Label label) {
+      return label.getDisplayForm(mainRepoMapping);
+    }
+
     // It'd be nice to build this into DerivedArtifact's CommandLine interface so we don't have
     // to explicitly check if an object is a DerivedArtifact. Unfortunately that would require
     // a lot more dependencies on the Java library DerivedArtifact is built into.
@@ -751,7 +783,17 @@ public Iterable<String> arguments(
       Iterator<Integer> startIndexIterator = argStartIndexes.iterator();
       int nextStartIndex = startIndexIterator.hasNext() ? startIndexIterator.next() : -1;
 
-      for (int argi = 0; argi < arguments.size(); ) {
+      RepositoryMapping mainRepoMapping;
+      int size;
+      if (arguments.getLast() instanceof RepositoryMapping) {
+        mainRepoMapping = (RepositoryMapping) arguments.getLast();
+        size = arguments.size() - 1;
+      } else {
+        mainRepoMapping = null;
+        size = arguments.size();
+      }
+
+      for (int argi = 0; argi < size; ) {
 
         // Record the actual beginning of each group.
         if (argi == nextStartIndex) {
@@ -761,11 +803,14 @@ public Iterable<String> arguments(
 
         Object arg = arguments.get(argi++);
         if (arg instanceof VectorArg) {
-          argi = ((VectorArg) arg).eval(arguments, argi, result, artifactExpander, pathMapper);
+          argi =
+              ((VectorArg) arg)
+                  .eval(arguments, argi, result, artifactExpander, pathMapper, mainRepoMapping);
         } else if (arg == SingleFormattedArg.MARKER) {
-          argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper);
+          argi = SingleFormattedArg.eval(arguments, argi, result, pathMapper, mainRepoMapping);
         } else {
-          result.add(StarlarkCustomCommandLine.expandToCommandLine(arg, pathMapper));
+          result.add(
+              StarlarkCustomCommandLine.expandToCommandLine(arg, pathMapper, mainRepoMapping));
         }
       }
 
@@ -796,7 +841,15 @@ public void addToFingerprint(
       Fingerprint fingerprint)
       throws CommandLineExpansionException, InterruptedException {
     List<Object> arguments = rawArgsAsList();
-    for (int argi = 0; argi < arguments.size(); ) {
+    int size;
+    if (arguments.getLast() instanceof RepositoryMapping mainRepoMapping) {
+      fingerprint.addStringMap(
+          Maps.transformValues(mainRepoMapping.entries(), RepositoryName::getName));
+      size = arguments.size() - 1;
+    } else {
+      size = arguments.size();
+    }
+    for (int argi = 0; argi < size; ) {
       Object arg = arguments.get(argi++);
       if (arg instanceof VectorArg) {
         argi =

src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java

@@ -31,6 +31,10 @@
  * <p>For repositories from the WORKSPACE file, if the requested repo doesn't exist in the mapping,
  * we fallback to the requested name. For repositories from Bzlmod, we return null to let the caller
  * decide what to do.
+ *
+ * <p>This class must not implement {@link net.starlark.java.eval.StarlarkValue} since instances of
+ * this class are used as markers by {@link
+ * com.google.devtools.build.lib.analysis.starlark.StarlarkCustomCommandLine}.
  */
 public class RepositoryMapping {
   /* A repo mapping that always falls back to using the apparent name as the canonical name. */