Repository: bazelbuild/bazel
Issue: same `module_extension` name in different bzl files could not be resolved while using bzlmod in bazel 6.3.1
Labels: type: bug
P1
team-ExternalDeps
area-Bzlmod

### Description of the bug:


We're using bzlmod.
This is a part of `MODULE.bazel`, which works fine in bazel 6.2.1.
```
a_ext = use_extension("//path/to/A:A.bzl", "my_ext")
use_repo(a_ext, "abc")
b_ext = use_extension("//path/to/B:B.bzl", "my_ext")
use_repo(b_ext, "xyz")
```
When I updated bazel to 6.3.1, there was an error in analysis phase.
```
ERROR: /my/workspace/library_path/BUILD:4:13: no such package '@_main~my_ext~2~xyz//':
The repository '@_main~my_ext~2~xyz' could not be resolved:
Repository '@_main~my_ext~2~xyz' is not defined and referenced by '//library_path:library_name'
ERROR: Analysis of target '//my_path:my_target' failed; build aborted:
```
It seems that in bazel 6.2.1, if there is a same `module_extension` name in different bzl files, the second one will be named as `@_main~my_ext2~xyz` in external directory. But in bazel 6.3.1, it seems that bazel is tring to find a module named as `@_main~my_ext~2~xyz` (adds a `~` after `ext`) which doesn't actually exist.

### Which category does this issue belong to?


_No response_

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


_No response_

### Which operating system are you running Bazel on?


macOS 13.4.1

### What is the output of `bazel info release`?


release 6.3.1

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.


_No response_

### Have you found anything relevant by searching the web?

No

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
Can confirm that this is a bug. The two extensions are assigned the prefixes `_main~my_ext~` and `_main~my_ext~2~` now, which is bad since the former is a prefix of the latter.

I am working on a fix.

CC @Wyverald 
@bazel-io flag
@bazel-io fork 6.4.0
@iancha1992 This is a regression from Bazel 6.2.0, so unfortunately we may need a 6.3.2 release to cherry-pick this into.
> 

cc: @bazelbuild/triage 

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java
index eaf358a011d05b..9e7c0a328e8356 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java
@@ -20,6 +20,7 @@
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.BiMap;
 import com.google.common.collect.HashBiMap;
 import com.google.common.collect.ImmutableBiMap;
@@ -247,39 +248,45 @@ static BzlmodFlagsAndEnvVars getFlagsAndEnvVars(Environment env) throws Interrup
 
   private ImmutableBiMap<String, ModuleExtensionId> calculateUniqueNameForUsedExtensionId(
       ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
-    // Calculate a unique name for each used extension id.
+    // 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:
+    // After appending a single `~` to each such name, none of the resulting strings is a prefix of
+    // any other such string.
     BiMap<String, ModuleExtensionId> extensionUniqueNames = HashBiMap.create();
     for (ModuleExtensionId id : extensionUsagesById.rowKeySet()) {
-      // 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();
-      // 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 "_".
-      String bestName =
-          id.getIsolationKey()
-              .map(
-                  namespace ->
-                      String.format(
-                          "%s~_%s~%s~%s~%s",
-                          nonEmptyRepoPart,
-                          id.getExtensionName(),
-                          namespace.getModule().getName(),
-                          namespace.getModule().getVersion(),
-                          namespace.getUsageExportedName()))
-              .orElse(nonEmptyRepoPart + "~" + id.getExtensionName());
-      if (extensionUniqueNames.putIfAbsent(bestName, id) == null) {
-        continue;
-      }
-      int suffix = 2;
-      while (extensionUniqueNames.putIfAbsent(bestName + "~" + suffix, id) != null) {
-        suffix++;
+      int attempt = 1;
+      while (extensionUniqueNames.putIfAbsent(makeUniqueNameCandidate(id, attempt), id) != null) {
+        attempt++;
       }
     }
     return ImmutableBiMap.copyOf(extensionUniqueNames);
   }
 
+  private static String makeUniqueNameCandidate(ModuleExtensionId id, int attempt) {
+    // 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();
+    // 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 "_".
+    Preconditions.checkArgument(attempt >= 1);
+    String extensionNameDisambiguator = attempt == 1 ? "" : String.valueOf(attempt);
+    return id.getIsolationKey()
+        .map(
+            namespace ->
+                String.format(
+                    "%s~_%s%s~%s~%s~%s",
+                    nonEmptyRepoPart,
+                    id.getExtensionName(),
+                    extensionNameDisambiguator,
+                    namespace.getModule().getName(),
+                    namespace.getModule().getVersion(),
+                    namespace.getUsageExportedName()))
+        .orElse(nonEmptyRepoPart + "~" + id.getExtensionName() + extensionNameDisambiguator);
+  }
+
   static class BazelDepGraphFunctionException extends SkyFunctionException {
     BazelDepGraphFunctionException(ExternalDepsException e, Transience transience) {
       super(e, transience);


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java
index fbe92cf779386f..b8602dba90d16e 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java
@@ -292,7 +292,7 @@ public void createValue_moduleExtensions() throws Exception {
             maven, "rules_jvm_external~1.0~maven",
             pip, "rules_python~2.0~pip",
             myext, "dep~2.0~myext",
-            myext2, "dep~2.0~myext~2");
+            myext2, "dep~2.0~myext2");
 
     assertThat(value.getFullRepoMapping(ModuleKey.ROOT))
         .isEqualTo(
@@ -323,7 +323,7 @@ public void createValue_moduleExtensions() throws Exception {
                 "oneext",
                 "dep~2.0~myext~myext",
                 "twoext",
-                "dep~2.0~myext~2~myext"));
+                "dep~2.0~myext2~myext"));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
index 5bd81dac41e5eb..e1e10dfbd92069 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
@@ -419,6 +419,55 @@ public void simpleExtension_nonCanonicalLabel_repoName() throws Exception {
     assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("foo:fu bar:ba quz:qu");
   }
 
+  @Test
+  public void multipleExtensions_sameName() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "bazel_dep(name='data_repo', version='1.0')",
+        "first_ext = use_extension('//first_ext:defs.bzl', 'ext')",
+        "first_ext.tag(name='foo', data='first_fu')",
+        "first_ext.tag(name='bar', data='first_ba')",
+        "use_repo(first_ext, first_foo='foo', first_bar='bar')",
+        "second_ext = use_extension('//second_ext:defs.bzl', 'ext')",
+        "second_ext.tag(name='foo', data='second_fu')",
+        "second_ext.tag(name='bar', data='second_ba')",
+        "use_repo(second_ext, second_foo='foo', second_bar='bar')");
+    scratch.file(workspaceRoot.getRelative("first_ext/BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("first_ext/defs.bzl").getPathString(),
+        "load('@data_repo//:defs.bzl','data_repo')",
+        "tag = tag_class(attrs = {'name':attr.string(),'data':attr.string()})",
+        "def _ext_impl(ctx):",
+        "  for mod in ctx.modules:",
+        "    for tag in mod.tags.tag:",
+        "      data_repo(name=tag.name,data=tag.data)",
+        "ext = module_extension(implementation=_ext_impl, tag_classes={'tag':tag})");
+    scratch.file(workspaceRoot.getRelative("second_ext/BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("second_ext/defs.bzl").getPathString(),
+        "load('//first_ext:defs.bzl', _ext = 'ext')",
+        "ext = _ext");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@first_foo//:data.bzl', first_foo_data='data')",
+        "load('@first_bar//:data.bzl', first_bar_data='data')",
+        "load('@second_foo//:data.bzl', second_foo_data='data')",
+        "load('@second_bar//:data.bzl', second_bar_data='data')",
+        "data = 'first_foo:'+first_foo_data+' first_bar:'+first_bar_data"
+            + "+' second_foo:'+second_foo_data+' second_bar:'+second_bar_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    EvaluationResult<BzlLoadValue> result =
+        evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    if (result.hasError()) {
+      throw result.getError().getException();
+    }
+    assertThat(result.get(skyKey).getModule().getGlobal("data"))
+        .isEqualTo(
+            "first_foo:first_fu first_bar:first_ba second_foo:second_fu " + "second_bar:second_ba");
+  }
+
   @Test
   public void multipleModules() throws Exception {
     scratch.file(