Implicit dependencies should be visible to rule/aspect definitions in…

… `.bzl` files in the same package

Fixes: []
PiperOrigin-RevId: 612579829
Change-Id: I429ca24931482cc94543692532ea4c08692020e3

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

@@ -653,9 +653,9 @@ java_library(
     deps = [
         ":analysis_cluster",
         ":rule_error_consumer",
+        ":visibility_provider",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",
-        "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",
         "//third_party:guava",
     ],

src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.packages.FunctionSplitTransitionAllowlist;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper;
+import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
@@ -79,13 +80,6 @@ private void validateDirectPrerequisiteVisibility(
 
     checkVisibilityAttributeContents(context, prerequisite, attribute, attrName, rule);
 
-    if (isSameLogicalPackage(
-        rule.getLabel().getPackageIdentifier(),
-        AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget())
-            .getPackageIdentifier())) {
-      return;
-    }
-
     // We don't check the visibility of late-bound attributes, because it would break some
     // features.
     if (Attribute.isLateBound(attrName)) {
@@ -113,7 +107,7 @@ private void validateDirectPrerequisiteVisibility(
         || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
         || !context.isStarlarkRuleOrAspect()) {
       // Default check: The attribute must be visible from the target.
-      if (!context.isVisible(prerequisite.getConfiguredTarget())) {
+      if (!isVisible(rule.getLabel(), prerequisite)) {
         handleVisibilityConflict(context, prerequisite, rule.getLabel());
       }
     } else {
@@ -134,15 +128,37 @@ private void validateDirectPrerequisiteVisibility(
       // prerequisite is visible from the target so that adopting this new style of checking
       // visibility is not a breaking change.
       if (implicitDefinition != null
-          && !RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())
-          && !context.isVisible(prerequisite.getConfiguredTarget())) {
+          && !isVisible(implicitDefinition, prerequisite)
+          && !isVisible(rule.getLabel(), prerequisite)) {
         // In the error message, always suggest making the prerequisite visible from the definition,
         // not the target.
         handleVisibilityConflict(context, prerequisite, implicitDefinition);
       }
     }
   }
 
+  private boolean isVisible(Label target, ConfiguredTargetAndData prerequisite) {
+    if (isSameLogicalPackage(
+        target.getPackageIdentifier(),
+        AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget())
+            .getPackageIdentifier())) {
+      return true;
+    }
+    // Check visibility attribute
+    for (PackageGroupContents specification :
+        prerequisite
+            .getConfiguredTarget()
+            .getProvider(VisibilityProvider.class)
+            .getVisibility()
+            .toList()) {
+      if (specification.containsPackage(target.getPackageIdentifier())) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
   private void checkVisibilityAttributeContents(
       RuleContext.Builder context,
       ConfiguredTargetAndData prerequisite,

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

@@ -1448,34 +1448,6 @@ public boolean isExecutedOnWindows() {
         .hasConstraintValue(OS_TO_CONSTRAINTS.get(OS.WINDOWS));
   }
 
-  /**
-   * Returns true if {@code label} is visible from {@code prerequisite}.
-   *
-   * <p>This only computes the logic as implemented by the visibility system. The final decision
-   * whether a dependency is allowed is made by {@link PrerequisiteValidator}.
-   */
-  public static boolean isVisible(Label label, TransitiveInfoCollection prerequisite) {
-    // Check visibility attribute
-    for (PackageGroupContents specification :
-        prerequisite.getProvider(VisibilityProvider.class).getVisibility().toList()) {
-      if (specification.containsPackage(label.getPackageIdentifier())) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
-  /**
-   * Returns true if {@code rule} is visible from {@code prerequisite}.
-   *
-   * <p>This only computes the logic as implemented by the visibility system. The final decision
-   * whether a dependency is allowed is made by {@link PrerequisiteValidator}.
-   */
-  public static boolean isVisible(Rule rule, TransitiveInfoCollection prerequisite) {
-    return isVisible(rule.getLabel(), prerequisite);
-  }
-
   /**
    * @return the set of features applicable for the current rule.
    */
@@ -1903,17 +1875,6 @@ public BuildConfigurationValue getConfiguration() {
       return configuration;
     }
 
-    /**
-     * @return true if {@code rule} is visible from {@code prerequisite}.
-     *     <p>This only computes the logic as implemented by the visibility system. The final
-     *     decision whether a dependency is allowed is made by {@link PrerequisiteValidator}, who is
-     *     supposed to call this method to determine whether a dependency is allowed as per
-     *     visibility rules.
-     */
-    public boolean isVisible(TransitiveInfoCollection prerequisite) {
-      return RuleContext.isVisible(target.getAssociatedRule(), prerequisite);
-    }
-
     @Nullable
     Aspect getMainAspect() {
       return Streams.findLast(aspects.stream()).orElse(null);

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

@@ -163,6 +163,54 @@ public void testConfigSettingVisibilityAlwaysCheckedAtUse() throws Exception {
     assertThat(hasErrors(getConfiguredTarget("//:my_target"))).isFalse();
   }
 
+  @Test
+  public void testImplicitDependency_samePackageAsDefinition_visible() throws Exception {
+    scratch.file(
+        "aspect_def/BUILD",
+        "sh_binary(",
+        "  name = 'aspect_tool',",
+        "  srcs = ['a.sh'],",
+        "  visibility = ['//visibility:private'])");
+    scratch.file(
+        "aspect_def/lib.bzl",
+        "def _impl_my_aspect(ctx, target):",
+        "  return []",
+        "my_aspect = aspect(",
+        "  _impl_my_aspect,",
+        "  attrs = { '_aspect_tool': attr.label(default = '//aspect_def:aspect_tool') },",
+        ")");
+    scratch.file(
+        "rule_def/BUILD",
+        "sh_binary(",
+        "  name = 'rule_tool',",
+        "  srcs = ['a.sh'],",
+        "  visibility = ['//visibility:private'])");
+    scratch.file(
+        "rule_def/lib.bzl",
+        "load('//aspect_def:lib.bzl', 'my_aspect')",
+        "def _impl(ctx):",
+        "  pass",
+        "my_rule = rule(",
+        "  _impl,",
+        "  attrs = {",
+        "    'dep': attr.label(aspects = [my_aspect]),",
+        "    '_rule_tool': attr.label(default = '//rule_def:rule_tool'),",
+        "  },",
+        ")",
+        "simple_starlark_rule = rule(",
+        "  _impl,",
+        ")");
+    scratch.file(
+        "foo/BUILD",
+        "load('//rule_def:lib.bzl','my_rule', 'simple_starlark_rule')",
+        "simple_starlark_rule(name = 'simple_dep')",
+        "my_rule(name = 'my_target', dep = ':simple_dep')");
+
+    update("//foo:my_target");
+
+    assertThat(hasErrors(getConfiguredTarget("//foo:my_target"))).isFalse();
+  }
+
   @Test
   public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exception {
     scratch.file("inner_aspect/BUILD");