From be7e80a9d3780aa23905773ff7572cd6a8631b28 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 21 Sep 2023 09:38:33 -0700 Subject: [PATCH 1/5] Fix visibility check at definition for aspects 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 (cherry picked from commit 7a537ca644ac17d565585cd4b3e21bf6243dc001) --- .../google/devtools/build/lib/analysis/BUILD | 1 + .../analysis/CommonPrerequisiteValidator.java | 34 +- .../build/lib/analysis/RuleContext.java | 15 + .../build/lib/analysis/VisibilityTest.java | 378 ++++++++++++++++++ 4 files changed, 421 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index af51abba76a983..0adf8dcf735fec 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/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", ], ) 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 index b5721702e1c3b5..855ed55e83d24e 100644 --- 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,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; @@ -89,18 +92,35 @@ private void validateDirectPrerequisiteVisibility( 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; + 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 index 6c532fd3c664e0..48b667ddc5d6a1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -84,6 +84,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 +2114,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 index 3a3e377974de0d..30b3e96b3a3862 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java @@ -18,6 +18,7 @@ import static org.junit.Assert.assertThrows; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -202,6 +203,383 @@ public void testConfigSettingVisibilityAlwaysCheckedAtUse() throws Exception { assertThat(hasErrors(getConfiguredTarget("//:my_target"))).isFalse(); } + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + + update("//foo:target_with_aspects"); + + assertThat(hasErrors(getConfiguredTarget("//foo:target_with_aspects"))).isFalse(); + } + + @Ignore( + "TODO(b/206127051): The aspects path implicit dependendencies with same name are incorrectly" + + " merged, enable this test when this is fixed.") + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + + update("//foo:target_with_aspects"); + + assertThat(hasErrors(getConfiguredTarget("//foo:target_with_aspects"))).isFalse(); + } + + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_outerAspectToolNotVisible() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = [", + " '//inner_aspect:__pkg__',", + " '//rule:__pkg__',", + " '//foo:__pkg__',", + " ],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + reporter.removeHandler(failFastHandler); + + assertThrows(ViewCreationFailedException.class, () -> update("//foo:target_with_aspects")); + assertContainsEvent( + "in //inner_aspect:lib.bzl%inner_aspect,//outer_aspect:lib.bzl%outer_aspect " + + "aspect on simple_starlark_rule rule //foo:simple_dep: target " + + "'//tool:outer_aspect_tool' is not visible from target '//outer_aspect:lib.bzl'"); + } + + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_innerAspectToolNotVisible() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = [", + " '//outer_aspect:__pkg__',", + " '//rule:__pkg__',", + " '//foo:__pkg__',", + " ],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + reporter.removeHandler(failFastHandler); + + assertThrows(ViewCreationFailedException.class, () -> update("//foo:target_with_aspects")); + assertContainsEvent( + "in //inner_aspect:lib.bzl%inner_aspect aspect on simple_starlark_rule " + + "rule //foo:simple_dep: target '//tool:inner_aspect_tool' is not visible from " + + "target '//inner_aspect:lib.bzl'"); + } + + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_ruleToolNotVisible() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = [", + " '//outer_aspect:__pkg__',", + " '//inner_aspect:__pkg__',", + " '//foo:__pkg__',", + " ],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + reporter.removeHandler(failFastHandler); + + assertThrows(ViewCreationFailedException.class, () -> update("//foo:target_with_aspects")); + assertContainsEvent( + "in my_rule rule //foo:target_with_aspects: target '//tool:rule_tool' is " + + "not visible from target '//rule:lib.bzl'"); + } + void setupFilesScenario(String wantRead) throws Exception { scratch.file("src/source.txt", "source"); scratch.file("src/BUILD", "exports_files(['source.txt'], visibility=['//pkg:__pkg__'])"); From 62bb8d04767578af772a1f6b4b2835aa408a81af Mon Sep 17 00:00:00 2001 From: Mai Essa Date: Tue, 12 Dec 2023 14:35:55 -0500 Subject: [PATCH 2/5] Add missing import --- .../java/com/google/devtools/build/lib/analysis/RuleContext.java | 1 + 1 file changed, 1 insertion(+) 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 index 48b667ddc5d6a1..ac42b1f791221f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/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; From 4cc16e747b77110862e46f22c59f829200954be1 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 29 Sep 2023 12:15:57 -0700 Subject: [PATCH 3/5] Flip `--incompatible_visibility_private_attributes_at_definition` and make it less breaking In addition to flipping the flag, also add a check for visibility from the target as a fallback to make the flip less breaking. Closes #19330 RELNOTES[INC]: --incompatible_visibility_private_attributes_at_definition is flipped to true. See https://github.com/bazelbuild/bazel/issues/19330 for details. Closes #19141. PiperOrigin-RevId: 569556377 Change-Id: Ic691c5585d9ba2d95b6ff3c32b9979f777147d8b (cherry picked from commit 0656103c9bc23b627e1a49fe4544f068469f8f1b) --- site/en/concepts/visibility.md | 28 +++++++++++-------- .../analysis/CommonPrerequisiteValidator.java | 16 +++++++---- .../semantics/BuildLanguageOptions.java | 6 ++-- .../build/lib/analysis/VisibilityTest.java | 12 ++++---- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/site/en/concepts/visibility.md b/site/en/concepts/visibility.md index 8bddd2b2b3b7ae..4214989e3834cf 100644 --- a/site/en/concepts/visibility.md +++ b/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} 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 index 855ed55e83d24e..d8e1780db43d2b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java @@ -86,10 +86,10 @@ private void validateDirectPrerequisiteVisibility( // 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() @@ -119,8 +119,14 @@ private void validateDirectPrerequisiteVisibility( 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())) { + && !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); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 469427b6f32856..248a2c5b6f05c8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/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 = 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 index 30b3e96b3a3862..8fcafca3813605 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java @@ -93,14 +93,15 @@ public void testToolVisibilityUseCheckAtUse() throws Exception { } @Test - public void testToolVisibilityUseCheckAtRule() throws Exception { + public void testToolVisibilityUseCheckAtRule_fallbackToUse() throws Exception { setupArgsScenario(); scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])"); scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//use:__pkg__'])"); useConfiguration("--incompatible_visibility_private_attributes_at_definition"); - reporter.removeHandler(failFastHandler); - assertThrows(ViewCreationFailedException.class, () -> update("//use:world")); + update("//use:world"); + + assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse(); } @Test @@ -274,7 +275,7 @@ public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exc } @Ignore( - "TODO(b/206127051): The aspects path implicit dependendencies with same name are incorrectly" + "TODO(b/206127051): The aspects path implicit dependencies with same name are incorrectly" + " merged, enable this test when this is fixed.") @Test public void testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision() @@ -402,7 +403,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_outerAspectToolNotVi " visibility = [", " '//inner_aspect:__pkg__',", " '//rule:__pkg__',", - " '//foo:__pkg__',", " ],", ")", "sh_binary(", @@ -485,7 +485,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_innerAspectToolNotVi " visibility = [", " '//outer_aspect:__pkg__',", " '//rule:__pkg__',", - " '//foo:__pkg__',", " ],", ")", "sh_binary(", @@ -568,7 +567,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_ruleToolNotVisible() " visibility = [", " '//outer_aspect:__pkg__',", " '//inner_aspect:__pkg__',", - " '//foo:__pkg__',", " ],", ")"); useConfiguration("--incompatible_visibility_private_attributes_at_definition"); From 9c4b2ec246e9ef2dfaabfdfbaa3b572583381f1d Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 16 Oct 2023 10:14:34 -0700 Subject: [PATCH 4/5] For aspect evaluation, only check visibility of aspect's implicit dependencies Move this check to also cover native aspects evaluation, otherwise it evaluates the visibility of the tools of the underlying target rule against the target itself. PiperOrigin-RevId: 573850253 Change-Id: I69beb6542128338fe859d2978d29018731fc1258 (cherry picked from commit bc1df918074883660b74c40e0ee43db9aa0ced87) --- .../analysis/CommonPrerequisiteValidator.java | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 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 index d8e1780db43d2b..7a40a6b03a6ed3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java @@ -83,6 +83,16 @@ 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 = @@ -104,16 +114,10 @@ private void validateDirectPrerequisiteVisibility( // 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()); - } + 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 = From dfb07ea9ea50be01f56303970512e748f0dd5da9 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 18 Oct 2023 09:45:40 -0700 Subject: [PATCH 5/5] Add test for native aspect not failing because of rule tools visibility native aspects should only check visibility of their own dependencies as dependencies of the rule are checked during its evaluation. PiperOrigin-RevId: 574504460 Change-Id: I4b2296ee4d471b98a3bd965616d9199a4e629828 (cherry picked from commit 7b97147263700067e4397fa6c9a25318fa2e98f7) --- .../build/lib/analysis/AspectTest.java | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java index fbba584ab58360..c93a4ceffd7f03 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java +++ b/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",