Cover missing cases during module extension label normalization

fmeum · copybara-github · commit 71787cf0552a · 2023-12-20T15:05:02.000-08:00
The previous logic missed to normalize cases such as `"extension.bzl"` and `"//extension.bzl"`, thus resulting in crashes if these styles are mixed as well as invalid buildozer commands for `use_repo` fixing. Instead of enumerating cases, parse the label and emit it in unambiguous canonical form with a leading `@` stripped. Closes #20482. PiperOrigin-RevId: 592666970 Change-Id: Ieea34b27a187545a11107a334bbae14fef974ae8
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
@@ -29,9 +29,13 @@
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileGlobals.ModuleExtensionUsageBuilder.ModuleExtensionProxy;
 import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.StarlarkExportable;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
@@ -480,17 +484,31 @@ public ModuleExtensionProxy useExtension(
   }
 
   private String normalizeLabelString(String rawExtensionBzlFile) {
-    // Normalize the label by adding the current module's repo_name if the label doesn't specify a
-    // repository name. This is necessary as ModuleExtensionUsages are grouped by the string value
-    // of this label, but later mapped to their Label representation. If multiple strings map to the
-    // same Label, this would result in a crash.
+    // Normalize the label by parsing and stringifying it with a repo mapping that preserves the
+    // apparent repository name, except that a reference to the main repository via the empty
+    // repo name is translated to using the module repo name. This is necessary as
+    // ModuleExtensionUsages are grouped by the string value of this label, but later mapped to
+    // their Label representation. If multiple strings map to the same Label, this would result in a
+    // crash.
     // ownName can't change anymore as calling module() after this results in an error.
     String ownName = module.getRepoName().orElse(module.getName());
-    if (module.getKey().equals(ModuleKey.ROOT) && rawExtensionBzlFile.startsWith("@//")) {
-      return "@" + ownName + rawExtensionBzlFile.substring(1);
-    } else if (rawExtensionBzlFile.startsWith("//")) {
-      return "@" + ownName + rawExtensionBzlFile;
-    } else {
+    RepositoryName ownRepoName = RepositoryName.createUnvalidated(ownName);
+    try {
+      ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
+      if (module.getKey().equals(ModuleKey.ROOT)) {
+        repoMapping = ImmutableMap.of("", ownRepoName);
+      }
+      Label label =
+          Label.parseWithPackageContext(
+              rawExtensionBzlFile,
+              Label.PackageContext.of(
+                  PackageIdentifier.create(ownRepoName, PathFragment.EMPTY_FRAGMENT),
+                  RepositoryMapping.createAllowingFallback(repoMapping)));
+      // Skip over the leading "@" of the unambiguous form.
+      return label.getUnambiguousCanonicalForm().substring(1);
+    } catch (LabelSyntaxException ignored) {
+      // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
+      // let the extension fail when evaluated.
       return rawExtensionBzlFile;
     }
   }
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
@@ -363,7 +363,10 @@ public void simpleExtension_nonCanonicalLabel() throws Exception {
         "use_repo(ext2, 'bar')",
         "ext3 = use_extension('@//:defs.bzl', 'ext')",
         "ext3.tag(name='quz', data='qu')",
-        "use_repo(ext3, 'quz')");
+        "use_repo(ext3, 'quz')",
+        "ext4 = use_extension('defs.bzl', 'ext')",
+        "ext4.tag(name='qor', data='qo')",
+        "use_repo(ext4, 'qor')");
     scratch.file(
         workspaceRoot.getRelative("defs.bzl").getPathString(),
         "load('@data_repo//:defs.bzl','data_repo')",
@@ -379,15 +382,17 @@ public void simpleExtension_nonCanonicalLabel() throws Exception {
         "load('@foo//:data.bzl', foo_data='data')",
         "load('@bar//:data.bzl', bar_data='data')",
         "load('@quz//:data.bzl', quz_data='data')",
-        "data = 'foo:'+foo_data+' bar:'+bar_data+' quz:'+quz_data");
+        "load('@qor//:data.bzl', qor_data='data')",
+        "data = 'foo:'+foo_data+' bar:'+bar_data+' quz:'+quz_data+' qor:'+qor_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("foo:fu bar:ba quz:qu");
+    assertThat(result.get(skyKey).getModule().getGlobal("data"))
+        .isEqualTo("foo:fu bar:ba quz:qu qor:qo");
   }
 
   @Test
