Fix label unambiguous canonical form to correctly report non-visible repo names

Wyverald · copybara-github · commit 911eedc0badc · 2023-01-20T04:47:05.000-08:00
This also affects Starlark `str(some_label)`. Fixes bazelbuild#17258 PiperOrigin-RevId: 503413176 Change-Id: I864c3c892233ede19b2478c3d9e1c806e4b43c11
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
@@ -183,7 +183,7 @@ public String getCanonicalForm() {
    * package.
    */
   public String getUnambiguousCanonicalForm() {
-    return String.format("@@%s//%s", getRepository().getName(), getPackageFragment());
+    return String.format("@%s//%s", getRepository().getNameWithAt(), getPackageFragment());
   }
 
   /**
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
@@ -29,9 +29,7 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/**
- * Tests for {@link Label}.
- */
+/** Tests for {@link Label}. */
 @RunWith(JUnit4.class)
 public class LabelTest {
 
@@ -175,10 +173,7 @@ public void testIdentities() throws Exception {
     Label l2 = Label.parseCanonical("//foo/bar:baz");
     Label l3 = Label.parseCanonical("//foo/bar:quux");
 
-    new EqualsTester()
-        .addEqualityGroup(l1, l2)
-        .addEqualityGroup(l3)
-        .testEquals();
+    new EqualsTester().addEqualityGroup(l1, l2).addEqualityGroup(l3).testEquals();
   }
 
   @Test
@@ -225,6 +220,7 @@ public void testDotDot() throws Exception {
 
   /**
    * Asserts that creating a label throws a SyntaxException.
+   *
    * @param label the label to create.
    */
   private static void assertSyntaxError(String expectedError, String label) {
@@ -238,12 +234,9 @@ private static void assertSyntaxError(String expectedError, String label) {
 
   @Test
   public void testBadCharacters() throws Exception {
-    assertSyntaxError("target names may not contain ':'",
-                      "//foo:bar:baz");
-    assertSyntaxError("target names may not contain ':'",
-                      "//foo:bar:");
-    assertSyntaxError("target names may not contain ':'",
-                      "//foo/bar::");
+    assertSyntaxError("target names may not contain ':'", "//foo:bar:baz");
+    assertSyntaxError("target names may not contain ':'", "//foo:bar:");
+    assertSyntaxError("target names may not contain ':'", "//foo/bar::");
   }
 
   @Test
@@ -267,9 +260,9 @@ public void testDotAsAPathSegment() throws Exception {
     assertSyntaxError(INVALID_TARGET_NAME, "//foo:./bar/baz");
     // TODO(bazel-team): enable when we have removed the "Workaround" in Label
     // that rewrites broken Labels by removing the trailing '.'
-    //assertSyntaxError(INVALID_PACKAGE_NAME,
+    // assertSyntaxError(INVALID_PACKAGE_NAME,
     //                  "//foo:bar/baz/.");
-    //assertSyntaxError(INVALID_PACKAGE_NAME,
+    // assertSyntaxError(INVALID_PACKAGE_NAME,
     //                  "//foo:.");
   }
 
@@ -280,11 +273,9 @@ public void testTrailingDotSegment() throws Exception {
 
   @Test
   public void testSomeOtherBadLabels() throws Exception {
-    assertSyntaxError("package names may not end with '/'",
-                      "//foo/:bar");
+    assertSyntaxError("package names may not end with '/'", "//foo/:bar");
     assertSyntaxError("package names may not start with '/'", "///p:foo");
-    assertSyntaxError("package names may not contain '//' path separators",
-                      "//a//b:foo");
+    assertSyntaxError("package names may not contain '//' path separators", "//a//b:foo");
   }
 
   @Test
@@ -305,24 +296,22 @@ public void testSomeGoodLabels() throws Exception {
 
   @Test
   public void testDoubleSlashPathSeparator() throws Exception {
-    assertSyntaxError("package names may not contain '//' path separators",
-                      "//foo//bar:baz");
-    assertSyntaxError("target names may not contain '//' path separator",
-                      "//foo:bar//baz");
+    assertSyntaxError("package names may not contain '//' path separators", "//foo//bar:baz");
+    assertSyntaxError("target names may not contain '//' path separator", "//foo:bar//baz");
   }
 
   @Test
   public void testNonPrintableCharacters() throws Exception {
     assertSyntaxError(
-      "target names may not contain non-printable characters: '\\x02'",
-      "//foo:..\002bar");
+        "target names may not contain non-printable characters: '\\x02'", "//foo:..\002bar");
   }
 
   /** Make sure that control characters - such as CR - are escaped on output. */
   @Test
   public void testInvalidLineEndings() throws Exception {
-    assertSyntaxError("invalid target name '..bar\\r': "
-        + "target names may not end with carriage returns", "//foo:..bar\r");
+    assertSyntaxError(
+        "invalid target name '..bar\\r': " + "target names may not end with carriage returns",
+        "//foo:..bar\r");
   }
 
   @Test
@@ -391,6 +380,22 @@ public void testWorkspaceName() throws Exception {
     assertThat(Label.parseCanonical("@//bar:baz").getWorkspaceName()).isEmpty();
   }
 
+  @Test
+  public void testUnambiguousCanonicalForm() throws Exception {
+    assertThat(Label.parseCanonical("//foo/bar:baz").getUnambiguousCanonicalForm())
+        .isEqualTo("@@//foo/bar:baz");
+    assertThat(Label.parseCanonical("@foo//bar:baz").getUnambiguousCanonicalForm())
+        .isEqualTo("@@foo//bar:baz");
+    assertThat(
+            Label.create(
+                    PackageIdentifier.create(
+                        RepositoryName.create("foo").toNonVisible(RepositoryName.create("bar")),
+                        PathFragment.create("baz")),
+                    "quux")
+                .getUnambiguousCanonicalForm())
+        .isEqualTo("@@[unknown repo 'foo' requested from @bar]//baz:quux");
+  }
+
   @Test
   public void starlarkStrAndRepr() throws Exception {
     Label label = Label.parseCanonical("//x");
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
@@ -93,6 +93,22 @@ public void testRunfilesDir() throws Exception {
         .isEqualTo(PathFragment.create("bar/baz"));
   }
 
+  @Test
+  public void testUnambiguousCanonicalForm() throws Exception {
+    assertThat(PackageIdentifier.createInMainRepo("foo/bar").getUnambiguousCanonicalForm())
+        .isEqualTo("@@//foo/bar");
+    assertThat(
+            PackageIdentifier.create("foo", PathFragment.create("bar"))
+                .getUnambiguousCanonicalForm())
+        .isEqualTo("@@foo//bar");
+    assertThat(
+            PackageIdentifier.create(
+                    RepositoryName.create("foo").toNonVisible(RepositoryName.create("bar")),
+                    PathFragment.create("baz"))
+                .getUnambiguousCanonicalForm())
+        .isEqualTo("@@[unknown repo 'foo' requested from @bar]//baz");
+  }
+
   @Test
   public void testDisplayFormInMainRepository() throws Exception {
     PackageIdentifier pkg =

Original file line number	Diff line number	Diff line change
`@@ -183,7 +183,7 @@ public String getCanonicalForm() {`
`183`	`183`	`* package.`
`184`	`184`	`*/`
`185`	`185`	`public String getUnambiguousCanonicalForm() {`
`186`		`- return String.format("@@%s//%s", getRepository().getName(), getPackageFragment());`
	`186`	`+ return String.format("@%s//%s", getRepository().getNameWithAt(), getPackageFragment());`
`187`	`187`	`}`
`188`	`188`
`189`	`189`	`/**`