Fix visibility check at definition for aspects

fmeum · copybara-github · commit 7a537ca644ac · 2023-09-21T09:40:17.000-07:00
With `--incompatible_visibility_private_attributes_at_definition`, the visibility of aspects (and aspects on aspects) into their implicit dependencies is now checked relative to the aspect's, not the rule's definition. This requires ensuring the precedence of aspect on aspect attributes over those of the underlying aspect in `DependencyResolutionHelper`, which was hinted at by a comment but not realized. Closes #19442. PiperOrigin-RevId: 567333681 Change-Id: I167c45fea8535e0f35c0298ca1cb9f845c9fe2d2
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -654,6 +654,7 @@ java_library(
         "//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",
     ],
 )
 
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
@@ -13,20 +13,23 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper.attributeOrNull;
 
 import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
 import com.google.devtools.build.lib.analysis.RuleContext.PrerequisiteValidator;
 import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.packages.Aspect;
 import com.google.devtools.build.lib.packages.Attribute;
 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.RawAttributeMapper;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.StarlarkAspectClass;
 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;
@@ -105,16 +108,33 @@ private void validateDirectPrerequisiteVisibility(
     if (!toolCheckAtDefinition
         || !attribute.isImplicit()
         || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
-        || rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null) {
+        || !context.isStarlarkRuleOrAspect()) {
       // 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())) {
+      // For implicit attributes of Starlark rules or aspects, check if the prerequisite is visible
+      // from the location of the definition that declares the attribute. Only perform this check
+      // for the current aspect.
+      Label implicitDefinition = null;
+      Aspect mainAspect = context.getMainAspect();
+      if (mainAspect != null) {
+        // Only verify visibility of implicit dependencies of the current aspect. Implicit
+        // dependencies of other aspects as well as the rule itself are checked when they are
+        // evaluated.
+        if (mainAspect.getDefinition().getAttributes().containsKey(attrName)) {
+          StarlarkAspectClass aspectClass = (StarlarkAspectClass) mainAspect.getAspectClass();
+          // Never null since we already checked that the aspect is Starlark-defined.
+          implicitDefinition = checkNotNull(aspectClass.getExtensionLabel());
+        }
+      } else {
+        // Never null since we already checked that the rule is a Starlark rule.
+        implicitDefinition =
+            checkNotNull(rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel());
+      }
+      if (implicitDefinition != null
+          && !RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) {
         handleVisibilityConflict(context, prerequisite, implicitDefinition);
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -81,6 +81,7 @@
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
+import com.google.devtools.build.lib.packages.StarlarkAspectClass;
 import com.google.devtools.build.lib.packages.StarlarkProviderWrapper;
 import com.google.devtools.build.lib.packages.SymbolGenerator;
 import com.google.devtools.build.lib.packages.Target;
@@ -2031,6 +2032,20 @@ public boolean isVisible(TransitiveInfoCollection prerequisite) {
       return RuleContext.isVisible(target.getAssociatedRule(), prerequisite);
     }
 
+    @Nullable
+    Aspect getMainAspect() {
+      return Streams.findLast(aspects.stream()).orElse(null);
+    }
+
+    boolean isStarlarkRuleOrAspect() {
+      Aspect mainAspect = getMainAspect();
+      if (mainAspect != null) {
+        return mainAspect.getAspectClass() instanceof StarlarkAspectClass;
+      } else {
+        return getRule().getRuleClassObject().getRuleDefinitionEnvironmentLabel() != null;
+      }
+    }
+
     private void validateDirectPrerequisiteFileTypes(
         ConfiguredTargetAndData prerequisite, Attribute attribute) {
       if (attribute.isSkipAnalysisTimeFileTypeCheck()) {
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

Original file line number	Diff line number	Diff line change
`@@ -654,6 +654,7 @@ java_library(`
`654`	`654`	`"//src/main/java/com/google/devtools/build/lib/packages",`
`655`	`655`	`"//src/main/java/com/google/devtools/build/lib/packages/semantics",`
`656`	`656`	`"//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data",`
	`657`	`+ "//third_party:guava",`
`657`	`658`	`],`
`658`	`659`	`)`
`659`	`660`