[7.2.0] Validate label attributes of repositories created by module e…

…xtensions (#22537) Catches a common gotcha of referencing a repo created by an extension elsewhere in an extension without a `use_repo` and provides actionable advice to user. This prevents lockfile corruption caused by non-visible labels. The same validation is applied to labels in tag attributes. Fixes #21845 Closes #22495. PiperOrigin-RevId: 636939357 Change-Id: Ib779207502f7767e4e8d3abc55ba7470f75821b9 Commit aa436c3 Co-authored-by: Fabian Meumertzheim <[email protected]>
bazelbuild · May 24, 2024 · f5c91a6 · f5c91a6
1 parent bfa9c6a
commit f5c91a6
Show file tree

Hide file tree

Showing 6 changed files with 266 additions and 6 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/AttributeValues.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/AttributeValues.java
@@ -15,12 +15,18 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
+import static java.util.Collections.singletonList;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.ryanharter.auto.value.gson.GenerateTypeAdapter;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import net.starlark.java.eval.Dict;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
 
 /** Wraps a dictionary of attribute names and values. Always uses a dict to represent them */
@@ -39,6 +45,49 @@ public static AttributeValues create(Map<String, Object> attribs) {
 
   public abstract Dict<String, Object> attributes();
 
+  public static void validateAttrs(AttributeValues attributes, String what) throws EvalException {
+    for (var entry : attributes.attributes().entrySet()) {
+      validateSingleAttr(entry.getKey(), entry.getValue(), what);
+    }
+  }
+
+  public static void validateSingleAttr(String attrName, Object attrValue, String what)
+      throws EvalException {
+    var maybeNonVisibleLabel = getFirstNonVisibleLabel(attrValue);
+    if (maybeNonVisibleLabel.isEmpty()) {
+      return;
+    }
+    Label label = maybeNonVisibleLabel.get();
+    String repoName = label.getRepository().getName();
+    throw Starlark.errorf(
+        "no repository visible as '@%s' to the %s, but referenced by label '@%s//%s:%s' in"
+            + " attribute '%s' of %s. Is the %s missing a bazel_dep or use_repo(..., \"%s\")?",
+        repoName,
+        label.getRepository().getOwnerRepoDisplayString(),
+        repoName,
+        label.getPackageName(),
+        label.getName(),
+        attrName,
+        what,
+        label.getRepository().getOwnerModuleDisplayString(),
+        repoName);
+  }
+
+  private static Optional<Label> getFirstNonVisibleLabel(Object nativeAttrValue) {
+    Collection<?> toValidate =
+        switch (nativeAttrValue) {
+          case List<?> list -> list;
+          case Map<?, ?> map -> map.keySet();
+          case null, default -> singletonList(nativeAttrValue);
+        };
+    for (var item : toValidate) {
+      if (item instanceof Label label && !label.getRepository().isVisible()) {
+        return Optional.of(label);
+      }
+    }
+    return Optional.empty();
+  }
+
   // TODO(salmasamy) this is a copy of Attribute::valueToStarlark, Maybe think of a better place?
   private static Object valueToStarlark(Object x) {
     // Is x a non-empty string_list_dict?

diff --git a/.../com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java b/.../com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java
@@ -115,11 +115,14 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
           Maps.filterKeys(
               Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k)), k -> !k.equals("name"));
       String bzlFile = ruleClass.getRuleDefinitionEnvironmentLabel().getUnambiguousCanonicalForm();
+      var attributesValue = AttributeValues.create(attributes);
+      AttributeValues.validateAttrs(
+          attributesValue, String.format("%s '%s'", rule.getRuleClass(), name));
       RepoSpec repoSpec =
           RepoSpec.builder()
               .setBzlFile(bzlFile)
               .setRuleClassName(ruleClass.getName())
-              .setAttributes(AttributeValues.create(attributes))
+              .setAttributes(attributesValue)
               .build();
 
       generatedRepos.put(name, RepoSpecAndLocation.create(repoSpec, thread.getCallerLocation()));

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
@@ -731,6 +731,7 @@ public RunModuleExtensionResult run(
         String name = (String) kwargs.get("name");
         String prefixedName = usagesValue.getExtensionUniqueName() + "~" + name;
         Rule ruleInstance;
+        AttributeValues attributesValue;
         try {
           ruleInstance =
               BzlmodRepoRuleCreator.createRule(
@@ -742,6 +743,13 @@ public RunModuleExtensionResult run(
                   "SingleExtensionEval.createInnateExtensionRepoRule",
                   repoRule.getRuleClass(),
                   Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
+          attributesValue =
+              AttributeValues.create(
+                  Maps.filterKeys(
+                      Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k)),
+                      k -> !k.equals("name")));
+          AttributeValues.validateAttrs(
+              attributesValue, String.format("%s '%s'", ruleInstance.getRuleClass(), name));
         } catch (InvalidRuleException | NoSuchPackageException | EvalException e) {
           throw new SingleExtensionEvalFunctionException(
               ExternalDepsException.withCauseAndMessage(
@@ -760,11 +768,7 @@ public RunModuleExtensionResult run(
                         .getRuleDefinitionEnvironmentLabel()
                         .getUnambiguousCanonicalForm())
                 .setRuleClassName(repoRule.getRuleClass().getName())
-                .setAttributes(
-                    AttributeValues.create(
-                        Maps.filterKeys(
-                            Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k)),
-                            k -> !k.equals("name"))))
+                .setAttributes(attributesValue)
                 .build();
         generatedRepoSpecs.put(name, repoSpec);
       }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java
@@ -98,6 +98,7 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l
     }
 
     // Check that all mandatory attributes have been specified, and fill in default values.
+    // Along the way, verify that labels in the attribute values refer to visible repos only.
     for (int i = 0; i < attrValues.length; i++) {
       Attribute attr = tagClass.getAttributes().get(i);
       if (attr.isMandatory() && attrValues[i] == null) {
@@ -110,6 +111,13 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l
       if (attrValues[i] == null) {
         attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked());
       }
+      try {
+        AttributeValues.validateSingleAttr(
+            attr.getPublicName(), attrValues[i], String.format("tag '%s'", tag.getTagName()));
+      } catch (EvalException e) {
+        throw ExternalDepsException.withMessage(
+            Code.BAD_MODULE, "in tag at %s: %s", tag.getLocation(), e.getMessage());
+      }
     }
     return new TypeCheckedTag(
         tagClass, attrValues, tag.isDevDependency(), tag.getLocation(), tag.getTagName());

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -207,6 +207,20 @@ public String getOwnerRepoDisplayString() {
     }
   }
 
+  // Must only be called if isVisible() returns true.
+  public String getOwnerModuleDisplayString() {
+    Preconditions.checkNotNull(ownerRepoIfNotVisible);
+    if (ownerRepoIfNotVisible.isMain()) {
+      return "root module";
+    } else {
+      return String.format(
+          "module '%s'",
+          ownerRepoIfNotVisible
+              .getName()
+              .substring(0, ownerRepoIfNotVisible.getName().indexOf('~')));
+    }
+  }
+
   /** Returns if this is the main repository. */
   public boolean isMain() {
     return equals(MAIN);

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
@@ -1389,6 +1389,188 @@ public void badRepoNameInExtensionImplFunction() throws Exception {
     assertContainsEvent("invalid user-provided repo name '_something'");
   }
 
+  @Test
+  public void nonVisibleLabelInLabelAttr() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "ext = use_extension('//:defs.bzl','ext')",
+        "use_repo(ext,'ext')");
+    scratch.file(
+        workspaceRoot.getRelative("defs.bzl").getPathString(),
+        "def _data_repo_impl(ctx):",
+        "  ctx.file('WORKSPACE')",
+        "  ctx.file('BUILD')",
+        "data_repo = repository_rule(",
+        "  implementation=_data_repo_impl,",
+        "  attrs={'data':attr.label()})",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='other_repo')",
+        "  data_repo(name='ext',data='@other_repo//:foo')",
+        "ext = module_extension(implementation=_ext_impl)");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@ext//:data.bzl', ext_data='data')",
+        "data=ext_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    assertContainsEvent(
+        "Error in repository_rule: no repository visible as '@other_repo' to the main repository,"
+            + " but referenced by label '@other_repo//:foo' in attribute 'data' of data_repo 'ext'."
+            + " Is the root module missing a bazel_dep or use_repo(..., \"other_repo\")?");
+  }
+
+  @Test
+  public void nonVisibleLabelInLabelAttrNonRootModule() throws Exception {
+    registry.addModule(
+        createModuleKey("ext_module", "1.0"), "module(name='ext_module',version='1.0')");
+    scratch.file(modulesRoot.getRelative("ext_module~v1.0/WORKSPACE").getPathString());
+    scratch.file(modulesRoot.getRelative("ext_module~v1.0/BUILD").getPathString());
+    scratch.file(
+        modulesRoot.getRelative("ext_module~v1.0/defs.bzl").getPathString(),
+        "def _data_repo_impl(ctx):",
+        "  ctx.file('WORKSPACE')",
+        "  ctx.file('BUILD')",
+        "data_repo = repository_rule(",
+        "  implementation=_data_repo_impl,",
+        "  attrs={'data':attr.label()})",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='other_repo')",
+        "  data_repo(name='ext',data='@other_repo//:foo')",
+        "ext = module_extension(implementation=_ext_impl)");
+
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "bazel_dep(name = 'ext_module', version = '1.0')",
+        "ext = use_extension('@ext_module//:defs.bzl','ext')",
+        "use_repo(ext,'ext')");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@ext//:data.bzl', ext_data='data')",
+        "data=ext_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    assertContainsEvent(
+        "Error in repository_rule: no repository visible as '@other_repo' to the repository"
+            + " '@@ext_module~', but referenced by label '@other_repo//:foo' in attribute 'data' of"
+            + " data_repo 'ext'. Is the module 'ext_module' missing a bazel_dep or use_repo(...,"
+            + " \"other_repo\")?");
+  }
+
+  @Test
+  public void nonVisibleLabelInLabelAttrForwardedFromTag() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "ext = use_extension('//:defs.bzl','ext')",
+        "ext.label(label = '@other_repo//:foo')",
+        "use_repo(ext,'ext')");
+    scratch.file(
+        workspaceRoot.getRelative("defs.bzl").getPathString(),
+        "def _data_repo_impl(ctx):",
+        "  ctx.file('WORKSPACE')",
+        "  ctx.file('BUILD')",
+        "data_repo = repository_rule(",
+        "  implementation=_data_repo_impl,",
+        "  attrs={'data':attr.label()})",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='other_repo')",
+        "  data_repo(name='ext',data=ctx.modules[0].tags.label[0].label)",
+        "label=tag_class(attrs={'label':attr.label()})",
+        "ext = module_extension(",
+        "  implementation=_ext_impl,",
+        "  tag_classes={'label':label},",
+        ")");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@ext//:data.bzl', ext_data='data')",
+        "data=ext_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    var result = evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+
+    assertThat(result.hasError()).isTrue();
+    assertThat(result.getError().getException())
+        .hasMessageThat()
+        .isEqualTo(
+            "in tag at /ws/MODULE.bazel:2:10: no repository visible as '@other_repo' to the main"
+                + " repository, but referenced by label '@other_repo//:foo' in attribute 'label' of"
+                + " tag 'label'. Is the root module missing a bazel_dep or use_repo(...,"
+                + " \"other_repo\")?");
+  }
+
+  @Test
+  public void nonVisibleLabelInLabelListAttr() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "ext = use_extension('//:defs.bzl','ext')",
+        "use_repo(ext,'ext')");
+    scratch.file(
+        workspaceRoot.getRelative("defs.bzl").getPathString(),
+        "def _data_repo_impl(ctx):",
+        "  ctx.file('WORKSPACE')",
+        "  ctx.file('BUILD')",
+        "data_repo = repository_rule(",
+        "  implementation=_data_repo_impl,",
+        "  attrs={'data':attr.label_list()})",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='other_repo')",
+        "  data_repo(name='ext',data=['@other_repo//:foo'])",
+        "ext = module_extension(implementation=_ext_impl)");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@ext//:data.bzl', ext_data='data')",
+        "data=ext_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    assertContainsEvent(
+        "Error in repository_rule: no repository visible as '@other_repo' to the main repository,"
+            + " but referenced by label '@other_repo//:foo' in attribute 'data' of data_repo 'ext'."
+            + " Is the root module missing a bazel_dep or use_repo(..., \"other_repo\")?");
+  }
+
+  @Test
+  public void nonVisibleLabelInLabelKeyedStringDictAttr() throws Exception {
+    scratch.file(
+        workspaceRoot.getRelative("MODULE.bazel").getPathString(),
+        "ext = use_extension('//:defs.bzl','ext')",
+        "use_repo(ext,'ext')");
+    scratch.file(
+        workspaceRoot.getRelative("defs.bzl").getPathString(),
+        "def _data_repo_impl(ctx):",
+        "  ctx.file('WORKSPACE')",
+        "  ctx.file('BUILD')",
+        "data_repo = repository_rule(",
+        "  implementation=_data_repo_impl,",
+        "  attrs={'data':attr.label_keyed_string_dict()})",
+        "def _ext_impl(ctx):",
+        "  data_repo(name='other_repo')",
+        "  data_repo(name='ext',data={'@other_repo//:foo':'bar'})",
+        "ext = module_extension(implementation=_ext_impl)");
+    scratch.file(workspaceRoot.getRelative("BUILD").getPathString());
+    scratch.file(
+        workspaceRoot.getRelative("data.bzl").getPathString(),
+        "load('@ext//:data.bzl', ext_data='data')",
+        "data=ext_data");
+
+    SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl"));
+    reporter.removeHandler(failFastHandler);
+    evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
+    assertContainsEvent(
+        "Error in repository_rule: no repository visible as '@other_repo' to the main repository,"
+            + " but referenced by label '@other_repo//:foo' in attribute 'data' of data_repo 'ext'."
+            + " Is the root module missing a bazel_dep or use_repo(..., \"other_repo\")?");
+  }
+
   @Test
   public void nativeExistingRuleIsEmpty() throws Exception {
     scratch.file(