Add flag --incompatible_use_plus_in_repo_names

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD

@@ -26,6 +26,7 @@ java_library(
     ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/net/starlark/java/eval",
         "//third_party:auto_value",
         "//third_party:gson",
@@ -251,6 +252,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/profiler",
         "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input",
         "//src/main/java/com/google/devtools/build/lib/rules:repository/repository_directory_value",

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

@@ -33,7 +33,9 @@
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.LabelConverter;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
+import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -42,6 +44,7 @@
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Map.Entry;
 import javax.annotation.Nullable;
+import net.starlark.java.eval.StarlarkSemantics;
 
 /**
  * This function runs Bazel module resolution, extracts the dependency graph from it and creates a
@@ -58,13 +61,14 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       throws BazelDepGraphFunctionException, InterruptedException {
     BazelModuleResolutionValue selectionResult =
         (BazelModuleResolutionValue) env.getValue(BazelModuleResolutionValue.KEY);
+    StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     if (env.valuesMissing()) {
       return null;
     }
     var depGraph = selectionResult.getResolvedDepGraph();
 
     ImmutableBiMap<RepositoryName, ModuleKey> canonicalRepoNameLookup =
-        computeCanonicalRepoNameLookup(depGraph);
+        computeCanonicalRepoNameLookup(depGraph, starlarkSemantics);
     ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById;
     try {
       extensionUsagesById = getExtensionUsagesById(depGraph, canonicalRepoNameLookup.inverse());
@@ -73,14 +77,17 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     }
 
     ImmutableBiMap<String, ModuleExtensionId> extensionUniqueNames =
-        calculateUniqueNameForUsedExtensionId(extensionUsagesById);
+        calculateUniqueNameForUsedExtensionId(extensionUsagesById, starlarkSemantics);
 
     return BazelDepGraphValue.create(
         depGraph,
         canonicalRepoNameLookup,
         depGraph.values().stream().map(AbridgedModule::from).collect(toImmutableList()),
         extensionUsagesById,
-        extensionUniqueNames.inverse());
+        extensionUniqueNames.inverse(),
+        starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES)
+            ? '+'
+            : '~');
   }
 
   private static ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
@@ -126,7 +133,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
   }
 
   private static ImmutableBiMap<RepositoryName, ModuleKey> computeCanonicalRepoNameLookup(
-      ImmutableMap<ModuleKey, Module> depGraph) {
+      ImmutableMap<ModuleKey, Module> depGraph, StarlarkSemantics semantics) {
     // Find modules with multiple versions in the dep graph. Currently, the only source of such
     // modules is multiple_version_override.
     ImmutableSet<String> multipleVersionsModules =
@@ -151,13 +158,14 @@ private static ImmutableBiMap<RepositoryName, ModuleKey> computeCanonicalRepoNam
             toImmutableBiMap(
                 key ->
                     multipleVersionsModules.contains(key.getName())
-                        ? key.getCanonicalRepoNameWithVersion()
-                        : key.getCanonicalRepoNameWithoutVersion(),
+                        ? key.getCanonicalRepoNameWithVersion(semantics)
+                        : key.getCanonicalRepoNameWithoutVersion(semantics),
                 key -> key));
   }
 
   private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExtensionId(
-      ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
+      ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById,
+      StarlarkSemantics starlarkSemantics) {
     // Calculate a unique name for each used extension id with the following property that is
     // required for BzlmodRepoRuleFunction to unambiguously identify the extension that generates a
     // given repo:
@@ -166,18 +174,23 @@ private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExte
     BiMap<String, ModuleExtensionId> extensionUniqueNames = HashBiMap.create();
     for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
       int attempt = 1;
-      while (extensionUniqueNames.putIfAbsent(makeUniqueNameCandidate(id, attempt), id) != null) {
+      while (extensionUniqueNames.putIfAbsent(
+              makeUniqueNameCandidate(id, attempt, starlarkSemantics), id)
+          != null) {
         attempt++;
       }
     }
     return ImmutableBiMap.copyOf(extensionUniqueNames);
   }
 
-  private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) {
+  private static String makeUniqueNameCandidate(
+      ModuleExtensionId id, int attempt, StarlarkSemantics starlarkSemantics) {
+    boolean usePlus =
+        starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES);
     // Ensure that the resulting extension name (and thus the repository names derived from it) do
     // not start with a tilde.
     RepositoryName repository = id.getBzlFileLabel().getRepository();
-    String nonEmptyRepoPart = repository.isMain() ? "_main" : repository.getName();
+    String nonEmptyRepoPart = repository.isMain() && !usePlus ? "_main" : repository.getName();
     // When using a namespace, prefix the extension name with "_" to distinguish the prefix from
     // those generated by non-namespaced extension usages. Extension names are identified by their
     // Starlark identifier, which in the case of an exported symbol cannot start with "_".
@@ -191,14 +204,18 @@ private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt)
         .map(
             namespace ->
                 String.format(
-                    "%s~_%s%s~%s~%s~%s",
+                    usePlus ? "%s+_%s%s+%s+%s+%s" : "%s~_%s%s~%s~%s~%s",
                     nonEmptyRepoPart,
                     id.getExtensionName(),
                     extensionNameDisambiguator,
                     namespace.getModule().getName(),
                     namespace.getModule().getVersion(),
                     namespace.getUsageExportedName()))
-        .orElse(nonEmptyRepoPart + "~" + id.getExtensionName() + extensionNameDisambiguator);
+        .orElse(
+            nonEmptyRepoPart
+                + (usePlus ? "+" : "~")
+                + id.getExtensionName()
+                + extensionNameDisambiguator);
   }
 
   static class BazelDepGraphFunctionException extends SkyFunctionException {

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

@@ -15,7 +15,6 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import static com.google.common.collect.ImmutableMap.toImmutableMap;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableBiMap;
@@ -43,13 +42,15 @@ public static BazelDepGraphValue create(
       ImmutableMap<RepositoryName, ModuleKey> canonicalRepoNameLookup,
       ImmutableList<AbridgedModule> abridgedModules,
       ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesTable,
-      ImmutableMap<ModuleExtensionId, String> extensionUniqueNames) {
+      ImmutableMap<ModuleExtensionId, String> extensionUniqueNames,
+      char repoNameSeparator) {
     return new AutoValue_BazelDepGraphValue(
         depGraph,
         ImmutableBiMap.copyOf(canonicalRepoNameLookup),
         abridgedModules,
         extensionUsagesTable,
-        extensionUniqueNames);
+        extensionUniqueNames,
+        repoNameSeparator);
   }
 
   public static BazelDepGraphValue createEmptyDepGraph() {
@@ -65,22 +66,16 @@ public static BazelDepGraphValue createEmptyDepGraph() {
             .build();
 
     ImmutableMap<ModuleKey, Module> emptyDepGraph = ImmutableMap.of(ModuleKey.ROOT, root);
-
     ImmutableMap<RepositoryName, ModuleKey> canonicalRepoNameLookup =
-        emptyDepGraph.keySet().stream()
-            .collect(
-                toImmutableMap(
-                    // All modules in the empty dep graph (just the root module) have an empty
-                    // version, so the choice of including it in the canonical repo name does not
-                    // matter.
-                    ModuleKey::getCanonicalRepoNameWithoutVersion, key -> key));
+        ImmutableMap.of(RepositoryName.MAIN, ModuleKey.ROOT);
 
     return BazelDepGraphValue.create(
         emptyDepGraph,
         canonicalRepoNameLookup,
         ImmutableList.of(),
         ImmutableTable.of(),
-        ImmutableMap.of());
+        ImmutableMap.of(),
+        '+');
   }
 
   /**
@@ -112,6 +107,9 @@ public static BazelDepGraphValue createEmptyDepGraph() {
    */
   public abstract ImmutableMap<ModuleExtensionId, String> getExtensionUniqueNames();
 
+  /** The character to use to separate the different segments of a canonical repo name. */
+  public abstract char getRepoNameSeparator();
+
   /**
    * Returns the full {@link RepositoryMapping} for the given module, including repos from Bazel
    * module deps and module extensions.
@@ -122,7 +120,7 @@ public final RepositoryMapping getFullRepoMapping(ModuleKey key) {
         getExtensionUsagesTable().column(key).entrySet()) {
       ModuleExtensionId extensionId = extIdAndUsage.getKey();
       ModuleExtensionUsage usage = extIdAndUsage.getValue();
-      String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + "~";
+      String repoNamePrefix = getExtensionUniqueNames().get(extensionId) + getRepoNameSeparator();
       for (ModuleExtensionUsage.Proxy proxy : usage.getProxies()) {
         for (Map.Entry<String, String> entry : proxy.getImports().entrySet()) {
           String canonicalRepoName = repoNamePrefix + entry.getValue();

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

@@ -117,6 +117,18 @@ public void afterCommand() {
     // lockfile that are still up-to-date and adding the newly resolved extension results.
     BazelLockFileValue newLockfile =
         BazelLockFileValue.builder()
+            // HACK: Flipping the flag `--incompatible_use_plus_in_repo_names` causes the canonical
+            // repo name format to change, which warrants a lockfile invalidation. We could do this
+            // properly by introducing another field in the lockfile, but that's very annoying since
+            // we'll need to 1) keep it around for a while; 2) be careful not to parse the rest of
+            // the lockfile to avoid triggering parsing errors ('~' will be an invalid character in
+            // repo names eventually). So we just increment the lockfile version if '+' is being
+            // used. This does mean that, while this hack exists, normal increments of the lockfile
+            // version need to be done by 2 at a time (i.e. keep LOCK_FILE_VERSION an odd number).
+            .setLockFileVersion(
+                depGraphValue.getRepoNameSeparator() == '+'
+                    ? BazelLockFileValue.LOCK_FILE_VERSION + 1
+                    : BazelLockFileValue.LOCK_FILE_VERSION)
             .setRegistryFileHashes(
                 ImmutableSortedMap.copyOf(moduleResolutionValue.getRegistryFileHashes()))
             .setSelectedYankedVersions(moduleResolutionValue.getSelectedYankedVersions())

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

@@ -35,6 +35,8 @@
 @GenerateTypeAdapter
 public abstract class BazelLockFileValue implements SkyValue, Postable {
 
+  // NOTE: See "HACK" note in BazelLockFileModule. While this hack exists, normal increments of the
+  // lockfile version need to be done by 2 at a time (i.e. keep LOCK_FILE_VERSION an odd number).
   public static final int LOCK_FILE_VERSION = 11;
 
   @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE;

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

@@ -60,6 +60,8 @@ public static IsolationKey create(ModuleKey module, String usageExportedName) {
 
     @Override
     public final String toString() {
+      // NOTE: Can't be bothered to switch this based on the flag. But DO change this to "+" by
+      // Bazel 8!
       return getModule() + "~" + getUsageExportedName();
     }
 

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

@@ -157,7 +157,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     GetModuleFileResult getModuleFileResult;
     try (SilentCloseable c =
         Profiler.instance().profile(ProfilerTask.BZLMOD, () -> "fetch module file: " + moduleKey)) {
-      getModuleFileResult = getModuleFile(moduleKey, moduleFileKey.getOverride(), env);
+      getModuleFileResult =
+          getModuleFile(moduleKey, moduleFileKey.getOverride(), starlarkSemantics, env);
     }
     if (getModuleFileResult == null) {
       return null;
@@ -481,7 +482,8 @@ public static RootModuleFileValue evaluateRootModuleFile(
                     // A module with a non-registry override always has a unique version across the
                     // entire dep graph.
                     name ->
-                        ModuleKey.create(name, Version.EMPTY).getCanonicalRepoNameWithoutVersion(),
+                        ModuleKey.create(name, Version.EMPTY)
+                            .getCanonicalRepoNameWithoutVersion(starlarkSemantics),
                     name -> name));
     ImmutableSet<PathFragment> moduleFilePaths =
         Stream.concat(
@@ -552,14 +554,17 @@ private record GetModuleFileResult(
 
   @Nullable
   private GetModuleFileResult getModuleFile(
-      ModuleKey key, @Nullable ModuleOverride override, Environment env)
+      ModuleKey key,
+      @Nullable ModuleOverride override,
+      StarlarkSemantics starlarkSemantics,
+      Environment env)
       throws ModuleFileFunctionException, InterruptedException {
     // If there is a non-registry override for this module, we need to fetch the corresponding repo
     // first and read the module file from there.
     if (override instanceof NonRegistryOverride) {
       // A module with a non-registry override always has a unique version across the entire dep
       // graph.
-      RepositoryName canonicalRepoName = key.getCanonicalRepoNameWithoutVersion();
+      RepositoryName canonicalRepoName = key.getCanonicalRepoNameWithoutVersion(starlarkSemantics);
       RepositoryDirectoryValue repoDir =
           (RepositoryDirectoryValue) env.getValue(RepositoryDirectoryValue.key(canonicalRepoName));
       if (repoDir == null) {

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

@@ -16,12 +16,15 @@
 package com.google.devtools.build.lib.bazel.bzlmod;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
+import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import java.util.Comparator;
 import java.util.List;
+import net.starlark.java.eval.StarlarkSemantics;
 
 /** A module name, version pair that identifies a module in the external dependency graph. */
 @AutoValue
@@ -79,35 +82,46 @@ public final String toString() {
    *
    * <p>This method must not be called if the module has a {@link NonRegistryOverride}.
    */
-  public RepositoryName getCanonicalRepoNameWithVersion() {
-    return getCanonicalRepoName(/* includeVersion= */ true);
+  public RepositoryName getCanonicalRepoNameWithVersion(StarlarkSemantics semantics) {
+    return getCanonicalRepoName(/* includeVersion= */ true, semantics);
+  }
+
+  @VisibleForTesting
+  public RepositoryName getCanonicalRepoNameWithVersionForTesting() {
+    return getCanonicalRepoNameWithVersion(StarlarkSemantics.DEFAULT);
   }
 
   /**
    * Returns the canonical name of the repo backing this module, excluding its version. This name is
    * only guaranteed to be unique when there is a single version of the module in the entire dep
    * graph.
    */
-  public RepositoryName getCanonicalRepoNameWithoutVersion() {
-    return getCanonicalRepoName(/* includeVersion= */ false);
+  public RepositoryName getCanonicalRepoNameWithoutVersion(StarlarkSemantics semantics) {
+    return getCanonicalRepoName(/* includeVersion= */ false, semantics);
+  }
+
+  @VisibleForTesting
+  public RepositoryName getCanonicalRepoNameWithoutVersionForTesting() {
+    return getCanonicalRepoNameWithoutVersion(StarlarkSemantics.DEFAULT);
   }
 
-  private RepositoryName getCanonicalRepoName(boolean includeVersion) {
+  private RepositoryName getCanonicalRepoName(boolean includeVersion, StarlarkSemantics semantics) {
     if (WELL_KNOWN_MODULES.containsKey(getName())) {
       return WELL_KNOWN_MODULES.get(getName());
     }
     if (ROOT.equals(this)) {
       return RepositoryName.MAIN;
     }
+    boolean usePlus = semantics.getBool(BuildLanguageOptions.INCOMPATIBLE_USE_PLUS_IN_REPO_NAMES);
     String suffix;
     if (includeVersion) {
       // getVersion().isEmpty() is true only for modules with non-registry overrides, which enforce
       // that there is a single version of the module in the dep graph.
       Preconditions.checkState(!getVersion().isEmpty());
-      // Prepend "v" to prevent canonical repo names, which form segments of file paths, from
-      // looking like a Windows short path. Such paths segments would incur additional file IO
-      // during analysis (see WindowsShortPath).
-      suffix = "v" + getVersion().toString();
+      // When using `~` as the separator, prepend "v" to prevent canonical repo names, which form
+      // segments of file paths, from looking like a Windows short path. Such paths segments would
+      // incur additional file IO during analysis (see WindowsShortPath).
+      suffix = usePlus ? getVersion().toString() : "v" + getVersion().toString();
     } else {
       // This results in canonical repository names such as `rules_foo~` for the module `rules_foo`.
       // This particular format is chosen since:
@@ -125,7 +139,8 @@ private RepositoryName getCanonicalRepoName(boolean includeVersion) {
       //   rarely used.
       suffix = "";
     }
-    return RepositoryName.createUnvalidated(String.format("%s~%s", getName(), suffix));
+    return RepositoryName.createUnvalidated(
+        String.format("%s%c%s", getName(), usePlus ? '+' : '~', suffix));
   }
 
   public static ModuleKey fromString(String s) throws Version.ParseException {