'@'-prefixed canonical repo names for Bzlmod repos

See https://docs.google.com/document/d/1N81qfCa8oskCk5LqTW-LNthy6EBrDot7bdUsjz6JFC4/edit#heading=h.z37pyziy8h33

* RepositoryName syntax is relaxed to allow an extra '@' in the beginning, signifying that the RepositoryName should bypass RepositoryMapping
* All repos generated by Bzlmod will have an extra '@' prefixed to their names (except for well-known modules)
* TODO: strip the '@' when generating paths under $outputBase/external or $execRoot/external

Work towards #15593.

RELNOTES: When Bzlmod is enabled, all Bzlmod-generated repos will have an extra '@' prepended to their names. This effectively enables the canonical label literal syntax for Bzlmod-generated repos (`@@canonicalRepoName//pkg:target`; see https://docs.google.com/document/d/1N81qfCa8oskCk5LqTW-LNthy6EBrDot7bdUsjz6JFC4/edit?usp=sharing).
PiperOrigin-RevId: 454156392
Change-Id: I2fb08495f066cfd15cc344534925f674a63764f6

Author: Wyverald

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

@@ -160,6 +160,11 @@ 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

@@ -31,8 +31,16 @@ public abstract class ModuleKey {
    * Therefore, we have to keep its canonical repository name the same as its well known repository
    * name. Eg. "@com_google_protobuf//:protoc" is used for --proto_compiler flag.
    *
-   * <p>TODO(pcloudy): Remove this hack after figuring out a correct way to deal with the above
-   * situation.
+   * <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 get rid of usage of com_google_protobuf in flag defaults, and make all
+   * flag values go through repo mapping, we can remove the concept of well-known modules
+   * altogether.
    */
   private static final ImmutableMap<String, RepositoryName> WELL_KNOWN_MODULES =
       ImmutableMap.of(
@@ -69,9 +77,7 @@ public RepositoryName getCanonicalRepoName() {
     if (ROOT.equals(this)) {
       return RepositoryName.MAIN;
     }
-    if (getVersion().isEmpty()) {
-      return RepositoryName.createUnvalidated(getName() + ".override");
-    }
-    return RepositoryName.createUnvalidated(getName() + "." + getVersion());
+    return RepositoryName.createUnvalidated(
+        String.format("@%s.%s", getName(), getVersion().isEmpty() ? "override" : getVersion()));
   }
 }

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

@@ -81,16 +81,20 @@ public RepositoryMapping withAdditionalMappings(RepositoryMapping additionalMapp
    * 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 apparentRepoName) {
-    RepositoryName canonicalRepoName = repositoryMapping().get(apparentRepoName);
+  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;
     }
     // If the owner repo is not present, that means we should fall back to the requested repo name.
     if (ownerRepo() == null) {
-      return RepositoryName.createUnvalidated(apparentRepoName);
+      return RepositoryName.createUnvalidated(preMappingName);
     } else {
-      return RepositoryName.createUnvalidated(apparentRepoName).toNonVisible(ownerRepo());
+      return RepositoryName.createUnvalidated(preMappingName).toNonVisible(ownerRepo());
     }
   }
 }

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

@@ -37,7 +37,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\\-.#]*");
 
   private static final LoadingCache<String, RepositoryName> repositoryNameCache =
       Caffeine.newBuilder()
@@ -49,8 +49,7 @@ public final class RepositoryName {
               });
 
   /**
-   * Makes sure that name is a valid repository name and creates a new RepositoryName using it. The
-   * given string must not begin with a '@'.
+   * Makes sure that name is a valid repository name and creates a new RepositoryName using it.
    *
    * @throws LabelSyntaxException if the name is invalid
    */
@@ -66,11 +65,8 @@ public static RepositoryName create(String name) throws LabelSyntaxException {
     }
   }
 
-  /**
-   * Creates a RepositoryName from a known-valid string. The given string must not begin with a '@'.
-   */
+  /** Creates a RepositoryName from a known-valid string. */
   public static RepositoryName createUnvalidated(String name) {
-    Preconditions.checkArgument(!name.startsWith("@"), "Do not prefix @ to repo names!");
     if (name.isEmpty()) {
       // NOTE(wyv): Without this `if` clause, a lot of Google-internal integration tests would start
       //   failing. This suggests to me that something is comparing RepositoryName objects using
@@ -147,8 +143,8 @@ static void validate(String name) throws LabelSyntaxException {
 
     if (!VALID_REPO_NAME.matcher(name).matches()) {
       throw LabelParser.syntaxErrorf(
-          "invalid repository name '@%s': repo names may contain only A-Z, a-z, 0-9, '-', '_' and"
-              + " '.'",
+          "invalid repository name '@%s': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.'"
+              + " and '#'",
           StringUtilities.sanitizeControlChars(name));
     }
   }

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(
@@ -156,15 +156,14 @@ public void createValue_moduleExtensions() throws Exception {
 
     ModuleExtensionId maven =
         ModuleExtensionId.create(
-            Label.parseAbsoluteUnchecked("@rules_jvm_external.1.0//:defs.bzl"), "maven");
+            Label.parseCanonical("@@rules_jvm_external.1.0//:defs.bzl"), "maven");
     ModuleExtensionId pip =
-        ModuleExtensionId.create(
-            Label.parseAbsoluteUnchecked("@rules_python.2.0//:defs.bzl"), "pip");
+        ModuleExtensionId.create(Label.parseCanonical("@@rules_python.2.0//:defs.bzl"), "pip");
     ModuleExtensionId myext =
-        ModuleExtensionId.create(Label.parseAbsoluteUnchecked("@dep.2.0//:defs.bzl"), "myext");
+        ModuleExtensionId.create(Label.parseCanonical("@@dep.2.0//:defs.bzl"), "myext");
     ModuleExtensionId myext2 =
         ModuleExtensionId.create(
-            Label.parseAbsoluteUnchecked("@dep.2.0//incredible:conflict.bzl"), "myext");
+            Label.parseCanonical("@@dep.2.0//incredible:conflict.bzl"), "myext");
 
     BazelModuleResolutionValue value =
         BazelModuleResolutionFunction.createValue(depGraph, depGraph, ImmutableMap.of());
@@ -182,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(
@@ -196,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,17 +147,18 @@ public void getRepoSpec_bazelModule() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("C.2.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@C.2.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("C.2.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C.2.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
-                .setAttributes(ImmutableMap.of("name", "C.2.0", "path", "/usr/local/modules/C.2.0"))
+                .setAttributes(
+                    ImmutableMap.of("name", "@C.2.0", "path", "/usr/local/modules/@C.2.0"))
                 .build());
   }
 
@@ -178,19 +179,20 @@ public void getRepoSpec_nonRegistryOverride() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("C.override")), evaluationContext);
+        evaluator.evaluate(
+            ImmutableList.of(getRepoSpecByNameKey("@C.override")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("C.override")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C.override")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
                 .setAttributes(
                     ImmutableMap.of(
-                        "name", "C.override",
+                        "name", "@C.override",
                         "path", "/foo/bar/C"))
                 .build());
   }
@@ -214,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("C.3.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@C.3.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("C.3.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@C.3.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
@@ -230,9 +232,9 @@ public void getRepoSpec_singleVersionOverride() throws Exception {
                 .setAttributes(
                     ImmutableMap.of(
                         "name",
-                        "C.3.0",
+                        "@C.3.0",
                         "path",
-                        "/usr/local/modules/C.3.0",
+                        "/usr/local/modules/@C.3.0",
                         "patches",
                         ImmutableList.of("//:foo.patch"),
                         "patch_args",
@@ -262,17 +264,18 @@ public void getRepoSpec_multipleVersionOverride() throws Exception {
     ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));
 
     EvaluationResult<GetRepoSpecByNameValue> result =
-        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("D.2.0")), evaluationContext);
+        evaluator.evaluate(ImmutableList.of(getRepoSpecByNameKey("@D.2.0")), evaluationContext);
     if (result.hasError()) {
       fail(result.getError().toString());
     }
 
-    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("D.2.0")).rule();
+    Optional<RepoSpec> repoSpec = result.get(getRepoSpecByNameKey("@D.2.0")).rule();
     assertThat(repoSpec)
         .hasValue(
             RepoSpec.builder()
                 .setRuleClassName("local_repository")
-                .setAttributes(ImmutableMap.of("name", "D.2.0", "path", "/usr/local/modules/D.2.0"))
+                .setAttributes(
+                    ImmutableMap.of("name", "@D.2.0", "path", "/usr/local/modules/@D.2.0"))
                 .build());
   }