Modify message and also check tags

Author: fmeum

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

@@ -22,7 +22,6 @@
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 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.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
@@ -101,61 +100,46 @@ public static Rule createRule(
     return rule;
   }
 
-  public static void validateLabelAttrs(
-      AttributeValues attributes, ModuleExtensionId owningExtension, String what)
-      throws EvalException {
+  public static void validateAttrs(AttributeValues attributes, String what) throws EvalException {
     for (var entry : attributes.attributes().entrySet()) {
-      validateSingleAttr(entry.getKey(), entry.getValue(), owningExtension, what);
+      validateSingleAttr(entry.getKey(), entry.getValue(), what);
     }
   }
 
-  public 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();
-  }
-
-  public static void validateSingleAttr(
-      String attrName, Object attrValue, ModuleExtensionId owningExtension, String 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();
-    RepositoryName owningModuleRepoName = owningExtension.getBzlFileLabel().getRepository();
-    String owningModule;
-    if (owningModuleRepoName.isMain()) {
-      owningModule = "root module";
-    } else {
-      owningModule =
-          String.format(
-              "module '%s'",
-              owningModuleRepoName
-                  .getName()
-                  .substring(0, owningModuleRepoName.getName().indexOf('~')));
-    }
     String repoName = label.getRepository().getName();
     throw Starlark.errorf(
-        "no repository visible as '@%s', but referenced by label '@%s//%s:%s' in attribute %s"
-            + " of %s. Only repositories visible to the %s can be referenced"
-            + " here, are you missing a bazel_dep or use_repo(..., \"%s\")?.",
+        "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,
-        owningModule,
+        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();
+  }
 }

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

@@ -118,8 +118,8 @@ public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleC
               Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k)), k -> !k.equals("name"));
       String bzlFile = ruleClass.getRuleDefinitionEnvironmentLabel().getUnambiguousCanonicalForm();
       var attributesValue = AttributeValues.create(attributes);
-      BzlmodRepoRuleCreator.validateLabelAttrs(
-          attributesValue, owningExtension, String.format("%s '%s'", rule.getRuleClass(), name));
+      BzlmodRepoRuleCreator.validateAttrs(
+          attributesValue, String.format("%s '%s'", rule.getRuleClass(), name));
       RepoSpec repoSpec =
           RepoSpec.builder()
               .setBzlFile(bzlFile)

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

@@ -748,10 +748,8 @@ public RunModuleExtensionResult run(
                   Maps.filterKeys(
                       Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k)),
                       k -> !k.equals("name")));
-          BzlmodRepoRuleCreator.validateLabelAttrs(
-              attributesValue,
-              extensionId,
-              String.format("%s '%s'", ruleInstance.getRuleClass(), name));
+          BzlmodRepoRuleCreator.validateAttrs(
+              attributesValue, String.format("%s '%s'", ruleInstance.getRuleClass(), name));
         } catch (InvalidRuleException | NoSuchPackageException | EvalException e) {
           throw new SingleExtensionEvalFunctionException(
               ExternalDepsException.withCauseAndMessage(

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 {
+        BzlmodRepoRuleCreator.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());

src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java

@@ -209,6 +209,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);

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

@@ -289,6 +289,8 @@ public void setup() throws Exception {
     RepositoryDelegatorFunction.IS_VENDOR_COMMAND.set(differencer, false);
     RepositoryDelegatorFunction.VENDOR_DIRECTORY.set(differencer, Optional.empty());
 
+    registry.addModule(
+        createModuleKey("platforms", "0.0.9"), "module(name='platforms', version = '0.0.9')");
     // Set up a simple repo rule.
     registry.addModule(
         createModuleKey("data_repo", "1.0"), "module(name='data_repo',version='1.0')");
@@ -1399,10 +1401,9 @@ public void nonVisibleLabelInLabelAttr() throws Exception {
     reporter.removeHandler(failFastHandler);
     evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
     assertContainsEvent(
-        "Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
-            + " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
-            + " visible to the root module can be referenced here, are you missing a bazel_dep or"
-            + " use_repo(..., \"other_repo\")?");
+        "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
@@ -1423,6 +1424,7 @@ public void nonVisibleLabelInLabelAttrNonRootModule() throws Exception {
         "  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')",
@@ -1438,10 +1440,10 @@ public void nonVisibleLabelInLabelAttrNonRootModule() throws Exception {
     reporter.removeHandler(failFastHandler);
     evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
     assertContainsEvent(
-        "Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
-            + " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
-            + " visible to the module 'ext_module' can be referenced here, are you missing a"
-            + " bazel_dep or use_repo(..., \"other_repo\")?");
+        "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
@@ -1475,12 +1477,16 @@ public void nonVisibleLabelInLabelAttrForwardedFromTag() throws Exception {
 
     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', but referenced by label"
-            + " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
-            + " visible to the root module can be referenced here, are you missing a bazel_dep or"
-            + " use_repo(..., \"other_repo\")?");
+    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
@@ -1511,10 +1517,9 @@ public void nonVisibleLabelInLabelListAttr() throws Exception {
     reporter.removeHandler(failFastHandler);
     evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
     assertContainsEvent(
-        "Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
-            + " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
-            + " visible to the root module can be referenced here, are you missing a bazel_dep or"
-            + " use_repo(..., \"other_repo\")?");
+        "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
@@ -1545,10 +1550,9 @@ public void nonVisibleLabelInLabelKeyedStringDictAttr() throws Exception {
     reporter.removeHandler(failFastHandler);
     evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext);
     assertContainsEvent(
-        "Error in repository_rule: no repository visible as '@other_repo', but referenced by label"
-            + " '@other_repo//:foo' in attribute data of data_repo 'ext'. Only repositories"
-            + " visible to the root module can be referenced here, are you missing a bazel_dep or"
-            + " use_repo(..., \"other_repo\")?");
+        "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