[7.1.0] Add Label.to_display_form() (#21312)

Fixes #20486 While `to_display_form()` can be called in all contexts, it only returns apparent names for BUILD threads, which are the most common use case for display form labels. Support for module extensions can be added later, but requires explicit tracking of inverse mappings in the lockfile. Also use `to_display_form()` to generate Clang module names in the correct form for external repositories. `java_*` rules require more delicate handling and will be migrated in a follow-up change. RELNOTES: The `to_display_form()` method on `Label` returns a string representation of a label optimized for readability by humans. Closes #21179. PiperOrigin-RevId: 606330539 Change-Id: Id6a4ad79fd3d50319320789b88c618aad28db28b Co-authored-by: Fabian Meumertzheim <[email protected]>
bazelbuild · Feb 13, 2024 · 4576e4a · 4576e4a
1 parent 1991f52
commit 4576e4a
Show file tree

Hide file tree

Showing 17 changed files with 329 additions and 14 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -373,13 +373,9 @@ private String getProgressMessageChecked(@Nullable RepositoryMapping mainReposit
   private String replaceProgressMessagePlaceholders(
       String progressMessage, @Nullable RepositoryMapping mainRepositoryMapping) {
     if (progressMessage.contains("%{label}") && owner.getLabel() != null) {
-      String labelString;
-      if (mainRepositoryMapping != null) {
-        labelString = owner.getLabel().getDisplayForm(mainRepositoryMapping);
-      } else {
-        labelString = owner.getLabel().toString();
-      }
-      progressMessage = progressMessage.replace("%{label}", labelString);
+      progressMessage =
+          progressMessage.replace(
+              "%{label}", owner.getLabel().getDisplayForm(mainRepositoryMapping));
     }
     if (progressMessage.contains("%{output}") && getPrimaryOutput() != null) {
       progressMessage =

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java b/src/main/java/com/google/devtools/build/lib/cmdline/BazelModuleContext.java
@@ -44,6 +44,13 @@ public abstract class BazelModuleContext {
   /** The repository mapping applicable to the repo where the .bzl file is located in. */
   public abstract RepositoryMapping repoMapping();
 
+  /**
+   * The repository mapping applicable to the main repository, possibly without WORKSPACE repos or
+   * null. This is purely meant to support {@link Label#getDisplayFormForStarlark(StarlarkThread)}.
+   */
+  @Nullable
+  public abstract RepositoryMapping bestEffortMainRepoMapping();
+
   /** Returns the name of the module's .bzl file, as provided to the parser. */
   public abstract String filename();
 
@@ -160,11 +167,12 @@ public static BazelModuleContext ofInnermostBzlOrFail(StarlarkThread thread, Str
   public static BazelModuleContext create(
       Label label,
       RepositoryMapping repoMapping,
+      @Nullable RepositoryMapping bestEffortMainRepoMapping,
       String filename,
       ImmutableList<Module> loads,
       byte[] bzlTransitiveDigest) {
     return new AutoValue_BazelModuleContext(
-        label, repoMapping, filename, loads, bzlTransitiveDigest);
+        label, repoMapping, bestEffortMainRepoMapping, filename, loads, bzlTransitiveDigest);
   }
 
   public final Label.PackageContext packageContext() {

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -437,10 +437,28 @@ public String getUnambiguousCanonicalForm() {
    * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
    * @return analogous to {@link PackageIdentifier#getDisplayForm(RepositoryMapping)}
    */
-  public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
+  public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
     return packageIdentifier.getDisplayForm(mainRepositoryMapping) + ":" + name;
   }
 
+  @StarlarkMethod(
+      name = "to_display_form",
+      useStarlarkThread = true,
+      doc =
+          "Returns a string representation of this label that is optimized for human readability."
+              + " Use this to format a <code>Label</code> for use in BUILD files. <p>The exact form"
+              + " of the return value is explicitly unspecified and subject to change. The"
+              + " following properties are guaranteed for a <code>Label</code> <code>l</code>:<ul> "
+              + " <li><code>l.to_display_form()</code> has no repository part if and only if"
+              + " <code>l</code> references the main repository;</li> "
+              + " <li><code>Label(l.to_display_form()) == l</code> if the call to"
+              + " <code>Label</code> occurs in the main repository.</li></ul>")
+  public String getDisplayFormForStarlark(StarlarkThread starlarkThread) throws EvalException {
+    checkRepoVisibilityForStarlark("to_display_form");
+    return getDisplayForm(
+        BazelModuleContext.ofInnermostBzlOrThrow(starlarkThread).bestEffortMainRepoMapping());
+  }
+
   /**
    * 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

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -22,6 +22,7 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
+import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -215,7 +216,7 @@ public String getUnambiguousCanonicalForm() {
    *       <dd>only with Bzlmod if the current package belongs to a repository that is not visible
    *           from the main module
    */
-  public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
+  public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
     return repository.getDisplayForm(mainRepositoryMapping) + "//" + pkgName;
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -247,20 +247,25 @@ public String getCanonicalForm() {
    *       <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"
+   *           is "protobuf" (in both cases only if mainRepositoryMapping is not null)
    *       <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) {
+  public String getDisplayForm(@Nullable RepositoryMapping mainRepositoryMapping) {
     Preconditions.checkArgument(
-        mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
+        mainRepositoryMapping == null
+            || mainRepositoryMapping.ownerRepo() == null
+            || mainRepositoryMapping.ownerRepo().isMain());
     if (!isVisible()) {
       return getNameWithAt();
     }
     if (isMain()) {
       // Packages in the main repository can always use repo-relative form.
       return "";
     }
+    if (mainRepositoryMapping == null) {
+      return getNameWithAt();
+    }
     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

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -68,6 +68,7 @@
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
@@ -765,6 +766,11 @@ private BzlLoadValue computeInternalWithCompiledBzl(
     if (repoMapping == null) {
       return null;
     }
+    Optional<RepositoryMapping> mainRepoMapping =
+        getMainRepositoryMapping(key, builtins.starlarkSemantics, env);
+    if (mainRepoMapping == null) {
+      return null;
+    }
     Label.RepoMappingRecorder repoMappingRecorder = new Label.RepoMappingRecorder();
     ImmutableList<Pair<String, Location>> programLoads = getLoadsFromProgram(prog);
     ImmutableList<Label> loadLabels =
@@ -843,6 +849,7 @@ private BzlLoadValue computeInternalWithCompiledBzl(
         BazelModuleContext.create(
             label,
             repoMapping,
+            mainRepoMapping.orElse(null),
             prog.getFilename(),
             ImmutableList.copyOf(loadMap.values()),
             transitiveDigest);
@@ -977,6 +984,34 @@ private static RepositoryMapping getRepositoryMapping(
     return repositoryMappingValue.getRepositoryMapping();
   }
 
+  @Nullable
+  private static Optional<RepositoryMapping> getMainRepositoryMapping(
+      BzlLoadValue.Key key, StarlarkSemantics starlarkSemantics, Environment env)
+      throws InterruptedException {
+    if (!starlarkSemantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
+      return Optional.empty();
+    }
+    RepositoryMappingValue.Key repoMappingKey;
+    // When adding cases for other key types such as WORKSPACE or Bzlmod, make sure to track the
+    // usages of the repo mapping in persistent caches, such as repository marker files and the
+    // MODULE.bazel.lock file.
+    if (key instanceof BzlLoadValue.KeyForBuild) {
+      repoMappingKey = RepositoryMappingValue.key(RepositoryName.MAIN);
+    } else if (key instanceof BzlLoadValue.KeyForBuiltins) {
+      // Using the full main repo mapping here results in a cycle as it depends on WORKSPACE, but
+      // builtins are injected into WORKSPACE. Fixing this fully would require adding a new key type
+      // for builtins (transitively) loaded from WORKSPACE.
+      repoMappingKey = RepositoryMappingValue.KEY_FOR_ROOT_MODULE_WITHOUT_WORKSPACE_REPOS;
+    } else {
+      return Optional.empty();
+    }
+    var mainRepositoryMappingValue = (RepositoryMappingValue) env.getValue(repoMappingKey);
+    if (mainRepositoryMappingValue == null) {
+      return null;
+    }
+    return Optional.of(mainRepositoryMappingValue.getRepositoryMapping());
+  }
+
   /**
    * Validates a label appearing in a {@code load()} statement, throwing {@link
    * LabelSyntaxException} on failure.

diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl
@@ -297,7 +297,7 @@ def _init_cc_compilation_context(
         if not module_map:
             module_map = cc_common.create_module_map(
                 file = actions.declare_file(label.name + ".cppmap"),
-                name = label.workspace_name + "//" + label.package + ":" + label.name,
+                name = label.to_display_form(),
             )
 
         # There are different modes for module compilation:

diff --git a/...est/java/com/google/devtools/build/lib/analysis/actions/BuildInfoFileWriteActionTest.java b/...est/java/com/google/devtools/build/lib/analysis/actions/BuildInfoFileWriteActionTest.java
@@ -78,6 +78,7 @@ private static Object exec(String... lines) throws Exception {
               BazelModuleContext.create(
                   Label.parseCanonicalUnchecked("//test:label"),
                   RepositoryMapping.ALWAYS_FALLBACK,
+                  /* bestEffortMainRepoMapping= */ null,
                   "test/label.bzl",
                   /* loads= */ ImmutableList.of(),
                   /* bzlTransitiveDigest= */ new byte[0])),

diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
@@ -2533,4 +2533,37 @@ public void innate_noSuchValue() throws Exception {
             "//:repo.bzl does not export a repository_rule called data_repo, yet its use is"
                 + " requested at /ws/MODULE.bazel");
   }
+
+  @Test
+  public void labelToDisplayForm() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "bazel_dep(name='data_repo', version='1.0')",
+        "ext = use_extension('//:defs.bzl', 'ext')",
+        "use_repo(ext, 'foo', 'bar', 'baz')");
+    scratch.file(
+        workspaceRoot.getRelative("defs.bzl").getPathString(),
+        "load('@data_repo//:defs.bzl','data_repo')",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='foo',data=Label('//:foo').to_display_form())",
+        "  data_repo(name='bar',data=Label('@data_repo//:bar').to_display_form())",
+        "  data_repo(name='baz',data=Label('@@canonical_name//:baz').to_display_form())",
+        "ext = module_extension(implementation=_ext_impl)");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@foo//:data.bzl', foo_data='data')",
+        "load('@bar//:data.bzl', bar_data='data')",
+        "load('@baz//:data.bzl', baz_data='data')",
+        "data = 'foo:'+foo_data+' bar:'+bar_data+' baz:'+baz_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    EvaluationResult<BzlLoadValue> result =
+        evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    if (result.hasError()) {
+      throw result.getError().getException();
+    }
+    assertThat(result.get(skyKey).getModule().getGlobal("data"))
+        .isEqualTo("foo://:foo bar:@@data_repo~//:bar baz:@@canonical_name//:baz");
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
@@ -418,6 +418,12 @@ public void testDisplayForm() throws Exception {
         .isEqualTo("@unremapped//:unremapped");
   }
 
+  @Test
+  public void testDisplayFormNullMapping() throws Exception {
+    assertThat(displayFormFor("//foo/bar:bar", null)).isEqualTo("//foo/bar:bar");
+    assertThat(displayFormFor("@@foo//bar:bar", null)).isEqualTo("@@foo//bar:bar");
+  }
+
   private static String shorthandDisplayFormFor(
       String rawLabel, RepositoryMapping repositoryMapping) throws Exception {
     return Label.parseCanonical(rawLabel).getShorthandDisplayForm(repositoryMapping);

diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
@@ -120,6 +120,7 @@ public void testDisplayFormInMainRepository() throws Exception {
                 RepositoryMapping.create(
                     ImmutableMap.of("foo", RepositoryName.create("bar")), RepositoryName.MAIN)))
         .isEqualTo("//some/pkg");
+    assertThat(pkg.getDisplayForm(null)).isEqualTo("//some/pkg");
   }
 
   @Test
@@ -139,5 +140,6 @@ public void testDisplayFormInExternalRepository() throws Exception {
                     ImmutableMap.of("local", RepositoryName.create("other_repo")),
                     RepositoryName.MAIN)))
         .isEqualTo("@@canonical//some/pkg");
+    assertThat(pkg.getDisplayForm(null)).isEqualTo("@@canonical//some/pkg");
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/RepositoryNameTest.java
@@ -105,4 +105,21 @@ public void testGetDisplayForm() throws Exception {
                 .getDisplayForm(repositoryMapping))
         .isEqualTo("@@[unknown repo 'local' requested from @@owner]");
   }
+
+  @Test
+  public void testGetDisplayFormWithNullMapping() throws Exception {
+    assertThat(RepositoryName.create("").getDisplayForm(null)).isEmpty();
+    assertThat(RepositoryName.create("canonical").getDisplayForm(null)).isEqualTo("@@canonical");
+
+    assertThat(
+            RepositoryName.create("")
+                .toNonVisible(RepositoryName.create("owner"))
+                .getDisplayForm(null))
+        .isEqualTo("@@[unknown repo '' requested from @@owner]");
+    assertThat(
+            RepositoryName.create("canonical")
+                .toNonVisible(RepositoryName.create("owner"))
+                .getDisplayForm(null))
+        .isEqualTo("@@[unknown repo 'canonical' requested from @@owner]");
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -4824,6 +4824,7 @@ public void testLabelWithStrictVisibility() throws Exception {
             bzlLabel,
             RepositoryMapping.create(
                 ImmutableMap.of("my_module", currentRepo, "dep", otherRepo), currentRepo),
+            /* bestEffortMainRepoMapping= */ null,
             "lib/label.bzl",
             /* loads= */ ImmutableList.of(),
             /* bzlTransitiveDigest= */ new byte[0]);

diff --git a/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/starlark/util/BazelEvaluationTestCase.java
@@ -175,6 +175,7 @@ private Object newModule(ImmutableMap.Builder<String, Object> predeclared) {
     return BazelModuleContext.create(
         label,
         RepositoryMapping.ALWAYS_FALLBACK,
+        /* bestEffortMainRepoMapping= */ null,
         "test/label.bzl",
         /* loads= */ ImmutableList.of(),
         /* bzlTransitiveDigest= */ new byte[0]);