Fix canonical label literals for well-known modules (and the main repo)

Currently, the canonical label literal syntax is only used when Bzlmod is enabled. This is not changing.

However, right now the label "@@//foo:bar" doesn't work even when Bzlmod is enabled. This is because the current implementation doesn't actually treat @@-prefixed labels as "canonical label literals"; instead, it just tries to find a repo whose canonical name is "@" (i.e. with the first '@' removed). Obviously no such repo exists (since the main repo's canonical name is the empty string). This implementation seems strange, but coupled with the hack where we prepend an '@' to the canonical names of Bzlmod-generated repos, this achieves the effect that canonical label literals are only enabled when Bzlmod is enabled. (bazelbuild@de37930 is the CL that introduced this implementation.)

This CL fixes it so that we do Bzlmod-exclusive canonical label literals in a more principled way. We remove the hack where we prepend an extra '@' to Bzlmod-generated repo names, and instead just skip repo mapping directly during label parsing if we see two '@'s. Additionally, we can now change the behavior of str(Label) to prepend the extra '@' iff Bzlmod is enabled. Everyone is happy :)

Work towards bazelbuild#15916

PiperOrigin-RevId: 469949029
Change-Id: Id77dc13752e9d945a99a823e91a2c4d9d8342623

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java

@@ -164,11 +164,6 @@ static BazelModuleResolutionValue createValue(
     for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
       String bestName =
           id.getBzlFileLabel().getRepository().getName() + "~" + id.getExtensionName();
-      if (!bestName.startsWith("@")) {
-        // We have to special-case extensions hosted in well-known modules, because *all* repos
-        // generated by Bzlmod have to have an '@'-prefixed name, except well-known modules.
-        bestName = "@" + bestName;
-      }
       if (extensionUniqueNames.putIfAbsent(bestName, id) == null) {
         continue;
       }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleKey.java

@@ -30,13 +30,6 @@ public abstract class ModuleKey {
    * normal format seen in {@link #getCanonicalRepoName()}) due to backwards compatibility reasons.
    * For example, bazel_tools must be known as "@bazel_tools" for WORKSPACE repos to work correctly.
    *
-   * <p>NOTE(wyv): We don't prepend an '@' to the repo names of well-known modules. This is because
-   * we still need the repo name to be 'bazel_tools' (not '@bazel_tools') since the command line
-   * flags still don't go through repo mapping yet, and they're asking for '@bazel_tools//:thing',
-   * not '@@bazel_tools//:thing'. We can't switch to the latter syntax because it doesn't work if
-   * Bzlmod is not enabled. On the other hand, this means we cannot write '@@bazel_tools//:thing' to
-   * bypass repo mapping at all, which can be awkward.
-   *
    * <p>TODO(wyv): After we make all flag values go through repo mapping, we can remove the concept
    * of well-known modules altogether.
    */
@@ -76,6 +69,6 @@ public RepositoryName getCanonicalRepoName() {
       return RepositoryName.MAIN;
     }
     return RepositoryName.createUnvalidated(
-        String.format("@%s~%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
+        String.format("%s~%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
   }
 }

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

@@ -144,6 +144,10 @@ private static RepositoryName computeRepoNameWithRepoContext(
           ? RepositoryName.MAIN
           : repoContext.currentRepo();
     }
+    if (parts.repoIsCanonical) {
+      // This label uses the canonical label literal syntax starting with two @'s ("@@foo//bar").
+      return RepositoryName.createUnvalidated(parts.repo);
+    }
     return repoContext.repoMapping().get(parts.repo);
   }
 
@@ -621,11 +625,24 @@ public void repr(Printer printer) {
 
   @Override
   public void str(Printer printer, StarlarkSemantics semantics) {
-    if (semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION)) {
-      printer.append(getUnambiguousCanonicalForm());
-    } else {
+    if (getRepository().isMain()
+        && !semantics.getBool(
+            BuildLanguageOptions.INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION)) {
+      // If this label is in the main repo and we're not using unambiguous label stringification,
+      // the result should always be "//foo:bar".
       printer.append(getCanonicalForm());
+      return;
+    }
+
+    if (semantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
+      // If Bzlmod is enabled, we use canonical label literal syntax here and prepend an extra '@'.
+      // So the result looks like "@@//foo:bar" for the main repo and "@@foo~1.0//bar:quux" for
+      // other repos.
+      printer.append('@');
     }
+    // If Bzlmod is not enabled, we just use a single '@'.
+    // So the result looks like "@//foo:bar" for the main repo and "@foo//bar:quux" for other repos.
+    printer.append(getUnambiguousCanonicalForm());
   }
 
   @Override

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

@@ -28,11 +28,20 @@ private LabelParser() {}
    */
   static final class Parts {
     /**
-     * The {@code @repo} part of the string (sans the {@literal @}); can be null if it doesn't have
-     * such a part.
+     * The {@code @repo} or {@code @@canonical_repo} part of the string (sans any leading
+     * {@literal @}s); can be null if it doesn't have such a part (i.e. if it doesn't start with a
+     * {@literal @}).
      */
     @Nullable final String repo;
-    /** Whether the package part of the string is prefixed by double-slash. */
+    /**
+     * Whether the repo part is using the canonical repo syntax (two {@literal @}s) or not (one
+     * {@literal @}). If there is no repo part, this is false.
+     */
+    final boolean repoIsCanonical;
+    /**
+     * Whether the package part of the string is prefixed by double-slash. This can only be false if
+     * the repo part is missing.
+     */
     final boolean pkgIsAbsolute;
     /** The package part of the string (sans double-slash, if any). */
     final String pkg;
@@ -42,51 +51,77 @@ static final class Parts {
     final String raw;
 
     private Parts(
-        @Nullable String repo, boolean pkgIsAbsolute, String pkg, String target, String raw) {
+        @Nullable String repo,
+        boolean repoIsCanonical,
+        boolean pkgIsAbsolute,
+        String pkg,
+        String target,
+        String raw) {
       this.repo = repo;
+      this.repoIsCanonical = repoIsCanonical;
       this.pkgIsAbsolute = pkgIsAbsolute;
       this.pkg = pkg;
       this.target = target;
       this.raw = raw;
     }
 
     private static Parts validateAndCreate(
-        @Nullable String repo, boolean pkgIsAbsolute, String pkg, String target, String raw)
+        @Nullable String repo,
+        boolean repoIsCanonical,
+        boolean pkgIsAbsolute,
+        String pkg,
+        String target,
+        String raw)
         throws LabelSyntaxException {
       validateRepoName(repo);
       validatePackageName(pkg, target);
-      return new Parts(repo, pkgIsAbsolute, pkg, validateAndProcessTargetName(pkg, target), raw);
+      return new Parts(
+          repo,
+          repoIsCanonical,
+          pkgIsAbsolute,
+          pkg,
+          validateAndProcessTargetName(pkg, target),
+          raw);
     }
 
     /**
      * Parses a raw label string into parts. The logic can be summarized by the following table:
      *
      * {@code
-     *  raw                 | repo   | pkgIsAbsolute | pkg       | target
-     * ---------------------+--------+---------------+-----------+-----------
-     *  foo/bar             | null   | false         | ""        | "foo/bar"
-     *  //foo/bar           | null   | true          | "foo/bar" | "bar"
-     *  @repo               | "repo" | true          | ""        | "repo"
-     *  @repo//foo/bar      | "repo" | true          | "foo/bar" | "bar"
-     *  :quux               | null   | false         | ""        | "quux"
-     *  foo/bar:quux        | null   | false         | "foo/bar" | "quux"
-     *  //foo/bar:quux      | null   | true          | "foo/bar" | "quux"
-     *  @repo//foo/bar:quux | "repo" | true          | "foo/bar" | "quux"
+     *  raw                  | repo   | repoIsCanonical | pkgIsAbsolute | pkg       | target
+     * ----------------------+--------+-----------------+---------------+-----------+-----------
+     *  foo/bar              | null   | false           | false         | ""        | "foo/bar"
+     *  //foo/bar            | null   | false           | true          | "foo/bar" | "bar"
+     *  @repo                | "repo" | false           | true          | ""        | "repo"
+     *  @@repo               | "repo" | true            | true          | ""        | "repo"
+     *  @repo//foo/bar       | "repo" | false           | true          | "foo/bar" | "bar"
+     *  @@repo//foo/bar      | "repo" | true            | true          | "foo/bar" | "bar"
+     *  :quux                | null   | false           | false         | ""        | "quux"
+     *  foo/bar:quux         | null   | false           | false         | "foo/bar" | "quux"
+     *  //foo/bar:quux       | null   | false           | true          | "foo/bar" | "quux"
+     *  @repo//foo/bar:quux  | "repo" | false           | true          | "foo/bar" | "quux"
+     *  @@repo//foo/bar:quux | "repo" | true            | true          | "foo/bar" | "quux"
      * }
      */
     static Parts parse(String rawLabel) throws LabelSyntaxException {
       @Nullable final String repo;
+      final boolean repoIsCanonical = rawLabel.startsWith("@@");
       final int startOfPackage;
       final int doubleSlashIndex = rawLabel.indexOf("//");
       final boolean pkgIsAbsolute;
       if (rawLabel.startsWith("@")) {
         if (doubleSlashIndex < 0) {
           // Special case: the label "@foo" is synonymous with "@foo//:foo".
-          repo = rawLabel.substring(1);
+          repo = rawLabel.substring(repoIsCanonical ? 2 : 1);
           return validateAndCreate(
-              repo, /*pkgIsAbsolute=*/ true, /*pkg=*/ "", /*target=*/ repo, rawLabel);
+              repo,
+              repoIsCanonical,
+              /*pkgIsAbsolute=*/ true,
+              /*pkg=*/ "",
+              /*target=*/ repo,
+              rawLabel);
         } else {
-          repo = rawLabel.substring(1, doubleSlashIndex);
+          repo = rawLabel.substring(repoIsCanonical ? 2 : 1, doubleSlashIndex);
           startOfPackage = doubleSlashIndex + 2;
           pkgIsAbsolute = true;
         }
@@ -114,7 +149,7 @@ static Parts parse(String rawLabel) throws LabelSyntaxException {
         pkg = "";
         target = rawLabel.substring(startOfPackage);
       }
-      return validateAndCreate(repo, pkgIsAbsolute, pkg, target, rawLabel);
+      return validateAndCreate(repo, repoIsCanonical, pkgIsAbsolute, pkg, target, rawLabel);
     }
 
     private static void validateRepoName(@Nullable String repo) throws LabelSyntaxException {

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

@@ -82,10 +82,6 @@ public RepositoryMapping withAdditionalMappings(RepositoryMapping additionalMapp
    * provided apparent repo name is assumed to be valid.
    */
   public RepositoryName get(String preMappingName) {
-    if (preMappingName.startsWith("@")) {
-      // The given name is actually a canonical, post-mapping repo name already.
-      return RepositoryName.createUnvalidated(preMappingName);
-    }
     RepositoryName canonicalRepoName = repositoryMapping().get(preMappingName);
     if (canonicalRepoName != null) {
       return canonicalRepoName;

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

@@ -39,7 +39,7 @@ public final class RepositoryName {
 
   @SerializationConstant public static final RepositoryName MAIN = new RepositoryName("");
 
-  private static final Pattern VALID_REPO_NAME = Pattern.compile("@?[\\w\\-.~]*");
+  private static final Pattern VALID_REPO_NAME = Pattern.compile("[\\w\\-.~]*");
 
   // Must start with a letter. Can contain ASCII letters and digits, underscore, dash, and dot.
   private static final Pattern VALID_USER_PROVIDED_NAME = Pattern.compile("[a-zA-Z][-.\\w]*$");

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunctionTest.java

@@ -93,13 +93,13 @@ public void createValue_basic() throws Exception {
         .containsExactly(
             RepositoryName.MAIN,
             ModuleKey.ROOT,
-            RepositoryName.create("@dep~1.0"),
+            RepositoryName.create("dep~1.0"),
             createModuleKey("dep", "1.0"),
-            RepositoryName.create("@dep~2.0"),
+            RepositoryName.create("dep~2.0"),
             createModuleKey("dep", "2.0"),
-            RepositoryName.create("@rules_cc~1.0"),
+            RepositoryName.create("rules_cc~1.0"),
             createModuleKey("rules_cc", "1.0"),
-            RepositoryName.create("@rules_java~override"),
+            RepositoryName.create("rules_java~override"),
             createModuleKey("rules_java", ""));
     assertThat(value.getModuleNameLookup())
         .containsExactly(
@@ -181,10 +181,10 @@ public void createValue_moduleExtensions() throws Exception {
 
     assertThat(value.getExtensionUniqueNames())
         .containsExactly(
-            maven, "@rules_jvm_external~1.0~maven",
-            pip, "@rules_python~2.0~pip",
-            myext, "@dep~2.0~myext",
-            myext2, "@dep~2.0~myext2");
+            maven, "rules_jvm_external~1.0~maven",
+            pip, "rules_python~2.0~pip",
+            myext, "dep~2.0~myext",
+            myext2, "dep~2.0~myext2");
 
     assertThat(value.getFullRepoMapping(ModuleKey.ROOT))
         .isEqualTo(
@@ -195,26 +195,26 @@ public void createValue_moduleExtensions() throws Exception {
                 "root",
                 "",
                 "rje",
-                "@rules_jvm_external~1.0",
+                "rules_jvm_external~1.0",
                 "rpy",
-                "@rules_python~2.0",
+                "rules_python~2.0",
                 "av",
-                "@rules_jvm_external~1.0~maven~autovalue",
+                "rules_jvm_external~1.0~maven~autovalue",
                 "numpy",
-                "@rules_python~2.0~pip~numpy"));
+                "rules_python~2.0~pip~numpy"));
     assertThat(value.getFullRepoMapping(depKey))
         .isEqualTo(
             createRepositoryMapping(
                 depKey,
                 "dep",
-                "@dep~2.0",
+                "dep~2.0",
                 "rules_python",
-                "@rules_python~2.0",
+                "rules_python~2.0",
                 "np",
-                "@rules_python~2.0~pip~numpy",
+                "rules_python~2.0~pip~numpy",
                 "oneext",
-                "@dep~2.0~myext~myext",
+                "dep~2.0~myext~myext",
                 "twoext",
-                "@dep~2.0~myext2~myext"));
+                "dep~2.0~myext2~myext"));
   }
 }

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodRepoRuleHelperTest.java

@@ -147,18 +147,18 @@ public void getRepoSpec_bazelModule() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@ccc~2.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("ccc~2.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ccc~2.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ccc~2.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
-                    ImmutableMap.of("name", "@ccc~2.0", "path", "/usr/local/modules/@ccc~2.0"))
+                    ImmutableMap.of("name", "ccc~2.0", "path", "/usr/local/modules/ccc~2.0"))
                 .build());
   }
 
@@ -180,19 +180,19 @@ public void getRepoSpec_nonRegistryOverride() throws Exception {
 
     EvaluationResult<GetRepoSpecByNameValue> result =
         evaluator.evaluate(
-            ImmutableList.of(getRepoSpecByNameKey("@ccc~override")), evaluationContext);
+            ImmutableList.of(getRepoSpecByNameKey("ccc~override")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ccc~override")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ccc~override")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
                     ImmutableMap.of(
-                        "name", "@ccc~override",
+                        "name", "ccc~override",
                         "path", "/foo/bar/C"))
                 .build());
   }
@@ -216,12 +216,12 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@ccc~3.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("ccc~3.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ccc~3.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ccc~3.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
@@ -232,9 +232,9 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
                 .setAttributes(
                     ImmutableMap.of(
                         "name",
-                        "@ccc~3.0",
+                        "ccc~3.0",
                         "path",
-                        "/usr/local/modules/@ccc~3.0",
+                        "/usr/local/modules/ccc~3.0",
                         "patches",
                         ImmutableList.of("//:foo.patch"),
                         "patch_args",
@@ -264,18 +264,18 @@ public void getRepoSpec_multipleVersionOverride() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@ddd~2.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("ddd~2.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@ddd~2.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("ddd~2.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
-                    ImmutableMap.of("name", "@ddd~2.0", "path", "/usr/local/modules/@ddd~2.0"))
+                    ImmutableMap.of("name", "ddd~2.0", "path", "/usr/local/modules/ddd~2.0"))
                 .build());
   }