Fix query command suggestion in error message with repo mappings

The package label in the query command suggested when a specified target
isn't found in a package looked like `@rules_cc~1.0.0//pkg`, which
doesn't work with Bzlmod as the repo name is canonical, but the label
isn't.

This is fixed by introducing a new function `getDisplayForm` on
`PackageIdentifier` that prints the most readable label representation
of a package from the context of the main repository given a repository
mapping (ideally that of the main repository).

This function will be used in a follow-up PR to decanonicalize labels in
{a,c,}query output when possible.

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

@@ -21,6 +21,7 @@
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Objects;
+import java.util.Optional;
 import javax.annotation.concurrent.Immutable;
 
 /**
@@ -177,6 +178,65 @@ public String getCanonicalForm() {
     return repository.getCanonicalForm() + "//" + getPackageFragment();
   }
 
+  /**
+   * Returns an absolutely unambiguous canonical form for this package in label form. Parsing this
+   * string in any environment, even when subject to repository mapping, should identify the same
+   * package.
+   */
+  public String getUnambiguousCanonicalForm() {
+    return String.format("@@%s//%s", getRepository().getName(), getPackageFragment());
+  }
+
+  /**
+   * 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.
+   *
+   * @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>
+   * <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 or as local name of a Bzlmod dependency of the main module</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) {
+    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.
+      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.repositoryMapping()
+        .inverse().get(repository).stream().findFirst()
+        .map(localName -> "@" + localName + "//" + getPackageFragment());
+    }
+    return maybeLocalName.orElseGet(this::getUnambiguousCanonicalForm);
+  }
+
   /**
    * Returns the package path, possibly qualified with a repository name.
    *

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

@@ -14,11 +14,16 @@
 
 package com.google.devtools.build.lib.cmdline;
 
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Map.Entry;
 import javax.annotation.Nullable;
 
 /**
@@ -35,7 +40,10 @@ public abstract class RepositoryMapping {
   // Always fallback to the requested name
   public static final RepositoryMapping ALWAYS_FALLBACK = createAllowingFallback(ImmutableMap.of());
 
-  abstract ImmutableMap<String, RepositoryName> repositoryMapping();
+  // A given String maps to exactly one RepositoryName, but a RepositoryName may have multiple
+  // Strings (that is, local repository names), mapping to it. In other words, the entries would
+  // form a valid ImmutableMap, but not necessarily a valid ImmutableBiMap.
+  abstract ImmutableListMultimap<String, RepositoryName> repositoryMapping();
 
   /**
    * The owner repo of this repository mapping. It is for providing useful debug information when
@@ -48,14 +56,19 @@ public abstract class RepositoryMapping {
   public static RepositoryMapping create(
       Map<String, RepositoryName> repositoryMapping, RepositoryName ownerRepo) {
     return new AutoValue_RepositoryMapping(
-        ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)),
+        ImmutableListMultimap.<String, RepositoryName>builder()
+            .putAll(Preconditions.checkNotNull(repositoryMapping).entrySet())
+            .build(),
         Preconditions.checkNotNull(ownerRepo));
   }
 
   public static RepositoryMapping createAllowingFallback(
       Map<String, RepositoryName> repositoryMapping) {
     return new AutoValue_RepositoryMapping(
-        ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), null);
+        ImmutableListMultimap.<String, RepositoryName>builder()
+            .putAll(Preconditions.checkNotNull(repositoryMapping).entrySet())
+            .build(),
+        null);
   }
 
   /**
@@ -64,25 +77,31 @@ public static RepositoryMapping createAllowingFallback(
    */
   public RepositoryMapping withAdditionalMappings(Map<String, RepositoryName> additionalMappings) {
     HashMap<String, RepositoryName> allMappings = new HashMap<>(additionalMappings);
-    allMappings.putAll(repositoryMapping());
-    return new AutoValue_RepositoryMapping(ImmutableMap.copyOf(allMappings), ownerRepo());
+    repositoryMapping().entries().forEach(e -> allMappings.put(e.getKey(), e.getValue()));
+    return new AutoValue_RepositoryMapping(
+        ImmutableListMultimap.<String, RepositoryName>builder()
+            .putAll(allMappings.entrySet())
+            .build(),
+        ownerRepo());
   }
 
   /**
    * Create a new {@link RepositoryMapping} instance based on existing repo mappings and given
    * additional mappings. If there are conflicts, existing mappings will take precedence. The owner
    * repo of the given additional mappings is ignored.
    */
-  public RepositoryMapping withAdditionalMappings(RepositoryMapping additionalMappings) {
-    return withAdditionalMappings(additionalMappings.repositoryMapping());
+  public RepositoryMapping withAdditionalMappings(RepositoryMapping baseMapping) {
+    return withAdditionalMappings(baseMapping.repositoryMapping().entries().stream()
+        .collect(toImmutableMap(Entry::getKey, Entry::getValue)));
   }
 
   /**
    * Returns the canonical repository name associated with the given apparent repo name. The
    * provided apparent repo name is assumed to be valid.
    */
   public RepositoryName get(String preMappingName) {
-    RepositoryName canonicalRepoName = repositoryMapping().get(preMappingName);
+    RepositoryName canonicalRepoName = Iterables.getOnlyElement(
+        repositoryMapping().get(preMappingName), null);
     if (canonicalRepoName != null) {
       return canonicalRepoName;
     }

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

@@ -731,7 +731,9 @@ private String getAlternateTargetSuggestion(String targetName) {
       String blazeQuerySuggestion =
           String.format(
               "Tip: use `query %s:*` to see all the targets in that package",
-              packageIdentifier.getCanonicalForm());
+              // TODO: Make the resulting package label more readable by passing in the main
+              //  repository's mapping.
+              packageIdentifier.getDisplayForm(repositoryMapping));
       return String.format(
           " (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion));
     }

src/test/java/com/google/devtools/build/lib/cmdline/BUILD

@@ -23,6 +23,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/cmdline:query_exception_marker_interface",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/java/net/starlark/java/eval",
         "//src/test/java/com/google/devtools/build/lib/testutil:TestThread",

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

@@ -17,6 +17,7 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.devtools.build.lib.vfs.PathFragment;
+import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -93,4 +94,38 @@ public void testRunfilesDir() throws Exception {
     assertThat(PackageIdentifier.create("", PathFragment.create("bar/baz")).getRunfilesPath())
         .isEqualTo(PathFragment.create("bar/baz"));
   }
+
+  @Test
+  public void testDisplayFormInMainRepository() throws Exception {
+    RepositoryName repo = RepositoryName.MAIN;
+    PathFragment pkgFragment = PathFragment.create("some/pkg");
+    PackageIdentifier pkg = PackageIdentifier.create(repo, pkgFragment);
+
+    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);
+
+    assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo(
+        "@canonical//some/pkg");
+    assertThat(pkg.getDisplayForm(
+        RepositoryMapping.create(Map.of("local", repo),
+            RepositoryName.MAIN))).isEqualTo("@local//some/pkg");
+    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");
+  }
 }