Pass main repository mapping into Package

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

@@ -192,48 +192,34 @@ public String getUnambiguousCanonicalForm() {
    * string is as simple as possible while referencing the current package when parsed in the
    * context of the main repository.
    *
-   * @param mainOrOtherRepoMapping the {@link RepositoryMapping} of the main repository or, if that
-   *                               is not available to the caller, the {@link RepositoryMapping} of
-   *                               the current package. The former will yield more readable output.
-   * @return <ul><li>if the main repository's mapping is provided:</li>
-   * <dl>
-   * <dt><code>//some/pkg</code></dt>
+   * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
+   * @return <dl><dt><code>//some/pkg</code></dt>
    * <dd>if this package lives in the main repository</dd>
    * <dt><code>@protobuf//some/pkg</code></dt>
    * <dd>if this package lives in a repository with "protobuf" as <code>name</code> of a repository
    * in WORKSPACE or as local name of a Bzlmod dependency of the main module</dd>
    * <dt><code>@@protobuf~3.19.2//some/pkg</code></dt>
    * <dd>only with Bzlmod if the current package belongs to a repository that is not visible from
    * the main module</dd>
-   *
-   * <li>if another repository's mapping is provided:</li>
-   * <dl>
-   * <dt><pre>//some/pkg</pre></dt>
-   * <dd>if this package lives in the main repository</dd>
-   * <dt><pre>@protobuf//some/pkg</pre></dt>
-   * <dd>if this package lives in a repository with "protobuf" as <code>name</code> of a repository
-   * in WORKSPACE</dd>
-   * <dt><pre>@@protobuf~3.19.2//some/pkg</pre></dt>
-   * <dd>if this package lives in a repository managed by Bzlmod</dd>
    */
-  public String getDisplayForm(RepositoryMapping mainOrOtherRepoMapping) {
+  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 (mainOrOtherRepoMapping.ownerRepo() == null) {
-      // Not using strict visibility, meaning that canonical and local names can be used
-      // interchangeably.
+    if (mainRepositoryMapping.ownerRepo() == null) {
+      // If the main repository mapping is not using strict visibility, then Bzlmod is certainly
+      // disabled, which means that canonical and local 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 local name the
     // main repository has for it, otherwise fall back to a canonical label.
-    Optional<String> maybeLocalName = Optional.empty();
-    if (mainOrOtherRepoMapping.ownerRepo().isMain()) {
-      maybeLocalName = mainOrOtherRepoMapping.getInverse(repository)
-        .map(localName -> "@" + localName + "//" + getPackageFragment());
-    }
-    return maybeLocalName.orElseGet(this::getUnambiguousCanonicalForm);
+    return mainRepositoryMapping.getInverse(repository)
+        .map(localName -> "@" + localName + "//" + getPackageFragment())
+        .orElseGet(this::getUnambiguousCanonicalForm);
   }
 
   /**

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

@@ -248,6 +248,13 @@ public enum ConfigSettingVisibilityPolicy {
    */
   private RepositoryMapping repositoryMapping;
 
+  /**
+   * The repository mapping of the main repository. This is only used internally to obtain
+   * user-friendly apparent names from canonical repository names in error message arising from this
+   * package.
+   */
+  private RepositoryMapping mainRepositoryMapping;
+
   private Set<Label> defaultCompatibleWith = ImmutableSet.of();
   private Set<Label> defaultRestrictedTo = ImmutableSet.of();
 
@@ -478,6 +485,7 @@ private void finishInit(Builder builder) {
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
     this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
+    this.mainRepositoryMapping = Preconditions.checkNotNull(builder.mainRepositoryMapping);
     ImmutableMap.Builder<RepositoryName, ImmutableMap<String, RepositoryName>>
         repositoryMappingsBuilder = ImmutableMap.builder();
     if (!builder.externalPackageRepositoryMappings.isEmpty() && !builder.isRepoRulePackage()) {
@@ -731,9 +739,7 @@ private String getAlternateTargetSuggestion(String targetName) {
       String blazeQuerySuggestion =
           String.format(
               "Tip: use `query %s:*` to see all the targets in that package",
-              // TODO: Make the resulting package label more readable by passing in the main
-              //  repository's mapping.
-              packageIdentifier.getDisplayForm(repositoryMapping));
+              packageIdentifier.getDisplayForm(mainRepositoryMapping));
       return String.format(
           " (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion));
     }
@@ -882,6 +888,7 @@ public static Builder newExternalPackageBuilder(
             LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
             workspaceName,
             starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+            mainRepoMapping,
             mainRepoMapping)
         .setFilename(workspacePath);
   }
@@ -896,6 +903,7 @@ public static Builder newExternalPackageBuilderForBzlmod(
             basePackageId,
             DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES,
             starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+            repoMapping,
             repoMapping)
         .setFilename(moduleFilePath);
   }
@@ -976,6 +984,11 @@ public boolean recordLoadedModules() {
      * workspace.
      */
     private final RepositoryMapping repositoryMapping;
+    /**
+     * The repository mapping of the main repository. This is only used to resolve user-friendly
+     * apparent names from canonical repository names in error message arising from this package.
+     */
+    private final RepositoryMapping mainRepositoryMapping;
     /** Converts label literals to Label objects within this package. */
     private final LabelConverter labelConverter;
 
@@ -1100,10 +1113,12 @@ public T intern(T sample) {
         PackageIdentifier id,
         String workspaceName,
         boolean noImplicitFileExport,
-        RepositoryMapping repositoryMapping) {
+        RepositoryMapping repositoryMapping,
+        RepositoryMapping mainRepositoryMapping) {
       this.pkg = new Package(id, workspaceName, packageSettings.succinctTargetNotFoundErrors());
       this.noImplicitFileExport = noImplicitFileExport;
       this.repositoryMapping = repositoryMapping;
+      this.mainRepositoryMapping = mainRepositoryMapping;
       this.labelConverter = new LabelConverter(id, repositoryMapping);
       if (pkg.getName().startsWith("javatests/")) {
         setDefaultTestonly(true);

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

@@ -465,13 +465,15 @@ public Package.Builder newPackageBuilder(
       PackageIdentifier packageId,
       String workspaceName,
       StarlarkSemantics starlarkSemantics,
-      RepositoryMapping repositoryMapping) {
+      RepositoryMapping repositoryMapping,
+      RepositoryMapping mainRepositoryMapping) {
     return new Package.Builder(
         packageSettings,
         packageId,
         workspaceName,
         starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
-        repositoryMapping);
+        repositoryMapping,
+        mainRepositoryMapping);
   }
 
   /** Returns a new {@link NonSkyframeGlobber}. */

src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java

@@ -33,6 +33,7 @@
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 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.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
@@ -1215,6 +1216,9 @@ private LoadedPackage loadPackage(
     RepositoryMappingValue repositoryMappingValue =
         (RepositoryMappingValue)
             env.getValue(RepositoryMappingValue.key(packageId.getRepository()));
+    RepositoryMappingValue mainRepositoryMappingValue = (RepositoryMappingValue) env.getValue(
+        RepositoryMappingValue.key(
+            RepositoryName.MAIN));
     RootedPath buildFileRootedPath = packageLookupValue.getRootedPath(packageId);
     FileValue buildFileValue = getBuildFileValue(env, buildFileRootedPath);
     RuleVisibility defaultVisibility = PrecomputedValue.DEFAULT_VISIBILITY.get(env);
@@ -1346,7 +1350,8 @@ private LoadedPackage loadPackage(
                   packageId,
                   workspaceName,
                   starlarkBuiltinsValue.starlarkSemantics,
-                  repositoryMapping)
+                  repositoryMapping,
+                  mainRepositoryMappingValue.getRepositoryMapping())
               .setFilename(buildFileRootedPath)
               .setDefaultVisibility(defaultVisibility)
               // "defaultVisibility" comes from the command line.

src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java

@@ -97,24 +97,19 @@ public void testRunfilesDir() throws Exception {
 
   @Test
   public void testDisplayFormInMainRepository() throws Exception {
-    RepositoryName repo = RepositoryName.MAIN;
-    PathFragment pkgFragment = PathFragment.create("some/pkg");
-    PackageIdentifier pkg = PackageIdentifier.create(repo, pkgFragment);
+    PackageIdentifier pkg = PackageIdentifier.create(RepositoryName.MAIN,
+        PathFragment.create("some/pkg"));
 
     assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("//some/pkg");
     assertThat(pkg.getDisplayForm(
         RepositoryMapping.create(Map.of("foo", RepositoryName.create("bar")),
             RepositoryName.MAIN))).isEqualTo("//some/pkg");
-    assertThat(pkg.getDisplayForm(
-        RepositoryMapping.create(Map.of("foo", RepositoryName.create("bar")),
-            RepositoryName.create("not_main")))).isEqualTo("//some/pkg");
   }
 
   @Test
   public void testDisplayFormInExternalRepository() throws Exception {
-    PathFragment pkgFragment = PathFragment.create("some/pkg");
     RepositoryName repo = RepositoryName.create("canonical");
-    PackageIdentifier pkg = PackageIdentifier.create(repo, pkgFragment);
+    PackageIdentifier pkg = PackageIdentifier.create(repo, PathFragment.create("some/pkg"));
 
     assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo(
         "@canonical//some/pkg");
@@ -124,8 +119,5 @@ public void testDisplayFormInExternalRepository() throws Exception {
     assertThat(pkg.getDisplayForm(
         RepositoryMapping.create(Map.of("local", RepositoryName.create("other_repo")),
             RepositoryName.MAIN))).isEqualTo("@@canonical//some/pkg");
-    assertThat(pkg.getDisplayForm(
-        RepositoryMapping.create(Map.of("local", repo),
-            RepositoryName.create("not_main")))).isEqualTo("@@canonical//some/pkg");
   }
 }

src/test/java/com/google/devtools/build/lib/packages/PackageTest.java

@@ -164,6 +164,7 @@ private Package.Builder pkgBuilder(String name) {
             PackageIdentifier.createInMainRepo(name),
             "workspace",
             /*noImplicitFileExport=*/ true,
+            RepositoryMapping.ALWAYS_FALLBACK,
             RepositoryMapping.ALWAYS_FALLBACK);
     result.setFilename(
         RootedPath.toRootedPath(

src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java

@@ -265,6 +265,7 @@ private Package.Builder createDummyPackageBuilder() {
             PackageIdentifier.createInMainRepo(TEST_PACKAGE_NAME),
             "TESTING",
             StarlarkSemantics.DEFAULT,
+            RepositoryMapping.ALWAYS_FALLBACK,
             RepositoryMapping.ALWAYS_FALLBACK)
         .setFilename(RootedPath.toRootedPath(root, testBuildfilePath));
   }

src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java

@@ -58,7 +58,8 @@ public final class RuleFactoryTest extends PackageLoadingTestCase {
   private Package.Builder newBuilder(PackageIdentifier id, Path filename) {
     return packageFactory
         .newPackageBuilder(
-            id, "TESTING", StarlarkSemantics.DEFAULT, RepositoryMapping.ALWAYS_FALLBACK)
+            id, "TESTING", StarlarkSemantics.DEFAULT, RepositoryMapping.ALWAYS_FALLBACK,
+            RepositoryMapping.ALWAYS_FALLBACK)
         .setFilename(RootedPath.toRootedPath(root, filename));
   }