From cd82f68cfdf44aabaea1931ea9ff870ec7bb4af1 Mon Sep 17 00:00:00 2001
From: Googler <arostovtsev@google.com>
Date: Mon, 30 Sep 2024 16:31:14 -0700
Subject: [PATCH] Allow //visibility:public and //visibility:private in
 non-singleton visibility declarations

... but simplify the resulting visibility appropriately when saving the
visibility attribute: $foo plus public is public; $foo plus private is $foo;
and an empty visibility list is canonicalized to ["//visibility:private"].

This makes it easier for symbolic macros to grant visibilities on targets
that they declare.

Work toward https://github.com/bazelbuild/bazel/issues/19922.

The canonicalization of an empty visibility list to ["//visibility:private"]
is technically an incompatible change affecting query output, but is very
unlikely to break anything in practice.

RELNOTES: Non-singleton target visibility lists can now contain "//visibility:public" and "//visibility:private" elements; the result is appropriately simplified when assigned to an attribute: ["//foo:__subpackages__", "//visibility:public"] is saved as ["//visibility:public"], ["//foo:__subpackages__", "//visibility:private"] is saved as ["//foo:__subpackages__"], and for consistency's sake, an empty target visibility list [] is saved as ["//visibility:private"].
PiperOrigin-RevId: 680761159
Change-Id: Ic2edaf13dfe48b0f0014d404ced34aa7632d33c7
---
 .../packages/PackageGroupsRuleVisibility.java |  21 +++-
 .../build/lib/packages/RuleClass.java         |   2 +-
 .../build/lib/packages/RuleVisibility.java    |  83 +++++++++----
 .../devtools/build/lib/packages/RuleTest.java |  52 +++++---
 .../lib/packages/RuleVisibilityTest.java      | 112 ++++++++++++++++--
 src/test/shell/bazel/bazel_rules_test.sh      |  16 ---
 6 files changed, 218 insertions(+), 68 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
index 2f4beb46810c51..048367e47f32a3 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroupsRuleVisibility.java
@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -29,16 +31,31 @@ public abstract class PackageGroupsRuleVisibility implements RuleVisibility {
   @Override
   public abstract ImmutableList<Label> getDeclaredLabels();
 
-  /** Creates a {@link PackageGroupsRuleVisibility} from a list of labels. */
-  public static PackageGroupsRuleVisibility create(List<Label> labels) {
+  /**
+   * Creates a {@link PackageGroupsRuleVisibility} from a non-empty list of labels, which must have
+   * been previously validated and simplified by {@link RuleVisibility#validateAndSimplify}, and
+   * which must not be ["//visibility:public"] or ["//visibility:private"].
+   *
+   * <p>To parse a public or private visibility, use {@link RuleVisibility#parseIfConstant}.
+   */
+  static PackageGroupsRuleVisibility create(List<Label> labels) {
     ImmutableList.Builder<PackageSpecification> directPackageBuilder = ImmutableList.builder();
     ImmutableList.Builder<Label> packageGroupBuilder = ImmutableList.builder();
 
+    checkArgument(!labels.isEmpty(), "labels must not be empty");
     for (Label label : labels) {
       PackageSpecification specification = PackageSpecification.fromLabel(label);
       if (specification != null) {
         directPackageBuilder.add(specification);
       } else {
+        checkArgument(
+            !label.equals(RuleVisibility.PUBLIC_LABEL)
+                && !label.equals(RuleVisibility.PRIVATE_LABEL),
+            "labels list %s must %s",
+            labels,
+            labels.size() == 1
+                ? "not equal [\"//visibility:public\"] or [\"//visibility:private\"]"
+                : "be validated and simplified");
         packageGroupBuilder.add(label);
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index e1c5e20676edd9..931b9e73448a6a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -2239,7 +2239,7 @@ private <T> BitSet populateDefinedRuleAttributeValues(
         @SuppressWarnings("unchecked")
         List<Label> vis = (List<Label>) nativeAttributeValue;
         try {
-          RuleVisibility.validate(vis);
+          nativeAttributeValue = RuleVisibility.validateAndSimplify(vis);
         } catch (EvalException e) {
           rule.reportError(rule.getLabel() + " " + e.getMessage(), eventHandler);
         }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java b/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java
index 1fb555c1e5e657..f564cb5d39234c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleVisibility.java
@@ -17,6 +17,7 @@
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.List;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
@@ -94,14 +95,15 @@ public String toString() {
 
   /** Validates and parses the given labels into a {@link RuleVisibility}. */
   static RuleVisibility parse(List<Label> labels) throws EvalException {
-    validate(labels);
-    return parseUnchecked(labels);
+    return parseUnchecked(validateAndSimplify(labels));
   }
 
   /**
-   * Same as {@link #parse} except does not perform validation checks.
+   * Same as {@link #parse} except does not perform validation checks or public/private
+   * simplification.
    *
-   * <p>Use only after the given labels have been {@linkplain #validate validated}.
+   * <p>Use only after the given labels have been {@linkplain #validateAndSimplify validated and
+   * simplified}.
    */
   static RuleVisibility parseUnchecked(List<Label> labels) {
     RuleVisibility result = parseIfConstant(labels);
@@ -115,14 +117,15 @@ static RuleVisibility parseUnchecked(List<Label> labels) {
    * If the given list of labels represents a constant {@link RuleVisibility} ({@link #PUBLIC} or
    * {@link #PRIVATE}), returns that visibility instance, otherwise returns {@code null}.
    *
-   * <p>Use only after the given labels have been {@linkplain #validate validated}.
+   * <p>Use only after the given labels have been {@linkplain #validateAndSimplify validated and
+   * simplified}.
    */
   @Nullable
   static RuleVisibility parseIfConstant(List<Label> labels) {
     if (labels.size() != 1) {
       return null;
     }
-    Label label = labels.get(0);
+    Label label = labels.getFirst();
     if (label.equals(PUBLIC_LABEL)) {
       return PUBLIC;
     }
@@ -132,25 +135,57 @@ static RuleVisibility parseIfConstant(List<Label> labels) {
     return null;
   }
 
-  static void validate(List<Label> labels) throws EvalException {
+  @CanIgnoreReturnValue
+  private static Label validate(Label label) throws EvalException {
+    if (label.getPackageIdentifier().equals(PUBLIC_LABEL.getPackageIdentifier())
+        && PackageSpecification.fromLabel(label) == null) {
+      // In other words, if the label is in //visibility and is not //visibility:public,
+      // //visibility:private, or (for the unusual case where //visibility exists as a package)
+      // //visibility:__pkg__ or //visibility:__subpackages__
+      throw Starlark.errorf(
+          "Invalid visibility label '%s'; did you mean //visibility:public or"
+              + " //visibility:private?",
+          label);
+    }
+    return label;
+  }
+
+  /**
+   * Validates visibility labels, simplifies a list containing "//visibility:public" to
+   * ["//visibility:public"], drops "//visibility:private" if it occurs with other labels, and
+   * canonicalizes an empty list to ["//visibility:private"].
+   *
+   * @param labels list of visibility labels; not modified even if mutable.
+   * @return either {@code labels} unmodified if it does not require simplification, or a new
+   *     simplified list of visibility labels.
+   */
+  // TODO(arostovtsev): we ought to uniquify the labels, matching the behavior of {@link
+  // #concatWithElement}; note that this would be an incompatible change (affects query output).
+  static List<Label> validateAndSimplify(List<Label> labels) throws EvalException {
+    boolean hasPublicLabel = false;
+    int numPrivateLabels = 0;
     for (Label label : labels) {
-      if (label.equals(PUBLIC_LABEL) || label.equals(PRIVATE_LABEL)) {
-        if (labels.size() > 1) {
-          throw Starlark.errorf(
-              "//visibility:public and //visibility:private cannot be used in combination with"
-                  + " other labels");
-        }
-      } else if (label.getPackageIdentifier().equals(PUBLIC_LABEL.getPackageIdentifier())
-          && PackageSpecification.fromLabel(label) == null) {
-        // In other words, if the label is in //visibility and is not //visibility:public,
-        // //visibility:private, or (for the unusual case where //visibility
-        // exists as a package) //visibility:__pkg__ or //visibility:__subpackages__
-        throw Starlark.errorf(
-            "Invalid visibility label '%s'; did you mean //visibility:public or"
-                + " //visibility:private?",
-            label);
+      if (label.equals(PUBLIC_LABEL)) {
+        // Do not short-circuit here; we want to validate all the labels.
+        hasPublicLabel = true;
+      } else if (label.equals(PRIVATE_LABEL)) {
+        numPrivateLabels++;
+      } else {
+        validate(label);
       }
     }
+    if (hasPublicLabel) {
+      return PUBLIC.getDeclaredLabels();
+    }
+    if (numPrivateLabels == labels.size()) {
+      return PRIVATE.getDeclaredLabels();
+    }
+    if (numPrivateLabels == 0) {
+      return labels;
+    }
+    return labels.stream()
+        .filter(label -> !label.equals(PRIVATE_LABEL))
+        .collect(ImmutableList.toImmutableList());
   }
 
   /**
@@ -184,8 +219,8 @@ static RuleVisibility concatWithElement(RuleVisibility visibility, Label element
       } else {
         ImmutableList.Builder<Label> newItems = new ImmutableList.Builder<>();
         newItems.addAll(items);
-        newItems.add(element);
-        return parse(newItems.build());
+        newItems.add(validate(element));
+        return parseUnchecked(newItems.build());
       }
     }
   }
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleTest.java
index ac255337541df8..a978fa6ad69770 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleTest.java
@@ -198,35 +198,53 @@ public void testVisibilityPkgSubpackages_whenVisibilityPackageExists_succeeds()
   }
 
   @Test
-  public void testVisibilityInvalidCombination() throws Exception {
+  public void testVisibilityMisspelling() throws Exception {
     scratch.file(
         "x/BUILD",
         """
         cc_binary(
             name = "is_this_public",
-            visibility = ["//some:__pkg__", "//visibility:public"],
+            visibility = ["//visibility:plubic"],
         )
         """);
+    reporter.removeHandler(failFastHandler);
+    Package pkg = getTarget("//x:BUILD").getPackage();
+    assertContainsEvent(
+        "Invalid visibility label '//visibility:plubic'; did you mean //visibility:public or"
+            + " //visibility:private?");
+    assertThat(pkg.containsErrors()).isTrue();
+  }
+
+  @Test
+  public void testPublicAndPrivateVisibility() throws Exception {
     scratch.file(
-        "y/BUILD",
+        "x/BUILD",
         """
+        package(default_visibility = ["//default:__pkg__"])
+
+        cc_binary(
+            name = "is_this_public",
+            visibility = ["//some:__pkg__", "//visibility:public"],
+        )
+
+        cc_binary(
+            name = "is_private_dropped",
+            visibility = ["//some:__pkg__", "//visibility:private"],
+        )
+
         cc_binary(
-            name = "is_this_private",
-            visibility = ["//other:__subpackages__", "//visibility:private"],
+            name = "is_empty_visibility_private",
+            visibility = [],
         )
         """);
-    reporter.removeHandler(failFastHandler);
-    Package pkgX = getTarget("//x:BUILD").getPackage();
-    assertContainsEvent(
-        "//visibility:public and //visibility:private cannot be used in combination with other"
-            + " labels");
-    assertThat(pkgX.containsErrors()).isTrue();
-    eventCollector.clear();
-    Package pkgY = getTarget("//y:BUILD").getPackage();
-    assertContainsEvent(
-        "//visibility:public and //visibility:private cannot be used in combination with other"
-            + " labels");
-    assertThat(pkgY.containsErrors()).isTrue();
+    Package pkg = getTarget("//x:BUILD").getPackage();
+    assertThat(pkg.containsErrors()).isFalse();
+    assertThat(pkg.getRule("is_this_public").getVisibility().getDeclaredLabels())
+        .containsExactly(Label.parseCanonicalUnchecked("//visibility:public"));
+    assertThat(pkg.getRule("is_private_dropped").getVisibility().getDeclaredLabels())
+        .containsExactly(Label.parseCanonicalUnchecked("//some:__pkg__"));
+    assertThat(pkg.getRule("is_empty_visibility_private").getVisibility().getDeclaredLabels())
+        .containsExactly(Label.parseCanonicalUnchecked("//visibility:private"));
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java
index af154dd4dd3513..b197f8e3104cc9 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java
@@ -16,9 +16,11 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.packages.RuleVisibility.concatWithElement;
+import static org.junit.Assert.assertThrows;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
+import net.starlark.java.eval.EvalException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -55,11 +57,109 @@ private static void assertEqual(RuleVisibility vis1, RuleVisibility vis2) {
   private static final RuleVisibility PRIVATE_VIS = RuleVisibility.PRIVATE;
 
   @Test
-  public void concatenation() throws Exception {
-    // Technically the empty visibility is a distinct object from private visibility, even though
-    // it has the same semantics.
-    RuleVisibility emptyVis = ruleVisibility();
+  public void validateAndSimplify_validates() throws Exception {
+    EvalException e1 =
+        assertThrows(
+            EvalException.class,
+            () ->
+                RuleVisibility.validateAndSimplify(
+                    ImmutableList.of(label("//visibility:pirvate"))));
+    assertThat(e1)
+        .hasMessageThat()
+        .contains(
+            "Invalid visibility label '//visibility:pirvate'; did you mean //visibility:public or"
+                + " //visibility:private?");
+
+    EvalException e2 =
+        assertThrows(
+            EvalException.class,
+            () ->
+                RuleVisibility.validateAndSimplify(
+                    ImmutableList.of(label(PUBLIC), label("//visibility:pbulic"))));
+    assertThat(e2)
+        .hasMessageThat()
+        .contains(
+            "Invalid visibility label '//visibility:pbulic'; did you mean //visibility:public or"
+                + " //visibility:private?");
+  }
+
+  @Test
+  public void validateAndSimplify_simplifiesPublic() throws Exception {
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(A), label(PUBLIC))))
+        .containsExactly(label(PUBLIC));
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PUBLIC), label(B))))
+        .containsExactly(label(PUBLIC));
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PUBLIC), label(PRIVATE))))
+        .containsExactly(label(PUBLIC));
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PUBLIC), label(PUBLIC))))
+        .containsExactly(label(PUBLIC));
+  }
 
+  @Test
+  public void validateAndSimplify_simplifiesPrivate() throws Exception {
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(A), label(PRIVATE))))
+        .containsExactly(label(A));
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PRIVATE), label(B))))
+        .containsExactly(label(B));
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(PRIVATE), label(PRIVATE))))
+        .containsExactly(label(PRIVATE));
+  }
+
+  @Test
+  public void emptyListCanonicalizedToPrivate() throws Exception {
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of()))
+        .containsExactly(label(PRIVATE));
+    IllegalArgumentException e =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> RuleVisibility.parseUnchecked(ImmutableList.of()));
+    assertThat(e).hasMessageThat().contains("must not be empty");
+  }
+
+  // TODO(arostovtsev): we ought to uniquify the labels, but that would be an incompatible change
+  // (affects query output).
+  @Test
+  public void validateAndSimplify_doesNotUniquify() throws Exception {
+    assertThat(RuleVisibility.validateAndSimplify(ImmutableList.of(label(A), label(A))))
+        .containsExactly(label(A), label(A));
+  }
+
+  @Test
+  public void packageGroupsRuleVisibility_create_requiresValidatedSimplifiedNonConstantLabels()
+      throws Exception {
+    IllegalArgumentException e1 =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> PackageGroupsRuleVisibility.create(ImmutableList.of()));
+    assertThat(e1).hasMessageThat().contains("must not be empty");
+    IllegalArgumentException e2 =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> PackageGroupsRuleVisibility.create(ImmutableList.of(label(PUBLIC), label(A))));
+    assertThat(e2).hasMessageThat().contains("must be validated and simplified");
+    IllegalArgumentException e3 =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> PackageGroupsRuleVisibility.create(ImmutableList.of(label(A), label(PRIVATE))));
+    assertThat(e3).hasMessageThat().contains("must be validated and simplified");
+    IllegalArgumentException e4 =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> PackageGroupsRuleVisibility.create(ImmutableList.of(label(PUBLIC))));
+    assertThat(e4)
+        .hasMessageThat()
+        .contains("must not equal [\"//visibility:public\"] or [\"//visibility:private\"]");
+    IllegalArgumentException e5 =
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> PackageGroupsRuleVisibility.create(ImmutableList.of(label(PRIVATE))));
+    assertThat(e5)
+        .hasMessageThat()
+        .contains("must not equal [\"//visibility:public\"] or [\"//visibility:private\"]");
+  }
+
+  @Test
+  public void concatenation() throws Exception {
     assertEqual(concatWithElement(ruleVisibility(A, B), label(C)), ruleVisibility(A, B, C));
     assertEqual(concatWithElement(ruleVisibility(A, B), label(PUBLIC)), PUBLIC_VIS);
     assertEqual(concatWithElement(ruleVisibility(A, B), label(PRIVATE)), ruleVisibility(A, B));
@@ -72,10 +172,6 @@ public void concatenation() throws Exception {
     assertEqual(concatWithElement(PRIVATE_VIS, label(PUBLIC)), PUBLIC_VIS);
     assertEqual(concatWithElement(PRIVATE_VIS, label(PRIVATE)), PRIVATE_VIS);
 
-    assertEqual(concatWithElement(emptyVis, label(C)), ruleVisibility(C));
-    assertEqual(concatWithElement(emptyVis, label(PUBLIC)), PUBLIC_VIS);
-    assertEqual(concatWithElement(emptyVis, label(PRIVATE)), emptyVis);
-
     // Duplicates are not added, though they are preserved.
     assertEqual(concatWithElement(ruleVisibility(A, B), label(A)), ruleVisibility(A, B));
     assertEqual(
diff --git a/src/test/shell/bazel/bazel_rules_test.sh b/src/test/shell/bazel/bazel_rules_test.sh
index e350d05adfc4c6..c9d2ca8ed64674 100755
--- a/src/test/shell/bazel/bazel_rules_test.sh
+++ b/src/test/shell/bazel/bazel_rules_test.sh
@@ -506,22 +506,6 @@ EOF
   bazel build //a:a || fail "build failed"
 }
 
-function test_visibility() {
-  mkdir visibility
-  cat > visibility/BUILD <<EOF
-cc_library(
-  name = "foo",
-  visibility = [
-    "//foo/bar:__pkg__",
-    "//visibility:public",
-  ],
-)
-EOF
-
-  bazel build //visibility:foo &> $TEST_log && fail "Expected failure" || true
-  expect_log "//visibility:public and //visibility:private cannot be used in combination with other labels"
-}
-
 function test_executable_without_default_files() {
   mkdir pkg
   cat >pkg/BUILD <<'EOF'