Move macro visibility appending out of Package.Builder

`Package.Builder` has a utility method `copyAppendingCurrentMacroLocation`, that doesn't really need to be there. Ahead of further refactoring of this class, I figured I'd spin off this method. (The purpose of the method is to determine the munged visibility values for targets declared within symbolic macros.)

The method is re-expressed as two helpers in `RuleVisibility` and one in `MacroInstance`, each of which does a narrower task more suited for its new home.

Made the convenience method `Builder#currentMacro` public, since it's much terser than `getCurrentMacroFrame().macroInstance`. Added a drive-by TODO in `StarlarkNativeModule`.

Work toward #19922.

PiperOrigin-RevId: 678786589
Change-Id: I5cc190bd6ef908a3e67094eacd13e9c91ef43d7c

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

@@ -149,6 +149,22 @@ public ImmutableMap<String, Object> getAttrValues() {
     return attrValues;
   }
 
+  /**
+   * Returns a {@code RuleVisibility} representing the result of concatenating this macro's {@link
+   * MacroClass}'s definition location to the given {@code visibility}.
+   *
+   * <p>The definition location of a macro class is the package containing the .bzl file from which
+   * the macro class was exported.
+   *
+   * <p>Logically, this represents the visibility that a target would have, if it were passed the
+   * given value for its {@code visibility} attribute, and if the target were declared directly in
+   * this macro (i.e. not in a submacro).
+   */
+  public RuleVisibility concatDefinitionLocationToVisibility(RuleVisibility visibility) {
+    PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier();
+    return RuleVisibility.concatWithPackage(visibility, macroLocation);
+  }
+
   /**
    * Visits all labels appearing in non-implicit attributes of {@link Type.LabelClass#DEPENDENCY}
    * label type, i.e. ignoring nodep labels.

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

@@ -1635,7 +1635,7 @@ public Globber getGlobber() {
      * Returns the innermost currently executing symbolic macro, or null if not in a symbolic macro.
      */
     @Nullable
-    private MacroInstance currentMacro() {
+    public MacroInstance currentMacro() {
       MacroFrame frame = regEnv.getCurrentMacroFrame();
       return frame == null ? null : frame.macroInstance;
     }
@@ -1961,36 +1961,6 @@ public void markMacroComplete(MacroInstance macro) {
       }
     }
 
-    /**
-     * If we are currently executing a symbolic macro, returns the result of unioning the given
-     * visibility with the location of the innermost macro's code. Otherwise, returns the given
-     * visibility unmodified.
-     *
-     * <p>The location of the macro's code is considered to be the package containing the .bzl file
-     * from which the macro's {@code MacroClass} was exported.
-     */
-    RuleVisibility copyAppendingCurrentMacroLocation(RuleVisibility visibility) {
-      if (currentMacro() == null) {
-        return visibility;
-      }
-      MacroClass macroClass = currentMacro().getMacroClass();
-      PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier();
-      Label newVisibilityItem = Label.createUnvalidated(macroLocation, "__pkg__");
-
-      if (visibility.equals(RuleVisibility.PRIVATE)) {
-        // Private is dropped.
-        return PackageGroupsRuleVisibility.create(ImmutableList.of(newVisibilityItem));
-      } else if (visibility.equals(RuleVisibility.PUBLIC)) {
-        // Public is idempotent.
-        return visibility;
-      } else {
-        ImmutableList.Builder<Label> items = new ImmutableList.Builder<>();
-        items.addAll(visibility.getDeclaredLabels());
-        items.add(newVisibilityItem);
-        return PackageGroupsRuleVisibility.create(items.build());
-      }
-    }
-
     void addRegisteredExecutionPlatforms(List<TargetPattern> platforms) {
       this.registeredExecutionPlatforms.addAll(platforms);
     }

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

@@ -289,7 +289,8 @@ private static String getGeneratorName(
   @Nullable
   private static List<Label> getModifiedVisibility(
       Package.Builder pkgBuilder, BuildLangTypedAttributeValuesMap args) {
-    if (pkgBuilder.getCurrentMacroFrame() == null) {
+    MacroInstance currentMacro = pkgBuilder.currentMacro();
+    if (currentMacro == null) {
       return null;
     }
 
@@ -311,7 +312,7 @@ private static List<Label> getModifiedVisibility(
       }
     }
 
-    return pkgBuilder.copyAppendingCurrentMacroLocation(visibility).getDeclaredLabels();
+    return currentMacro.concatDefinitionLocationToVisibility(visibility).getDeclaredLabels();
   }
 
   /**

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

@@ -15,6 +15,7 @@
 
 import com.google.common.collect.ImmutableList;
 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 java.util.List;
 import javax.annotation.Nullable;
@@ -151,5 +152,50 @@ static void validate(List<Label> labels) throws EvalException {
       }
     }
   }
-}
 
+  /**
+   * Returns a {@code RuleVisibility} representing the logical result of concatenating the given
+   * {@code visibility} with the additional {@code element}.
+   *
+   * <p>If {@code element} or {@code visibility} is public, the result is public. If {@code element}
+   * or {@code visibility} is private, the result is {@code visibility} or a visibility consisting
+   * solely of {@code element}, respectively.
+   *
+   * @throws EvalException if there's a problem parsing {@code element} into a visibility
+   */
+  static RuleVisibility concatWithElement(RuleVisibility visibility, Label element)
+      throws EvalException {
+    if (visibility.equals(RuleVisibility.PRIVATE)) {
+      // Left-side private is dropped.
+      return parse(ImmutableList.of(element));
+    } else if (element.equals(PRIVATE_LABEL)) {
+      // Right-side private is dropped.
+      return visibility;
+    } else if (visibility.equals(RuleVisibility.PUBLIC) || element.equals(PUBLIC_LABEL)) {
+      // 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());
+    }
+  }
+
+  /**
+   * Convenience wrapper for {@link #concatWithElement} where the added element is the given
+   * package.
+   *
+   * <p>Unlike that method, this does not throw EvalException.
+   */
+  static RuleVisibility concatWithPackage(
+      RuleVisibility visibility, PackageIdentifier packageIdentifier) {
+    Label pkgItem = Label.createUnvalidated(packageIdentifier, "__pkg__");
+    try {
+      return concatWithElement(visibility, pkgItem);
+    } catch (EvalException ex) {
+      throw new AssertionError(
+          String.format("Unreachable; couldn't parse %s as visibility", pkgItem), ex);
+    }
+  }
+}

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

@@ -553,7 +553,10 @@ public NoneType exportsFiles(
             : RuleVisibility.parse(
                 BuildType.LABEL_LIST.convert(
                     visibilityO, "'exports_files' operand", pkgBuilder.getLabelConverter()));
-    visibility = pkgBuilder.copyAppendingCurrentMacroLocation(visibility);
+    MacroInstance currentMacro = pkgBuilder.currentMacro();
+    if (currentMacro != null) {
+      visibility = currentMacro.concatDefinitionLocationToVisibility(visibility);
+    }
 
     // TODO(bazel-team): is licenses plural or singular?
     License license = BuildType.LICENSE.convertOptional(licensesO, "'exports_files' operand");

src/test/java/com/google/devtools/build/lib/packages/RuleVisibilityTest.java

@@ -0,0 +1,86 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.packages;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.Label;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link RuleVisibility}. */
+@RunWith(JUnit4.class)
+public final class RuleVisibilityTest {
+
+  private static Label label(String labelString) {
+    return Label.parseCanonicalUnchecked(labelString);
+  }
+
+  private static RuleVisibility ruleVisibility(String... labelStrings) {
+    ImmutableList.Builder<Label> labels = ImmutableList.builder();
+    for (String labelString : labelStrings) {
+      labels.add(label(labelString));
+    }
+    return RuleVisibility.parseUnchecked(labels.build());
+  }
+
+  // Needed because RuleVisibility has no equals() override.
+  private static void assertEqual(RuleVisibility vis1, RuleVisibility vis2) {
+    assertThat(vis1.getDependencyLabels()).isEqualTo(vis2.getDependencyLabels());
+    assertThat(vis1.getDeclaredLabels()).isEqualTo(vis2.getDeclaredLabels());
+  }
+
+  @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(RuleVisibility.concatWithElement(publicVis, label("//c:__pkg__")), publicVis);
+    assertEqual(
+        RuleVisibility.concatWithElement(publicVis, label("//visibility:public")), publicVis);
+    assertEqual(
+        RuleVisibility.concatWithElement(publicVis, label("//visibility:private")), publicVis);
+
+    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(
+        RuleVisibility.concatWithElement(emptyVis, label("//c:__pkg__")),
+        ruleVisibility("//c:__pkg__"));
+    assertEqual(
+        RuleVisibility.concatWithElement(emptyVis, label("//visibility:public")), publicVis);
+    assertEqual(
+        RuleVisibility.concatWithElement(emptyVis, label("//visibility:private")), emptyVis);
+  }
+}