Simplify visibility() arg parsing logic, support visibility("//foo")

BazelBuildApiGlobals:
- An arg of "some_string" is treated as syntactic sugar for ["some_string"]. This supports args of form "//foo".
- Deleted special casing for "public" and "private". These are now handled by the sugar and by the new PackageSpecification syntax added in unknown commit.

BzlLoadFunctionTest:
- Add tests for `visibility([])`, `visibility("//foo")`, and the unsupported case of negated package specs

BzlVisibility:
- Add interning of special public/private cases; lift construction to factory method.
- Rename PackageListBzlVisibility since it's really package specifications, not packages being listed. Make private since users don't need direct access to it. (Kept PUBLIC/PRIVATE singletons as public since it's reasonable for client code to directly refer to those.)

Since it's likely we won't support negation, we could even eliminate the BzlVisibility class altogether in favor of lists of PackageSpecifications. This would simplify interning of public/private (but complicate any hypothetical future interning of non-trivial allowlists). But having a separate class is more convenient/readable for the bzl machinery.

StarlarkBuildApiGlobals:
- Move description of valid visibility() arg values to the arg's doc.
- Condense the mention of the distinction between bzl-visibility and target visibility.
- Mention behavior relative to --incompatible_fix_package_group_reporoot_syntax.

Work toward #11261.

PiperOrigin-RevId: 479369691
Change-Id: Ib9b2844abc03b0596399cc3fe09e9b89c6416e90

src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java

@@ -17,6 +17,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.BazelStarlarkContext;
 import com.google.devtools.build.lib.packages.BzlInitThreadContext;
 import com.google.devtools.build.lib.packages.BzlVisibility;
@@ -57,34 +58,28 @@ public void visibility(Object value, StarlarkThread thread) throws EvalException
       throw Starlark.errorf(".bzl visibility may not be set more than once");
     }
 
-    BzlVisibility bzlVisibility = null;
-    // `visibility("public")` and `visibility("private")`
+    RepositoryName repo = context.getBzlFile().getRepository();
+    ImmutableList<PackageSpecification> specs;
     if (value instanceof String) {
-      if (value.equals("public")) {
-        bzlVisibility = BzlVisibility.PUBLIC;
-      } else if (value.equals("private")) {
-        bzlVisibility = BzlVisibility.PRIVATE;
-      }
-      // `visibility(["//pkg1", "//pkg2", ...])`
+      // `visibility("public")`, `visibility("private")`, visibility("//pkg")
+      specs =
+          ImmutableList.of(PackageSpecification.fromStringForBzlVisibility(repo, (String) value));
     } else if (value instanceof StarlarkList) {
+      // `visibility(["//pkg1", "//pkg2", ...])`
       List<String> specStrings = Sequence.cast(value, String.class, "visibility list");
-      ImmutableList.Builder<PackageSpecification> specs =
+      ImmutableList.Builder<PackageSpecification> specsBuilder =
           ImmutableList.builderWithExpectedSize(specStrings.size());
       for (String specString : specStrings) {
         PackageSpecification spec =
-            PackageSpecification.fromStringForBzlVisibility(
-                context.getBzlFile().getRepository(), specString);
-        specs.add(spec);
+            PackageSpecification.fromStringForBzlVisibility(repo, specString);
+        specsBuilder.add(spec);
       }
-      bzlVisibility = new BzlVisibility.PackageListBzlVisibility(specs.build());
-    }
-    if (bzlVisibility == null) {
+      specs = specsBuilder.build();
+    } else {
       throw Starlark.errorf(
-          "Invalid bzl-visibility: got '%s', want \"public\", \"private\", or list of package"
-              + " specification strings",
-          Starlark.type(value));
+          "Invalid bzl-visibility: got '%s', want string or list of strings", Starlark.type(value));
     }
-    context.setBzlVisibility(bzlVisibility);
+    context.setBzlVisibility(BzlVisibility.of(specs));
   }
 
   private void checkVisibilityAllowlist(PackageIdentifier pkgId, List<String> allowlist)

src/main/java/com/google/devtools/build/lib/packages/BzlVisibility.java

@@ -18,7 +18,19 @@
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import java.util.List;
 
-/** A visibility level governing the loading of a .bzl module. */
+/**
+ * A visibility level governing the loading of a .bzl module.
+ *
+ * <p>This is just a container for a list of PackageSpecifications; the scope of visibility is the
+ * union of all specifications.
+ */
+// TODO(brandjon): If we ever support negation then this class would become a bit fancier -- it'd
+// have to assemble multiple distinct lists, a la PackageSpecificationProvider and
+// PackageSpecification.PackageGroupContents. At the moment we don't anticipate that though.
+//
+// TODO(brandjon): If we have a large allowlist consumed by many .bzl files, we may end up with many
+// copies of the allowlist embedded in different instances of this class. Consider more aggressive
+// interning.
 public abstract class BzlVisibility {
 
   private BzlVisibility() {}
@@ -30,6 +42,23 @@ private BzlVisibility() {}
    */
   public abstract boolean allowsPackage(PackageIdentifier pkg);
 
+  /**
+   * Returns a (possibly interned) {@code BzlVisibility} that allows access as per the given package
+   * specifications.
+   */
+  public static BzlVisibility of(List<PackageSpecification> specs) {
+    if (specs.isEmpty()
+        || (specs.size() == 1 && specs.get(0) instanceof PackageSpecification.NoPackages)) {
+      return PRIVATE;
+    }
+    for (PackageSpecification spec : specs) {
+      if (spec instanceof PackageSpecification.AllPackages) {
+        return PUBLIC;
+      }
+    }
+    return new ListBzlVisibility(specs);
+  }
+
   /** A visibility indicating that everyone may load the .bzl */
   public static final BzlVisibility PUBLIC =
       new BzlVisibility() {
@@ -53,16 +82,12 @@ public boolean allowsPackage(PackageIdentifier pkg) {
 
   /**
    * A visibility that decides whether a package's BUILD and .bzl files may load the .bzl by
-   * checking the package against a set of package specifications.
-   *
-   * <p>Package specifications may have the form {@code //foo/bar} or {@code //foo/bar/...}. Negated
-   * package specifications are not yet supported.
+   * checking the package against a set of {@link PackageSpecifications}s.
    */
-  // TODO(b/22193153): Negation.
-  public static class PackageListBzlVisibility extends BzlVisibility {
+  private static class ListBzlVisibility extends BzlVisibility {
     private final ImmutableList<PackageSpecification> specs;
 
-    public PackageListBzlVisibility(List<PackageSpecification> specs) {
+    public ListBzlVisibility(List<PackageSpecification> specs) {
       this.specs = ImmutableList.copyOf(specs);
     }
 

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java

@@ -33,36 +33,45 @@ public interface StarlarkBuildApiGlobals {
               + " <code>--experimental_bzl_visibility_allowlist</code> are permitted to call this"
               + " function. Known issue: This feature currently may not work under bzlmod.)</i>"
               + "<p>Sets the bzl-visibility of the .bzl module currently being initialized."
-              + "<p>The bzl-visibility of a .bzl module (not to be confused with target visibility)"
-              + " governs whether or not a <code>load()</code> of that .bzl is permitted from"
-              + " within the BUILD and .bzl files of a particular package. Allowed values include:"
-              + "<ul>"
-              + "<li><code>\"public\"</code> <i>(default)</i>: the .bzl can be loaded anywhere."
-              + "<li><code>\"private\"</code>: the .bzl can only be loaded by files in the same"
-              + " package (subpackages are excluded)."
-              + "<li>a list of package specifications (e.g. <code>[\"//pkg1\","
-              + "\"//pkg2/subpkg/...\"]</code>): the .bzl can be loaded by files in any package"
-              + " matching one of the listed specifications. Package specifications may be package"
-              + " paths, or package paths with a trailing <code>\"/...\"</code> to include all"
-              + " subpackages; negated patterns are not currently supported. All package"
-              + " specifications are within the current repository; the \"@\" syntax is disallowed."
-              + "</ul>"
+              + "<p>The bzl-visibility of a module governs whether or not other BUILD and .bzl"
+              + " files may load it. (This is distinct from the target visibility of the underlying"
+              + " .bzl source file, which governs whether the file may appear as a dependency of"
+              + " other targets.) Bzl-visibility works at the level of packages: To load a"
+              + " module, the file doing the loading must live in a package that has been granted"
+              + " visibility to the module. A module can always be loaded within its own package,"
+              + " regardless of its visibility."
               + "<p>Generally, <code>visibility()</code> is called at the top of the .bzl file,"
               + " immediately after its <code>load()</code> statements. (It is poor style to put"
               + " this declaration later in the file or in a helper method.) It may not be called"
               + " more than once per .bzl, or after the .bzl's top-level code has finished"
-              + " executing."
-              + "<p>Note that a .bzl module having a public bzl-visibility does not necessarily"
-              + " imply that its corresponding file target has public visibility. This means that"
-              + " it's possible to be able to <code>load()</code> a .bzl file without being able to"
-              + " depend on it in a <code>filegroup</code> or other target.",
+              + " executing.",
       parameters = {
         @Param(
             name = "value",
             named = false,
             doc =
-                "The bzl-visibility level to set. May be <code>\"public\"</code>,"
-                    + " <code>\"private\"</code>, or a list of packages.")
+                "A list of package specification strings, or a single package specification string."
+                    + "<p>Package specifications follow the same format as for"
+                    + " <code><a href='${link functions#package_group}'>package_group</a></code>,"
+                    + " except that negative package specifications are not permitted. That is, a"
+                    + " specification may have the forms:"
+                    + "<ul>"
+                    + "<li><code>\"//foo\"</code>: the package <code>//foo</code>" //
+                    + "<li><code>\"//foo/...\"</code>: the package <code>//foo</code> and all of"
+                    + " its subpackages." //
+                    + "<li><code>\"public\"</code> or <code>\"private\"</code>: all packages or no"
+                    + " packages, respectively"
+                    + "</ul>"
+                    + "<p>The \"@\" syntax is not allowed; all specifications are interpreted"
+                    + " relative to the current module's repository."
+                    + "<p>If <code>value</code> is a list of strings, the set of packages granted"
+                    + " visibility to this module is the union of the packages represented by each"
+                    + " specification. (An empty list has the same effect as <code>private</code>.)"
+                    + " If <code>value</code> is a single string, it is treated as if it were the"
+                    + " singleton list <code>[value]</code>."
+                    + "<p>Note that the specification <code>\"//...\"</code> is always interpreted"
+                    + " as \"all packages in the current repository\", regardless of the value of"
+                    + " the <code>--incompatible_fix_package_group_reporoot_syntax</code> flag.")
       },
       // Ordinarily we'd use enableOnlyWithFlag here to gate access on
       // --experimental_bzl_visibility. However, the StarlarkSemantics isn't available at the point

src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java

@@ -600,6 +600,27 @@ public void testBzlVisibility_privateDifferentPackage() throws Exception {
     assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a.");
   }
 
+  @Test
+  public void testBzlVisibility_emptyListMeansPrivate() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo.bzl", //
+        "load(\"//b:bar.bzl\", \"x\")");
+    scratch.file("b/BUILD");
+    scratch.file(
+        "b/bar.bzl", //
+        "visibility([])",
+        "x = 1");
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup(
+        "//a:foo.bzl", "module //a:foo.bzl contains .bzl load-visibility violations");
+    assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a.");
+  }
+
   @Test
   public void testBzlVisibility_publicListElement() throws Exception {
     setBuildLanguageOptions(
@@ -753,6 +774,35 @@ public void testBzlVisibility_enumeratedPackages() throws Exception {
     assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a2.");
   }
 
+  @Test
+  public void testBzlVisibility_singleEnumeratedPackageAsString() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone");
+
+    scratch.file("a1/BUILD");
+    scratch.file(
+        "a1/foo1.bzl", //
+        "load(\"//b:bar.bzl\", \"x\")");
+    scratch.file("a2/BUILD");
+    scratch.file(
+        "a2/foo2.bzl", //
+        "load(\"//b:bar.bzl\", \"x\")");
+    scratch.file("b/BUILD");
+    scratch.file(
+        "b/bar.bzl", //
+        // Note: "//a1", not ["//a1"]
+        "visibility(\"//a1\")",
+        "x = 1");
+
+    checkSuccessfulLookup("//a1:foo1.bzl");
+    assertNoEvents();
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup(
+        "//a2:foo2.bzl", "module //a2:foo2.bzl contains .bzl load-visibility violations");
+    assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a2.");
+  }
+
   @Test
   public void testBzlVisibility_enumeratedPackagesMultipleRepos() throws Exception {
     setBuildLanguageOptions(
@@ -918,9 +968,7 @@ public void testBzlVisibility_invalid_badType() throws Exception {
 
     reporter.removeHandler(failFastHandler);
     checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed");
-    assertContainsEvent(
-        "Invalid bzl-visibility: got 'int', want \"public\", \"private\", or list of package"
-            + " specification strings");
+    assertContainsEvent("Invalid bzl-visibility: got 'int', want string or list of strings");
   }
 
   @Test
@@ -953,6 +1001,21 @@ public void testBzlVisibility_invalid_packageOutsideRepo() throws Exception {
     assertContainsEvent("invalid package name '@repo//b': must start with '//'");
   }
 
+  @Test
+  public void testBzlVisibility_invalid_negationNotSupported() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo.bzl", //
+        "visibility([\"-//a\"])");
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed");
+    assertContainsEvent("Cannot use negative package patterns here");
+  }
+
   @Test
   public void testLoadFromNonExistentRepository_producesMeaningfulError() throws Exception {
     scratch.file("BUILD", "load(\"@repository//dir:file.bzl\", \"foo\")");