Bzl visibility prototype: Add support for all-subpackages wildcard

This enables `//foo/...` wildcards both in the allowlists passed to `visibility()`, and in the allowlist guarding the experimental feature itself (`--experimental_bzl_visibility_allowlist`). Both limitations proved to be a major hurdle for onboarding experimental users: There are over four hundred packages owned by the relatively small number of teams that have expressed interest.

Changes:
- Ad hoc parsing of visibility() strings in BazelBuildApiGlobals is replaced by reusing PackageSpecification parsing logic, which governs package_group's `packages` attribute. (This is something we would've wanted to do eventually anyway.)
- To support this, a new fromString variant is added to PackageSpecification, since the existing one is not public, nor is its exception type, nor is the capability to distinguish (and thereby disallow) negative patterns.
- Updated the allowlist string / packageid logic to tolerate `/...` in BazelBuildApiGlobals.
- Drive-by cleanups to visibility tests: Use "everyone" allowlist in test cases that are not testing the allowlist feature. Flip a couple private/public visibilities that are incorrect/irrelevant for what is being tested.
- Drive-by cleanups to misleading comments in PathFragment-related code.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 469723348
Change-Id: Ieaca652a921d95c033456f4bca05897714c16e78

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

@@ -14,14 +14,13 @@
 
 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.PackageSpecification;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkBuildApiGlobals;
 import java.util.List;
@@ -68,33 +67,21 @@ public void visibility(Object value, StarlarkThread thread) throws EvalException
  }
  // `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()));
+ List<String> specStrings = Sequence.cast(value, String.class, "visibility list");
+ ImmutableList.Builder<PackageSpecification> specs =
+ ImmutableList.builderWithExpectedSize(specStrings.size());
+ for (String specString : specStrings) {
+ PackageSpecification spec =
+ PackageSpecification.fromStringForBzlVisibility(
+ context.getBzlFile().getRepository(), specString);
+ specs.add(spec);
  }
- bzlVisibility = new BzlVisibility.PackageListBzlVisibility(packages.build());
+ bzlVisibility = new BzlVisibility.PackageListBzlVisibility(specs.build());
  }
  if (bzlVisibility == null) {
  throw Starlark.errorf(
- "Invalid bzl-visibility: got '%s', want \"public\", \"private\", or list of package path"
- + " strings",
+ "Invalid bzl-visibility: got '%s', want \"public\", \"private\", or list of package"
+ + " specification strings",
  Starlark.type(value));
  }
  context.setBzlVisibility(bzlVisibility);
@@ -109,21 +96,36 @@ private void checkVisibilityAllowlist(PackageIdentifier pkgId, List<String> allo
  // 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
- // map. (This shouldn't matter within Google's monorepo, which doesn't use a repo map.)
- try {
- // Special constant to disable allowlisting. For migration to enable the feature globally.
-  if (allowedPkgString.equals("everyone")) {
-  foundMatch = true;
-  break;
-  }
- PackageIdentifier allowedPkgId = PackageIdentifier.parse(allowedPkgString);
- if (pkgId.equals(allowedPkgId)) {
- foundMatch = true;
- break;
+ // Special constant to disable allowlisting. For migration to enable the feature globally.
+ if (allowedPkgString.equals("everyone")) {
+  foundMatch = true;
+ break;
+ }
+ // The wildcard syntax /... is not valid for PackageIdentifiers, so we extract it first.
+ boolean allBeneath = allowedPkgString.endsWith("/...");
+ if (allBeneath) {
+ allowedPkgString = allowedPkgString.substring(0, allowedPkgString.length() - 4);
+ if (allowedPkgString.equals("/")) {
+ // was "//..."
+ allowedPkgString = "//";
  }
+ }
+ PackageIdentifier allowedPkgId;
+ try {
+ // TODO(b/22193153): This seems incorrect since parse doesn't take into account any
+ // repository map. (This shouldn't matter within Google's monorepo, which doesn't use a repo
+ // map.)
+ allowedPkgId = PackageIdentifier.parse(allowedPkgString);
  } catch (LabelSyntaxException ex) {
- throw new EvalException("Invalid bzl-visibility allowlist", ex);
+ throw Starlark.errorf("Invalid bzl-visibility allowlist: %s", ex.getMessage());
+ }
+
+ if (pkgId.equals(allowedPkgId)
+ || (allBeneath
+ // Again, we're erroneously ignoring repo.
+ && pkgId.getPackageFragment().startsWith(allowedPkgId.getPackageFragment()))) {
+ foundMatch = true;
+ break;
  }
  }
  if (!foundMatch) {

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

@@ -52,19 +52,28 @@ public boolean allowsPackage(PackageIdentifier pkg) {
  };
 
  /**
- * A visibility that enumerates the packages whose BUILD and .bzl files may load the .bzl.
- * Subpackages are not implicitly included.
+ * 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.
  */
+ // TODO(b/22193153): Negation.
  public static class PackageListBzlVisibility extends BzlVisibility {
- private final ImmutableList<PackageIdentifier> packages;
+ private final ImmutableList<PackageSpecification> specs;
 
- public PackageListBzlVisibility(List<PackageIdentifier> packages) {
- this.packages = ImmutableList.copyOf(packages);
+ public PackageListBzlVisibility(List<PackageSpecification> specs) {
+ this.specs = ImmutableList.copyOf(specs);
  }
 
  @Override
  public boolean allowsPackage(PackageIdentifier pkg) {
- return packages.contains(pkg);
+ for (PackageSpecification spec : specs) {
+ if (spec.containsPackage(pkg)) {
+ return true;
+ }
+ }
+ return false;
  }
  }
 }

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

@@ -27,6 +27,8 @@
 import java.util.LinkedHashMap;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
+import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.Starlark;
 
 /**
  * Represents one of the following:
@@ -130,6 +132,27 @@ private static PackageSpecification fromStringPositive(RepositoryName repository
  : new SinglePackage(packageIdForSpecifiedRepository);
  }
 
+ /**
+ * Parses a string to a {@code PackageSpecification} for use with .bzl visibility.
+ *
+ * <p>This rejects negative package patterns, and translates the exception type into {@code
+ * EvalException}.
+ */
+ // TODO(b/22193153): Support negatives too.
+ public static PackageSpecification fromStringForBzlVisibility(
+ RepositoryName repositoryName, String spec) throws EvalException {
+ PackageSpecification result;
+ try {
+ result = fromString(repositoryName, spec);
+ } catch (InvalidPackageSpecificationException e) {
+ throw new EvalException(e.getMessage());
+ }
+ if (result instanceof NegativePackageSpecification) {
+ throw Starlark.errorf("Cannot use negative package patterns here");
+ }
+ return result;
+ }
+
  /**
  * Parses the provided {@link Label} into a {@link PackageSpecification} specific to the {@link
  * RepositoryName} associated with the label.

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -156,9 +156,10 @@ public final class BuildLanguageOptions extends OptionsBase {
  help =
  "A comma-separated list of packages (sans \"//\") which, if --experimental_bzl_visibility"
  + " is enabled, are permitted to contain .bzl files that set a bzl-visibility by"
- + " calling the `visibility()` function. (Known issue: This flag may not work"
- + " correctly in the presence of repository remapping, which is used by bzlmod.)"
- + " If the list includes the special item \"everyone\", all packages are permitted.")
+ + " calling the `visibility()` function. Subpackages may also be included by"
+ + " appending `/...`. (Known issue: This flag may not work correctly in the presence"
+ + " of repository remapping, which is used by bzlmod.) If the list includes the"
+ + " special item \"everyone\", all packages are permitted.")
  public List<String> experimentalBzlVisibilityAllowlist;
 
  @Option(

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

@@ -40,10 +40,12 @@ public interface StarlarkBuildApiGlobals {
  + "<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 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. Subpackage globs (<code>\"//pkg/...\"</code>) are not supported."
+ + "<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>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"

src/main/java/com/google/devtools/build/lib/vfs/OsPathPolicy.java

@@ -64,14 +64,14 @@ public interface OsPathPolicy {
  /**
  * Returns whether the passed string starts with the given prefix, given the OS case sensitivity.
  *
- * <p>This is a pure string operation and doesn't need to worry about matching path segments.
+ * <p>This is a pure string operation and doesn't account for path separators.
  */
  boolean startsWith(String path, String prefix);
 
  /**
- * Returns whether the passed string ends with the given prefix, given the OS case sensitivity.
+ * Returns whether the passed string ends with the given suffix, given the OS case sensitivity.
  *
- * <p>This is a pure string operation and doesn't need to worry about matching path segments.
+ * <p>This is a pure string operation and doesn't account for path separators.
  */
  boolean endsWith(String path, String suffix);
 

src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java

@@ -338,7 +338,7 @@ public PathFragment relativeTo(String base) {
  }
 
  /**
- * Returns whether this path is an ancestor of another path.
+ * Returns true iff {@code other} is an ancestor of this path.
  *
  * <p>If this == other, true is returned.
  *
@@ -361,8 +361,8 @@ public boolean startsWith(PathFragment other) {
  }
 
  /**
- * Returns true iff {@code suffix}, considered as a list of path segments, is relative and a
- * suffix of {@code this}, or both are absolute and equal.
+ * Returns true iff {@code other}, considered as a list of path segments, is relative and a suffix
+ * of {@code this}, or both are absolute and equal.
  *
  * <p>This is a reflexive, transitive, anti-symmetric relation (i.e. a partial order)
  */