From 6f705ce66fb8a37324542be4eebacd0a0f511e12 Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Sat, 28 Sep 2024 07:08:18 -0700
Subject: [PATCH] RuleVisibility concatenation doesn't add redundant items

This is useful if you have several nested symbolic macros, since it avoids generating a long visibility value like `["//A:__pkg__", "//A:__pkg__", ..., "//A:__pkg__"]` when each macro's body is defined in the same package `A`.

Also clarify the purpose of the label accessors on `RuleVisibility` and flip their declaration order.

Work toward #19922.

PiperOrigin-RevId: 679962558
Change-Id: Iebb55d0256d72d7bc0a8b575446dea83c9bbf632
---
 .../build/lib/packages/RuleVisibility.java    | 52 ++++++++++--------
 .../lib/packages/PackageFactoryTest.java      |  7 +--
 .../lib/packages/RuleVisibilityTest.java      | 54 +++++++++----------
 3 files changed, 61 insertions(+), 52 deletions(-)

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 3b93a041560b38..1fb555c1e5e657 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
@@ -34,20 +34,20 @@
 public interface RuleVisibility {
 
   /**
-   * Returns the list of labels that need to be loaded so that the visibility decision can be made
-   * during analysis time. E.g. for package group visibility, this is the list of package groups
-   * referenced. Does not include labels that have special meanings in the visibility declaration,
-   * e.g. "//visibility:*" or "//*:__pkg__".
+   * Returns the list of all labels comprising this visibility.
+   *
+   * <p>This includes labels that are not loadable, such as //visibility:public and //foo:__pkg__.
    */
-  List<Label> getDependencyLabels();
+  List<Label> getDeclaredLabels();
 
   /**
-   * Returns the list of labels used during the declaration of this visibility. These do not
-   * necessarily represent loadable labels: for example, for public or private visibilities, the
-   * special labels "//visibility:*" will be returned, and so will be the special "//*:__pkg__"
-   * labels indicating a single package.
+   * Same as {@link #getDeclaredLabels}, but excludes labels that cannot be loaded.
+   *
+   * <p>I.e., this returns the labels of the top-level {@code package_group}s that must be loaded in
+   * order to determine the complete set of packages represented by this visibility. (Additional
+   * {@code package_group}s may need to be loaded due to their {@code includes} attribute.)
    */
-  List<Label> getDeclaredLabels();
+  List<Label> getDependencyLabels();
 
   @SerializationConstant Label PUBLIC_LABEL = Label.parseCanonicalUnchecked("//visibility:public");
 
@@ -58,13 +58,13 @@ public interface RuleVisibility {
   RuleVisibility PUBLIC =
       new RuleVisibility() {
         @Override
-        public ImmutableList<Label> getDependencyLabels() {
-          return ImmutableList.of();
+        public ImmutableList<Label> getDeclaredLabels() {
+          return ImmutableList.of(PUBLIC_LABEL);
         }
 
         @Override
-        public ImmutableList<Label> getDeclaredLabels() {
-          return ImmutableList.of(PUBLIC_LABEL);
+        public ImmutableList<Label> getDependencyLabels() {
+          return ImmutableList.of();
         }
 
         @Override
@@ -77,13 +77,13 @@ public String toString() {
   RuleVisibility PRIVATE =
       new RuleVisibility() {
         @Override
-        public ImmutableList<Label> getDependencyLabels() {
-          return ImmutableList.of();
+        public ImmutableList<Label> getDeclaredLabels() {
+          return ImmutableList.of(PRIVATE_LABEL);
         }
 
         @Override
-        public ImmutableList<Label> getDeclaredLabels() {
-          return ImmutableList.of(PRIVATE_LABEL);
+        public ImmutableList<Label> getDependencyLabels() {
+          return ImmutableList.of();
         }
 
         @Override
@@ -161,6 +161,9 @@ static void validate(List<Label> labels) throws EvalException {
    * or {@code visibility} is private, the result is {@code visibility} or a visibility consisting
    * solely of {@code element}, respectively.
    *
+   * <p>If {@code element} is already present in {@code visibility}, the result is just {@code
+   * visibility}.
+   *
    * @throws EvalException if there's a problem parsing {@code element} into a visibility
    */
   static RuleVisibility concatWithElement(RuleVisibility visibility, Label element)
@@ -175,10 +178,15 @@ static RuleVisibility concatWithElement(RuleVisibility visibility, Label element
       // Public is idempotent.
       return RuleVisibility.PUBLIC;
     } else {
-      ImmutableList.Builder<Label> items = new ImmutableList.Builder<>();
-      items.addAll(visibility.getDeclaredLabels());
-      items.add(element);
-      return parse(items.build());
+      List<Label> items = visibility.getDeclaredLabels();
+      if (items.contains(element)) {
+        return visibility;
+      } else {
+        ImmutableList.Builder<Label> newItems = new ImmutableList.Builder<>();
+        newItems.addAll(items);
+        newItems.add(element);
+        return parse(newItems.build());
+      }
     }
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 2e428e39d4cc83..6612b1f2970044 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -1693,9 +1693,10 @@ def _impl(name, visibility):
     Package pkg = loadPackageAndAssertSuccess("pkg");
     assertVisibilityIs(pkg.getTarget("foo_public"), "//visibility:public");
     assertVisibilityIs(pkg.getTarget("foo_private"), "//lib:__pkg__");
-    // Visibility is a label_list. Label lists don't do duplicate elimination. (Nor can we eliminate
-    // all logical redundancies anyway, since visibilities may refer to redundant package groups.)
-    assertVisibilityIs(pkg.getTarget("foo_selfvisible"), "//lib:__pkg__", "//lib:__pkg__");
+    // The visibility concatenation operation does not add any label that would duplicate an
+    // existing one. (Note that we can't eliminate *all* possible redundancy, since the visibility
+    // list's semantics depend on expanding package_groups.)
+    assertVisibilityIs(pkg.getTarget("foo_selfvisible"), "//lib:__pkg__");
   }
 
   @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 49db8e0f098fb4..af154dd4dd3513 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
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.packages.RuleVisibility.concatWithElement;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -44,43 +45,42 @@ private static void assertEqual(RuleVisibility vis1, RuleVisibility vis2) {
     assertThat(vis1.getDeclaredLabels()).isEqualTo(vis2.getDeclaredLabels());
   }
 
+  private static final String A = "//a:__pkg__";
+  // Package group labels are represented differently than __pkg__ labels, so cover both cases.
+  private static final String B = "//b:pkggroup";
+  private static final String C = "//c:__pkg__";
+  private static final String PUBLIC = "//visibility:public";
+  private static final String PRIVATE = "//visibility:private";
+  private static final RuleVisibility PUBLIC_VIS = RuleVisibility.PUBLIC;
+  private static final RuleVisibility PRIVATE_VIS = RuleVisibility.PRIVATE;
+
   @Test
   public void concatenation() throws Exception {
-    RuleVisibility normalVis = ruleVisibility("//a:__pkg__", "//b:__pkg__");
-    RuleVisibility publicVis = RuleVisibility.PUBLIC;
-    RuleVisibility privateVis = RuleVisibility.PRIVATE;
     // Technically the empty visibility is a distinct object from private visibility, even though
     // it has the same semantics.
     RuleVisibility emptyVis = ruleVisibility();
 
-    assertEqual(
-        RuleVisibility.concatWithElement(normalVis, label("//c:__pkg__")),
-        ruleVisibility("//a:__pkg__", "//b:__pkg__", "//c:__pkg__"));
-    assertEqual(
-        RuleVisibility.concatWithElement(normalVis, label("//visibility:public")), publicVis);
-    assertEqual(
-        RuleVisibility.concatWithElement(normalVis, label("//visibility:private")), normalVis);
+    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));
 
-    assertEqual(RuleVisibility.concatWithElement(publicVis, label("//c:__pkg__")), publicVis);
-    assertEqual(
-        RuleVisibility.concatWithElement(publicVis, label("//visibility:public")), publicVis);
-    assertEqual(
-        RuleVisibility.concatWithElement(publicVis, label("//visibility:private")), publicVis);
+    assertEqual(concatWithElement(PUBLIC_VIS, label(C)), PUBLIC_VIS);
+    assertEqual(concatWithElement(PUBLIC_VIS, label(PUBLIC)), PUBLIC_VIS);
+    assertEqual(concatWithElement(PUBLIC_VIS, label(PRIVATE)), PUBLIC_VIS);
 
-    assertEqual(
-        RuleVisibility.concatWithElement(privateVis, label("//c:__pkg__")),
-        ruleVisibility("//c:__pkg__"));
-    assertEqual(
-        RuleVisibility.concatWithElement(privateVis, label("//visibility:public")), publicVis);
-    assertEqual(
-        RuleVisibility.concatWithElement(privateVis, label("//visibility:private")), privateVis);
+    assertEqual(concatWithElement(PRIVATE_VIS, label(C)), ruleVisibility(C));
+    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(
-        RuleVisibility.concatWithElement(emptyVis, label("//c:__pkg__")),
-        ruleVisibility("//c:__pkg__"));
-    assertEqual(
-        RuleVisibility.concatWithElement(emptyVis, label("//visibility:public")), publicVis);
+        concatWithElement(ruleVisibility(A, B, B, A), label(A)), ruleVisibility(A, B, B, A));
     assertEqual(
-        RuleVisibility.concatWithElement(emptyVis, label("//visibility:private")), emptyVis);
+        concatWithElement(ruleVisibility(A, B, B, A), label(C)), ruleVisibility(A, B, B, A, C));
   }
 }