Relax Label repo visibility validation

fmeum · copybara-github · commit cf3f48ca49f0 · 2022-10-27T06:05:57.000-07:00
Failing the build eagerly when `Label` is passed a label string referencing an unknown apparent repository name turned out to be too strict and causes failures on invalid, but unused labels: https://buildkite.com/bazel/bazel-bazel-with-bzlmod/builds/667#01841861-3d3a-4b65-91b7-b7083ee6b42b Instead, fail when a method that requires a valid repository name is called on an invalid `Label` instance. Closes bazelbuild#16578. PiperOrigin-RevId: 484232254 Change-Id: Ic64ea5db691f39a20dda47e60e2d6f5436ebca1e
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -1004,15 +1004,7 @@ public Label label(String labelString, StarlarkThread thread) throws EvalExcepti
     BazelModuleContext moduleContext =
         BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
     try {
-      Label label = Label.parseWithRepoContext(labelString, moduleContext.packageContext());
-      if (!label.getRepository().isVisible()) {
-        throw Starlark.errorf(
-            "Invalid label string '%s': no repository visible as '@%s' from %s",
-            labelString,
-            label.getRepository().getName(),
-            label.getRepository().getOwnerRepoDisplayString());
-      }
-      return label;
+      return Label.parseWithRepoContext(labelString, moduleContext.packageContext());
     } catch (LabelSyntaxException e) {
       throw Starlark.errorf("Illegal absolute label syntax: %s", e.getMessage());
     }
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -338,7 +338,8 @@ public String getPackageName() {
               + " \"external/repo\"</pre>",
       useStarlarkSemantics = true)
   @Deprecated
-  public String getWorkspaceRootForStarlarkOnly(StarlarkSemantics semantics) {
+  public String getWorkspaceRootForStarlarkOnly(StarlarkSemantics semantics) throws EvalException {
+    checkRepoVisibilityForStarlark("workspace_root");
     return packageIdentifier
         .getRepository()
         .getExecPath(semantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT))
@@ -431,7 +432,8 @@ public String getUnambiguousCanonicalForm() {
           "The repository part of this label. For instance, "
               + "<pre class=language-python>Label(\"@foo//bar:baz\").workspace_name"
               + " == \"foo\"</pre>")
-  public String getWorkspaceName() {
+  public String getWorkspaceName() throws EvalException {
+    checkRepoVisibilityForStarlark("workspace_name");
     return packageIdentifier.getRepository().getName();
   }
 
@@ -504,21 +506,10 @@ public Label getLocalTargetLabel(String targetName) throws LabelSyntaxException
         @Param(name = "relName", doc = "The label that will be resolved relative to this one.")
       },
       useStarlarkThread = true)
-  public Label getRelative(String relName, StarlarkThread thread)
-      throws LabelSyntaxException, EvalException {
-    Label label =
-        getRelativeWithRemapping(
-            relName,
-            BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread))
-                .repoMapping());
-    if (!label.getRepository().isVisible()) {
-      throw Starlark.errorf(
-          "Invalid label string '%s': no repository visible as '@%s' from %s",
-          relName,
-          label.getRepository().getName(),
-          label.getRepository().getOwnerRepoDisplayString());
-    }
-    return label;
+  public Label getRelative(String relName, StarlarkThread thread) throws LabelSyntaxException {
+    return getRelativeWithRemapping(
+        relName,
+        BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).repoMapping());
   }
 
   /**
@@ -676,4 +667,10 @@ public String expandToCommandLine() {
     // TODO(wyv): Consider using StarlarkSemantics here too for optional unambiguity.
     return getCanonicalForm();
   }
+
+  private void checkRepoVisibilityForStarlark(String method) throws EvalException {
+    if (!getRepository().isVisible()) {
+      throw Starlark.errorf("'%s' is not allowed on invalid Label %s", method, this);
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
@@ -163,7 +163,7 @@ public static void main(String[] args)
                   providerInfoMap,
                   userDefinedFunctions,
                   aspectInfoMap);
-    } catch (StarlarkEvaluationException exception) {
+    } catch (StarlarkEvaluationException | EvalException exception) {
       exception.printStackTrace();
       System.err.println("Stardoc documentation generation failed: " + exception.getMessage());
       System.exit(1);
@@ -386,7 +386,8 @@ private Module recursiveEval(
       List<RuleInfoWrapper> ruleInfoList,
       List<ProviderInfoWrapper> providerInfoList,
       List<AspectInfoWrapper> aspectInfoList)
-      throws InterruptedException, IOException, LabelSyntaxException, StarlarkEvaluationException {
+      throws InterruptedException, IOException, LabelSyntaxException, StarlarkEvaluationException,
+          EvalException {
     Path path = pathOfLabel(label, semantics);
 
     if (pending.contains(path)) {
@@ -473,7 +474,7 @@ path, load, pathOfLabel(relativeLabel, semantics), depRoots),
     return module;
   }
 
-  private Path pathOfLabel(Label label, StarlarkSemantics semantics) {
+  private Path pathOfLabel(Label label, StarlarkSemantics semantics) throws EvalException {
     String workspacePrefix = "";
     if (!label.getWorkspaceRootForStarlarkOnly(semantics).isEmpty()
         && !label.getWorkspaceName().equals(workspaceName)) {
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java
@@ -2874,18 +2874,20 @@ public void testLabelWithStrictVisibility() throws Exception {
         .isEqualTo("external/dep~4.5");
     assertThat(eval(module, "Label('@@//foo:bar').workspace_root")).isEqualTo("");
 
-    assertThat(assertThrows(EvalException.class, () -> eval(module, "Label('@//foo:bar')")))
+    assertThat(eval(module, "str(Label('@//foo:bar'))")).isEqualTo("@//foo:bar");
+    assertThat(
+            assertThrows(
+                EvalException.class, () -> eval(module, "Label('@//foo:bar').workspace_name")))
         .hasMessageThat()
-        .contains(
-            "Invalid label string '@//foo:bar': no repository visible as '@' from repository "
-                + "'@module~1.2.3'");
+        .isEqualTo(
+            "'workspace_name' is not allowed on invalid Label @[unknown repo '' requested from"
+                + " @module~1.2.3]//foo:bar");
     assertThat(
             assertThrows(
-                EvalException.class,
-                () -> eval(module, "Label('@@//foo:bar').relative('@not_dep//foo:bar')")))
+                EvalException.class, () -> eval(module, "Label('@//foo:bar').workspace_root")))
         .hasMessageThat()
-        .contains(
-            "Invalid label string '@not_dep//foo:bar': no repository visible as '@not_dep' "
-                + "from repository '@module~1.2.3'");
+        .isEqualTo(
+            "'workspace_root' is not allowed on invalid Label @[unknown repo '' requested from"
+                + " @module~1.2.3]//foo:bar");
   }
 }
