From 26be41a5f4ef2b3812665b36f136d2e44ae12688 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 6 Oct 2022 09:54:44 -0700 Subject: [PATCH] Fix a NPE in compatible target skipping Under a complex set of conditions a NPE is triggered. It was caused by IncompatibleTargetChecker constructing a RuleConfiguredTarget without adding a VisibilityProviderImpl to it. All other locations where RuleConfiguredTarget is constructed add the provider, namely RuleConfiguredTargetBuilder. When the set of conditions is met, code path checking the visibility retrieves the provider without checking if it's available. The data in VisibilityProvider is duplicated with the data already in RuleConfiguredTarget. Fix by removing the provider and retrieving the data directly. Add the regression test with description how it happens. PiperOrigin-RevId: 479339612 Change-Id: I8e29a9882de87d884f53e104ae14d6a126a8096b --- .../analysis/RuleConfiguredTargetBuilder.java | 1 - .../FileConfiguredTarget.java | 13 +- .../MergedConfiguredTarget.java | 14 +- .../RuleConfiguredTarget.java | 11 +- .../skyframe/ToolchainsForTargetsTest.java | 226 ++++++++++++++++++ 5 files changed, 256 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index 7ca795ec6f2c2a..8a6a9728894f70 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -94,7 +94,6 @@ public RuleConfiguredTargetBuilder(RuleContext ruleContext) { this.ruleContext = ruleContext; // Avoid building validations in analysis tests (b/143988346) add(LicensesProvider.class, LicensesProviderImpl.of(ruleContext)); - add(VisibilityProvider.class, new VisibilityProviderImpl(ruleContext.getVisibility())); } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java index bfcbacd861c565..9e173504b5a22c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java @@ -108,9 +108,16 @@ public String filePathForFileTypeMatcher() { } @Override - public

P getProvider(Class

provider) { - AnalysisUtils.checkProvider(provider); - return providers.getProvider(provider); + public

P getProvider(Class

providerClass) { + AnalysisUtils.checkProvider(providerClass); + final P provider = providers.getProvider(providerClass); + if (provider != null) { + return provider; + } else if (providerClass.isAssignableFrom(getClass())) { + return providerClass.cast(this); + } else { + return null; + } } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java index 05575109324735..b4d9d1802567ae 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java @@ -73,11 +73,17 @@ public

P getProvider(Class

providerClass) AnalysisUtils.checkProvider(providerClass); P provider = nonBaseProviders.getProvider(providerClass); - if (provider == null) { - provider = base.getProvider(providerClass); + if (provider != null) { + return provider; } - - return provider; + provider = base.getProvider(providerClass); + if (provider != null) { + return provider; + } + if (providerClass.isAssignableFrom(getClass())) { + return providerClass.cast(this); + } + return null; } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java index c52131d3a536a4..7726bcb245d897 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java @@ -21,6 +21,7 @@ import com.google.common.collect.Interner; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider; @@ -198,7 +199,15 @@ public String getRuleClassString() { public

P getProvider(Class

providerClass) { // TODO(bazel-team): Should aspects be allowed to override providers on the configured target // class? - return providers.getProvider(providerClass); + AnalysisUtils.checkProvider(providerClass); + final P provider = providers.getProvider(providerClass); + if (provider != null) { + return provider; + } + if (providerClass.isAssignableFrom(getClass())) { + return providerClass.cast(this); + } + return null; } @Override diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java index 63f1271e6fb255..888fde941e7135 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java @@ -21,8 +21,11 @@ import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.analysis.AnalysisResult; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.TargetAndConfiguration; @@ -42,6 +45,7 @@ import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -517,6 +521,228 @@ public void toolchainWithDifferentExecutionPlatforms_doesNotGenerateConflictingC assertThat(toolchainBContext).hasResolvedToolchain("//toolchains:toolchain_1_impl"); } + @CanIgnoreReturnValue + private AnalysisResult updateExplicitTarget(String label) throws Exception { + return update( + new EventBus(), + defaultFlags().with(Flag.KEEP_GOING), + /*explicitTargetPatterns=*/ ImmutableSet.of(Label.parseAbsoluteUnchecked(label)), + /*aspects=*/ ImmutableList.of(), + /*aspectsParameters=*/ ImmutableMap.of(), + label); + } + + @Test + public void targetCompatibleWith_matchesExecCompatibleWith() throws Exception { + scratch.file( + "platforms/BUILD", + "constraint_setting(name = 'local_setting')", + "constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')", + "constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')", + "platform(name = 'local_platform_a',", + " constraint_values = [':local_value_a'],", + ")", + "platform(name = 'local_platform_b',", + " constraint_values = [':local_value_b'],", + ")"); + useConfiguration( + "--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b"); + scratch.file( + "foo/BUILD", + "sh_binary(name = 'tool', srcs = ['a.sh'], ", + " target_compatible_with = ['//platforms:local_value_b'])", + "genrule(name = 'runtool', tools = [':tool'], cmd = '', outs = ['b.txt'],", + " exec_compatible_with = ['//platforms:local_value_b'])"); + + AnalysisResult result = updateExplicitTarget("//foo:runtool"); + + assertThat(result.hasError()).isFalse(); + assertNoEvents(); + } + + @Test + public void targetCompatibleWith_mismatchesExecCompatibleWith() throws Exception { + scratch.file( + "platforms/BUILD", + "constraint_setting(name = 'local_setting')", + "constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')", + "constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')", + "platform(name = 'local_platform_a',", + " constraint_values = [':local_value_a'],", + ")", + "platform(name = 'local_platform_b',", + " constraint_values = [':local_value_b'],", + ")"); + useConfiguration( + "--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b"); + scratch.file( + "foo/BUILD", + "sh_binary(name = 'tool', srcs = ['a.sh'], ", + " target_compatible_with = ['//platforms:local_value_a'])", + "genrule(name = 'runtool', tools = [':tool'], cmd = '', outs = ['b.txt'],", + " exec_compatible_with = ['//platforms:local_value_b'])"); + + reporter.removeHandler(failFastHandler); + AnalysisResult result = updateExplicitTarget("//foo:runtool"); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "Target //foo:runtool is incompatible and cannot be built, but was explicitly requested"); + } + + @Test + public void targetCompatibleWith_mismatchesExecCompatibleInDifferentPackage() throws Exception { + // Regression test for a case where incompatibility happens in an aspect tool in a different + // package over a Starlark target with + // --incompatible_visibility_private_attributes_at_definition + // The tool is replaced with a fake target {@link + // IncommpatibleTargetChecker#createIncompatibleRuleConfiguredTarget) + // and it's verified if the tool is visible to the Starlark target. + scratch.file( + "platforms/BUILD", + "constraint_setting(name = 'local_setting')", + "constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')", + "constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')", + "platform(name = 'local_platform_a',", + " constraint_values = [':local_value_a'],", + ")", + "platform(name = 'local_platform_b',", + " constraint_values = [':local_value_b'],", + ")"); + + useConfiguration( + "--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b", + "--incompatible_visibility_private_attributes_at_definition"); + scratch.file( + "foo/lib.bzl", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = { '_my_tool': attr.label(default = '//tool') },", + ")"); + scratch.file( + "tool/BUILD", + "sh_binary(name = 'tool', srcs = ['a.cc'], ", + " target_compatible_with = ['//platforms:local_value_a'])"); + scratch.file( + "foo/BUILD", // + "load(':lib.bzl','my_rule')", + "my_rule(name = 'target_in_different_package')"); + + reporter.removeHandler(failFastHandler); + AnalysisResult result = updateExplicitTarget("//foo:target_in_different_package"); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "Target //foo:target_in_different_package is incompatible and cannot be built, but was" + + " explicitly requested"); + } + + @Test + public void targetCompatibleWith_mismatchesExecCompatibleDepInDifferentPackage() + throws Exception { + scratch.file( + "platforms/BUILD", + "constraint_setting(name = 'local_setting')", + "constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')", + "constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')", + "platform(name = 'local_platform_a',", + " constraint_values = [':local_value_a'],", + ")", + "platform(name = 'local_platform_b',", + " constraint_values = [':local_value_b'],", + ")"); + + useConfiguration( + "--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b", + "--incompatible_visibility_private_attributes_at_definition"); + scratch.file( + "foo/lib.bzl", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = { '_my_tool': attr.label(default = '//tool') },", + ")"); + scratch.file( + "tool/BUILD", + "sh_binary(name = 'tool', srcs = ['a.cc'], ", + " target_compatible_with = ['//platforms:local_value_a'])"); + scratch.file( + "foo/BUILD", + "load(':lib.bzl','my_rule')", + "my_rule(name = 'dep')", + "filegroup(name = 'target_with_dep', srcs = [':dep'])"); + + reporter.removeHandler(failFastHandler); + AnalysisResult result = updateExplicitTarget("//foo:target_with_dep"); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "Target //foo:target_with_dep is incompatible and cannot be built, but was" + + " explicitly requested"); + } + + @Test + public void targetCompatibleWith_mismatchesExecCompatibleWithinAspect() throws Exception { + // Regression test for a case where incompatibility happens in an aspect tool in a different + // package over a Starlark target with + // --incompatible_visibility_private_attributes_at_definition + // The tool is replaced with a fake target {@link + // IncommpatibleTargetChecker#createIncompatibleRuleConfiguredTarget) + // and it's verified if the tool is visible to the Starlark target the aspect is over. + scratch.file( + "platforms/BUILD", + "constraint_setting(name = 'local_setting')", + "constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')", + "constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')", + "platform(name = 'local_platform_a',", + " constraint_values = [':local_value_a'],", + ")", + "platform(name = 'local_platform_b',", + " constraint_values = [':local_value_b'],", + ")"); + + useConfiguration( + "--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b", + "--incompatible_visibility_private_attributes_at_definition"); + scratch.file( + "foo/lib.bzl", + "def _impl_aspect(ctx, target):", + " return []", + "my_aspect = aspect(", + " _impl_aspect,", + " attrs = { '_my_tool': attr.label(default = '//tool') },", + " exec_compatible_with = ['//platforms:local_value_a'],", + ")", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = { 'deps': attr.label_list(aspects = [my_aspect]) },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "tool/BUILD", + "sh_binary(name = 'tool', srcs = ['a.cc'], ", + " target_compatible_with = ['//platforms:local_value_b'])"); + scratch.file( + "foo/BUILD", + "load(':lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspect', deps = [':simple_dep'])"); + + reporter.removeHandler(failFastHandler); + AnalysisResult result = updateExplicitTarget("//foo:target_with_aspect"); + + // TODO(bazel-team): This should report an error similarly to {@code + // #targetCompatibleWith_mismatchesExecCompatibleDepInDifferentPackage} + assertThat(result.hasError()).isFalse(); + } + private void assertHasBaselineCoverageAction(String label, String progressMessage) throws InterruptedException { Action coverageAction = Iterables.getOnlyElement(getActions(label));