Always check $config_dependencies visibility at use

All dependencies for `select` expressions are collected in an implicit `$config_dependencies` attribute. Even with `--incompatible_visibility_private_attributes_at_definition`, visibility for this attribute is now checked relative to the use rather than the definition of the rule. Fixes bazelbuild#19126 Closes bazelbuild#19139. PiperOrigin-RevId: 553830083 Change-Id: Ic4af0f5ad9a0c627c973cd5ce88dd1e1bf51474e
iancha1992 · Aug 7, 2023 · e70af95 · e70af95
1 parent 90e8ffd
commit e70af95
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 15 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java
@@ -25,7 +25,7 @@
 import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.Target;
+import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
@@ -87,20 +87,26 @@ private void validateDirectPrerequisiteVisibility(
               .getBool(
                   BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION);
 
-      if (!toolCheckAtDefinition
-          || !attribute.isImplicit()
-          || rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null) {
-        // Default check: The attribute must be visible from the target.
-        if (!context.isVisible(prerequisite.getConfiguredTarget())) {
-          handleVisibilityConflict(context, prerequisite, rule.getLabel());
-        }
-      } else {
-        // For implicit attributes, check if the prerequisite is visible from the location of the
-        // rule definition
-        Label implicitDefinition = rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel();
-        if (!RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) {
-          handleVisibilityConflict(context, prerequisite, implicitDefinition);
-        }
+    // Determine if we should use the new visibility rules for tools.
+    boolean toolCheckAtDefinition =
+        context
+            .getStarlarkSemantics()
+            .getBool(BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION);
+
+    if (!toolCheckAtDefinition
+        || !attribute.isImplicit()
+        || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
+        || rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null) {
+      // Default check: The attribute must be visible from the target.
+      if (!context.isVisible(prerequisite.getConfiguredTarget())) {
+        handleVisibilityConflict(context, prerequisite, rule.getLabel());
+      }
+    } else {
+      // For implicit attributes, check if the prerequisite is visible from the location of the
+      // rule definition
+      Label implicitDefinition = rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel();
+      if (!RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) {
+        handleVisibilityConflict(context, prerequisite, implicitDefinition);
       }
     }
 

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java
@@ -166,6 +166,42 @@ public void testDataVisibilityPrivateCheckPrivateAtUse() throws Exception {
     assertThrows(ViewCreationFailedException.class, () -> update("//use:world"));
   }
 
+  @Test
+  public void testConfigSettingVisibilityAlwaysCheckedAtUse() throws Exception {
+    scratch.file(
+        "BUILD",
+        "load('//build_defs:defs.bzl', 'my_rule')",
+        "my_rule(",
+        "    name = 'my_target',",
+        "    value = select({",
+        "        '//config_setting:my_setting': 'foo',",
+        "        '//conditions:default': 'bar',",
+        "    }),",
+        ")");
+    scratch.file("build_defs/BUILD");
+    scratch.file(
+        "build_defs/defs.bzl",
+        "def _my_rule_impl(ctx):",
+        "    pass",
+        "my_rule = rule(",
+        "    implementation = _my_rule_impl,",
+        "    attrs = {",
+        "        'value': attr.string(mandatory = True),",
+        "    },",
+        ")");
+    scratch.file(
+        "config_setting/BUILD",
+        "config_setting(",
+        "    name = 'my_setting',",
+        "    values = {'cpu': 'does_not_matter'},",
+        "    visibility = ['//:__pkg__'],",
+        ")");
+    useConfiguration("--incompatible_visibility_private_attributes_at_definition");
+
+    update("//:my_target");
+    assertThat(hasErrors(getConfiguredTarget("//:my_target"))).isFalse();
+  }
+
   void setupFilesScenario(String wantRead) throws Exception {
     scratch.file("src/source.txt", "source");
     scratch.file("src/BUILD", "exports_files(['source.txt'], visibility=['//pkg:__pkg__'])");