Munge visibility attribute for symbolic macros

This implements the behavior that the `visibility` attribute of symbolic macros is whatever the user specifies concatenated with the callsite's location. It also ensures that the package's default visibility is used for top-level symbolic macros (and only top-level ones).

The bulk of the work is done in the attribute value processing logic of `MacroClass#instantiateMacro`. The new code is strategically placed before most of the other attribute processing to avoid violating invariants (like "by this point, all attr values have had `copyAndLift...()` applied to them).

Note that, unlike the `visibility` attribute of rules, the `visibility` attribute of macros always materializes the true visibility, even for macros that are declared at the top level. I.e., if you already have a macro's computed visibility value, you don't need to also have the package's default visibility value.

`MacroInstance` gains a `getVisibility()` accessor, the value of which it validates in its constructor. This means the constructor can now throw EvalException.

In `SymbolicMacroTest`, delete unused scratch files and do minor formatting fixes for symmetry with .bzl style expectations.

Work toward bazelbuild#19922.

PiperOrigin-RevId: 680661002
Change-Id: Ibaba32f458f69d3b7ad14db0afc68aaf9468e3a7

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

@@ -23,13 +23,15 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.packages.TargetRecorder.MacroFrame;
 import com.google.devtools.build.lib.packages.TargetRecorder.NameConflictException;
 import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.ArrayList;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
@@ -227,6 +229,41 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
       attrValues.put(attrName, value);
     }
 
+    // Special processing of the "visibility" attribute.
+    @Nullable MacroFrame parentMacroFrame = pkgBuilder.getCurrentMacroFrame();
+    @Nullable Object rawVisibility = attrValues.get("visibility");
+    RuleVisibility parsedVisibility;
+    if (rawVisibility == null) {
+      // Visibility wasn't explicitly supplied. If we're not in another symbolic macro, use the
+      // package's default visibility, otherwise use private visibility.
+      if (parentMacroFrame == null) {
+        parsedVisibility = pkgBuilder.getPartialPackageArgs().defaultVisibility();
+      } else {
+        parsedVisibility = RuleVisibility.PRIVATE;
+      }
+    } else {
+      @SuppressWarnings("unchecked")
+      List<Label> liftedVisibility =
+          (List<Label>)
+              BuildType.copyAndLiftStarlarkValue(
+                  name, VISIBILITY_ATTRIBUTE, rawVisibility, pkgBuilder.getLabelConverter());
+      parsedVisibility = RuleVisibility.parse(liftedVisibility);
+    }
+    // Concatenate the visibility (as previously populated) with the instantiation site's location.
+    PackageIdentifier instantiatingLoc;
+    if (parentMacroFrame == null) {
+      instantiatingLoc = pkgBuilder.getPackageIdentifier();
+    } else {
+      instantiatingLoc =
+          parentMacroFrame
+              .macroInstance
+              .getMacroClass()
+              .getDefiningBzlLabel()
+              .getPackageIdentifier();
+    }
+    parsedVisibility = RuleVisibility.concatWithPackage(parsedVisibility, instantiatingLoc);
+    attrValues.put("visibility", parsedVisibility.getDeclaredLabels());
+
     // Populate defaults for the rest, and validate that no mandatory attr was missed.
     for (Attribute attr : attributes.values()) {
       if (attrValues.containsKey(attr.getName())) {
@@ -272,7 +309,6 @@ private MacroInstance instantiateMacro(Package.Builder pkgBuilder, Map<String, O
     String name = (String) Preconditions.checkNotNull(attrValues.get("name"));
     // Determine the id for this macro. If we're in another macro by the same name, increment the
     // number, otherwise use 1 for the number.
-    @Nullable MacroFrame parentMacroFrame = pkgBuilder.getCurrentMacroFrame();
     int sameNameDepth =
         parentMacroFrame == null || !name.equals(parentMacroFrame.macroInstance.getName())
             ? 1

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

@@ -16,14 +16,17 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.BuildType.SelectorList;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
+import java.util.List;
 import java.util.Map;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
+import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
 
 /**
@@ -58,16 +61,28 @@ public final class MacroInstance {
   /**
    * Instantiates the given macro class with the given attribute values.
    *
+   * <p>{@code attrValues} must have already been normalized based on the types of the attributes;
+   * see {@link MacroClass#instantiateMacro}. Values for the {@code "name"} and {@code "visibility"}
+   * attributes must exist with the correct types, and the {@code "visibility"} value must satisfy
+   * {@link RuleVisibility#validate}.
+   *
    * <p>{@code sameNameDepth} is the number of macro instances that this one is inside of that share
    * its name. For most instances it is 1, but for the main submacro of a parent macro it is one
    * more than the parent's depth.
+   *
+   * @throws EvalException if there's a problem with the attribute values (currently, only thrown if
+   *     the {@code visibility} value is invalid)
    */
+  // TODO: #19922 - Better encapsulate the invariant around attrValues, by either transitioning to
+  // storing internal-typed values (the way Rules do) instead of Starlark-typed values, or else just
+  // making the constructor private and moving instantiateMacro() to this class.
   public MacroInstance(
       Package pkg,
       @Nullable MacroInstance parent,
       MacroClass macroClass,
       Map<String, Object> attrValues,
-      int sameNameDepth) {
+      int sameNameDepth)
+      throws EvalException {
     this.pkg = pkg;
     this.parent = parent;
     this.macroClass = macroClass;
@@ -139,6 +154,22 @@ public String getName() {
     return (String) Preconditions.checkNotNull(attrValues.get("name"));
   }
 
+  /**
+   * Returns the visibility of this macro instance.
+   *
+   * <p>This value will be observed as the {@code visibility} parameter of the implementation
+   * function. It is not necessarily the same as the {@code visibility} value passed in when
+   * instantiating the macro, since the latter needs processing to add the call site's location and
+   * possibly apply the package's default visibility.
+   *
+   * <p>It can be assumed that the returned list satisfies {@link RuleVisibility#validate}.
+   */
+  public ImmutableList<Label> getVisibility() {
+    @SuppressWarnings("unchecked")
+    List<Label> visibility = (List<Label>) Preconditions.checkNotNull(attrValues.get("visibility"));
+    return ImmutableList.copyOf(visibility);
+  }
+
   /**
    * Dictionary of attributes for this instance.
    *
@@ -160,6 +191,8 @@ public ImmutableMap<String, Object> getAttrValues() {
    * given value for its {@code visibility} attribute, and if the target were declared directly in
    * this macro (i.e. not in a submacro).
    */
+  // TODO: #19922 - Maybe refactor this to getDefinitionLocation and let the caller do the concat,
+  // so we don't have to basically repeat it in MacroClass#instantiateMacro.
   public RuleVisibility concatDefinitionLocationToVisibility(RuleVisibility visibility) {
     PackageIdentifier macroLocation = macroClass.getDefiningBzlLabel().getPackageIdentifier();
     return RuleVisibility.concatWithPackage(visibility, macroLocation);

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

@@ -1684,7 +1684,8 @@ Rule createRule(
      * Package} associated with this {@link Builder}.
      */
     MacroInstance createMacro(
-        MacroClass macroClass, Map<String, Object> attrValues, int sameNameDepth) {
+        MacroClass macroClass, Map<String, Object> attrValues, int sameNameDepth)
+        throws EvalException {
       MacroInstance parent = currentMacro();
       return new MacroInstance(pkg, parent, macroClass, attrValues, sameNameDepth);
     }

src/test/java/com/google/devtools/build/lib/analysis/BUILD

@@ -377,6 +377,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//third_party:error_prone_annotations",
+        "//third_party:guava",
         "//third_party:jsr305",
         "//third_party:junit4",
         "//third_party:truth",

src/test/java/com/google/devtools/build/lib/analysis/MacroVisibilityTest.java

@@ -29,7 +29,7 @@
  * Macro-Aware Visibility design.
  *
  * <p>This does *not* include tests of how the {@code visibility} attribute's value gets determined
- * and threaded through macros.
+ * and threaded through macros. See SymbolicMacroTest.java for that.
  */
 @RunWith(TestParameterInjector.class)
 public final class MacroVisibilityTest extends BuildViewTestCase {
@@ -821,6 +821,14 @@ def _impl(name, visibility, _v2P_implicitdep, _v2M_implicitdep):
     assertVisibilityPermits("//pkg:foo_consumes_v2M_hardcoded", "//common:v2M_hardcoded");
     // Having an implicit dep doesn't count as the caller passing it in.
     assertVisibilityPermits("//pkg:foo_consumes_v2M_implicitdep", "//common:v2M_implicitdep");
+
+    // Theoretically another behavior we could try to demonstrate might be that having a label in
+    // the "visibility" attribute does not qualify as the caller passing it in. However, the only
+    // non-synthetic labels that can appear in visibility are package_groups or rules that imitate
+    // them by providing PackageSpecificationProvider. package_groups always have public visibility,
+    // so we can't manufacture a situation demonstrating that delegation has failed unless we make
+    // an alternate rule class for that purpose, which doens't seem worth it. See also
+    // SymbolicMacroTest#labelVisitation which covers this case at a lower level.
   }
 
   @Test
@@ -983,11 +991,6 @@ public void packageDefaultVisibilityDoesNotPropagateToInsideMacro() throws Excep
   /*
    * TODO: #19922 - Tests cases to add:
    *
-   * ---- Propagating target usages from parent macro to child ----
-   *
-   * - When checking a parent macro to see if the label occurs, only normal dep attributes are
-   *   considered, not nodep labels like visibility (that's a funny test case).
-   *
    * ---- Implicit deps ----
    *
    * - If a rule's implicit dep is defined in a macro, the check of the rule's def loc against the
@@ -1003,22 +1006,10 @@ public void packageDefaultVisibilityDoesNotPropagateToInsideMacro() throws Excep
    *
    * ---- Visibility attr usage ----
    *
-   * - Visibility attr is passed and contains the call site's package.
-   *
-   * - Exporting via visibility = visibility works, including transitively.
-   *
-   * - Passing visibility to a macro does not force that visibility upon the macro's internal
-   *   targets that don't declare a visibility.
-   *
    * - Can compose public and private visibilities with other visibilities via concatenation.
    *   Visibility attr is normalized. (Unclear whether to apply normalization to targets defined
    *   outside symbolic macros.)
    *
-   * ---- default_visibility ----
-   *
-   * - default_visibility affects the visibility of a top-level macro that does not set
-   *   visibility=..., and does not affect a top-level macro that does set visibility=...
-   *
    * ---- Accounting for CommonPrerequisiteValidator#isSameLogicalPackage() ----
    *
    * - Targets of a macro defined in //javatests/foo can see targets defined in //java/foo, even