Ensure that extension unique names followed by ~ are prefix-free

fmeum · copybara-github · commit 896b358cdeb3 · 2023-08-03T13:11:37.000-07:00
Fixes errors such as the following when a module provides two extensions that share a name: ``` 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: ``` This was a consequence of the extension unique names followed by `~` being ``` _main~my_ext~2~ _main~my_ext~ ``` since 19a9710. Before, they were of the following form, which was prefix-free: ``` _main~my_ext2~ _main~my_ext~ ``` Fixes #19155 Closes #19156. PiperOrigin-RevId: 553567725 Change-Id: I98650663fea3bfee77752a06a99132e507d91aef
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
@@ -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);
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
@@ -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
@@ -427,6 +427,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(
