From b3a8ef0d211305bcfaeccd1058e7b5db780415ae Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Thu, 6 Oct 2022 10:22:52 -0700
Subject: [PATCH] package_group: Add "public"/"private" constants, fix "//..."
 meaning

This CL does three things:

1. It adds the special package specifications "public" and "private, guarded behind the flag `--incompatible_package_group_has_public_syntax`.

2. It fixes the behavior of package specification "//..." to mean "all packages in the current repo" rather than "all packages in any repo", guarded behind the flag `--incompatible_fix_package_group_reporoot_syntax`.

3. It fixes the behavior of `bazel query --output=proto` (and `--output=xml`) so that the `packages` attribute is serialized without dropping the leading `//` from package names -- it is now `"//foo/bar/..."` instead of `"foo/bar/..."`. This behavioral change is guarded behind `--incompatible_package_group_includes_double_slash`.

The .bzl `visibility()` builtin always acts as if the first two flags are enabled.

Work toward #11261.
Work toward #16323.
Work toward #16355.
Work toward #16391.

RELNOTES: Added three `package_group`-related flags: `--incompatible_package_group_includes_double_slash` (#16391), `--incompatible_package_group_has_public_syntax` (#16355), and `--incompatible_fix_package_group_reporoot_syntax` (#16323). With these flags, `package_group` can now easily specify "all packages", "no packages", and "all packages in the current repo".

In PackageSpecification:
- fromString(), fromStringPositive(): rewrote docstring, added bool args for flags, renamed or removed vars for simplicity, added handling of public/private and flag behavior
- AllPackagesBeneath(): fixed asStringWithoutRepository() for case of //..., which wasn't likely to arise before.
- normalize order of equals(), hashCode(), etc.
- AllPackages: change hash, and change str representation to "public"
- Add NoPackages, symmetric to AllPackages
- NegativePackageSpecification: don't reuse the delegate's hash as our own hash verbatim

In PackageGroupTest:
- Move `testEverythingSpecificationWorks` to above `testNegativeEverything`, and made it use "public" syntax in place of "//...".
- Add test for "private" syntax.
- Add test for flag guarding "public"/"private".
- Add tests for old and new "//..." behavior.
- Add tests that you can't negate "public" or "private".
- Add test for illegal combination of flags.
- Add assertions for stringification of "public"/"private".

Other changes:
- Update XmlOutputFormatterTest and ProtoOutputFormatterTest with test cases for the flag. In the latter case, parameterize a helper to allow setting per-test-case flags.
- Add test cases to BzlLoadFunctionTest for bzl visibility accepting the list forms `["public"]`/`["private"]`, and for its behavior being unaffected by the new flags.

PiperOrigin-RevId: 479347358
Change-Id: I124ca5406bff15615adaa1256e2d7bef69b9d9a5
---
 site/en/concepts/visibility.md                |   3 +-
 .../build/docgen/templates/be/functions.vm    |  30 ++-
 .../devtools/build/lib/packages/Package.java  |  24 +-
 .../build/lib/packages/PackageGroup.java      |   8 +-
 .../lib/packages/PackageSpecification.java    | 247 +++++++++++++-----
 .../lib/packages/StarlarkNativeModule.java    |  13 +-
 .../semantics/BuildLanguageOptions.java       |  35 +++
 .../lib/query2/common/CommonQueryOptions.java |  11 +
 .../query/output/ProtoOutputFormatter.java    |   5 +-
 .../query/output/XmlOutputFormatter.java      |   5 +-
 .../PackageGroupStaticInitializationTest.java |   7 +-
 .../build/lib/packages/PackageGroupTest.java  | 205 +++++++++++++--
 .../lib/skyframe/BzlLoadFunctionTest.java     | 107 ++++++++
 13 files changed, 591 insertions(+), 109 deletions(-)

diff --git a/site/en/concepts/visibility.md b/site/en/concepts/visibility.md
index 89efcfbd6d4ba3..596e838628ec60 100644
--- a/site/en/concepts/visibility.md
+++ b/site/en/concepts/visibility.md
@@ -45,7 +45,8 @@ target.
     *   Package groups use a different syntax for specifying packages. Within a
         package group, the forms `"//foo/bar:__pkg__"` and
         `"//foo/bar:__subpackages__"` are respectively replaced by `"//foo/bar"`
-        and `"//foo/bar/..."`.
+        and `"//foo/bar/..."`. Likewise, `"//visibility:public"` and
+        `"//visibility:private"` are just `"public"` and `"private"`.
 
 For example, if `//some/package:mytarget` has its `visibility` set to
 `[":__subpackages__", "//tests:__pkg__"]`, then it could be used by any target
diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm
index 164ece718cf14d..c8c64fd2aea3aa 100644
--- a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm
+++ b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm
@@ -178,13 +178,18 @@ not themselves have any visibility protection.</p>
 
         <li>As above, but with a trailing <code>/...</code>. For example, <code>
         //foo/...</code> specifies the set of <code>//foo</code> and all its
-        subpackages. As a special case, <code>//...</code> specifies all
-        packages in any repository (in other words, public).
+        subpackages. <code>//...</code> specifies all packages in the current
+        repository.
+
+        <li>The strings <code>public</code> or <code>private</code>, which
+        respectively specify every package or no package. (This form requires
+        the flag <code>--incompatible_package_group_has_public_syntax</code> to
+        be set.)
 
         </ol>
 
-        <p>In addition, a package specification may also be prefixed with
-        <code>-</code> to indicate that it is negated.</p>
+        <p>In addition, the first two kinds of package specifications may also
+        be prefixed with <code>-</code> to indicate that they are negated.</p>
 
         <p>The package group contains any package that matches at least one of
         its positive specifications and none of its negative specifications
@@ -193,11 +198,24 @@ not themselves have any visibility protection.</p>
         subpackages of <code>//foo/tests</code>. (<code>//foo</code> itself is
         included while </code>//foo/tests</code> itself is not.)</p>
 
-        <p>Aside from <code>//...</code>, there is no way to directly specify
+        <p>Aside from public visibility, there is no way to directly specify
         packages outside the current repository.</p>
 
         <p>If this attribute is missing, it is the same as setting it to an
-        empty list (in other words, private).
+        empty list, which is also the same as setting it to a list containing
+        only <code>private</code>.
+
+        <p><i>Note:</i> The specification <code>//...</code> has a legacy behavior
+        of being the same as <code>public</code>. This behavior is disabled when
+        <code>--incompatible_fix_package_group_reporoot_syntax</code> is
+        enabled.</p>
+
+        <p><i>Note:</i> As a legacy behavior, when this attribute is serialized as
+        part of <code>bazel query --output=proto</code> (or
+        <code>--output=xml</code>), the leading slashes are omitted if
+        <code>--incompatible_package_group_includes_double_slash</code> is not
+        enabled. For instance, <code>//pkg/foo/...</code> will output as
+        <code>\"pkg/foo/...\"</code>.</p>
       </td>
     </tr>
     <tr>
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index f9dec2a7979f18..a73a3d38f20d15 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1652,14 +1652,26 @@ Label createLabel(String targetName) throws LabelSyntaxException {
       return Label.create(pkg.getPackageIdentifier(), targetName);
     }
 
-    /**
-     * Adds a package group to the package.
-     */
-    void addPackageGroup(String name, Collection<String> packages, Collection<Label> includes,
-        EventHandler eventHandler, Location location)
+    /** Adds a package group to the package. */
+    void addPackageGroup(
+        String name,
+        Collection<String> packages,
+        Collection<Label> includes,
+        boolean allowPublicPrivate,
+        boolean repoRootMeansCurrentRepo,
+        EventHandler eventHandler,
+        Location location)
         throws NameConflictException, LabelSyntaxException {
       PackageGroup group =
-          new PackageGroup(createLabel(name), pkg, packages, includes, eventHandler, location);
+          new PackageGroup(
+              createLabel(name),
+              pkg,
+              packages,
+              includes,
+              allowPublicPrivate,
+              repoRootMeansCurrentRepo,
+              eventHandler,
+              location);
       Target existing = targets.get(group.getName());
       if (existing != null) {
         throw nameConflict(group, existing);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
index ebad8d7076dafe..f9f9a79c902e06 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
@@ -47,6 +47,8 @@ public PackageGroup(
       Package pkg,
       Collection<String> packageSpecifications,
       Collection<Label> includes,
+      boolean allowPublicPrivate,
+      boolean repoRootMeansCurrentRepo,
       EventHandler eventHandler,
       Location location) {
     this.label = label;
@@ -61,7 +63,11 @@ public PackageGroup(
       PackageSpecification specification = null;
       try {
         specification =
-            PackageSpecification.fromString(label.getRepository(), packageSpecification);
+            PackageSpecification.fromString(
+                label.getRepository(),
+                packageSpecification,
+                allowPublicPrivate,
+                repoRootMeansCurrentRepo);
       } catch (PackageSpecification.InvalidPackageSpecificationException e) {
         errorsFound = true;
         eventHandler.handle(
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
index 8c5433d1280ebf..51696ae8ea8f3d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
@@ -47,11 +47,15 @@
  * packages" specification.
  */
 public abstract class PackageSpecification {
-  private static final String PACKAGE_LABEL = "__pkg__";
-  private static final String SUBTREE_LABEL = "__subpackages__";
+  private static final String PUBLIC_VISIBILITY = "public";
+  private static final String PRIVATE_VISIBILITY = "private";
   private static final String ALL_BENEATH_SUFFIX = "/...";
   private static final String NEGATIVE_PREFIX = "-";
 
+  // Used for interpreting `visibility` labels.
+  private static final String PACKAGE_LABEL = "__pkg__";
+  private static final String SUBTREE_LABEL = "__subpackages__";
+
   /** Returns {@code true} if the package spec includes the provided {@code packageName}. */
   protected abstract boolean containsPackage(PackageIdentifier packageName);
 
@@ -82,9 +86,13 @@ public abstract class PackageSpecification {
    * additional context. But, for instance, if interpreted with respect to a {@code package_group}'s
    * {@code packages} attribute, the strings always have the same repository as the package group.
    */
-  // TODO(brandjon): This method's main benefit is that it's round-trippable. We could eliminate
-  // it in favor of asString() if we provided a public variant of fromString() that tolerates
-  // repositories.
+  // TODO(brandjon): This method's main benefit is that it's round-trippable. But we can already
+  // achieve the same thing with asString(), if the caller parses out the repo to pass to
+  // fromString() as a separate arg. It'd be nice to eliminate this method in favor of asString()
+  // and make a version of fromString() that can parse repo names in the label. We'd just have to
+  // mimic the Label parsing of repo (we can't reuse Label parsing directly since it won't like the
+  // `/...` syntax). Repo remapping shouldn't come up, since the names we get from stringification
+  // ought to already be canonical/absolute.
   protected abstract String asStringWithoutRepository();
 
   @Override
@@ -93,71 +101,124 @@ public String toString() {
   }
 
   /**
-   * Parses the provided {@link String} into a {@link PackageSpecification}.
+   * Parses the string {@code spec} into a {@link PackageSpecification}, within the context of the
+   * given repository name.
+   *
+   * <p>{@code spec} may have the following forms:
+   *
+   * <ol>
+   *   <li>The full name of a single package, without repository qualification, prefixed with "//"
+   *       (e.g. "//foo/bar"). The resulting specification contains exactly that package.
+   *   <li>The same, but suffixed with "/..." for a non-root package ("//foo/bar/...") or "..." for
+   *       the root package ("//..."). The resulting specification contains that package and all its
+   *       subpackages.
+   *   <li>The string constants "public" or "private". The resulting specification contains either
+   *       all packages or no packages, respectively.
+   * </ol>
+   *
+   * In the first two cases, the repository of the given package name is taken to be {@code
+   * repositoryName}. In the third case the repository name is ignored.
+   *
+   * <p>In the first two cases, {@code spec} may also be prefixed by a "-". The resulting
+   * specification contains the same set of packages but is marked as being negated. (Negation logic
+   * is applied at the level of {@link PackageGroupContents}.)
    *
-   * <p>The {@link String} must have one of the following forms:
+   * <p>Setting {@code allowPublicPrivate} to false disallows the string constants "public" and
+   * "private". Note that if {@link #asString} is called with {@code includeDoubleSlash} set to
+   * false, the stringification of "public" and "//public" is ambiguous (likewise for private),
+   * hence why it might be appropriate to prohibit these forms.
    *
-   * <ul>
-   * <li>The full name of a single package, without repository qualification, prefixed with "//"
-   *     (e.g. "//foo/bar"). This results in a {@link PackageSpecification} that contains exactly
-   *     the named package.
-   * <li>The full name of a single package, without repository qualification, prefixed with "//",
-   *     and suffixed with "/..." (e.g. "//foo/bar/...") This results in a {@link
-   *     PackageSpecification} that contains all transitive subpackages of that package, inclusive.
-   * <li>Exactly "//...". This results in a {@link PackageSpecification} that contains all packages.
-   * </ul>
+   * <p>Setting {@code repoRootMeansCurrentRepo} to false restores the following legacy behavior: In
+   * the specific case where {@code spec} is "//..." (or its negation), the package specification
+   * contains <i>all</i> packages (possibly marked as negated) rather than just those packages in
+   * {@code repositoryName}. In other words, "//..." behaves the same as "public". However, "//"
+   * still represents just the root package of {@code repositoryName}.
    *
-   * <p>If and only if the {@link String} is one of the first two forms, the returned {@link
-   * PackageSpecification} is specific to the provided {@link RepositoryName}. Note that it is not
-   * possible to construct a repository-specific {@link PackageSpecification} for all transitive
-   * subpackages of the root package (i.e. a repository-specific "//...").
+   * <p>To protect against requiring users to update to a disallowed syntax, it is illegal to
+   * specify {@code repoRootMeansCurrentRepo} without also specifying {@code allowPublicPrivate}.
    *
-   * <p>Throws {@link InvalidPackageSpecificationException} if the {@link String} cannot be parsed.
+   * @throws InvalidPackageSpecificationException if the string does not fit one of these forms
    */
-  public static PackageSpecification fromString(RepositoryName repositoryName, String spec)
+  // TODO(#16365): Remove allowPublicPrivate.
+  // TODO(#16324): Remove legacy behavior and repoRootMeansCurrentRepo param.
+  public static PackageSpecification fromString(
+      RepositoryName repositoryName,
+      String spec,
+      boolean allowPublicPrivate,
+      boolean repoRootMeansCurrentRepo)
       throws InvalidPackageSpecificationException {
-    String result = spec;
+    if (repoRootMeansCurrentRepo && !allowPublicPrivate) {
+      throw new InvalidPackageSpecificationException(
+          "Cannot use new \"//...\" meaning without allowing new \"public\" syntax. Try enabling"
+              + " --incompatible_package_group_has_public_syntax or disabling"
+              + " --incompatible_fix_package_group_reporoot_syntax.");
+    }
+    if (!allowPublicPrivate
+        && (spec.equals(PUBLIC_VISIBILITY) || spec.equals(PRIVATE_VISIBILITY))) {
+      throw new InvalidPackageSpecificationException(
+          String.format(
+              "Use of \"%s\" package specification requires enabling"
+                  + " --incompatible_package_group_has_public_syntax",
+              spec));
+    }
     boolean negative = false;
-    if (result.startsWith(NEGATIVE_PREFIX)) {
+    if (spec.startsWith(NEGATIVE_PREFIX)) {
       negative = true;
-      result = result.substring(NEGATIVE_PREFIX.length());
+      spec = spec.substring(NEGATIVE_PREFIX.length());
+      if (spec.equals(PUBLIC_VISIBILITY) || spec.equals(PRIVATE_VISIBILITY)) {
+        throw new InvalidPackageSpecificationException(
+            String.format("Cannot negate \"%s\" package specification", spec));
+      }
     }
-    PackageSpecification packageSpecification = fromStringPositive(repositoryName, result);
+    PackageSpecification packageSpecification =
+        fromStringPositive(repositoryName, spec, repoRootMeansCurrentRepo);
     return negative ? new NegativePackageSpecification(packageSpecification) : packageSpecification;
   }
 
-  private static PackageSpecification fromStringPositive(RepositoryName repositoryName, String spec)
+  private static PackageSpecification fromStringPositive(
+      RepositoryName repositoryName, String spec, boolean repoRootMeansCurrentRepo)
       throws InvalidPackageSpecificationException {
-    String result = spec;
+    if (spec.equals(PUBLIC_VISIBILITY)) {
+      return AllPackages.INSTANCE;
+    } else if (spec.equals(PRIVATE_VISIBILITY)) {
+      return NoPackages.INSTANCE;
+    }
+    if (!spec.startsWith("//")) {
+      throw new InvalidPackageSpecificationException(
+          String.format(
+              "invalid package name '%s': must start with '//' or be 'public' or 'private'", spec));
+    }
+
+    String pkgPath;
     boolean allBeneath = false;
-    if (result.endsWith(ALL_BENEATH_SUFFIX)) {
+    if (spec.endsWith(ALL_BENEATH_SUFFIX)) {
       allBeneath = true;
-      result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length());
-      if (result.equals("/")) {
+      pkgPath = spec.substring(0, spec.length() - ALL_BENEATH_SUFFIX.length());
+      if (pkgPath.equals("/")) {
         // spec was "//...".
-        return AllPackages.EVERYTHING;
+        if (repoRootMeansCurrentRepo) {
+          pkgPath = "//";
+        } else {
+          // Legacy behavior: //... is "public".
+          return AllPackages.INSTANCE;
+        }
       }
+    } else {
+      pkgPath = spec;
     }
 
-    if (!spec.startsWith("//")) {
-      throw new InvalidPackageSpecificationException(
-          String.format("invalid package name '%s': must start with '//'", spec));
-    }
-
-    PackageIdentifier packageId;
+    PackageIdentifier unqualifiedPkgId;
     try {
-      packageId = PackageIdentifier.parse(result);
+      unqualifiedPkgId = PackageIdentifier.parse(pkgPath);
     } catch (LabelSyntaxException e) {
       throw new InvalidPackageSpecificationException(
           String.format("invalid package name '%s': %s", spec, e.getMessage()));
     }
-    Verify.verify(packageId.getRepository().isMain());
+    Verify.verify(unqualifiedPkgId.getRepository().isMain());
 
-    PackageIdentifier packageIdForSpecifiedRepository =
-        PackageIdentifier.create(repositoryName, packageId.getPackageFragment());
-    return allBeneath
-        ? new AllPackagesBeneath(packageIdForSpecifiedRepository)
-        : new SinglePackage(packageIdForSpecifiedRepository);
+    PackageIdentifier pkgId =
+        PackageIdentifier.create(repositoryName, unqualifiedPkgId.getPackageFragment());
+    return allBeneath ? new AllPackagesBeneath(pkgId) : new SinglePackage(pkgId);
   }
 
   /**
@@ -165,13 +226,21 @@ private static PackageSpecification fromStringPositive(RepositoryName repository
    *
    * <p>This rejects negative package patterns, and translates the exception type into {@code
    * EvalException}.
+   *
+   * <p>Note that .bzl visibility package specifications always behave as if {@code
+   * --incompatible_package_group_has_public_syntax} and {@code
+   * --incompatible_fix_package_group_reporoot_syntax} are enabled.
    */
-  // TODO(b/22193153): Support negatives too.
   public static PackageSpecification fromStringForBzlVisibility(
       RepositoryName repositoryName, String spec) throws EvalException {
     PackageSpecification result;
     try {
-      result = fromString(repositoryName, spec);
+      result =
+          fromString(
+              repositoryName,
+              spec,
+              /*allowPublicPrivate=*/ true,
+              /*repoRootMeansCurrentRepo=*/ true);
     } catch (InvalidPackageSpecificationException e) {
       throw new EvalException(e.getMessage());
     }
@@ -193,8 +262,8 @@ public static PackageSpecification fromStringForBzlVisibility(
    *
    * <p>If the label's name is neither "__pkg__" nor "__subpackages__", this returns {@code null}.
    *
-   * <p>Note that there is no {@link Label} associated with the {@link RepositoryName}-agnostic "all
-   * packages" specification (corresponding to {@code #fromString(null, "//...")}).
+   * <p>Note that there is no {@link Label} associated with the {@link RepositoryName}-agnostic
+   * "public" specification ("//..." under legacy semantics).
    */
   @Nullable
   static PackageSpecification fromLabel(Label label) {
@@ -208,7 +277,11 @@ static PackageSpecification fromLabel(Label label) {
   }
 
   public static PackageSpecification everything() {
-    return AllPackages.EVERYTHING;
+    return AllPackages.INSTANCE;
+  }
+
+  public static PackageSpecification nothing() {
+    return NoPackages.INSTANCE;
   }
 
   private static final class SinglePackage extends PackageSpecification {
@@ -291,7 +364,12 @@ protected String asString(boolean includeDoubleSlash) {
 
     @Override
     protected String asStringWithoutRepository() {
-      return "//" + prefix.getPackageFragment().getPathString() + ALL_BENEATH_SUFFIX;
+      if (prefix.getPackageFragment().equals(PathFragment.EMPTY_FRAGMENT)) {
+        // Special case: Emit "//..." rather than suffixing "/...", which would yield "///...".
+        return "//...";
+      } else {
+        return "//" + prefix.getPackageFragment().getPathString() + ALL_BENEATH_SUFFIX;
+      }
     }
 
     @Override
@@ -335,11 +413,6 @@ protected String asStringWithoutRepository() {
       return "-" + delegate.asStringWithoutRepository();
     }
 
-    @Override
-    public int hashCode() {
-      return delegate.hashCode();
-    }
-
     @Override
     public boolean equals(Object obj) {
       if (this == obj) {
@@ -348,12 +421,17 @@ public boolean equals(Object obj) {
       return obj instanceof NegativePackageSpecification
           && delegate.equals(((NegativePackageSpecification) obj).delegate);
     }
+
+    @Override
+    public int hashCode() {
+      return NegativePackageSpecification.class.hashCode() ^ delegate.hashCode();
+    }
   }
 
   @VisibleForSerialization
   static final class AllPackages extends PackageSpecification {
     @SerializationConstant @VisibleForSerialization
-    static final PackageSpecification EVERYTHING = new AllPackages();
+    static final PackageSpecification INSTANCE = new AllPackages();
 
     @Override
     protected boolean containsPackage(PackageIdentifier packageName) {
@@ -362,14 +440,15 @@ protected boolean containsPackage(PackageIdentifier packageName) {
 
     @Override
     protected String asString(boolean includeDoubleSlash) {
-      // Note that "//..." is the desired result, not "...", even under the legacy behavior of
-      // includeDoubleSlash=false.
-      return "//...";
+      // Under legacy formatting rules, use legacy syntax. This avoids ambiguity between "public"
+      // and "//public", and ensures that AllPackages is round-trippable when the value of
+      // includeDoubleSlash matches allowPublicPrivate.
+      return includeDoubleSlash ? "public" : "//...";
     }
 
     @Override
     protected String asStringWithoutRepository() {
-      return "//...";
+      return "public";
     }
 
     @Override
@@ -379,7 +458,38 @@ public boolean equals(Object o) {
 
     @Override
     public int hashCode() {
-      return "//...".hashCode();
+      return AllPackages.class.hashCode();
+    }
+  }
+
+  @VisibleForSerialization
+  static final class NoPackages extends PackageSpecification {
+    @SerializationConstant @VisibleForSerialization
+    static final PackageSpecification INSTANCE = new NoPackages();
+
+    @Override
+    protected boolean containsPackage(PackageIdentifier packageName) {
+      return false;
+    }
+
+    @Override
+    protected String asString(boolean includeDoubleSlash) {
+      return "private";
+    }
+
+    @Override
+    protected String asStringWithoutRepository() {
+      return "private";
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      return o instanceof NoPackages;
+    }
+
+    @Override
+    public int hashCode() {
+      return NoPackages.class.hashCode();
     }
   }
 
@@ -437,7 +547,11 @@ public static PackageGroupContents create(
       for (PackageSpecification spec : packageSpecifications) {
         if (spec instanceof SinglePackage) {
           singlePositives.put(((SinglePackage) spec).singlePackageName, spec);
-        } else if (spec instanceof AllPackages || spec instanceof AllPackagesBeneath) {
+        } else if (spec instanceof AllPackages
+            || spec instanceof AllPackagesBeneath
+            // We can't drop NoPackages because it still needs to be serialized, e.g. in bazel query
+            // output.
+            || spec instanceof NoPackages) {
           otherPositives.add(spec);
         } else if (spec instanceof NegativePackageSpecification) {
           negatives.add(spec);
@@ -483,6 +597,10 @@ public boolean containsPackage(PackageIdentifier packageIdentifier) {
      *
      * <p>Note that strings for specs that cross repositories can't be reparsed using {@link
      * PackageSpecification#fromString}.
+     *
+     * <p>The special public constant will serialize as {@code "public"} if {@code
+     * includeDoubleSlash} is true, and {@code "//..."} otherwise. The private constant will always
+     * serialize as {@code "private"},
      */
     public Stream<String> streamPackageStrings(boolean includeDoubleSlash) {
       return streamSpecs().map(spec -> spec.asString(includeDoubleSlash));
@@ -492,6 +610,9 @@ public Stream<String> streamPackageStrings(boolean includeDoubleSlash) {
      * Maps {@link PackageSpecification#asStringWithoutRepository} to the component package specs.
      *
      * <p>Note that this is ambiguous w.r.t. specs that reference other repositories.
+     *
+     * <p>The special public and private constants will serialize as {@code "public"} and {@code
+     * "private"} respectively.
      */
     public Stream<String> streamPackageStringsWithoutRepository() {
       return streamSpecs().map(PackageSpecification::asStringWithoutRepository);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
index f45c882c1edad8..703b9deb329d0a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
@@ -542,7 +542,18 @@ public NoneType packageGroup(
 
     Location loc = thread.getCallerLocation();
     try {
-      context.pkgBuilder.addPackageGroup(name, packages, includes, context.eventHandler, loc);
+      context.pkgBuilder.addPackageGroup(
+          name,
+          packages,
+          includes,
+          /*allowPublicPrivate=*/ thread
+              .getSemantics()
+              .getBool(BuildLanguageOptions.INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX),
+          /*repoRootMeansCurrentRepo=*/ thread
+              .getSemantics()
+              .getBool(BuildLanguageOptions.INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX),
+          context.eventHandler,
+          loc);
       return Starlark.NONE;
     } catch (LabelSyntaxException e) {
       throw Starlark.errorf("package group has invalid name: %s: %s", name, e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
index a3518ad3c1cf64..460506127e3fef 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java
@@ -442,6 +442,31 @@ public final class BuildLanguageOptions extends OptionsBase {
               + "instead return a list of provider instances.")
   public boolean incompatibleDisallowStructProviderSyntax;
 
+  @Option(
+      name = "incompatible_package_group_has_public_syntax",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "In package_group's `packages` attribute, allows writing \"public\" or \"private\" to"
+              + " refer to all packages or no packages respectively.")
+  public boolean incompatiblePackageGroupHasPublicSyntax;
+
+  @Option(
+      name = "incompatible_fix_package_group_reporoot_syntax",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "In package_group's `packages` attribute, changes the meaning of the value \"//...\" to"
+              + " refer to all packages in the current repository instead of all packages in any"
+              + " repository. You can use the special value \"public\" in place of \"//...\" to"
+              + " obtain the old behavior. This flag requires"
+              + " that --incompatible_package_group_has_public_syntax also be enabled.")
+  public boolean incompatibleFixPackageGroupReporootSyntax;
+
   @Option(
       name = "incompatible_visibility_private_attributes_at_definition",
       defaultValue = "false",
@@ -675,6 +700,12 @@ public StarlarkSemantics toStarlarkSemantics() {
             .setBool(
                 INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX,
                 incompatibleDisallowStructProviderSyntax)
+            .setBool(
+                INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX,
+                incompatiblePackageGroupHasPublicSyntax)
+            .setBool(
+                INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX,
+                incompatibleFixPackageGroupReporootSyntax)
             .setBool(INCOMPATIBLE_JAVA_COMMON_PARAMETERS, incompatibleJavaCommonParameters)
             .setBool(INCOMPATIBLE_NEW_ACTIONS_API, incompatibleNewActionsApi)
             .setBool(INCOMPATIBLE_NO_ATTR_LICENSE, incompatibleNoAttrLicense)
@@ -761,6 +792,10 @@ public StarlarkSemantics toStarlarkSemantics() {
   public static final String INCOMPATIBLE_DISALLOW_EMPTY_GLOB = "-incompatible_disallow_empty_glob";
   public static final String INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX =
       "-incompatible_disallow_struct_provider_syntax";
+  public static final String INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX =
+      "-incompatible_package_group_has_public_syntax";
+  public static final String INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX =
+      "-incompatible_fix_package_group_reporoot_syntax";
   public static final String INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE =
       "+incompatible_do_not_split_linking_cmdline";
   public static final String INCOMPATIBLE_JAVA_COMMON_PARAMETERS =
diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
index 5bd8261fa9a61a..eb44883d47703f 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
@@ -137,6 +137,17 @@ public String getLineTerminator() {
               + "query: no-op (aspects are always followed).")
   public boolean useAspects;
 
+  @Option(
+      name = "incompatible_package_group_includes_double_slash",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.QUERY,
+      effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "If enabled, when outputting package_group's `packages` attribute, the leading `//`"
+              + " will not be omitted.")
+  public boolean incompatiblePackageGroupIncludesDoubleSlash;
+
   /** Return the current options as a set of QueryEnvironment settings. */
   public Set<Setting> toSettings() {
     Set<Setting> settings = EnumSet.noneOf(Setting.class);
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
index d3f85d8af73a82..3c1eb27e152b7a 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
@@ -101,6 +101,7 @@ public class ProtoOutputFormatter extends AbstractUnorderedFormatter {
 
   private AspectResolver aspectResolver;
   private DependencyFilter dependencyFilter;
+  private boolean packageGroupIncludesDoubleSlash;
   private boolean relativeLocations;
   private boolean displaySourceFileLocation;
   private boolean includeDefaultValues = true;
@@ -126,6 +127,7 @@ public void setOptions(
     super.setOptions(options, aspectResolver, hashFunction);
     this.aspectResolver = aspectResolver;
     this.dependencyFilter = FormatUtils.getDependencyFilter(options);
+    this.packageGroupIncludesDoubleSlash = options.incompatiblePackageGroupIncludesDoubleSlash;
     this.relativeLocations = options.relativeLocations;
     this.displaySourceFileLocation = options.displaySourceFileLocation;
     this.includeDefaultValues = options.protoIncludeDefaultValues;
@@ -344,9 +346,8 @@ public Build.Target toTargetProtoBuffer(Target target, Object extraDataForAttrHa
       PackageGroup packageGroup = (PackageGroup) target;
       Build.PackageGroup.Builder packageGroupPb =
           Build.PackageGroup.newBuilder().setName(packageGroup.getLabel().toString());
-      // TODO(b/77598306): Migrate to format with leading double slash
       for (String containedPackage :
-          packageGroup.getContainedPackages(/*includeDoubleSlash=*/ false)) {
+          packageGroup.getContainedPackages(packageGroupIncludesDoubleSlash)) {
         packageGroupPb.addContainedPackage(containedPackage);
       }
       for (Label include : packageGroup.getIncludes()) {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java
index bcac1c684b0af2..1fff552a1a61ce 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java
@@ -61,6 +61,7 @@ class XmlOutputFormatter extends AbstractUnorderedFormatter {
 
   private AspectResolver aspectResolver;
   private DependencyFilter dependencyFilter;
+  private boolean packageGroupIncludesDoubleSlash;
   private boolean relativeLocations;
   private boolean displaySourceFileLocation;
   private QueryOptions queryOptions;
@@ -83,6 +84,7 @@ public void setOptions(
     super.setOptions(options, aspectResolver, hashFunction);
     this.aspectResolver = aspectResolver;
     this.dependencyFilter = FormatUtils.getDependencyFilter(options);
+    this.packageGroupIncludesDoubleSlash = options.incompatiblePackageGroupIncludesDoubleSlash;
     this.relativeLocations = options.relativeLocations;
     this.displaySourceFileLocation = options.displaySourceFileLocation;
 
@@ -208,8 +210,7 @@ private Element createTargetElement(Document doc, Target target)
           createValueElement(
               doc,
               Type.STRING_LIST,
-              // TODO(b/77598306): Migrate to format with leading double slash
-              packageGroup.getContainedPackages(/*includeDoubleSlash=*/ false));
+              packageGroup.getContainedPackages(packageGroupIncludesDoubleSlash));
       packages.setAttribute("name", "packages");
       elem.appendChild(packages);
     } else if (target instanceof OutputFile) {
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
index ed83559fdaea06..07c9a770ae030d 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
@@ -43,7 +43,12 @@ public void testNoDeadlockOnPackageGroupCreation() throws Exception {
               try {
                 RepositoryName defaultRepoName =
                     Label.parseAbsoluteUnchecked("//context").getRepository();
-                groupQueue.put(PackageSpecification.fromString(defaultRepoName, "//fruits/..."));
+                groupQueue.put(
+                    PackageSpecification.fromString(
+                        defaultRepoName,
+                        "//fruits/...",
+                        /*allowPublicPrivate=*/ true,
+                        /*repoRootMeansCurrentRepo=*/ true));
               } catch (Exception e) {
                 // Can't throw from Runnable, but this will cause the test to timeout
                 // when the consumer can't take the object.
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
index b6baeaa59e8498..f91a8492252171 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
@@ -218,7 +218,133 @@ public void testNegative_subpackages() throws Exception {
   }
 
   @Test
-  public void testNegative_everything() throws Exception {
+  public void testEverythingSpecificationWorks() throws Exception {
+    setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true");
+
+    scratch.file(
+        "fruits/BUILD", //
+        "package_group(",
+        "    name = 'mango',",
+        "    packages = ['public'],",
+        ")");
+    PackageGroup grp = getPackageGroup("fruits", "mango");
+
+    // Assert that we're using the right package spec.
+    assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true)).containsExactly("public");
+    // Assert that this package spec contains packages from both inside and outside the main repo.
+    assertThat(grp.contains(pkgId("pkg"))).isTrue();
+    assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue();
+  }
+
+  @Test
+  public void testNothingSpecificationWorks() throws Exception {
+    setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true");
+
+    scratch.file(
+        "fruits/BUILD", //
+        "package_group(",
+        "    name = 'mango',",
+        "    packages = ['private'],",
+        ")");
+    PackageGroup grp = getPackageGroup("fruits", "mango");
+
+    // Assert that we're using the right package spec.
+    assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true)).containsExactly("private");
+    assertThat(grp.contains(pkgId("anything"))).isFalse();
+  }
+
+  @Test
+  public void testPublicPrivateAreNotAccessibleWithoutFlag() throws Exception {
+    setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=false");
+
+    scratch.file(
+        "foo/BUILD", //
+        "package_group(",
+        "    name = 'grp1',",
+        "    packages = ['public'],",
+        ")");
+    scratch.file(
+        "bar/BUILD", //
+        "package_group(",
+        "    name = 'grp2',",
+        "    packages = ['private'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    getPackageGroup("foo", "grp1");
+    assertContainsEvent(
+        "Use of \"public\" package specification requires enabling"
+            + " --incompatible_package_group_has_public_syntax");
+    getPackageGroup("bar", "grp2");
+    assertContainsEvent(
+        "Use of \"private\" package specification requires enabling"
+            + " --incompatible_package_group_has_public_syntax");
+  }
+
+  @Test
+  public void testRepoRootSubpackagesIsPublic_withoutFlag() throws Exception {
+    setBuildLanguageOptions("--incompatible_fix_package_group_reporoot_syntax=false");
+
+    scratch.file(
+        "fruits/BUILD", //
+        "package_group(",
+        "    name = 'mango',",
+        "    packages = ['//...'],",
+        ")");
+    PackageGroup grp = getPackageGroup("fruits", "mango");
+
+    // Use includeDoubleSlash=true to make package spec stringification distinguish AllPackages from
+    // AllPackagesBeneath with empty package path.
+    assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true))
+        // Assert that "//..." gave us AllPackages.
+        .containsExactly("public");
+    assertThat(grp.contains(pkgId("pkg"))).isTrue();
+    assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue();
+  }
+
+  @Test
+  public void testRepoRootSubpackagesIsNotPublic_withFlag() throws Exception {
+    setBuildLanguageOptions(
+        "--incompatible_package_group_has_public_syntax=true",
+        "--incompatible_fix_package_group_reporoot_syntax=true");
+
+    scratch.file(
+        "fruits/BUILD", //
+        "package_group(",
+        "    name = 'mango',",
+        "    packages = ['//...'],",
+        ")");
+    PackageGroup grp = getPackageGroup("fruits", "mango");
+
+    // Use includeDoubleSlash=true to make package spec stringification distinguish AllPackages from
+    // AllPackagesBeneath with empty package path.
+    assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true))
+        // Assert that "//..." gave us AllPackagesBeneath.
+        .containsExactly("//...");
+    assertThat(grp.contains(pkgId("pkg"))).isTrue();
+    assertThat(grp.contains(pkgId("somerepo", "pkg"))).isFalse();
+  }
+
+  @Test
+  public void testCannotUseNewRepoRootSyntaxWithoutPublicSyntax() throws Exception {
+    setBuildLanguageOptions(
+        "--incompatible_package_group_has_public_syntax=false",
+        "--incompatible_fix_package_group_reporoot_syntax=true");
+
+    scratch.file(
+        "fruits/BUILD", //
+        "package_group(",
+        "    name = 'mango',",
+        "    packages = ['//something'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    getPackageGroup("fruits", "mango");
+    assertContainsEvent("Cannot use new \"//...\" meaning without allowing new \"public\" syntax.");
+  }
+
+  @Test
+  public void testNegative_repoRootSubpackages() throws Exception {
     scratch.file(
         "test/BUILD",
         "package_group(",
@@ -236,20 +362,35 @@ public void testNegative_everything() throws Exception {
   }
 
   @Test
-  public void testEverythingSpecificationWorks() throws Exception {
+  public void testNegative_public() throws Exception {
+    setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true");
+
     scratch.file(
-        "fruits/BUILD", //
+        "fruits/BUILD",
         "package_group(",
-        "    name = 'mango',",
-        "    packages = ['//...'],",
+        "    name = 'apple',",
+        "    packages = ['-public'],",
         ")");
-    PackageGroup grp = getPackageGroup("fruits", "mango");
 
-    // Assert that we're actually using the "everything" package specification, and assert that this
-    // means we include packages from the main repo but also from other repos.
-    assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ false)).containsExactly("//...");
-    assertThat(grp.contains(pkgId("pkg"))).isTrue();
-    assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue();
+    reporter.removeHandler(failFastHandler);
+    getPackageGroup("fruits", "apple");
+    assertContainsEvent("Cannot negate \"public\" package specification");
+  }
+
+  @Test
+  public void testNegative_private() throws Exception {
+    setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true");
+
+    scratch.file(
+        "fruits/BUILD",
+        "package_group(",
+        "    name = 'apple',",
+        "    packages = ['-private'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    getPackageGroup("fruits", "apple");
+    assertContainsEvent("Cannot negate \"private\" package specification");
   }
 
   @Test
@@ -275,16 +416,18 @@ public void testStringification() throws Exception {
     PackageGroupContents contents =
         PackageGroupContents.create(
             ImmutableList.of(
-                PackageSpecification.fromString(main, "//a"),
-                PackageSpecification.fromString(main, "//a/b/..."),
-                PackageSpecification.fromString(main, "-//c"),
-                PackageSpecification.fromString(main, "-//c/d/..."),
-                PackageSpecification.fromString(main, "//..."),
-                PackageSpecification.fromString(main, "-//..."),
-                PackageSpecification.fromString(main, "//"),
-                PackageSpecification.fromString(main, "-//"),
-                PackageSpecification.fromString(other, "//z"),
-                PackageSpecification.fromString(other, "//...")));
+                pkgSpec(main, "//a"),
+                pkgSpec(main, "//a/b/..."),
+                pkgSpec(main, "-//c"),
+                pkgSpec(main, "-//c/d/..."),
+                pkgSpec(main, "//..."),
+                pkgSpec(main, "-//..."),
+                pkgSpec(main, "//"),
+                pkgSpec(main, "-//"),
+                pkgSpec(other, "//z"),
+                pkgSpec(other, "//..."),
+                pkgSpec(main, "public"),
+                pkgSpec(main, "private")));
     assertThat(
             contents.streamPackageStrings(/*includeDoubleSlash=*/ false).collect(toImmutableList()))
         .containsExactly(
@@ -297,8 +440,9 @@ public void testStringification() throws Exception {
             "",
             "-",
             "@other//z",
-            // TODO(#16323): When parsing is fixed, change this "//..." to "@other//...".
-            "//...");
+            "@other//...",
+            "//...", // legacy syntax for public
+            "private");
     assertThat(
             contents.streamPackageStrings(/*includeDoubleSlash=*/ true).collect(toImmutableList()))
         .containsExactly(
@@ -311,8 +455,9 @@ public void testStringification() throws Exception {
             "//",
             "-//",
             "@other//z",
-            // TODO(#16323): When parsing is fixed, change this "//..." to "@other//...".
-            "//...");
+            "@other//...",
+            "public",
+            "private");
     assertThat(contents.streamPackageStringsWithoutRepository().collect(toImmutableList()))
         .containsExactly(
             "//a",
@@ -324,7 +469,15 @@ public void testStringification() throws Exception {
             "//",
             "-//",
             "//z",
-            "//...");
+            "//...",
+            "public",
+            "private");
+  }
+
+  /** Convenience method for obtaining a PackageSpecification. */
+  private PackageSpecification pkgSpec(RepositoryName repository, String spec) throws Exception {
+    return PackageSpecification.fromString(
+        repository, spec, /*allowPublicPrivate=*/ true, /*repoRootMeansCurrentRepo=*/ true);
   }
 
   /** Convenience method for obtaining a PackageIdentifier. */
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
index f0ac79ea294c65..c6c712a92c535c 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java
@@ -600,6 +600,54 @@ public void testBzlVisibility_privateDifferentPackage() throws Exception {
     assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a.");
   }
 
+  @Test
+  public void testBzlVisibility_publicListElement() 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", //
+        // Tests "public" as a list item, and alongside other list items.
+        "visibility([\"public\", \"//c\"])",
+        "x = 1");
+
+    checkSuccessfulLookup("//a:foo.bzl");
+    assertNoEvents();
+  }
+
+  @Test
+  public void testBzlVisibility_privateListElement() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone");
+
+    scratch.file("a1/BUILD");
+    scratch.file(
+        "a1/foo.bzl", //
+        "load(\"//b:bar.bzl\", \"x\")");
+    scratch.file("a2/BUILD");
+    scratch.file(
+        "a2/foo.bzl", //
+        "load(\"//b:bar.bzl\", \"x\")");
+    scratch.file("b/BUILD");
+    scratch.file(
+        "b/bar.bzl", //
+        // Tests "private" as a list item, and alongside other list items.
+        "visibility([\"private\", \"//a1\"])",
+        "x = 1");
+
+    checkSuccessfulLookup("//a1:foo.bzl");
+    assertNoEvents();
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup(
+        "//a2:foo.bzl", "module //a2:foo.bzl contains .bzl load-visibility violations");
+    assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a2.");
+  }
+
   @Test
   public void testBzlVisibility_failureInDependency() throws Exception {
     setBuildLanguageOptions(
@@ -743,6 +791,65 @@ public void testBzlVisibility_enumeratedPackagesMultipleRepos() throws Exception
         "Starlark file @repo//lib:bar.bzl is not visible for loading from package //pkg.");
   }
 
+  // TODO(#16365): This test case can be deleted once --incompatible_package_group_has_public_syntax
+  // is deleted (not just flipped).
+  @Test
+  public void testBzlVisibility_canUsePublicPrivate_regardlessOfFlag() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true",
+        "--experimental_bzl_visibility_allowlist=everyone",
+        // Test that we can use "public" and "private" visibility for .bzl files even when the
+        // incompatible flag is disabled.
+        "--incompatible_package_group_has_public_syntax=false");
+
+    scratch.file("a/BUILD");
+    scratch.file(
+        "a/foo1.bzl", //
+        "visibility(\"public\")");
+    scratch.file(
+        "a/foo2.bzl", //
+        "visibility(\"private\")");
+
+    checkSuccessfulLookup("//a:foo1.bzl");
+    checkSuccessfulLookup("//a:foo2.bzl");
+    assertNoEvents();
+  }
+
+  // TODO(#16324): Once --incompatible_fix_package_group_reporoot_syntax is deleted (not just
+  // flipped), this test case will be redundant with tests for //... in PackageGroupTest. At that
+  // point we'll just delete this test case.
+  @Test
+  public void testBzlVisibility_repoRootSubpackagesIsNotPublic_regardlessOfFlag() throws Exception {
+    setBuildLanguageOptions(
+        "--experimental_bzl_visibility=true",
+        "--experimental_bzl_visibility_allowlist=everyone",
+        // Test that we get the fixed behavior even when the incompatible flag is disabled.
+        "--incompatible_fix_package_group_reporoot_syntax=false");
+
+    scratch.overwriteFile(
+        "WORKSPACE", //
+        "local_repository(",
+        "    name = 'repo',",
+        "    path = 'repo'",
+        ")");
+    scratch.file("repo/WORKSPACE");
+    scratch.file("repo/a/BUILD");
+    scratch.file(
+        "repo/a/foo.bzl", //
+        "load(\"@//b:bar.bzl\", \"x\")");
+    scratch.file("b/BUILD");
+    scratch.file(
+        "b/bar.bzl", //
+        "visibility([\"//...\"])",
+        "x = 1");
+
+    reporter.removeHandler(failFastHandler);
+    checkFailingLookup(
+        "@repo//a:foo.bzl", "module @repo//a:foo.bzl contains .bzl load-visibility violations");
+    assertContainsEvent(
+        "Starlark file //b:bar.bzl is not visible for loading from package @repo//a.");
+  }
+
   @Test
   public void testBzlVisibility_disallowsSubpackagesWithoutWildcard() throws Exception {
     setBuildLanguageOptions(