Add visibility attribute to all symbolic macros

This has a blast radius extending to all symbolic macros (i.e. all our symbolic macro unit tests).

Also move the addition of the builtin macro attributes (just `name` and `visibility`) to `MacroClass.Builder`, and make `MacroClass`'s constructor private for good measure.

Fixed a few test cases that incorrectly defined rules with implementation function's whose signatures looked like macros, but which didn't affect the test behavior.

Work toward bazelbuild#19922.

PiperOrigin-RevId: 679954938
Change-Id: I958211c92fa2f9749cd6a7672af3936961a95285

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -373,7 +373,7 @@ public StarlarkCallable macro(
     }
 
     MacroClass.Builder builder = new MacroClass.Builder(implementation);
-    builder.addAttribute(RuleClass.NAME_ATTRIBUTE);
+    // "name" and "visibility" attributes are added automatically by the builder.
     for (Map.Entry<String, Descriptor> descriptorEntry :
         Dict.cast(attrs, String.class, Descriptor.class, "attrs").entrySet()) {
       String attrName = descriptorEntry.getKey();

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

@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.packages;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.devtools.build.lib.packages.BuildType.NODEP_LABEL_LIST;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -59,14 +60,25 @@ public final class MacroClass {
   public static final ImmutableSet<String> RESERVED_MACRO_ATTR_NAMES =
       ImmutableSet.of("name", "visibility");
 
+  /**
+   * "visibility" attribute present on all symbolic macros.
+   *
+   * <p>This is similar to the visibility attribute for rules, but lacks the exec transitions.
+   */
+  public static final Attribute VISIBILITY_ATTRIBUTE =
+      Attribute.attr("visibility", NODEP_LABEL_LIST)
+          .orderIndependent()
+          .nonconfigurable("special attribute integrated more deeply into Bazel's core logic")
+          .build();
+
   private final String name;
   private final Label definingBzlLabel;
   private final StarlarkFunction implementation;
   // Implicit attributes are stored under their given name ("_foo"), not a mangled name ("$foo").
   private final ImmutableMap<String, Attribute> attributes;
   private final boolean isFinalizer;
 
-  public MacroClass(
+  private MacroClass(
       String name,
       Label definingBzlLabel,
       StarlarkFunction implementation,
@@ -116,6 +128,9 @@ public static final class Builder {
 
     public Builder(StarlarkFunction implementation) {
       this.implementation = implementation;
+
+      addAttribute(RuleClass.NAME_ATTRIBUTE);
+      addAttribute(VISIBILITY_ATTRIBUTE);
     }
 
     @CanIgnoreReturnValue

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

@@ -27,6 +27,9 @@
 /**
  * Tests for the how the visibility system works with respect to symbolic macros, i.e. the
  * Macro-Aware Visibility design.
+ *
+ * <p>This does *not* include tests of how the {@code visibility} attribute's value gets determined
+ * and threaded through macros.
  */
 @RunWith(TestParameterInjector.class)
 public final class MacroVisibilityTest extends BuildViewTestCase {
@@ -180,7 +183,7 @@ public void buildFileAccessToMacroTargetsIsControlled() throws Exception {
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(
                 name = name + "_exported",
                 visibility = ["//pkg:__pkg__"],
@@ -226,7 +229,7 @@ public void macroAccessToBuildFileTargetsIsControlled() throws Exception {
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(name = name + "_consumes_exported_ruletarget", dep = "//pkg:exported")
             simple_rule(name = name + "_consumes_exported_output", dep = "//pkg:exported.bin")
             simple_rule(name = name + "_consumes_exported_input", dep = "//pkg:exported_input")
@@ -272,7 +275,7 @@ public void macroAccessToOtherMacrosInSameBuildFileIsControlled() throws Excepti
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(
                 name = name + "_internal",
             )
@@ -293,7 +296,7 @@ def _impl(name):
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(
                 name = name + "_consumes_internal",
                 dep = "//pkg:foo_internal",
@@ -334,7 +337,7 @@ public void siblingsInSameMacroCanSeeEachOther() throws Exception {
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(
                 name = name + "_ruletarget",
             )
@@ -382,7 +385,7 @@ public void buildFileTargetIsVisibleToTargetsDefinedByMacroFromSamePackage() thr
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(name = name + "_macro_target", dep = "//pkg:build_target")
 
         my_macro = macro(implementation=_impl)
@@ -436,7 +439,7 @@ def helper(name):
         "B/impl.bzl",
         """
         load("//A:helper.bzl", "helper")
-        def impl(name):
+        def impl(name, visibility):
             helper(name)
         """);
     scratch.file("C/BUILD");
@@ -504,7 +507,7 @@ public void locationOfTargetDeclarationIsInnermostMacro() throws Exception {
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name):
+        def _impl(name, visibility):
             simple_rule(
                 name = name + "_wants_vis_to_inner",
                 dep = "//common:vis_to_inner",
@@ -523,7 +526,7 @@ def _impl(name):
         load("//rules:simple_rule.bzl", "simple_rule")
         load("//inner:macro.bzl", "inner_macro")
 
-        def _impl(name):
+        def _impl(name, visibility):
             inner_macro(name = name + "_inner")
             simple_rule(
                 name = name + "_wants_vis_to_inner",
@@ -566,7 +569,7 @@ public void buildFileCanDelegateVisibilityPrivilegesToMacro(boolean depIsConfigu
             """
             load("//rules:simple_rule.bzl", "simple_rule")
 
-            def _impl(name, dep):
+            def _impl(name, visibility, dep):
                 simple_rule(
                     name = name,
                     dep = dep,
@@ -622,7 +625,7 @@ private void defineWrappingMacro(String name, String wraps, String attrExpr) thr
             """
             load("%2$s", "%3$s")
 
-            def _impl(name, dep):
+            def _impl(name, visibility, dep):
                 %3$s(
                     name = name,
                     %4$s,
@@ -772,7 +775,7 @@ public void noDelegationIfCallerDoesNotPassInTarget() throws Exception {
         """
         load("//rules:simple_rule.bzl", "simple_rule")
 
-        def _impl(name, _v2P_implicitdep, _v2M_implicitdep):
+        def _impl(name, visibility, _v2P_implicitdep, _v2M_implicitdep):
             simple_rule(
                 name = name + "_consumes_v2P_hardcoded",
                 dep = "//common:v2P_hardcoded",