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 as the repo name is canonical, but the label isn't.

This is fixed by reusing the logic from `Label#getUnambiguousCanonicalForm`.

Closes #16482.

PiperOrigin-RevId: 483358185
Change-Id: I5a6eeae9950c614d5268aed62474f5feb31a8ce6

Author: fmeum

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

@@ -177,6 +177,53 @@ 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 mainRepositoryMapping the {@link RepositoryMapping} of the main repository
+   * @return
+   *     <dl>
+   *       <dt><code>//some/pkg</code>
+   *       <dd>if this package lives in the main repository
+   *       <dt><code>@protobuf//some/pkg</code>
+   *       <dd>if this package lives in a repository with "protobuf" as <code>name</code> of a
+   *           repository in WORKSPACE or as apparent name of a Bzlmod dependency of the main module
+   *       <dt><code>@@protobuf~3.19.2//some/pkg</code>
+   *       <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) {
+    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);
+  }
+
   /**
    * Returns the package path, possibly qualified with a repository name.
    *

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

@@ -19,6 +19,8 @@
 import com.google.common.collect.ImmutableMap;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Optional;
 import javax.annotation.Nullable;
 
 /**
@@ -93,4 +95,22 @@ public RepositoryName get(String preMappingName) {
       return RepositoryName.createUnvalidated(preMappingName).toNonVisible(ownerRepo());
     }
   }
+
+  /**
+   * Whether the repo with this mapping is subject to strict deps; when strict deps is off, unknown
+   * apparent names are silently treated as canonical names.
+   */
+  public boolean usesStrictDeps() {
+    return ownerRepo() != null;
+  }
+
+  /**
+   * Returns the first apparent name in this mapping that maps to the given canonical name, if any.
+   */
+  public Optional<String> getInverse(RepositoryName postMappingName) {
+    return repositoryMapping().entrySet().stream()
+        .filter(e -> e.getValue().equals(postMappingName))
+        .map(Entry::getKey)
+        .findFirst();
+  }
 }

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,7 +739,7 @@ private String getAlternateTargetSuggestion(String targetName) {
       String blazeQuerySuggestion =
           String.format(
               "Tip: use `query %s:*` to see all the targets in that package",
-              packageIdentifier.getCanonicalForm());
+              packageIdentifier.getDisplayForm(mainRepositoryMapping));
       return String.format(
           " (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion));
     }
@@ -880,6 +888,7 @@ public static Builder newExternalPackageBuilder(
             LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
             workspaceName,
             starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+            mainRepoMapping,
             mainRepoMapping)
         .setFilename(workspacePath);
   }
@@ -894,7 +903,11 @@ public static Builder newExternalPackageBuilderForBzlmod(
             basePackageId,
             DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES,
             starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
-            repoMapping)
+            repoMapping,
+            // This mapping is *not* the main repository's mapping, but since it is only used to
+            // construct a query command in an error message and the package built here can't be
+            // seen by query, the particular value does not matter.
+            RepositoryMapping.ALWAYS_FALLBACK)
         .setFilename(moduleFilePath);
   }
 
@@ -974,6 +987,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;
 
@@ -1098,10 +1116,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,8 @@ 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 +1349,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

@@ -16,14 +16,13 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/**
- * Unit tests for {@link PackageIdentifier}.
- */
+/** Unit tests for {@link PackageIdentifier}. */
 @RunWith(JUnit4.class)
 public class PackageIdentifierTest {
   @Test
@@ -93,4 +92,36 @@ public void testRunfilesDir() throws Exception {
     assertThat(PackageIdentifier.create("", PathFragment.create("bar/baz")).getRunfilesPath())
         .isEqualTo(PathFragment.create("bar/baz"));
   }
+
+  @Test
+  public void testDisplayFormInMainRepository() throws Exception {
+    PackageIdentifier pkg =
+        PackageIdentifier.create(RepositoryName.MAIN, PathFragment.create("some/pkg"));
+
+    assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("//some/pkg");
+    assertThat(
+            pkg.getDisplayForm(
+                RepositoryMapping.create(
+                    ImmutableMap.of("foo", RepositoryName.create("bar")), RepositoryName.MAIN)))
+        .isEqualTo("//some/pkg");
+  }
+
+  @Test
+  public void testDisplayFormInExternalRepository() throws Exception {
+    RepositoryName repo = RepositoryName.create("canonical");
+    PackageIdentifier pkg = PackageIdentifier.create(repo, PathFragment.create("some/pkg"));
+
+    assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK))
+        .isEqualTo("@canonical//some/pkg");
+    assertThat(
+            pkg.getDisplayForm(
+                RepositoryMapping.create(ImmutableMap.of("local", repo), RepositoryName.MAIN)))
+        .isEqualTo("@local//some/pkg");
+    assertThat(
+            pkg.getDisplayForm(
+                RepositoryMapping.create(
+                    ImmutableMap.of("local", RepositoryName.create("other_repo")),
+                    RepositoryName.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,11 @@ 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));
   }