Bzl-visibility: Add ability to list specific packages

E.g., `visibility(["//foo", "//bar/subpkg"])`. Package globs like "//pkg/*" are not allowed, nor are anything that uses repo syntax ('@'). This is sufficient for prototyping bzl-visibility within Google.

For production usage in the final design, we likely will want to allow //pkg/* syntax, but not repo syntax since it makes little sense to allow something in another repo without also allowing the whole world (public visibility). This also matches how package_group does not currently permit including individual packages from other repos. We might still at some point allow "@the_current_repo_name//pkg" in order to make this check a semantic one on the package name rather than a syntactic one on the string itself.

Work towards #11261.

PiperOrigin-RevId: 458577792
Change-Id: I31605909580ebca13007be3d81e76dfc358e877f

Author: brandjon

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -2215,6 +2215,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
         "//src/main/java/net/starlark/java/eval",
+        "//third_party:guava",
     ],
 )
 

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

@@ -14,16 +14,21 @@
 
 package com.google.devtools.build.lib.analysis.starlark;
 
+import com.google.common.base.Preconditions;
+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;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkBuildApiGlobals;
 import java.util.List;
 import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Sequence;
 import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkList;
 import net.starlark.java.eval.StarlarkThread;
 
 /**
@@ -35,27 +40,73 @@
 public class BazelBuildApiGlobals implements StarlarkBuildApiGlobals {
 
   @Override
-  public void visibility(String value, StarlarkThread thread) throws EvalException {
+  public void visibility(Object value, StarlarkThread thread) throws EvalException {
     // Manually check the experimental flag because enableOnlyWithFlag doesn't work for top-level
     // builtins.
     if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY)) {
       throw Starlark.errorf("Use of `visibility()` requires --experimental_bzl_visibility");
     }
 
+    // Fail if we're not initializing a .bzl module, or if that .bzl module isn't on the
+    // experimental allowlist, or if visibility is already set.
     BzlInitThreadContext context = BzlInitThreadContext.fromOrFailFunction(thread, "visibility");
+    PackageIdentifier pkgId = context.getBzlFile().getPackageIdentifier();
+    List<String> allowlist =
+        thread.getSemantics().get(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST);
+    checkVisibilityAllowlist(pkgId, allowlist);
     if (context.getBzlVisibility() != null) {
       throw Starlark.errorf(".bzl visibility may not be set more than once");
     }
 
-    // Check the currently initializing .bzl file's package against this experimental feature's
-    // allowlist. BuildLanguageOptions isn't allowed to depend on Label, etc., so this is
-    // represented as a list of strings. For simplicity we convert the strings to PackageIdentifiers
-    // here, at linear cost and redundantly for each call to `visibility()`. This is ok because the
-    // allowlist is temporary, expected to remain small, and calls to visibility() are relatively
-    // infrequent.
-    List<String> allowlist =
-        thread.getSemantics().get(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST);
-    PackageIdentifier pkgId = context.getBzlFile().getPackageIdentifier();
+    BzlVisibility bzlVisibility = null;
+    // `visibility("public")` and `visibility("private")`
+    if (value instanceof String) {
+      if (value.equals("public")) {
+        bzlVisibility = BzlVisibility.PUBLIC;
+      } else if (value.equals("private")) {
+        bzlVisibility = BzlVisibility.PRIVATE;
+      }
+      // `visibility(["//pkg1", "//pkg2", ...])`
+    } else if (value instanceof StarlarkList) {
+      List<String> packageStrings = Sequence.cast(value, String.class, "visibility list");
+      ImmutableList.Builder<PackageIdentifier> packages =
+          ImmutableList.builderWithExpectedSize(packageStrings.size());
+      for (String packageString : packageStrings) {
+        PackageIdentifier packageId;
+        // Disallow "@foo//pkg", or even "@//pkg" or "@the_current_repo//pkg".
+        if (packageString.startsWith("@")) {
+          throw Starlark.errorf("package specifiers cannot begin with '@'");
+        }
+        try {
+          packageId = PackageIdentifier.parse(packageString);
+        } catch (LabelSyntaxException ex) {
+          throw Starlark.errorf("Invalid package: %s", ex.getMessage());
+        }
+        // PackageIdentifier.parse() on a string without a repo qualifier returns an identifier in
+        // the main repo. Substitute it with our own repo.
+        Preconditions.checkState(packageId.getRepository().equals(RepositoryName.MAIN));
+        packages.add(
+            PackageIdentifier.create(
+                context.getBzlFile().getRepository(), packageId.getPackageFragment()));
+      }
+      bzlVisibility = new BzlVisibility.PackageListBzlVisibility(packages.build());
+    }
+    if (bzlVisibility == null) {
+      throw Starlark.errorf(
+          "Invalid bzl-visibility: got '%s', want \"public\", \"private\", or list of package path"
+              + " strings",
+          Starlark.type(value));
+    }
+    context.setBzlVisibility(bzlVisibility);
+  }
+
+  private void checkVisibilityAllowlist(PackageIdentifier pkgId, List<String> allowlist)
+      throws EvalException {
+    // The allowlist is represented as a list of strings because BuildLanguageOptions isn't allowed
+    // to depend on Label, PackageIdentifier, etc. For simplicity we just convert the strings to
+    // PackageIdentifiers here, at linear cost and redundantly for each call to `visibility()`. This
+    // is ok because the allowlist is not intended to stay permanent, it is expected to remain
+    // small, and calls to visibility() are relatively infrequent.
     boolean foundMatch = false;
     for (String allowedPkgString : allowlist) {
       // TODO(b/22193153): This seems incorrect since parse doesn't take into account any repository
@@ -67,7 +118,7 @@ public void visibility(String value, StarlarkThread thread) throws EvalException
           break;
         }
       } catch (LabelSyntaxException ex) {
-        throw new EvalException("Invalid bzl visibility allowlist", ex);
+        throw new EvalException("Invalid bzl-visibility allowlist", ex);
       }
     }
     if (!foundMatch) {
@@ -76,16 +127,6 @@ public void visibility(String value, StarlarkThread thread) throws EvalException
               + "--experimental_bzl_visibility_allowlist",
           pkgId.getCanonicalForm());
     }
-
-    BzlVisibility bzlVisibility;
-    if (value.equals("public")) {
-      bzlVisibility = BzlVisibility.PUBLIC;
-    } else if (value.equals("private")) {
-      bzlVisibility = BzlVisibility.PRIVATE;
-    } else {
-      throw Starlark.errorf("Invalid .bzl visibility: '%s'", value);
-    }
-    context.setBzlVisibility(bzlVisibility);
   }
 
   @Override

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

@@ -14,13 +14,57 @@
 
 package com.google.devtools.build.lib.packages;
 
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import java.util.List;
+
 /** A visibility level governing the loading of a .bzl module. */
-// TODO(brandjon): Replace with a proper allowlist having the same granularity as target
-// visibility (i.e. package path specifications).
-public enum BzlVisibility {
-  /** Loadable by everyone (default). */
-  PUBLIC,
-
-  /** Loadable by BUILD and .bzl files within the same package (not subpackages). */
-  PRIVATE
+public abstract class BzlVisibility {
+
+  private BzlVisibility() {}
+
+  /**
+   * Returns whether the given package's BUILD and .bzl files may load the .bzl according to this
+   * visibility restriction. This does not include cases where {@code pkg} is the same package as
+   * the one containing the .bzl (i.e. this method may return false in that case).
+   */
+  public abstract boolean allowsPackage(PackageIdentifier pkg);
+
+  /** A visibility indicating that everyone may load the .bzl */
+  public static final BzlVisibility PUBLIC =
+      new BzlVisibility() {
+        @Override
+        public boolean allowsPackage(PackageIdentifier pkg) {
+          return true;
+        }
+      };
+
+  /**
+   * A visibility indicating that only BUILD and .bzl files within the same package (not including
+   * subpackages) may load the .bzl.
+   */
+  public static final BzlVisibility PRIVATE =
+      new BzlVisibility() {
+        @Override
+        public boolean allowsPackage(PackageIdentifier pkg) {
+          return false;
+        }
+      };
+
+  /**
+   * A visibility that enumerates the packages whose BUILD and .bzl files may load the .bzl.
+   * Subpackages are not implicitly included.
+   */
+  public static class PackageListBzlVisibility extends BzlVisibility {
+    private final ImmutableList<PackageIdentifier> packages;
+
+    public PackageListBzlVisibility(List<PackageIdentifier> packages) {
+      this.packages = ImmutableList.copyOf(packages);
+    }
+
+    @Override
+    public boolean allowsPackage(PackageIdentifier pkg) {
+      return packages.contains(pkg);
+    }
+  }
 }

src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java

@@ -1112,7 +1112,8 @@ static void checkLoadVisibilities(
       BzlVisibility loadVisibility = loadValues.get(i).getBzlVisibility();
       Label loadLabel = loadKeys.get(i).getLabel();
       PackageIdentifier loadPackage = loadLabel.getPackageIdentifier();
-      if (loadVisibility.equals(BzlVisibility.PRIVATE) && !requestingPackage.equals(loadPackage)) {
+      if (!(requestingPackage.equals(loadPackage)
+          || loadVisibility.allowsPackage(requestingPackage))) {
         handler.handle(
             Event.error(
                 programLoads.get(i).second,

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

@@ -35,12 +35,15 @@ public interface StarlarkBuildApiGlobals {
               + "<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. There are currently two"
-              + " allowed values:"
+              + " 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>\"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)"
+              + " package (subpackages are excluded)."
+              + "<li>a list of package paths (e.g. <code>[\"//pkg1\", \"//pkg2/subpkg\","
+              + " ...]</code>): the .bzl can be loaded by files in any of the listed packages. Only"
+              + " packages in the current repository may be specified; the repository \"@\" syntax"
+              + " is disallowed."
               + "</ul>"
               + "<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"
@@ -66,7 +69,7 @@ public interface StarlarkBuildApiGlobals {
       // semantics). So instead we make this builtin unconditionally defined, but have it fail at
       // call time if used without the flag.
       useStarlarkThread = true)
-  void visibility(String value, StarlarkThread thread) throws EvalException;
+  void visibility(Object value, StarlarkThread thread) throws EvalException;
 
   @StarlarkMethod(
       name = "configuration_field",

src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeBuildApiGlobals.java

@@ -23,7 +23,7 @@
 public class FakeBuildApiGlobals implements StarlarkBuildApiGlobals {
 
   @Override
-  public void visibility(String value, StarlarkThread thread) throws EvalException {}
+  public void visibility(Object value, StarlarkThread thread) throws EvalException {}
 
   @Override
   public LateBoundDefaultApi configurationField(String fragment, String name, StarlarkThread thread)

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

@@ -423,6 +423,23 @@ public void testBzlVisibility_disabledWithoutAllowlist() throws Exception {
     assertContainsEvent("`visibility() is not enabled for package //b");
   }
 
+  @Test
+  public void testBzlVisibility_malformedAllowlist() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true",
+        // Not a valid package name.
+        "--experimental_bzl_visibility_allowlist=:::");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo.bzl", //
+        "visibility(\"public\")");
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed");
+    assertContainsEvent("Invalid bzl-visibility allowlist");
+  }
+
   @Test
   public void testBzlVisibility_publicExplicit() throws Exception {
     setBuildLanguageOptions(
@@ -579,6 +596,119 @@ public void testBzlVisibility_cannotBeSetTwice() throws Exception {
     assertContainsEvent(".bzl visibility may not be set more than once");
   }
 
+  @Test
+  public void testBzlVisibility_enumeratedPackages() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=b");
+
+    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", //
+        "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(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=@repo//lib");
+
+    // @repo//pkg:foo1.bzl and @//pkg:foo2.bzl both try to access @repo//lib:bar.bzl. Test that when
+    // bar.bzl declares a visibility allowing "//pkg", it means @repo//pkg and *not* @//pkg.
+    scratch.overwriteFile(
+        "WORKSPACE", //
+        "local_repository(",
+        "    name = 'repo',",
+        "    path = 'repo'",
+        ")");
+    scratch.file("repo/WORKSPACE");
+    scratch.file("repo/pkg/BUILD");
+    scratch.file(
+        "repo/pkg/foo1.bzl", //
+        "load(\"//lib:bar.bzl\", \"x\")");
+    scratch.file("repo/lib/BUILD");
+    scratch.file(
+        "repo/lib/bar.bzl", //
+        "visibility([\"//pkg\"])",
+        "x = 1");
+    scratch.file("pkg/BUILD");
+    scratch.file(
+        "pkg/foo2.bzl", //
+        "load(\"@repo//lib:bar.bzl\", \"x\")");
+
+    checkSuccessfulLookup("@repo//pkg:foo1.bzl");
+    assertNoEvents();
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup(
+        "//pkg:foo2.bzl", "module //pkg:foo2.bzl contains .bzl load-visibility violations");
+    assertContainsEvent(
+        "Starlark file @repo//lib:bar.bzl is not visible for loading from package //pkg.");
+  }
+
+  @Test
+  public void testBzlVisibility_invalid_badType() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo.bzl", //
+        "visibility(123)");
+
+    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 path"
+            + " strings");
+  }
+
+  @Test
+  public void testBzlVisibility_invalid_badElementType() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo.bzl", //
+        "visibility([\"//a\", 123])");
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed");
+    assertContainsEvent("at index 0 of visibility list, got element of type int, want string");
+  }
+
+  @Test
+  public void testBzlVisibility_invalid_packageOutsideRepo() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo.bzl", //
+        "visibility([\"@repo//b\"])");
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed");
+    assertContainsEvent("package specifiers cannot begin with '@'");
+  }
+
   @Test
   public void testLoadFromNonExistentRepository_producesMeaningfulError() throws Exception {
     scratch.file("BUILD", "load(\"@repository//dir:file.bzl\", \"foo\")");