Ignore InstrumentedFilesInfo from base rule when also returned by aspect
Currently, aspects should not return InstrumentedFilesInfo, since that may be returned by any rule target. Doing so introduces an inadvertent brittle assumption that the rule targets visited by an aspect do not provide InstrumentedFilesInfo.
For example, if foo_library and foo_proto_library share an implementation so that foo_proto_library (which traverses proto_library targets) carelessly returns InstrumentedFilesInfo, this doesn't currently influence coverage behavior. But it will break as soon as proto_library starts returning InstrumentedFilesInfo.
This brittleness will come into play when the default behavior for coverage is changed from "forward nothing" to "forward from all non-tool dependencies" (currently conditioned on the flag --experimental_forward_instrumented_files_info_by_default).
Instead, ignore the InstrumentedFilesInfo from the base rule target if it's returned by an aspect.
RELNOTES: None.
PiperOrigin-RevId: 379467851
diff --git a/site/docs/skylark/aspects.md b/site/docs/skylark/aspects.md
index ab539d5..533c6f1 100644
--- a/site/docs/skylark/aspects.md
+++ b/site/docs/skylark/aspects.md
@@ -323,10 +323,12 @@
implementation of aspect A. The providers that a rule implementation propagates
are created and frozen before aspects are applied and cannot be modified from an
aspect. It is an error if a target and an aspect that is applied to it each
-provide a provider with the same type, so aspects should not return
-[`DefaultInfo`](lib/DefaultInfo.html),
-[`InstrumentedFilesInfo`](lib/InstrumentedFilesInfo.html), or any other
-provider that might be returned by the underlying rule target.
+provide a provider with the same type, with the exceptions of
+[`OutputGroupInfo`](lib/OutputGroupInfo.html) (which is merged, so long as the
+rule and aspect specify different output groups) and
+[`InstrumentedFilesInfo`](lib/InstrumentedFilesInfo.html) (which is taken from
+the aspect). This means that aspect implementations may never return
+[`DefaultInfo`](lib/DefaultInfo.html).
The parameters and private attributes are passed in the attributes of the
``ctx``. This example references the ``extension`` parameter and determines
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 9967904..506530f 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
@@ -29,11 +29,11 @@
import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder;
import com.google.devtools.build.lib.analysis.test.AnalysisFailure;
import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo;
+import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.Provider;
-import com.google.devtools.build.lib.packages.Provider.Key;
import com.google.devtools.build.lib.starlarkbuildapi.ActionApi;
import java.util.ArrayList;
import java.util.List;
@@ -192,8 +192,15 @@
}
nonBaseProviders.put(legacyId, providers.getProviderInstanceAt(i));
} else if (providerKey instanceof Provider.Key) {
- Provider.Key key = (Key) providerKey;
- if (base.get(key) != null || nonBaseProviders.contains(key)) {
+ Provider.Key key = (Provider.Key) providerKey;
+ // If InstrumentedFilesInfo is on both the base target and an aspect, ignore the one from
+ // the base. Otherwise, sharing implementation between a rule which returns
+ // InstrumentedFilesInfo (e.g. *_library) and a related aspect (e.g. *_proto_library) can
+ // add an implicit brittle assumption that the underlying rule (e.g. proto_library) does
+ // not return InstrumentedFilesInfo.
+ if ((!InstrumentedFilesInfo.STARLARK_CONSTRUCTOR.getKey().equals(key)
+ && base.get(key) != null)
+ || nonBaseProviders.contains(key)) {
throw new DuplicateException("Provider " + key + " provided twice");
}
nonBaseProviders.put((Info) providers.getProviderInstanceAt(i));
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/AnalysisFailureInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/AnalysisFailureInfoApi.java
index a467557..faac069 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/AnalysisFailureInfoApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/AnalysisFailureInfoApi.java
@@ -40,7 +40,8 @@
+ " object describing the failure.</li> <li>If one or more of a target's dependencies"
+ " propagated <code>AnalysisFailureInfo</code>, then propagate a provider with"
+ " <code>causes</code> equal to the union of the <code>causes</code> of the "
- + "dependencies.</li></ul>",
+ + "dependencies.</li></ul> If an aspect and the rule target that it is applied to both "
+ + "provide AnalysisFailureInfo, the instances are merged.",
documented = false)
public interface AnalysisFailureInfoApi<AnalysisFailureApiT extends AnalysisFailureApi>
extends StarlarkValue {
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/InstrumentedFilesInfoApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/InstrumentedFilesInfoApi.java
index e9a5357..116c4d0 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/InstrumentedFilesInfoApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/InstrumentedFilesInfoApi.java
@@ -35,7 +35,9 @@
+ "<code>metadata_files</code></a> are passed to the test action as inputs, with the "
+ "manifest's path noted in the environment variable <code>COVERAGE_MANIFEST</code>. "
+ "The metadata files, but not the source files, are also passed to the test action "
- + "as inputs.")
+ + "as inputs. When <code>InstrumentedFilesInfo</code> is returned by an "
+ + "<a href=\"../aspects.html\">aspect</a>'s implementation function, any "
+ + "<code>InstrumentedFilesInfo</code> from the base rule target is ignored.")
public interface InstrumentedFilesInfoApi extends StructApi {
@StarlarkMethod(
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 a1f9d2a..ea8376e 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
@@ -25,8 +25,10 @@
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil.NullAction;
import com.google.devtools.build.lib.analysis.config.HostTransition;
+import com.google.devtools.build.lib.analysis.test.InstrumentedFilesInfo;
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
import com.google.devtools.build.lib.analysis.util.MockRule;
import com.google.devtools.build.lib.analysis.util.TestAspects;
@@ -987,4 +989,146 @@
"ConflictException: for foo/aspect.out, previous action: action 'Action for aspect .',"
+ " attempted action: action 'Action for aspect .'");
}
+
+ @Test
+ public void aspectDuplicatesRuleProviderError() throws Exception {
+ setRulesAndAspectsAvailableInTests(ImmutableList.of(), ImmutableList.of());
+ scratch.file(
+ "aspect/build_defs.bzl",
+ "def _aspect_impl(target, ctx):",
+ " return [DefaultInfo()]",
+ "",
+ "returns_default_info_aspect = aspect(implementation = _aspect_impl)",
+ "",
+ "def _rule_impl(ctx):",
+ " pass",
+ "",
+ "duplicate_provider_aspect_applying_rule = rule(",
+ " implementation = _rule_impl,",
+ " attrs = {'to': attr.label(aspects = [returns_default_info_aspect])},",
+ ")");
+ scratch.file(
+ "aspect/BUILD",
+ "load('build_defs.bzl', 'duplicate_provider_aspect_applying_rule')",
+ "cc_library(name = 'rule_target')",
+ "duplicate_provider_aspect_applying_rule(name='applies_aspect', to=':rule_target')");
+ assertThat(
+ assertThrows(
+ AssertionError.class, () -> getConfiguredTarget("//aspect:applies_aspect")))
+ .hasMessageThat()
+ .contains("Provider DefaultInfo provided twice");
+ }
+
+ @Test
+ public void instrumentedFilesInfoFromBaseRuleAndAspectUsesAspect() throws Exception {
+ scratch.file(
+ "aspect/build_defs.bzl",
+ "def _instrumented_files_info_aspect_impl(target, ctx):",
+ " return [coverage_common.instrumented_files_info(ctx, source_attributes=['a'])]",
+ "",
+ "instrumented_files_info_aspect = aspect(",
+ " implementation = _instrumented_files_info_aspect_impl,",
+ ")",
+ "",
+ "def _no_instrumented_files_info_aspect_impl(target, ctx):",
+ " return []",
+ "",
+ "no_instrumented_files_info_aspect = aspect(",
+ " implementation = _no_instrumented_files_info_aspect_impl,",
+ ")",
+ "",
+ "def _applies_aspect_impl(ctx):",
+ " return coverage_common.instrumented_files_info(ctx, dependency_attributes=['to'])",
+ "",
+ "instrumented_files_info_aspect_rule = rule(",
+ " implementation = _applies_aspect_impl,",
+ " attrs = {'to': attr.label(aspects = [instrumented_files_info_aspect])},",
+ ")",
+ "",
+ "no_instrumented_files_info_aspect_rule = rule(",
+ " implementation = _applies_aspect_impl,",
+ " attrs = {'to': attr.label(aspects = [no_instrumented_files_info_aspect])},",
+ ")",
+ "",
+ "def _base_rule_impl(ctx):",
+ " return [coverage_common.instrumented_files_info(ctx, source_attributes=['b'])]",
+ "",
+ "base_rule = rule(",
+ " implementation = _base_rule_impl,",
+ " attrs = {'a': attr.label(allow_files=True), 'b': attr.label(allow_files=True)},",
+ ")",
+ "",
+ "def _base_rule_no_coverage_impl(ctx):",
+ " return []",
+ "",
+ "base_rule_no_coverage = rule(",
+ " implementation = _base_rule_no_coverage_impl,",
+ " attrs = {'a': attr.label(allow_files=True), 'b': attr.label(allow_files=True)},",
+ ")");
+ scratch.file(
+ "aspect/BUILD",
+ "load(",
+ " 'build_defs.bzl',",
+ " 'base_rule',",
+ " 'base_rule_no_coverage',",
+ " 'instrumented_files_info_aspect_rule',",
+ " 'no_instrumented_files_info_aspect_rule',",
+ ")",
+ "",
+ "base_rule(",
+ " name = 'rule_target',",
+ // Ends up in coverage sources when instrumented_files_info_aspect is applied
+ " a = 'a',",
+ // Ends up in coverage sources for the base rule's InstrumentedFilesInfo is used
+ " b = 'b',",
+ ")",
+ "",
+ "instrumented_files_info_aspect_rule(",
+ " name='duplicate_instrumented_file_info',",
+ " to=':rule_target',",
+ ")",
+ "",
+ "no_instrumented_files_info_aspect_rule(",
+ " name='instrumented_file_info_from_base_target',",
+ " to=':rule_target',",
+ ")",
+ "",
+ "base_rule_no_coverage(",
+ " name = 'rule_target_no_coverage',",
+ // Ends up in coverage sources when instrumented_files_info_aspect is applied
+ " a = 'a',",
+ // Ends up in coverage sources never
+ " b = 'b',",
+ ")",
+ "",
+ "instrumented_files_info_aspect_rule(",
+ " name='instrumented_files_info_only_from_aspect',",
+ " to=':rule_target_no_coverage',",
+ ")",
+ "",
+ "no_instrumented_files_info_aspect_rule(",
+ " name='no_instrumented_files_info',",
+ " to=':rule_target_no_coverage',",
+ ")");
+ useConfiguration("--collect_code_coverage", "--instrumentation_filter=.*");
+ update();
+ assertThat(getInstrumentedFiles("//aspect:rule_target")).containsExactly("b");
+ assertThat(getInstrumentedFiles("//aspect:duplicate_instrumented_file_info"))
+ .containsExactly("a");
+ assertThat(getInstrumentedFiles("//aspect:instrumented_file_info_from_base_target"))
+ .containsExactly("b");
+ assertThat(getInstrumentedFilesInfo("//aspect:rule_target_no_coverage")).isNull();
+ assertThat(getInstrumentedFiles("//aspect:instrumented_files_info_only_from_aspect"))
+ .containsExactly("a");
+ assertThat(getInstrumentedFiles("//aspect:no_instrumented_files_info")).isEmpty();
+ }
+
+ private List<String> getInstrumentedFiles(String label) throws InterruptedException {
+ return ActionsTestUtil.baseArtifactNames(
+ getInstrumentedFilesInfo(label).getInstrumentedFiles());
+ }
+
+ private InstrumentedFilesInfo getInstrumentedFilesInfo(String label) throws InterruptedException {
+ return getConfiguredTarget(label).get(InstrumentedFilesInfo.STARLARK_CONSTRUCTOR);
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index 1d3107b..353c813 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -103,6 +103,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:starlark/starlark_custom_command_line",
"//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure",
"//src/main/java/com/google/devtools/build/lib/analysis:test/analysis_failure_info",
+ "//src/main/java/com/google/devtools/build/lib/analysis:test/instrumented_files_info",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:test/test_trimming_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
