[6.5.0] Flip --incompatible_visibility_private_attributes_at_definiti…

…on (#20520)

Cherry-picks:
7a537ca,
0656103,
bc1df91,
7b97147

Fixes #20480

---------

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>

site/en/concepts/visibility.md

@@ -232,18 +232,22 @@ every instance of that rule. For example, a `cc_library` rule might create an
 implicit dependency from each of its rule targets to an executable target
 representing a C++ compiler.
 
-Currently, for visibility purposes these implicit dependencies are treated like
-any other dependency. This means that the target being depended on (such as our
-C++ compiler) must be visible to every instance of the rule. In practice this
-usually means the target must have public visibility.
-
-You can change this behavior by setting
-[`--incompatible_visibility_private_attributes_at_definition`](https://github.com/bazelbuild/proposals/blob/master/designs/2019-10-15-tool-visibility.md){: .external}. When enabled, the
-target in question need only be visible to the rule declaring it an implicit
-dependency. That is, it must be visible to the package containing the `.bzl`
-file in which the rule is defined. In our example, the C++ compiler could be
-private so long as it lives in the same package as the definition of the
-`cc_library` rule.
+The visibility of such an implicit dependency is checked with respect to the
+package containing the `.bzl` file in which the rule (or aspect) is defined. In
+our example, the C++ compiler could be private so long as it lives in the same
+package as the definition of the `cc_library` rule. As a fallback, if the
+implicit dependency is not visible from the definition, it is checked with
+respect to the `cc_library` target.
+
+You can change this behavior by disabling
+[`--incompatible_visibility_private_attributes_at_definition`](https://github.com/bazelbuild/proposals/blob/master/designs/2019-10-15-tool-visibility.md){: .external}.
+When disabled, implicit dependencies are treated like any other dependency.
+This means that the target being depended on (such as our C++ compiler) must be
+visible to every instance of the rule. In practice this usually means the target
+must have public visibility.
+
+If you want to restrict the usage of a rule to certain packages, use
+[load visibility](#load-visibility) instead.
 
 ## Load visibility {:#load-visibility}
 

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

@@ -663,6 +663,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",
  ],
 )
 

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

@@ -13,10 +13,12 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode;
 import com.google.devtools.build.lib.analysis.RuleContext.PrerequisiteValidator;
 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;
@@ -27,6 +29,7 @@
 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.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;
@@ -80,27 +83,54 @@ private void validateDirectPrerequisiteVisibility(
  AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget())
  .getPackageIdentifier())
  && !Attribute.isLateBound(attrName)) {
+ // Only verify visibility of implicit dependencies of the current aspect.
+ // Dependencies of other aspects as well as the rule itself are checked when they are
+ // evaluated.
+ Aspect mainAspect = context.getMainAspect();
+ if (mainAspect != null) {
+ if (!attribute.isImplicit()
+ || !mainAspect.getDefinition().getAttributes().containsKey(attrName)) {
+ return;
+ }
+ }
 
  // Determine if we should use the new visibility rules for tools.
  boolean toolCheckAtDefinition =
- context
- .getStarlarkSemantics()
- .getBool(
- BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION);
+  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) {
+  || !attribute.isImplicit()
+  || 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())) {
  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;
+ if (mainAspect != null) {
+ 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());
+ }
+ // Check that the prerequisite is visible from the definition. As a fallback, check if the
+ // 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())) {
+ // In the error message, always suggest making the prerequisite visible from the definition,
+ // not the target.
  handleVisibilityConflict(context, prerequisite, implicitDefinition);
  }
  }

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

@@ -34,6 +34,7 @@
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Multimaps;
+import com.google.common.collect.Streams;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionLookupKey;
@@ -84,6 +85,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.SymbolGenerator;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
@@ -2113,6 +2115,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()) {

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -472,13 +472,13 @@ public final class BuildLanguageOptions extends OptionsBase {
 
  @Option(
  name = "incompatible_visibility_private_attributes_at_definition",
- defaultValue = "false",
+ defaultValue = "true",
  documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
  effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
  metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
  help =
  "If set to true, the visibility of private rule attributes is checked with respect "
- + "to the rule definition, rather than the rule usage.")
+ + "to the rule definition, falling back to rule usage if not visible.")
  public boolean incompatibleVisibilityPrivateAttributesAtDefinition;
 
  @Option(
@@ -865,7 +865,7 @@ public StarlarkSemantics toStarlarkSemantics() {
  public static final String INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION =
  "+incompatible_unambiguous_label_stringification";
  public static final String INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION =
- "-incompatible_visibility_private_attributes_at_definition";
+ "+incompatible_visibility_private_attributes_at_definition";
  public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS =
  "-incompatible_top_level_aspects_require_providers";
  public static final String INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS =

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

@@ -1348,6 +1348,71 @@ public void aspectSeesAspectHintsAttributeOnStarlarkRule() throws Exception {
  assertThat(info.truncateToInt()).isEqualTo(2);
  }
 
+ @Test
+ public void ruleDepsVisibilityNotAffectNativeAspect() throws Exception {
+ setRulesAndAspectsAvailableInTests(
+ ImmutableList.of(TestAspects.ALL_ATTRIBUTES_ASPECT), ImmutableList.of());
+ useConfiguration("--incompatible_visibility_private_attributes_at_definition");
+ scratch.file("defs/BUILD");
+ scratch.file(
+ "defs/build_defs.bzl",
+ "def _rule_impl(ctx):",
+ " pass",
+ "",
+ "implicit_dep_rule = rule(",
+ " implementation = _rule_impl,",
+ " attrs = {",
+ " '_tool': attr.label(default = '//tool:tool'),",
+ " 'deps': attr.label_list()",
+ " },",
+ ")");
+ scratch.file("tool/BUILD", "sh_library(name='tool', visibility = ['//defs:__pkg__'])");
+ scratch.file(
+ "pkg/BUILD",
+ "load('//defs:build_defs.bzl', 'implicit_dep_rule')",
+ "implicit_dep_rule(name='y')",
+ "implicit_dep_rule(name='x', deps = [':y'])");
+
+ AnalysisResult result =
+ update(
+ new EventBus(),
+ defaultFlags(),
+ ImmutableList.of(TestAspects.ALL_ATTRIBUTES_ASPECT.getName()),
+ "//pkg:x");
+
+ assertThat(result.hasError()).isFalse();
+ }
+
+ @Test
+ public void nativeAspectFailIfDepsNotVisible() throws Exception {
+ scratch.file("tool/BUILD", "sh_library(name='tool', visibility = ['//visibility:private'])");
+ ExtraAttributeAspect extraAttributeAspect = new ExtraAttributeAspect("//tool:tool", false);
+ setRulesAndAspectsAvailableInTests(ImmutableList.of(extraAttributeAspect), ImmutableList.of());
+ scratch.file(
+ "pkg/build_defs.bzl",
+ "def _rule_impl(ctx):",
+ " pass",
+ "",
+ "simple_rule = rule(",
+ " implementation = _rule_impl,",
+ ")");
+ scratch.file(
+ "pkg/BUILD", "load('//pkg:build_defs.bzl', 'simple_rule')", "simple_rule(name='x')");
+ reporter.removeHandler(failFastHandler);
+
+ assertThrows(
+ ViewCreationFailedException.class,
+ () ->
+ update(
+ new EventBus(),
+ defaultFlags(),
+ ImmutableList.of(extraAttributeAspect.getName()),
+ "//pkg:x"));
+ assertContainsEvent(
+ "ExtraAttributeAspect_//tool:tool_false aspect on simple_rule rule //pkg:x: target"
+ + " '//tool:tool' is not visible from target '//pkg:x'.");
+ }
+
  private void setupAspectHints() throws Exception {
  scratch.file(
  "aspect_hints/hints.bzl",