Extra-actions originating in Aspects are reported even if the rule we attach to also registers extra-actions.

ExtraActionArtifactsProvider was using a set of ExtraArtifactSet, whose key was derived from the label of the owner of the extra-action.
Since actions registered by Aspects have the same label as those registered by the base rule, the former ones would disappear from the set.

An alternative to this CL would be to use an ArtifactOwner instead of a label as the key of the ExtraArtifactSet, but
1. BuildView only has access to ConfiguredTarget's, which lack the information to manipulate ArtifactOwner's; and
2. I don't see what ExtraArtifactSet gains us. In particular, ExtraArtifactSet.getLabel() is not used by anything outside of ExtraArtifactSet.

The only disadvantage of this CL is that filtering (--experimental_extra_action_filter) can be slower if targets register a massive number of actions.
Before this CL, we would match a single label per rule, while after this CL we match a single label per action.

--
MOS_MIGRATED_REVID=139591862

3 files changed, 64 insertions, 13 deletions

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 e3664cce0e..af00a7ee76 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
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
-import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.analysis.BaseRuleClasses.ACTION_LISTENER;
 import static com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode.TARGET;
@@ -424,10 +423,11 @@ public class AspectTest extends AnalysisTestCase {
     update();
 
     ConfiguredTarget a = getConfiguredTarget("//a:a");
-    NestedSet<ExtraActionArtifactsProvider.ExtraArtifactSet> extraActionArtifacts =
-        a.getProvider(ExtraActionArtifactsProvider.class)
-            .getTransitiveExtraActionArtifacts();
-    assertThat(getOnlyElement(extraActionArtifacts).getLabel()).isEqualTo(Label.create("@//a", "b"));
+    NestedSet<Artifact> extraActionArtifacts =
+        a.getProvider(ExtraActionArtifactsProvider.class).getTransitiveExtraActionArtifacts();
+    for (Artifact artifact : extraActionArtifacts) {
+      assertThat(artifact.getOwnerLabel()).isEqualTo(Label.create("@//a", "b"));
+    }
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index b90e3d1929..a73032f907 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -60,8 +60,10 @@ import com.google.devtools.build.skyframe.NotifyingHelper.Listener;
 import com.google.devtools.build.skyframe.NotifyingHelper.Order;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.TrackingAwaiter;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.regex.Pattern;
@@ -1231,6 +1233,56 @@ public class BuildViewTest extends BuildViewTestBase {
         .containsExactly("one", "two");
   }
 
+  /**
+   * Here, injecting_rule injects an aspect which acts on a action_rule() and registers an action.
+   * The action_rule() registers another action of its own.
+   *
+   * <p>This test asserts that both actions are reported.
+   */
+  @Test
+  public void ruleExtraActionsDontHideAspectExtraActions() throws Exception {
+    useConfiguration("--experimental_action_listener=//pkg:listener");
+
+    scratch.file(
+        "x/BUILD",
+        "load(':extension.bzl', 'injecting_rule', 'action_rule')",
+        "injecting_rule(name='a', deps=[':b'])",
+        "action_rule(name='b')");
+
+    scratch.file(
+        "x/extension.bzl",
+        "def _aspect1_impl(target, ctx):",
+        "  ctx.empty_action(mnemonic='Mnemonic')",
+        "  return struct()",
+        "aspect1 = aspect(_aspect1_impl, attr_aspects=['deps'])",
+        "",
+        "def _injecting_rule_impl(ctx):",
+        "  return struct()",
+        "injecting_rule = rule(_injecting_rule_impl, ",
+        "    attrs = { 'deps' : attr.label_list(aspects = [aspect1]) })",
+        "",
+        "def _action_rule_impl(ctx):",
+        "  out = ctx.new_file(ctx.label.name)",
+        "  ctx.action(outputs = [out], command = 'dontcare', mnemonic='Mnemonic')",
+        "  return struct()",
+        "action_rule = rule(_action_rule_impl, attrs = { 'deps' : attr.label_list() })");
+
+    scratch.file(
+        "pkg/BUILD",
+        "extra_action(name='xa', cmd='echo dont-care')",
+        "action_listener(name='listener', mnemonics=['Mnemonic'], extra_actions=[':xa'])");
+
+    BuildView.AnalysisResult analysisResult = update("//x:a");
+
+    List<String> owners = new ArrayList<>();
+    for (Artifact artifact : analysisResult.getAdditionalArtifactsToBuild()) {
+      if ("xa".equals(artifact.getExtension())) {
+        owners.add(artifact.getOwnerLabel().toString());
+      }
+    }
+    assertThat(owners).containsExactly("//x:b", "//x:b");
+  }
+
   /** Runs the same test with the reduced loading phase. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 33ee6f0ca3..62889d5f54 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -57,7 +57,6 @@ import com.google.devtools.build.lib.analysis.ConfiguredAttributeMapper;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider;
-import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider.ExtraArtifactSet;
 import com.google.devtools.build.lib.analysis.FileConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
@@ -1410,13 +1409,13 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
    */
   protected ImmutableList<ExtraAction> getTransitiveExtraActionActions(ConfiguredTarget target) {
     ImmutableList.Builder<ExtraAction> result = new ImmutableList.Builder<>();
-    for (ExtraArtifactSet set : target.getProvider(ExtraActionArtifactsProvider.class)
-        .getTransitiveExtraActionArtifacts()) {
-      for (Artifact artifact : set.getArtifacts()) {
-        Action action = getGeneratingAction(artifact);
-        if (action instanceof ExtraAction) {
-          result.add((ExtraAction) action);
-        }
+    for (Artifact artifact :
+        target
+            .getProvider(ExtraActionArtifactsProvider.class)
+            .getTransitiveExtraActionArtifacts()) {
+      Action action = getGeneratingAction(artifact);
+      if (action instanceof ExtraAction) {
+        result.add((ExtraAction) action);
       }
     }
     return result.build();