Change RepositoryMapping's key type to String

An instance of RepositoryName should always represent a post-repo-mapping, canonical repo name (as it already does except when used with the RepositoryMapping class). Well, also except when ownerRepo() is non-null :)

PiperOrigin-RevId: 454141003
Change-Id: Ie79a9c0d6ead773f9b3f7633e1a7d260a0c49970

Author: Wyverald

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

@@ -100,17 +100,15 @@ public static BazelModuleResolutionValue create(
    * module deps and module extensions.
    */
   public final RepositoryMapping getFullRepoMapping(ModuleKey key) {
-    ImmutableMap.Builder<RepositoryName, RepositoryName> mapping = ImmutableMap.builder();
+    ImmutableMap.Builder<String, RepositoryName> mapping = ImmutableMap.builder();
     for (Map.Entry<ModuleExtensionId, ModuleExtensionUsage> e :
         getExtensionUsagesTable().column(key).entrySet()) {
       ModuleExtensionId extensionId = e.getKey();
       ModuleExtensionUsage usage = e.getValue();
       for (Map.Entry<String, String> entry : usage.getImports().entrySet()) {
         String canonicalRepoName =
             getExtensionUniqueNames().get(extensionId) + "." + entry.getValue();
-        mapping.put(
-            RepositoryName.createUnvalidated(entry.getKey()),
-            RepositoryName.createUnvalidated(canonicalRepoName));
+        mapping.put(entry.getKey(), RepositoryName.createUnvalidated(canonicalRepoName));
       }
     }
     return getDepGraph()

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

@@ -106,24 +106,21 @@ public final String getCanonicalRepoName() {
    * extensions. For the full mapping, see {@link BazelModuleResolutionValue#getFullRepoMapping}.
    */
   public final RepositoryMapping getRepoMappingWithBazelDepsOnly() {
-    ImmutableMap.Builder<RepositoryName, RepositoryName> mapping = ImmutableMap.builder();
+    ImmutableMap.Builder<String, RepositoryName> mapping = ImmutableMap.builder();
     // If this is the root module, then the main repository should be visible as `@`.
     if (getKey().equals(ModuleKey.ROOT)) {
-      mapping.put(RepositoryName.MAIN, RepositoryName.MAIN);
+      mapping.put("", RepositoryName.MAIN);
     }
     // Every module should be able to reference itself as @<module name>.
     // If this is the root module, this perfectly falls into @<module name> => @
     if (!getName().isEmpty()) {
-      mapping.put(
-          RepositoryName.createUnvalidated(getName()),
-          RepositoryName.createUnvalidated(getCanonicalRepoName()));
+      mapping.put(getName(), RepositoryName.createUnvalidated(getCanonicalRepoName()));
     }
     for (Map.Entry<String, ModuleKey> dep : getDeps().entrySet()) {
       // Special note: if `dep` is actually the root module, its ModuleKey would be ROOT whose
       // canonicalRepoName is the empty string. This perfectly maps to the main repo ("@").
       mapping.put(
-          RepositoryName.createUnvalidated(dep.getKey()),
-          RepositoryName.createUnvalidated(dep.getValue().getCanonicalRepoName()));
+          dep.getKey(), RepositoryName.createUnvalidated(dep.getValue().getCanonicalRepoName()));
     }
     return RepositoryMapping.create(mapping.buildOrThrow(), getCanonicalRepoName());
   }

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

@@ -102,8 +102,7 @@ private static RepositoryName computeRepoNameWithRepoContext(
       // disregarding the current repo and repo mappings.
       return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg) ? RepositoryName.MAIN : currentRepo;
     }
-    // TODO(b/200024947): Make repo mapping take a string and return a RepositoryName.
-    return repoMapping.get(RepositoryName.createUnvalidated(parts.repo));
+    return repoMapping.get(parts.repo);
   }
 
   /**
@@ -174,7 +173,7 @@ public static Label parseAbsolute(String absName, RepositoryMapping repositoryMa
 
   // TODO(b/200024947): Remove this.
   public static Label parseAbsolute(
-      String absName, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
+      String absName, ImmutableMap<String, RepositoryName> repositoryMapping)
       throws LabelSyntaxException {
     return parseAbsolute(absName, RepositoryMapping.createAllowingFallback(repositoryMapping));
   }
@@ -486,7 +485,7 @@ public Label getRelativeWithRemapping(String relName, RepositoryMapping reposito
 
   // TODO(b/200024947): Remove this.
   public Label getRelativeWithRemapping(
-      String relName, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
+      String relName, ImmutableMap<String, RepositoryName> repositoryMapping)
       throws LabelSyntaxException {
     return getRelativeWithRemapping(
         relName, RepositoryMapping.createAllowingFallback(repositoryMapping));

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

@@ -35,7 +35,7 @@ public abstract class RepositoryMapping {
   // Always fallback to the requested name
   public static final RepositoryMapping ALWAYS_FALLBACK = createAllowingFallback(ImmutableMap.of());
 
-  abstract ImmutableMap<RepositoryName, RepositoryName> repositoryMapping();
+  abstract ImmutableMap<String, RepositoryName> repositoryMapping();
 
   /**
    * The owner repo of this repository mapping. It is for providing useful debug information when
@@ -46,14 +46,14 @@ public abstract class RepositoryMapping {
   abstract String ownerRepo();
 
   public static RepositoryMapping create(
-      Map<RepositoryName, RepositoryName> repositoryMapping, String ownerRepo) {
+      Map<String, RepositoryName> repositoryMapping, String ownerRepo) {
     return new AutoValue_RepositoryMapping(
         ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)),
         Preconditions.checkNotNull(ownerRepo));
   }
 
   public static RepositoryMapping createAllowingFallback(
-      Map<RepositoryName, RepositoryName> repositoryMapping) {
+      Map<String, RepositoryName> repositoryMapping) {
     return new AutoValue_RepositoryMapping(
         ImmutableMap.copyOf(Preconditions.checkNotNull(repositoryMapping)), null);
   }
@@ -62,9 +62,8 @@ public static RepositoryMapping createAllowingFallback(
    * Create a new {@link RepositoryMapping} instance based on existing repo mappings and given
    * additional mappings. If there are conflicts, existing mappings will take precedence.
    */
-  public RepositoryMapping withAdditionalMappings(
-      Map<RepositoryName, RepositoryName> additionalMappings) {
-    HashMap<RepositoryName, RepositoryName> allMappings = new HashMap<>(additionalMappings);
+  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());
   }
@@ -78,13 +77,20 @@ public RepositoryMapping withAdditionalMappings(RepositoryMapping additionalMapp
     return withAdditionalMappings(additionalMappings.repositoryMapping());
   }
 
-  public RepositoryName get(RepositoryName repositoryName) {
-    // If the owner repo is not present, that means we should fallback to the requested repo name.
+  /**
+   * 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);
+    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 repositoryMapping().getOrDefault(repositoryName, repositoryName);
+      return RepositoryName.createUnvalidated(apparentRepoName);
     } else {
-      return repositoryMapping()
-          .getOrDefault(repositoryName, repositoryName.toNonVisible(ownerRepo()));
+      return RepositoryName.createUnvalidated(apparentRepoName).toNonVisible(ownerRepo());
     }
   }
 }

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

@@ -29,7 +29,7 @@
 import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 
-/** The name of an external repository. */
+/** The canonical name of an external repository. */
 public final class RepositoryName {
 
   @SerializationConstant

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

@@ -909,11 +909,13 @@ public TargetPattern parse(String pattern) throws TargetParsingException {
           throw new TargetParsingException(
               "Couldn't find package in target " + pattern, TargetPatterns.Code.PACKAGE_NOT_FOUND);
         }
+        String repoPart = pattern.substring(1, pkgStart);
         try {
-          repository = repoMapping.get(RepositoryName.create(pattern.substring(1, pkgStart)));
+          RepositoryName.validate(repoPart);
         } catch (LabelSyntaxException e) {
           throw new TargetParsingException(e.getMessage(), TargetPatterns.Code.LABEL_SYNTAX_ERROR);
         }
+        repository = repoMapping.get(repoPart);
         if (!repository.isVisible()) {
           throw new TargetParsingException(
               String.format(

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

@@ -221,12 +221,11 @@ public enum ConfigSettingVisibilityPolicy {
   private Set<License.DistributionType> defaultDistributionSet;
 
   /**
-   * The map from each repository to that repository's remappings map.
-   * This is only used in the //external package, it is an empty map for all other packages.
-   * For example, an entry of {"@foo" : {"@x", "@y"}} indicates that, within repository foo,
-   * "@x" should be remapped to "@y".
+   * The map from each repository to that repository's remappings map. This is only used in the
+   * //external package, it is an empty map for all other packages. For example, an entry of {"@foo"
+   * : {"@x", "@y"}} indicates that, within repository foo, "@x" should be remapped to "@y".
    */
-  private ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
+  private ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>>
       externalPackageRepositoryMappings;
 
   /**
@@ -287,25 +286,15 @@ public PackageIdentifier getPackageIdentifier() {
   /**
    * Returns the repository mapping for the requested external repository.
    *
-   * @throws UnsupportedOperationException if called from a package other than
-   *     the //external package
+   * @throws UnsupportedOperationException if called from a package other than the //external
+   *     package
    */
-  public ImmutableMap<RepositoryName, RepositoryName> getRepositoryMapping(
-      RepositoryName repository) {
+  public ImmutableMap<String, RepositoryName> getRepositoryMapping(RepositoryName repository) {
     if (!packageIdentifier.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) {
       throw new UnsupportedOperationException("Can only access the external package repository"
           + "mappings from the //external package");
     }
-
-    // We are passed a repository name as seen from the main repository, not necessarily
-    // a canonical repository name. So, we first have to find the canonical name for the
-    // repository in question before we can look up the mapping for it.
-    RepositoryName actualRepositoryName =
-        externalPackageRepositoryMappings
-            .getOrDefault(RepositoryName.MAIN, ImmutableMap.of())
-            .getOrDefault(repository, repository);
-
-    return externalPackageRepositoryMappings.getOrDefault(actualRepositoryName, ImmutableMap.of());
+    return externalPackageRepositoryMappings.getOrDefault(repository, ImmutableMap.of());
   }
 
   /** Get the repository mapping for this package. */
@@ -319,7 +308,7 @@ public RepositoryMapping getRepositoryMapping() {
    * @throws UnsupportedOperationException if called from a package other than the //external
    *     package
    */
-  ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
+  ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>>
       getExternalPackageRepositoryMappings() {
     if (!packageIdentifier.equals(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER)) {
       throw new UnsupportedOperationException(
@@ -467,7 +456,7 @@ private void finishInit(Builder builder) {
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
     this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
-    ImmutableMap.Builder<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
+    ImmutableMap.Builder<RepositoryName, ImmutableMap<String, RepositoryName>>
         repositoryMappingsBuilder = ImmutableMap.builder();
     if (!builder.externalPackageRepositoryMappings.isEmpty() && !builder.isWorkspace()) {
       // 'repo_mapping' should only be used in the //external package, i.e. should only appear
@@ -935,7 +924,7 @@ public boolean recordLoadedModules() {
 
     // The map from each repository to that repository's remappings map.
     // This is only used in the //external package, it is an empty map for all other packages.
-    private final HashMap<RepositoryName, HashMap<RepositoryName, RepositoryName>>
+    private final HashMap<RepositoryName, HashMap<String, RepositoryName>>
         externalPackageRepositoryMappings = new HashMap<>();
     /**
      * The map of repository reassignments for BUILD packages loaded within external repositories.
@@ -1092,30 +1081,30 @@ String getPackageWorkspaceName() {
     }
 
     /**
-     * Updates the externalPackageRepositoryMappings entry for {@code repoWithin}. Adds new
-     * entry from {@code localName} to {@code mappedName} in {@code repoWithin}'s map.
+     * Updates the externalPackageRepositoryMappings entry for {@code repoWithin}. Adds new entry
+     * from {@code localName} to {@code mappedName} in {@code repoWithin}'s map.
      *
      * @param repoWithin the RepositoryName within which the mapping should apply
-     * @param localName the RepositoryName that actually appears in the WORKSPACE and BUILD files
-     *    in the {@code repoWithin} repository
+     * @param localName the name that actually appears in the WORKSPACE and BUILD files in the
+     *     {@code repoWithin} repository
      * @param mappedName the RepositoryName by which localName should be referenced
      */
     Builder addRepositoryMappingEntry(
-        RepositoryName repoWithin, RepositoryName localName, RepositoryName mappedName) {
-      HashMap<RepositoryName, RepositoryName> mapping =
-          externalPackageRepositoryMappings
-              .computeIfAbsent(repoWithin, (RepositoryName k) -> new HashMap<>());
+        RepositoryName repoWithin, String localName, RepositoryName mappedName) {
+      HashMap<String, RepositoryName> mapping =
+          externalPackageRepositoryMappings.computeIfAbsent(
+              repoWithin, (RepositoryName k) -> new HashMap<>());
       mapping.put(localName, mappedName);
       return this;
     }
 
     /** Adds all the mappings from a given {@link Package}. */
     Builder addRepositoryMappings(Package aPackage) {
-      ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
-          repositoryMappings = aPackage.externalPackageRepositoryMappings;
-      for (Map.Entry<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>> repositoryName :
+      ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>> repositoryMappings =
+          aPackage.externalPackageRepositoryMappings;
+      for (Map.Entry<RepositoryName, ImmutableMap<String, RepositoryName>> repositoryName :
           repositoryMappings.entrySet()) {
-        for (Map.Entry<RepositoryName, RepositoryName> repositoryNameRepositoryNameEntry :
+        for (Map.Entry<String, RepositoryName> repositoryNameRepositoryNameEntry :
             repositoryName.getValue().entrySet()) {
           addRepositoryMappingEntry(
               repositoryName.getKey(),
@@ -1163,7 +1152,7 @@ Label getBuildFileLabel() {
      * the main workspace to the canonical main name '@').
      */
     RepositoryMapping getRepositoryMappingFor(RepositoryName name) {
-      Map<RepositoryName, RepositoryName> mapping = externalPackageRepositoryMappings.get(name);
+      Map<String, RepositoryName> mapping = externalPackageRepositoryMappings.get(name);
       if (mapping == null) {
         return RepositoryMapping.ALWAYS_FALLBACK;
       } else {

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

@@ -105,7 +105,7 @@ public static void addMainRepoEntry(
       // is not valid.
       builder.addRepositoryMappingEntry(
           RepositoryName.create(externalRepoName),
-          RepositoryName.create(builder.getPackageWorkspaceName()),
+          builder.getPackageWorkspaceName(),
           RepositoryName.MAIN);
     }
   }
@@ -128,15 +128,26 @@ public static void addRepoMappings(
         // prefixed with an @.
         if (!e.getKey().startsWith("@")) {
           throw new LabelSyntaxException(
-              "invalid repository name '" + e.getKey() + "': repo names must start with '@'");
+              "invalid repository name '"
+                  + e.getKey()
+                  + "': repo names used in the repo_mapping attribute must start with '@'");
         }
         if (!e.getValue().startsWith("@")) {
           throw new LabelSyntaxException(
-              "invalid repository name '" + e.getValue() + "': repo names must start with '@'");
+              "invalid repository name '"
+                  + e.getValue()
+                  + "': repo names used in the repo_mapping attribute must start with '@'");
+        }
+        if (!WorkspaceGlobals.isLegalWorkspaceName(e.getKey().substring(1))) {
+          throw new LabelSyntaxException(
+              "invalid repository name '"
+                  + e.getKey().substring(1)
+                  + "': must start with a letter and contain only letters, digits, '.', '-', or"
+                  + " '_'");
         }
         builder.addRepositoryMappingEntry(
             RepositoryName.create(externalRepoName),
-            RepositoryName.create(e.getKey().substring(1)),
+            e.getKey().substring(1),
             RepositoryName.create(e.getValue().substring(1)));
       }
     }

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

@@ -102,7 +102,7 @@ public String toString() {
   private final ImmutableMap<String, Object> bindings;
   private final ImmutableMap<String, Module> loadedModules;
   private final ImmutableMap<String, Integer> loadToChunkMap;
-  private final ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
+  private final ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>>
       repositoryMapping;
   // Mapping of the relative paths of the incrementally updated managed directories
   // to the managing external repositories
@@ -221,8 +221,7 @@ public ImmutableMap<String, Integer> getLoadToChunkMap() {
     return loadToChunkMap;
   }
 
-  public ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
-      getRepositoryMapping() {
+  public ImmutableMap<RepositoryName, ImmutableMap<String, RepositoryName>> getRepositoryMapping() {
     return repositoryMapping;
   }
 

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

@@ -100,8 +100,7 @@ public void workspace(
     }
     // Add entry in repository map from "@name" --> "@" to avoid issue where bazel
     // treats references to @name as a separate external repo
-    builder.addRepositoryMappingEntry(
-        RepositoryName.MAIN, RepositoryName.createUnvalidated(name), RepositoryName.MAIN);
+    builder.addRepositoryMappingEntry(RepositoryName.MAIN, name, RepositoryName.MAIN);
     parseManagedDirectories(
         thread.getSemantics().getBool(BuildLanguageOptions.INCOMPATIBLE_DISABLE_MANAGE_DIRECTORIES),
         Dict.cast(managedDirectories, String.class, Object.class, "managed_directories"));