[7.2.0] Fail early on certain invalid labels in module files (#22506)

The following validation checks were not enforced due to backwards
compatibility concerns, but ended up crashing Bazel when invalid labels
made it into the lockfile, which is enabled by default. We might as well
enable them now:

* Fail if a label passed to `use_extension` is not valid
* Fail if a label passed to the `patches` attribute of an override is
not valid or points to a repo other than the main repo

Work towards #21845

Closes #22487.

PiperOrigin-RevId: 636255834
Change-Id: I55dda374cd1716e514c4d78642479b136fd3ad43

Commit
cdace8f

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

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 '~'");
+  }
 }