[7.2.0] Fail early on certain invalid labels in module files

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

@@ -18,14 +18,15 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
+import com.google.devtools.build.lib.cmdline.Label;
 
 /** Specifies that a module should be retrieved from an archive. */
 @AutoValue
 public abstract class ArchiveOverride implements NonRegistryOverride {
 
   public static ArchiveOverride create(
       ImmutableList<String> urls,
-      ImmutableList<Object> patches,
+      ImmutableList<Label> patches,
       ImmutableList<String> patchCmds,
       String integrity,
       String stripPrefix,
@@ -38,7 +39,7 @@ public static ArchiveOverride create(
   public abstract ImmutableList<String> getUrls();
 
   /** The labels of patches to apply after extracting the archive. */
-  public abstract ImmutableList<Object> getPatches();
+  public abstract ImmutableList<Label> getPatches();
 
   /** The patch commands to execute after extracting the archive. Should be a list of commands. */
   public abstract ImmutableList<String> getPatchCmds();

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

@@ -18,6 +18,7 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import net.starlark.java.eval.StarlarkInt;
 
@@ -54,7 +55,7 @@ public ArchiveRepoSpecBuilder setStripPrefix(String stripPrefix) {
   }
 
   @CanIgnoreReturnValue
-  public ArchiveRepoSpecBuilder setPatches(ImmutableList<Object> patches) {
+  public ArchiveRepoSpecBuilder setPatches(ImmutableList<Label> patches) {
     attrBuilder.put("patches", patches);
     return this;
   }

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

@@ -18,14 +18,15 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
+import com.google.devtools.build.lib.cmdline.Label;
 
 /** Specifies that a module should be retrieved from a Git repository. */
 @AutoValue
 public abstract class GitOverride implements NonRegistryOverride {
   public static GitOverride create(
       String remote,
       String commit,
-      ImmutableList<Object> patches,
+      ImmutableList<Label> patches,
       ImmutableList<String> patchCmds,
       int patchStrip,
       boolean initSubmodules,
@@ -41,7 +42,7 @@ public static GitOverride create(
   public abstract String getCommit();
 
   /** The labels of patches to apply after fetching from Git. */
-  public abstract ImmutableList<Object> getPatches();
+  public abstract ImmutableList<Label> getPatches();
 
   /** The patch commands to execute after fetching from Git. Should be a list of commands. */
   public abstract ImmutableList<String> getPatchCmds();

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

@@ -16,6 +16,7 @@
 package com.google.devtools.build.lib.bazel.bzlmod;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.List;
 
@@ -71,7 +72,7 @@ public GitRepoSpecBuilder setStripPrefix(String stripPrefix) {
   }
 
   @CanIgnoreReturnValue
-  public GitRepoSpecBuilder setPatches(List<Object> patches) {
+  public GitRepoSpecBuilder setPatches(List<Label> patches) {
     return setAttr("patches", patches);
   }
 

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

@@ -15,7 +15,6 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableCollection;
@@ -457,7 +456,8 @@ public ModuleExtensionProxy useExtension(
     return new ModuleExtensionProxy(newUsageBuilder, proxyBuilder);
   }
 
-  private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile) {
+  private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile)
+      throws EvalException {
     // 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
@@ -467,45 +467,49 @@ private String normalizeLabelString(InterimModule.Builder module, String rawExte
     // ownName can't change anymore as calling module() after this results in an error.
     String ownName = module.getRepoName().orElse(module.getName());
     RepositoryName ownRepoName = RepositoryName.createUnvalidated(ownName);
+    ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
+    if (module.getKey().equals(ModuleKey.ROOT)) {
+      repoMapping = ImmutableMap.of("", ownRepoName);
+    }
+    Label label;
     try {
-      ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
-      if (module.getKey().equals(ModuleKey.ROOT)) {
-        repoMapping = ImmutableMap.of("", ownRepoName);
-      }
-      Label 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;
+    } catch (LabelSyntaxException e) {
+      throw Starlark.errorf("invalid label \"%s\": %s", rawExtensionBzlFile, e.getMessage());
     }
+    // Skip over the leading "@" of the unambiguous form.
+    return label.getUnambiguousCanonicalForm().substring(1);
   }
 
-  /**
-   * Returns a {@link Label} when the given string is a valid label, otherwise the string itself.
-   */
-  private Object parseOverrideLabelAttribute(InterimModule.Builder module, String rawLabel) {
+  private Label convertAndValidatePatchLabel(InterimModule.Builder module, String rawLabel)
+      throws EvalException {
     RepositoryMapping repoMapping =
         RepositoryMapping.create(
             ImmutableMap.<String, RepositoryName>builder()
                 .put("", RepositoryName.MAIN)
                 .put(module.getRepoName().orElse(module.getName()), RepositoryName.MAIN)
                 .buildKeepingLast(),
             RepositoryName.MAIN);
+    Label label;
     try {
-      return Label.parseWithPackageContext(
-          rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
+      label =
+          Label.parseWithPackageContext(
+              rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
     } catch (LabelSyntaxException e) {
-      // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
-      // let the module repo fail when fetched.
-      return rawLabel;
+      throw Starlark.errorf("invalid label \"%s\" in 'patches': %s", rawLabel, e.getMessage());
     }
+    if (!label.getRepository().isVisible()) {
+      throw Starlark.errorf(
+          "invalid label in 'patches': only patches in the main repository can be applied, not from"
+              + " '@%s'",
+          label.getRepository().getName());
+    }
+    return label;
   }
 
   @StarlarkBuiltin(name = "module_extension_proxy", documented = false)
@@ -798,16 +802,18 @@ public void singleVersionOverride(
     try {
       parsedVersion = Version.parse(version);
     } catch (ParseException e) {
-      throw new EvalException("Invalid version in single_version_override()", e);
+      throw Starlark.errorf("Invalid version in single_version_override(): %s", version);
+    }
+    ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
+    for (String patch : Sequence.cast(patches, String.class, "patches")) {
+      patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
     }
     context.addOverride(
         moduleName,
         SingleVersionOverride.create(
             parsedVersion,
             registry,
-            Sequence.cast(patches, String.class, "patches").stream()
-                .map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
-                .collect(toImmutableList()),
+            patchesBuilder.build(),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             patchStrip.toInt("single_version_override.patch_strip")));
   }
@@ -948,13 +954,15 @@ public void archiveOverride(
         urls instanceof String
             ? ImmutableList.of((String) urls)
             : Sequence.cast(urls, String.class, "urls").getImmutableList();
+    ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
+    for (String patch : Sequence.cast(patches, String.class, "patches")) {
+      patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
+    }
     context.addOverride(
         moduleName,
         ArchiveOverride.create(
             urlList,
-            Sequence.cast(patches, String.class, "patches").stream()
-                .map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
-                .collect(toImmutableList()),
+            patchesBuilder.build(),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             integrity,
             stripPrefix,
@@ -1040,14 +1048,16 @@ public void gitOverride(
     ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "git_override()");
     context.setNonModuleCalled();
     validateModuleName(moduleName);
+    ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
+    for (String patch : Sequence.cast(patches, String.class, "patches")) {
+      patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
+    }
     context.addOverride(
         moduleName,
         GitOverride.create(
             remote,
             commit,
-            Sequence.cast(patches, String.class, "patches").stream()
-                .map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
-                .collect(toImmutableList()),
+            patchesBuilder.build(),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             patchStrip.toInt("git_override.patch_strip"),
             initSubmodules,

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

@@ -17,6 +17,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.Label;
 
 /**
  * Specifies that the module should:
@@ -33,7 +34,7 @@ public abstract class SingleVersionOverride implements RegistryOverride {
   public static SingleVersionOverride create(
       Version version,
       String registry,
-      ImmutableList<Object> patches,
+      ImmutableList<Label> patches,
       ImmutableList<String> patchCmds,
       int patchStrip) {
     return new AutoValue_SingleVersionOverride(version, registry, patches, patchCmds, patchStrip);
@@ -49,7 +50,7 @@ public static SingleVersionOverride create(
   public abstract String getRegistry();
 
   /** The labels of patches to apply after retrieving per the registry. */
-  public abstract ImmutableList<Object> getPatches();
+  public abstract ImmutableList<Label> getPatches();
 
   /**
    * The patch commands to execute after retrieving per the registry. Should be a list of commands.

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java

@@ -1537,4 +1537,45 @@ public void isolatedUsage_notEnabled() throws Exception {
             + "and thus unavailable with the current flags. It may be enabled by setting "
             + "--experimental_isolated_extension_usages");
   }
+
+  @Test
+  public void testInvalidRepoInPatches() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("MODULE.bazel").getPathString(),
+        "module(name='aaa')",
+        "single_version_override(",
+        "    module_name = 'bbb',",
+        "    version = '1.0',",
+        "    patches = ['@unknown_repo//:patch.bzl'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<RootModuleFileValue> result =
+        evaluator.evaluate(
+            ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+
+    assertContainsEvent(
+        "Error in single_version_override: invalid label in 'patches': only patches in the main"
+            + " repository can be applied, not from '@unknown_repo'");
+  }
+
+  @Test
+  public void testInvalidUseExtensionLabel() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("MODULE.bazel").getPathString(),
+        "module(name='aaa')",
+        "use_extension('@foo/bar:extensions.bzl', 'my_ext')");
+
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<RootModuleFileValue> result =
+        evaluator.evaluate(
+            ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+
+    assertContainsEvent(
+        "Error in use_extension: invalid label \"@foo/bar:extensions.bzl\": invalid repository"
+            + " name 'foo/bar:extensions.bzl': repo names may contain only A-Z, a-z, 0-9, '-',"
+            + " '_', '.' and '~' and must not start with '~'");
+  }
 }