From 9c7d5879d5b746e527eb72fcc2c5e83af5d21680 Mon Sep 17 00:00:00 2001
From: Googler <brandjon@google.com>
Date: Thu, 26 Sep 2024 18:10:33 -0700
Subject: [PATCH] Don't propagate default_visibility to within a symbolic macro

Work toward #19922.

PiperOrigin-RevId: 679364770
Change-Id: I486c4147fb0234b4d78f8ed2ab3f71dca5d5e118
---
 .../build/lib/packages/RuleFactory.java       |  3 +-
 .../lib/analysis/MacroVisibilityTest.java     | 58 +++++++++++++++++--
 .../lib/packages/PackageFactoryTest.java      |  6 +-
 3 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
index 79911953561069..07de534cc2aaba 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFactory.java
@@ -297,8 +297,7 @@ private static List<Label> getModifiedVisibility(
     RuleVisibility visibility = null;
     Object uncheckedVisibilityAttr = args.getAttributeValue("visibility");
     if (uncheckedVisibilityAttr == null) {
-      // TODO: #19922 - Don't use default_visibility, we're in a symbolic macro.
-      visibility = pkgBuilder.getPartialPackageArgs().defaultVisibility();
+      visibility = RuleVisibility.PRIVATE;
     } else {
       try {
         List<Label> visibilityAttr =
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 ca97748aed1196..13b9a5a667bcf5 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
@@ -24,7 +24,10 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-/** Tests for the how the visibility system works with respect to symbolic macros. */
+/**
+ * Tests for the how the visibility system works with respect to symbolic macros, i.e. the
+ * Macro-Aware Visibility design.
+ */
 @RunWith(TestParameterInjector.class)
 public final class MacroVisibilityTest extends BuildViewTestCase {
 
@@ -924,6 +927,56 @@ public void delegationDoesNotFollowAliases() throws Exception {
         /* dependencyIsAlias= */ true);
   }
 
+  @Test
+  public void packageDefaultVisibilityDoesNotPropagateToInsideMacro() throws Exception {
+    defineSimpleRule();
+    defineWrappingMacroWithSameDep("my_macro", "//rules:simple_rule.bzl%simple_rule");
+    defineWrappingMacroWithSameDep("inner", "//rules:simple_rule.bzl%simple_rule");
+    defineWrappingMacroWithSameDep("outer", "//inner:macro.bzl%inner");
+    scratch.file(
+        "lib/BUILD",
+        """
+        load("//rules:simple_rule.bzl", "simple_rule")
+        load("//my_macro:macro.bzl", "my_macro")
+        load("//outer:macro.bzl", "outer")
+
+        package(default_visibility=["//pkg:__pkg__"])
+
+        simple_rule(name = "foo")
+
+        my_macro(name = "bar")
+
+        outer(name = "baz")
+        """);
+    scratch.file(
+        "pkg/BUILD",
+        """
+        load("//rules:simple_rule.bzl", "simple_rule")
+
+        simple_rule(
+            name = "consumes_foo",
+            dep = "//lib:foo",
+        )
+
+        simple_rule(
+            name = "consumes_bar",
+            dep = "//lib:bar",
+        )
+
+        simple_rule(
+            name = "consumes_baz",
+            dep = "//lib:baz",
+        )
+        """);
+
+    // Not in a macro, default_visibility applies.
+    assertVisibilityPermits("//pkg:consumes_foo", "//lib:foo");
+    // In a macro, default_visibility does not propagate.
+    assertVisibilityDisallows("//pkg:consumes_bar", "//lib:bar");
+    // In more than one macro, default_visibility still does not propagate.
+    assertVisibilityDisallows("//pkg:consumes_baz", "//lib:baz");
+  }
+
   /*
    * TODO: #19922 - Tests cases to add:
    *
@@ -960,9 +1013,6 @@ public void delegationDoesNotFollowAliases() throws Exception {
    *
    * ---- default_visibility ----
    *
-   * - default_visibility does not propagate to inside any symbolic macro, to either macros or
-   *   targets.
-   *
    * - 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=...
    *
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 c8b29cf85c0e48..e95fdb097dbc2e 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
@@ -1633,7 +1633,7 @@ def _impl(name):
   }
 
   @Test
-  public void testDeclarationVisibilityUnioning_respectsPackageDefaultVisibility()
+  public void testDeclarationVisibilityUnioning_doesNotApplyPackageDefaultVisibility()
       throws Exception {
     enableMacrosAndUsePrivateVisibility();
     scratch.file("lib/BUILD");
@@ -1656,9 +1656,7 @@ def _impl(name):
 
     Package pkg = loadPackageAndAssertSuccess("pkg");
     assertVisibilityIs(pkg.getTarget("foo"), "//other_pkg:__pkg__");
-    // TODO: #19922 - Change this behavior, default visibility should not propagate to within a
-    // macro.
-    assertVisibilityIs(pkg.getTarget("bar"), "//other_pkg:__pkg__", "//lib:__pkg__");
+    assertVisibilityIs(pkg.getTarget("bar"), "//lib:__pkg__");
   }
 
   @Test