From 2ed46a716822b1b9b4f257b0b73584fed4ef0699 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 7 Sep 2023 06:59:12 +0200 Subject: [PATCH] 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. --- .../google/devtools/build/lib/analysis/BUILD | 1 + .../analysis/CommonPrerequisiteValidator.java | 30 +- .../build/lib/analysis/RuleContext.java | 15 + .../build/lib/analysis/VisibilityTest.java | 379 ++++++++++++++++++ 4 files changed, 420 insertions(+), 5 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 4e63fefd06168c..60db7a46db298d 100644 --- 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 index e315d89e1c2a3a..5c61304cfc3f05 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,6 +13,7 @@ // 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; @@ -20,6 +21,7 @@ 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; @@ -27,6 +29,7 @@ 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 index 9a2e39da17f18d..b22ede16d50065 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 @@ -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; @@ -2091,6 +2092,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 5055dca7084783..4dc3a95ba0e03d 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,384 @@ 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 = '//tools: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 = '//tools: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 = '//tools: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( + "tools/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( + "Aspect attribute values and visibility checks for them currently use different precedence" + + " rules in case of name collisions. This test requires them to be consistent. See" + + " https://github.com/bazelbuild/bazel/pull/19449") + @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 = '//tools: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 = '//tools: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 = '//tools: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( + "tools/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 = '//tools: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 = '//tools: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 = '//tools: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( + "tools/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 " + + "'//tools: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 = '//tools: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 = '//tools: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 = '//tools: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( + "tools/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 '//tools: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 = '//tools: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 = '//tools: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 = '//tools: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( + "tools/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 '//tools: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__'])");