starlark_doc_extract: stringify label-valued default attribute values

For human-readable docs, want to stringify labels in a way that
(1) elides the name part when unambiguous, matching buildifier behavior; and
(2) respects repo mapping, so we don't print full bzlmod canonical repo names.

Unfortunately, this means introducing yet another stringification method to
Label, which combines the behavior of getDisplayForm() and toShorthandString()
but cannot be conveniently composed from them. Alas.

PiperOrigin-RevId: 523504039
Change-Id: Ia09ebb77cff09aaeb270f43ed7e319c90a44aa52

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

@@ -390,7 +390,7 @@ public String getUnambiguousCanonicalForm() {
   }
 
   /**
-   * Returns a label string that is suitable for display, i.e., it resolves to this label when
+   * Returns a full label string that is suitable for display, i.e., it resolves to this label when
    * parsed in the context of the main repository and has a repository part that is as simple as
    * possible.
    *
@@ -401,6 +401,34 @@ public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
     return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name;
   }
 
+  /**
+   * Returns a shorthand label string that is suitable for display, i.e. in addition to simplifying
+   * the repository part, labels of the form {@code [@repo]//foo/bar:bar} are simplified to the
+   * shorthand form {@code [@repo]//foo/bar}, and labels of the form {@code @repo//:repo} and
+   * {@code @@repo//:repo} are simplified to {@code @repo}. The returned shorthand string resolves
+   * back to this label only when parsed in the context of the main repository whose repository
+   * mapping is provided.
+   *
+   * <p>Unlike {@link #getDisplayForm}, this method elides the name part of the label if possible.
+   *
+   * <p>Unlike {@link #toShorthandString}, this method respects {@link RepositoryMapping}.
+   *
+   * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
+   */
+  public String getShorthandDisplayForm(RepositoryMapping mainRepositoryMapping) {
+    if (getPackageFragment().getBaseName().equals(name)) {
+      return packageIdentifier.getDisplayForm(mainRepositoryMapping);
+    } else if (getPackageFragment().getBaseName().isEmpty()) {
+      String repositoryDisplayForm =
+          getPackageIdentifier().getRepository().getDisplayForm(mainRepositoryMapping);
+      // Simplify @foo//:foo or @@foo//:foo to @foo; note that `name` cannot start with '@'
+      if (repositoryDisplayForm.equals("@" + name) || repositoryDisplayForm.equals("@@" + name)) {
+        return repositoryDisplayForm;
+      }
+    }
+    return getDisplayForm(mainRepositoryMapping);
+  }
+
   /** Return the name of the repository label refers to without the leading `at` symbol. */
   @StarlarkMethod(
       name = "workspace_name",
@@ -419,6 +447,8 @@ public String getWorkspaceName() throws EvalException {
    *
    * <p>Labels with canonical form {@code //foo/bar:bar} have the shorthand form {@code //foo/bar}.
    * All other labels have identical shorthand and canonical forms.
+   *
+   * <p>Unlike {@link #getShorthandDisplayForm}, this method does not respect repository mapping.
    */
   public String toShorthandString() {
     if (!getPackageFragment().getBaseName().equals(name)) {

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

@@ -189,7 +189,7 @@ public String getUnambiguousCanonicalForm() {
   /**
    * Returns a label representation for this package that is suitable for display. The returned
    * string is as simple as possible while referencing the current package when parsed in the
-   * context of the main repository.
+   * context of the main repository whose repository mapping is provided.
    *
    * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
    * @return
@@ -204,24 +204,8 @@ public String getUnambiguousCanonicalForm() {
    *           from the main module
    */
   public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
-    Preconditions.checkArgument(
-        mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
-    if (repository.isMain()) {
-      // Packages in the main repository can always use repo-relative form.
-      return "//" + getPackageFragment();
-    }
-    if (!mainRepositoryMapping.usesStrictDeps()) {
-      // If the main repository mapping is not using strict visibility, then Bzlmod is certainly
-      // disabled, which means that canonical and apparent names can be used interchangeably from
-      // the context of the main repository.
-      return repository.getNameWithAt() + "//" + getPackageFragment();
-    }
-    // If possible, represent the repository with a non-canonical label using the apparent name the
-    // main repository has for it, otherwise fall back to a canonical label.
-    return mainRepositoryMapping
-        .getInverse(repository)
-        .map(apparentName -> "@" + apparentName + "//" + getPackageFragment())
-        .orElseGet(this::getUnambiguousCanonicalForm);
+    return String.format(
+        "%s//%s", getRepository().getDisplayForm(mainRepositoryMapping), getPackageFragment());
   }
 
   /**

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

@@ -230,6 +230,45 @@ public String getCanonicalForm() {
     return isMain() ? "" : getNameWithAt();
   }
 
+  /**
+   * Returns the repository part of a {@link Label}'s string representation suitable for display.
+   * The returned string is as simple as possible in the context of the main repo whose repository
+   * mapping is provided: an empty string for the main repo, or a string prefixed with a leading
+   * "{@literal @}" or "{@literal @@}" otherwise.
+   *
+   * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
+   * @return
+   *     <dl>
+   *       <dt>the empty string
+   *       <dd>if this is the main repository
+   *       <dt><code>@protobuf</code>
+   *       <dd>if this repository is a WORKSPACE dependency and its <code>name</code> is "protobuf",
+   *           or if this repository is a Bzlmod dependency of the main module and its apparent name
+   *           is "protobuf"
+   *       <dt><code>@@protobuf~3.19.2</code>
+   *       <dd>only with Bzlmod, if this a repository that is not visible from the main module
+   */
+  public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
+    Preconditions.checkArgument(
+        mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
+    if (isMain()) {
+      // Packages in the main repository can always use repo-relative form.
+      return "";
+    }
+    if (!mainRepositoryMapping.usesStrictDeps()) {
+      // If the main repository mapping is not using strict visibility, then Bzlmod is certainly
+      // disabled, which means that canonical and apparent names can be used interchangeably from
+      // the context of the main repository.
+      return getNameWithAt();
+    }
+    // If possible, represent the repository with a non-canonical label using the apparent name the
+    // main repository has for it, otherwise fall back to a canonical label.
+    return mainRepositoryMapping
+        .getInverse(this)
+        .map(apparentName -> "@" + apparentName)
+        .orElse("@" + getNameWithAt());
+  }
+
   /**
    * Returns the runfiles/execRoot path for this repository. If we don't know the name of this repo
    * (i.e., it is in the main repository), return an empty path fragment.

src/main/java/com/google/devtools/build/lib/packages/Attribute.java

@@ -2084,7 +2084,6 @@ public Object getDefaultValue() {
    * Returns the default value of this attribute, even if it is a computed default, or a late-bound
    * default.
    */
-  @VisibleForTesting
   public Object getDefaultValueUnchecked() {
     return defaultValue;
   }

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

@@ -29,6 +29,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/skyframe:bzl_load_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:repository_mapping_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/main/java/com/google/devtools/build/skydoc/rendering:function_util",

src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/ModuleInfoExtractor.java

@@ -15,9 +15,12 @@
 package com.google.devtools.build.lib.rules.starlarkdocextract;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleClassFunctions.StarlarkRuleFunction;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -35,17 +38,23 @@
 import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderInfo;
 import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ProviderNameGroup;
 import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.RuleInfo;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.Predicate;
+import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Module;
 import net.starlark.java.eval.Printer;
 import net.starlark.java.eval.StarlarkFunction;
+import net.starlark.java.eval.StarlarkList;
 import net.starlark.java.eval.Structure;
 
 /** API documentation extractor for a compiled, loaded Starlark module. */
 final class ModuleInfoExtractor {
+  private final Predicate<String> isWantedName;
+  private final RepositoryMapping repositoryMapping;
+
   @VisibleForTesting
   static final AttributeInfo IMPLICIT_NAME_ATTRIBUTE_INFO =
       AttributeInfo.newBuilder()
@@ -59,14 +68,21 @@ final class ModuleInfoExtractor {
   // FakeRepositoryModule currently does?
 
   /**
-   * Extracts structured documentation for the exported symbols (meaning symbols whose first
-   * character is alphabetic) of a given module.
+   * Constructs an instance of {@code ModuleInfoExtractor}.
    *
-   * @param isWantedName a filter applied to symbols; only those symbols for which the filter
-   *     returns true will be documented
+   * @param isWantedName a filter applied to symbols names; only those symbols which both are
+   *     exportable (meaning the first character is alphabetic) and for which the filter returns
+   *     true will be documented
+   * @param repositoryMapping the repository mapping for the repo in which we want to render labels
+   *     as strings
    */
-  public static ModuleInfo extractFrom(Module module, Predicate<String> isWantedName)
-      throws ExtractionException {
+  public ModuleInfoExtractor(Predicate<String> isWantedName, RepositoryMapping repositoryMapping) {
+    this.isWantedName = isWantedName;
+    this.repositoryMapping = repositoryMapping;
+  }
+
+  /** Extracts structured documentation for the exported symbols of a given module. */
+  public ModuleInfo extractFrom(Module module) throws ExtractionException {
     ModuleInfo.Builder builder = ModuleInfo.newBuilder();
     Optional.ofNullable(module.getDocumentation()).ifPresent(builder::setModuleDocstring);
     for (var entry : module.getGlobals().entrySet()) {
@@ -103,9 +119,8 @@ public ExtractionException(String message, Throwable cause) {
    *     for field bar of exported struct foo
    * @param value documentable Starlark value
    */
-  private static void addInfo(ModuleInfo.Builder builder, String name, Object value)
+  private void addInfo(ModuleInfo.Builder builder, String name, Object value)
       throws ExtractionException {
-    // Note that may be exported under a different name than its getName() value.
     if (value instanceof StarlarkRuleFunction) {
       addRuleInfo(builder, name, (StarlarkRuleFunction) value);
     } else if (value instanceof StarlarkProvider) {
@@ -126,7 +141,7 @@ private static void addInfo(ModuleInfo.Builder builder, String name, Object valu
     // TODO(b/276733504): should we recurse into dicts to search for documentable values?
   }
 
-  private static void addStructureInfo(ModuleInfo.Builder builder, String name, Structure structure)
+  private void addStructureInfo(ModuleInfo.Builder builder, String name, Structure structure)
       throws ExtractionException {
     for (String fieldName : structure.getFieldNames()) {
       if (isExportableName(fieldName)) {
@@ -182,7 +197,54 @@ private static AttributeType getAttributeType(Attribute attribute, String where)
             where, attribute.getPublicName(), type.getClass().getSimpleName()));
   }
 
-  private static AttributeInfo buildAttributeInfo(Attribute attribute, String where)
+  /**
+   * Recursively transforms labels to strings via {@link Label#getShorthandDisplayForm}.
+   *
+   * @return the label's shorthand display string if {@code o} is a label; a container with label
+   *     elements transformed into shorthand display strings recursively if {@code o} is a Starlark
+   *     container; or the original object {@code o} if no label stringification was performed.
+   */
+  private Object stringifyLabels(Object o) {
+    if (o instanceof Label) {
+      return ((Label) o).getShorthandDisplayForm(repositoryMapping);
+    } else if (o instanceof Map) {
+      return stringifyLabelsOfMap((Map<?, ?>) o);
+    } else if (o instanceof List) {
+      return stringifyLabelsOfList((List<?>) o);
+    } else {
+      return o;
+    }
+  }
+
+  private Object stringifyLabelsOfMap(Map<?, ?> dict) {
+    boolean neededToStringify = false;
+    ImmutableMap.Builder<Object, Object> builder = ImmutableMap.builder();
+    for (Map.Entry<?, ?> entry : dict.entrySet()) {
+      Object keyWithStringifiedLabels = stringifyLabels(entry.getKey());
+      Object valueWithStringifiedLabels = stringifyLabels(entry.getValue());
+      if (keyWithStringifiedLabels != entry.getKey()
+          || valueWithStringifiedLabels != entry.getValue() /* as Objects */) {
+        neededToStringify = true;
+      }
+      builder.put(keyWithStringifiedLabels, valueWithStringifiedLabels);
+    }
+    return neededToStringify ? Dict.immutableCopyOf(builder.buildOrThrow()) : dict;
+  }
+
+  private Object stringifyLabelsOfList(List<?> list) {
+    boolean neededToStringify = false;
+    ImmutableList.Builder<Object> builder = ImmutableList.builder();
+    for (Object element : list) {
+      Object elementWithStringifiedLabels = stringifyLabels(element);
+      if (elementWithStringifiedLabels != element /* as Objects */) {
+        neededToStringify = true;
+      }
+      builder.add(elementWithStringifiedLabels);
+    }
+    return neededToStringify ? StarlarkList.immutableCopyOf(builder.build()) : list;
+  }
+
+  private AttributeInfo buildAttributeInfo(Attribute attribute, String where)
       throws ExtractionException {
     AttributeInfo.Builder builder = AttributeInfo.newBuilder();
     builder.setName(attribute.getPublicName());
@@ -201,15 +263,13 @@ private static AttributeInfo buildAttributeInfo(Attribute attribute, String wher
     }
 
     if (!attribute.isMandatory()) {
-      // TODO(b/276733504): deeply canonicalize label objects to strings
-      Object defaultValue = attribute.getDefaultValueUnchecked();
-      builder.setDefaultValue(
-          new Printer().repr(Attribute.valueToStarlark(defaultValue)).toString());
+      Object defaultValue = Attribute.valueToStarlark(attribute.getDefaultValueUnchecked());
+      builder.setDefaultValue(new Printer().repr(stringifyLabels(defaultValue)).toString());
     }
     return builder.build();
   }
 
-  private static void addRuleInfo(
+  private void addRuleInfo(
       ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkRuleFunction ruleFunction)
       throws ExtractionException {
     RuleInfo.Builder ruleInfoBuilder = RuleInfo.newBuilder();
@@ -249,7 +309,7 @@ private static void addProviderInfo(
     moduleInfoBuilder.addProviderInfo(providerInfoBuilder);
   }
 
-  private static void addAspectInfo(
+  private void addAspectInfo(
       ModuleInfo.Builder moduleInfoBuilder, String exportedName, StarlarkDefinedAspect aspect)
       throws ExtractionException {
     AspectInfo.Builder aspectInfoBuilder = AspectInfo.newBuilder();
@@ -265,6 +325,4 @@ private static void addAspectInfo(
     }
     moduleInfoBuilder.addAspectInfo(aspectInfoBuilder);
   }
-
-  private ModuleInfoExtractor() {}
 }

src/main/java/com/google/devtools/build/lib/rules/starlarkdocextract/StarlarkDocExtract.java

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.rules.starlarkdocextract;
 
 import static com.google.common.base.Verify.verify;
+import static com.google.common.base.Verify.verifyNotNull;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
 import static com.google.devtools.build.lib.packages.ImplicitOutputsFunction.fromTemplates;
 import static com.google.devtools.build.lib.packages.Type.STRING_LIST;
@@ -44,6 +45,8 @@
 import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException;
 import com.google.devtools.build.lib.skyframe.BzlLoadValue;
+import com.google.devtools.build.lib.skyframe.RepositoryMappingValue;
+import com.google.devtools.build.lib.skyframe.RepositoryMappingValue.RepositoryMappingResolutionException;
 import com.google.devtools.build.skydoc.rendering.proto.StardocOutputProtos.ModuleInfo;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.common.options.Option;
@@ -162,10 +165,34 @@ private static Module loadModule(RuleContext ruleContext)
   }
 
   private static ModuleInfo getModuleInfo(RuleContext ruleContext, Module module)
-      throws RuleErrorException {
+      throws RuleErrorException, InterruptedException {
+    RepositoryMappingValue repositoryMappingValue;
+    try {
+      // We get the starlark_doc_extract target's repository's repo mapping to ensure label
+      // stringification does not change regardless of whether we are the main repo or a dependency.
+      // However, this does mean that label stringifactions we produce could be invalid in the main
+      // repo.
+      repositoryMappingValue =
+          (RepositoryMappingValue)
+              ruleContext
+                  .getAnalysisEnvironment()
+                  .getSkyframeEnv()
+                  .getValueOrThrow(
+                      RepositoryMappingValue.key(ruleContext.getRepository()),
+                      RepositoryMappingResolutionException.class);
+    } catch (RepositoryMappingResolutionException e) {
+      ruleContext.ruleError(e.getMessage());
+      throw new RuleErrorException(e);
+    }
+    verifyNotNull(repositoryMappingValue);
+
     ModuleInfo moduleInfo;
     try {
-      moduleInfo = ModuleInfoExtractor.extractFrom(module, getWantedSymbolPredicate(ruleContext));
+      moduleInfo =
+          new ModuleInfoExtractor(
+                  getWantedSymbolPredicate(ruleContext),
+                  repositoryMappingValue.getRepositoryMapping())
+              .extractFrom(module);
     } catch (ModuleInfoExtractor.ExtractionException e) {
       ruleContext.ruleError(e.getMessage());
       throw new RuleErrorException(e);