From 888ab1f33a2c2a55ec0015a9dd5bb4a0fa6487e8 Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Mon, 30 Sep 2024 11:47:12 -0700
Subject: [PATCH] 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 #19922.

PiperOrigin-RevId: 680661002
Change-Id: Ibaba32f458f69d3b7ad14db0afc68aaf9468e3a7
---
 .../build/lib/packages/MacroClass.java        |  38 +++-
 .../build/lib/packages/MacroInstance.java     |  35 +++-
 .../devtools/build/lib/packages/Package.java  |   3 +-
 .../google/devtools/build/lib/analysis/BUILD  |   1 +
 .../lib/analysis/MacroVisibilityTest.java     |  27 +--
 .../build/lib/analysis/SymbolicMacroTest.java | 195 ++++++++++++++++--
 6 files changed, 260 insertions(+), 39 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
index 6b0415da8fe243..7bc2640e639c20 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MacroClass.java
@@ -23,6 +23,7 @@
 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;
@@ -30,6 +31,7 @@
 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
diff --git a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java b/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
index 6591c38d7747d4..417ec32b1b1ae3 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MacroInstance.java
+++ b/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);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index c312fe5b56c850..d9f2ec5c765be0 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/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);
     }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index e105c5346b1088..78acee97eacc68 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/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",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/MacroVisibilityTest.java b/src/test/java/com/google/devtools/build/lib/analysis/MacroVisibilityTest.java
index 63be8d2320faf6..240f10cf1eccbe 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/MacroVisibilityTest.java
+++ b/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
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
index 34ff471868ded2..b4c7123cc83022 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/SymbolicMacroTest.java
@@ -14,8 +14,10 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.test.AnalysisFailure;
 import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
@@ -25,10 +27,12 @@
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.Target;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
 import com.google.testing.junit.testparameterinjector.TestParameters;
 import java.util.ArrayList;
+import java.util.List;
 import javax.annotation.Nullable;
 import org.junit.Assert;
 import org.junit.Before;
@@ -65,6 +69,37 @@ private void assertPackageNotInError(@Nullable Package pkg) {
     assertThat(pkg.containsErrors()).isFalse();
   }
 
+  /** Retrieves the macro with the given id, which must exist in the package. */
+  private static MacroInstance getMacroById(Package pkg, String id) {
+    MacroInstance macro = pkg.getMacrosById().get(id);
+    assertThat(macro).isNotNull();
+    return macro;
+  }
+
+  /** Maps a list of labels to a more convenient list of strings. */
+  private static ImmutableList<String> asStringList(List<Label> labelList) {
+    return labelList.stream().map(Label::getCanonicalForm).collect(toImmutableList());
+  }
+
+  /**
+   * Retrieves the visibility labels of the target with the given name, which must exist in the
+   * package.
+   */
+  private static ImmutableList<String> getTargetVisibility(Package pkg, String name)
+      throws Exception {
+    Target target = pkg.getTarget(name);
+    assertThat(target).isNotNull();
+    return asStringList(target.getVisibility().getDeclaredLabels());
+  }
+
+  /**
+   * Retrieves the visibility labels of the macro with the given id, which must exist in the
+   * package.
+   */
+  private static ImmutableList<String> getMacroVisibility(Package pkg, String id) throws Exception {
+    return asStringList(getMacroById(pkg, id).getVisibility());
+  }
+
   /**
    * Convenience method for asserting that a package evaluates in error and produces an event
    * containing the given substring.
@@ -731,7 +766,7 @@ def _impl(name, visibility, dep_nonconfigurable, dep_configurable):
             print("dep_nonconfigurable is %s" % dep_nonconfigurable)
             print("dep_configurable is %s" % dep_configurable)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               # Test label type, since LabelType#getDefaultValue returns null.
               "dep_nonconfigurable": attr.label(configurable=False),
@@ -761,7 +796,7 @@ public void defaultAttrValue_isUsedWhenNotOverridden() throws Exception {
         def _impl(name, visibility, xyz):
             print("xyz is %s" % xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.string(default="DEFAULT", configurable=False)
             },
@@ -787,7 +822,7 @@ public void defaultAttrValue_canBeOverridden() throws Exception {
         def _impl(name, visibility, xyz):
             print("xyz is %s" % xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.string(default="DEFAULT", configurable=False)
             },
@@ -816,7 +851,7 @@ public void defaultAttrValue_isUsed_whenAttrIsImplicit() throws Exception {
         def _impl(name, visibility, _xyz):
             print("xyz is %s" % _xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "_xyz": attr.string(default="IMPLICIT", configurable=False)
             },
@@ -878,7 +913,7 @@ public void noneAttrValue_doesNotOverrideDefault() throws Exception {
         def _impl(name, visibility, xyz):
             print("xyz is %s" % xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.string(default="DEFAULT", configurable=False)
             },
@@ -965,7 +1000,7 @@ def _impl(name, visibility, xyz, _xyz):
             print("xyz is %s" % xyz)
             print("_xyz is %s" % _xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.label(configurable = False),
               "_xyz": attr.label(default=":BUILD", configurable=False)
@@ -996,7 +1031,7 @@ public void cannotMutateAttrValues() throws Exception {
         def _impl(name, visibility, xyz):
             xyz.append(4)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.int_list(configurable=False),
             },
@@ -1017,14 +1052,13 @@ def _impl(name, visibility, xyz):
 
   @Test
   public void attrsAllowSelectsByDefault() throws Exception {
-    scratch.file("lib/BUILD");
     scratch.file(
         "pkg/foo.bzl",
         """
         def _impl(name, visibility, xyz):
             print("xyz is %s" % xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.string(),
             },
@@ -1068,7 +1102,7 @@ def _impl(name, visibility, attr_using_schema_default, attr_using_hardcoded_nonn
               % attr_using_hardcoded_nonnull_default)
     print("attr_using_hardcoded_null_default is %s" % attr_using_hardcoded_null_default)
 my_macro = macro(
-    implementation=_impl,
+    implementation = _impl,
     attrs = {
       "attr_using_schema_default": attr.string(default="some_default"),
       "attr_using_hardcoded_nonnull_default": attr.string(),
@@ -1117,7 +1151,6 @@ attr_using_hardcoded_null_default is select({Label("//common:some_configsetting"
 
   @Test
   public void configurableAttrValuesArePromotedToSelects() throws Exception {
-    scratch.file("lib/BUILD");
     scratch.file(
         "pkg/foo.bzl",
         """
@@ -1128,7 +1161,7 @@ def _impl(name, visibility, configurable_xyz, nonconfigurable_xyz):
                     (str(nonconfigurable_xyz), type(nonconfigurable_xyz)))
 
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "configurable_xyz": attr.string(),
               "nonconfigurable_xyz": attr.string(configurable=False),
@@ -1155,14 +1188,13 @@ def _impl(name, visibility, configurable_xyz, nonconfigurable_xyz):
 
   @Test
   public void nonconfigurableAttrValuesProhibitSelects() throws Exception {
-    scratch.file("lib/BUILD");
     scratch.file(
         "pkg/foo.bzl",
         """
         def _impl(name, visibility, xyz):
             print("xyz is %s" % xyz)
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.string(configurable=False),
             },
@@ -1184,7 +1216,6 @@ def _impl(name, visibility, xyz):
   // TODO(b/331193690): Prevent selects from being evaluated as bools
   @Test
   public void selectableAttrCanBeEvaluatedAsBool() throws Exception {
-    scratch.file("lib/BUILD");
     scratch.file(
         "pkg/foo.bzl",
         """
@@ -1198,7 +1229,7 @@ def _impl(name, visibility, xyz):
               print("xyz evaluates to False")
 
         my_macro = macro(
-            implementation=_impl,
+            implementation = _impl,
             attrs = {
               "xyz": attr.string(),
             },
@@ -1259,16 +1290,17 @@ def _impl(name, visibility, **kwargs):
                 "//conditions:default": "//D:D",
             }),
             configurable_withdefault = select({"//conditions:default": None}),
+            visibility = ["//common:my_package_group"],
         )
         """);
 
     Package pkg = getPackage("pkg");
     assertPackageNotInError(pkg);
-    MacroInstance macroInstance = pkg.getMacrosById().get("abc:1");
+    MacroInstance macroInstance = getMacroById(pkg, "abc:1");
     ArrayList<Label> labels = new ArrayList<>();
     macroInstance.visitExplicitAttributeLabels(labels::add);
     // Order is the same as the attribute definition order.
-    assertThat(labels.stream().map(Label::toString))
+    assertThat(asStringList(labels))
         .containsExactly(
             "//A:A",
             "//A:A", // duplicate not pruned
@@ -1281,7 +1313,134 @@ def _impl(name, visibility, **kwargs):
             // `omitted` ignored, it has no default
             // `_implicit_default` ignored because it's implicit
             "//common:explicit_default" // from attr default
+            // `visibility` ignored, it's a NODEP label list
             )
         .inOrder();
   }
+
+  @Test
+  public void macrosThreadVisibilityAttrThroughWithCallsiteLocationAdded() throws Exception {
+    // Don't use test machinery's convenience setup that makes everything public by default.
+    setPackageOptions("--default_visibility=private");
+
+    // Submacro defines two targets, one exported (visibility = visibility) and one internal
+    // (private to the submacro's package).
+    scratch.file("lib1/BUILD");
+    scratch.file(
+        "lib1/macro.bzl",
+        """
+        def _impl(name, visibility):
+            native.cc_library(
+                name = name + "_exported",
+                visibility = visibility)
+            native.cc_library(name = name + "_internal")
+
+        submacro = macro(implementation=_impl)
+        """);
+    // Outer macro also defines two targets, but in addition calls the submacro twice, as an
+    // exported submacro and an internal one. So a total of six targets across three macro
+    // instances.
+    scratch.file("lib2/BUILD");
+    scratch.file(
+        "lib2/macro.bzl",
+        """
+        load("//lib1:macro.bzl", "submacro")
+
+        def _impl(name, visibility):
+            native.cc_library(
+                name = name + "_exported",
+                visibility = visibility)
+            native.cc_library(name = name + "_internal")
+            submacro(name=name + "_subexported", visibility = visibility)
+            submacro(name=name + "_subinternal")
+
+
+        outer_macro = macro(implementation=_impl)
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load("//lib2:macro.bzl", "outer_macro")
+        outer_macro(name="abc")
+        """);
+
+    Package pkg = getPackage("pkg");
+    assertPackageNotInError(pkg);
+
+    // Outer macro visible to BUILD file.
+    assertThat(getMacroVisibility(pkg, "abc:1")).containsExactly("//pkg:__pkg__");
+
+    // The outer macro's exported target and the exported submacro are visible to both the BUILD
+    // file and the outer macro.
+    assertThat(getTargetVisibility(pkg, "abc_exported"))
+        .containsExactly("//pkg:__pkg__", "//lib2:__pkg__");
+    assertThat(getMacroVisibility(pkg, "abc_subexported:1"))
+        .containsExactly("//pkg:__pkg__", "//lib2:__pkg__");
+
+    // The outer macro's internal target and the internal submacro are visible only to the outer
+    // macro.
+    assertThat(getTargetVisibility(pkg, "abc_internal")).containsExactly("//lib2:__pkg__");
+    assertThat(getMacroVisibility(pkg, "abc_subinternal:1")).containsExactly("//lib2:__pkg__");
+
+    // The exported submacro's exported target is visible to everything (exports all the way down).
+    assertThat(getTargetVisibility(pkg, "abc_subexported_exported"))
+        .containsExactly("//pkg:__pkg__", "//lib2:__pkg__", "//lib1:__pkg__");
+
+    // The internal submacro's exported target is visible to the outer macro and submacro.
+    assertThat(getTargetVisibility(pkg, "abc_subinternal_exported"))
+        .containsExactly("//lib2:__pkg__", "//lib1:__pkg__");
+
+    // Finally, the internal targets of both the exported and internal submacros are visible only to
+    // the submacro.
+    assertThat(getTargetVisibility(pkg, "abc_subexported_internal"))
+        .containsExactly("//lib1:__pkg__");
+    assertThat(getTargetVisibility(pkg, "abc_subinternal_internal"))
+        .containsExactly("//lib1:__pkg__");
+  }
+
+  @Test
+  public void defaultVisibilityAppliesOnlyToTopLevelMacros() throws Exception {
+    scratch.file("lib/BUILD");
+    scratch.file(
+        "lib/macro.bzl",
+        """
+        def _sub_impl(name, visibility):
+            pass
+
+        submacro = macro(implementation=_sub_impl)
+
+        def _impl(name, visibility):
+            submacro(name=name + "_sub")
+
+        outer_macro = macro(implementation=_impl)
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load("//lib:macro.bzl", "outer_macro")
+
+        package(default_visibility=["//defaulted:__pkg__"])
+
+        outer_macro(
+            name = "macro_with_explicit_vis",
+            visibility = ["//explicit:__pkg__"],
+        )
+        outer_macro(name="macro_without_explicit_vis")
+        """);
+
+    Package pkg = getPackage("pkg");
+    assertPackageNotInError(pkg);
+
+    // Default visibility only applies when visibility is not specified.
+    assertThat(getMacroVisibility(pkg, "macro_with_explicit_vis:1"))
+        .containsExactly("//explicit:__pkg__", "//pkg:__pkg__");
+    // When it does apply, we still append the callsite location.
+    assertThat(getMacroVisibility(pkg, "macro_without_explicit_vis:1"))
+        .containsExactly("//defaulted:__pkg__", "//pkg:__pkg__");
+    // Default visibility never applies inside a symbolic macro (i.e. to submacros).
+    assertThat(getMacroVisibility(pkg, "macro_with_explicit_vis_sub:1"))
+        .containsExactly("//lib:__pkg__");
+    assertThat(getMacroVisibility(pkg, "macro_without_explicit_vis_sub:1"))
+        .containsExactly("//lib:__pkg__");
+  }
 }