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

site/docs/skylark/aspects.md

@@ -323,10 +323,12 @@ that come from the implementation of a rule for target X and from the
 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

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 @@ public static ConfiguredTarget of(ConfiguredTarget base, Iterable<ConfiguredAspe
           }
           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));

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 {

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(

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 @@ public void topLevelConflictDetected() throws Exception {
             "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);
+  }
 }

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

@@ -103,6 +103,7 @@ java_library(
         "//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",