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

6 files changed, 97 insertions, 99 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 5cd90d2da5..1f21e65dc7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -33,7 +33,6 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.Root;
-import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider.ExtraArtifactSet;
 import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
@@ -682,30 +681,31 @@ public class BuildView {
       Collection<ConfiguredTarget> configuredTargets,
       Collection<AspectValue> aspects,
       Set<Artifact> artifactsToBuild) {
-    Iterable<ExtraArtifactSet> xaSets =
+    Iterable<Artifact> extraActionArtifacts =
         concat(
             addExtraActionsFromTargets(viewOptions, configuredTargets),
             addExtraActionsFromAspects(viewOptions, aspects));
 
     RegexFilter filter = viewOptions.extraActionFilter;
-    for (ExtraArtifactSet set : xaSets) {
-      boolean filterMatches = filter == null || filter.isIncluded(set.getLabel().toString());
+    for (Artifact artifact : extraActionArtifacts) {
+      boolean filterMatches =
+          filter == null || filter.isIncluded(artifact.getOwnerLabel().toString());
       if (filterMatches) {
-        artifactsToBuild.addAll(set.getArtifacts());
+        artifactsToBuild.add(artifact);
       }
     }
   }
 
-  private NestedSet<ExtraArtifactSet> addExtraActionsFromTargets(
+  private NestedSet<Artifact> addExtraActionsFromTargets(
       BuildView.Options viewOptions, Collection<ConfiguredTarget> configuredTargets) {
-    NestedSetBuilder<ExtraArtifactSet> builder = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
     for (ConfiguredTarget target : configuredTargets) {
       ExtraActionArtifactsProvider provider =
           target.getProvider(ExtraActionArtifactsProvider.class);
       if (provider != null) {
         if (viewOptions.extraActionTopLevelOnly) {
           if (!viewOptions.extraActionTopLevelOnlyWithAspects) {
-            builder.add(ExtraArtifactSet.of(target.getLabel(), provider.getExtraActionArtifacts()));
+            builder.addTransitive(provider.getExtraActionArtifacts());
           } else {
             // Collect all aspect-classes that topLevel might inject.
             Set<AspectClass> aspectClasses = new HashSet<>();
@@ -713,16 +713,10 @@ public class BuildView {
               aspectClasses.addAll(attr.getAspectClasses());
             }
 
-            Iterable<Artifact> artifacts;
-            if (aspectClasses.isEmpty()) {
-              artifacts = provider.getExtraActionArtifacts();
-            } else {
-              ImmutableList.Builder<Artifact> artifactBuilder = ImmutableList.builder();
-              artifactBuilder.addAll(provider.getExtraActionArtifacts());
-              artifactBuilder.addAll(filterTransitiveExtraActions(provider, aspectClasses));
-              artifacts = artifactBuilder.build();
+            builder.addTransitive(provider.getExtraActionArtifacts());
+            if (!aspectClasses.isEmpty()) {
+              builder.addAll(filterTransitiveExtraActions(provider, aspectClasses));
             }
-            builder.add(ExtraArtifactSet.of(target.getLabel(), artifacts));
           }
         } else {
           builder.addTransitive(provider.getTransitiveExtraActionArtifacts());
@@ -741,28 +735,26 @@ public class BuildView {
     ImmutableList.Builder<Artifact> artifacts = ImmutableList.builder();
     // Add to 'artifacts' all extra-actions which were registered by aspects which 'topLevel'
     // might have injected.
-    for (ExtraArtifactSet extraArtifactSet : provider.getTransitiveExtraActionArtifacts()) {
-      for (Artifact artifact : extraArtifactSet.getArtifacts()) {
-        ArtifactOwner owner = artifact.getArtifactOwner();
-        if (owner instanceof AspectKey) {
-          if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) {
-            artifacts.add(artifact);
-          }
+    for (Artifact artifact : provider.getTransitiveExtraActionArtifacts()) {
+      ArtifactOwner owner = artifact.getArtifactOwner();
+      if (owner instanceof AspectKey) {
+        if (aspectClasses.contains(((AspectKey) owner).getAspectClass())) {
+          artifacts.add(artifact);
         }
       }
     }
     return artifacts.build();
   }
 
-  private NestedSet<ExtraArtifactSet> addExtraActionsFromAspects(
+  private NestedSet<Artifact> addExtraActionsFromAspects(
       BuildView.Options viewOptions, Collection<AspectValue> aspects) {
-    NestedSetBuilder<ExtraArtifactSet> builder = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
     for (AspectValue aspect : aspects) {
       ExtraActionArtifactsProvider provider =
           aspect.getConfiguredAspect().getProvider(ExtraActionArtifactsProvider.class);
       if (provider != null) {
         if (viewOptions.extraActionTopLevelOnly) {
-          builder.add(ExtraArtifactSet.of(aspect.getLabel(), provider.getExtraActionArtifacts()));
+          builder.addTransitive(provider.getExtraActionArtifacts());
         } else {
           builder.addTransitive(provider.getTransitiveExtraActionArtifacts());
         }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionArtifactsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionArtifactsProvider.java
index 01b23d988f..3ea9a5a8de 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionArtifactsProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionArtifactsProvider.java
@@ -14,9 +14,7 @@
 
 package com.google.devtools.build.lib.analysis;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -30,48 +28,11 @@ public final class ExtraActionArtifactsProvider implements TransitiveInfoProvide
   public static final ExtraActionArtifactsProvider EMPTY =
       new ExtraActionArtifactsProvider(
           NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER),
-          NestedSetBuilder.<ExtraArtifactSet>emptySet(Order.STABLE_ORDER));
+          NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER));
 
-  /**
-   * The set of extra artifacts provided by a single configured target.
-   */
-  @Immutable
-  public static final class ExtraArtifactSet {
-    private final Label label;
-    private final ImmutableList<Artifact> artifacts;
-
-    private ExtraArtifactSet(Label label, Iterable<Artifact> artifacts) {
-      this.label = label;
-      this.artifacts = ImmutableList.copyOf(artifacts);
-    }
-
-    public Label getLabel() {
-      return label;
-    }
-
-    public ImmutableList<Artifact> getArtifacts() {
-      return artifacts;
-    }
-
-    public static ExtraArtifactSet of(Label label, Iterable<Artifact> artifacts) {
-      return new ExtraArtifactSet(label, artifacts);
-    }
-
-    @Override
-    public int hashCode() {
-      return label.hashCode();
-    }
-
-    @Override
-    public boolean equals(Object other) {
-      return other == this
-          || (other instanceof ExtraArtifactSet
-              && label.equals(((ExtraArtifactSet) other).getLabel()));
-    }
-  }
-
-  public static ExtraActionArtifactsProvider create(NestedSet<Artifact> extraActionArtifacts,
-      NestedSet<ExtraArtifactSet> transitiveExtraActionArtifacts) {
+  public static ExtraActionArtifactsProvider create(
+      NestedSet<Artifact> extraActionArtifacts,
+      NestedSet<Artifact> transitiveExtraActionArtifacts) {
     if (extraActionArtifacts.isEmpty() && transitiveExtraActionArtifacts.isEmpty()) {
       return EMPTY;
     }
@@ -81,8 +42,7 @@ public final class ExtraActionArtifactsProvider implements TransitiveInfoProvide
   public static ExtraActionArtifactsProvider merge(
       Iterable<ExtraActionArtifactsProvider> providers) {
     NestedSetBuilder<Artifact> artifacts = NestedSetBuilder.stableOrder();
-    NestedSetBuilder<ExtraArtifactSet> transitiveExtraActionArtifacts =
-        NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> transitiveExtraActionArtifacts = NestedSetBuilder.stableOrder();
 
     for (ExtraActionArtifactsProvider provider : providers) {
       artifacts.addTransitive(provider.getExtraActionArtifacts());
@@ -94,13 +54,12 @@ public final class ExtraActionArtifactsProvider implements TransitiveInfoProvide
 
   /** The outputs of the extra actions associated with this target. */
   private final NestedSet<Artifact> extraActionArtifacts;
-  private final NestedSet<ExtraArtifactSet> transitiveExtraActionArtifacts;
+  private final NestedSet<Artifact> transitiveExtraActionArtifacts;
 
-  /**
-   * Use {@link #create} instead.
-   */
-  private ExtraActionArtifactsProvider(NestedSet<Artifact> extraActionArtifacts,
-      NestedSet<ExtraArtifactSet> transitiveExtraActionArtifacts) {
+  /** Use {@link #create} instead. */
+  private ExtraActionArtifactsProvider(
+      NestedSet<Artifact> extraActionArtifacts,
+      NestedSet<Artifact> transitiveExtraActionArtifacts) {
     this.extraActionArtifacts = extraActionArtifacts;
     this.transitiveExtraActionArtifacts = transitiveExtraActionArtifacts;
   }
@@ -112,10 +71,8 @@ public final class ExtraActionArtifactsProvider implements TransitiveInfoProvide
     return extraActionArtifacts;
   }
 
-  /**
-   * The outputs of the extra actions in the whole transitive closure.
-   */
-  public NestedSet<ExtraArtifactSet> getTransitiveExtraActionArtifacts() {
+  /** The outputs of the extra actions in the whole transitive closure. */
+  public NestedSet<Artifact> getTransitiveExtraActionArtifacts() {
     return transitiveExtraActionArtifacts;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java
index 782eaa2ef1..99fd04cda7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ExtraActionUtils.java
@@ -18,14 +18,12 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider.ExtraArtifactSet;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.rules.extra.ExtraActionMapProvider;
 import com.google.devtools.build.lib.rules.extra.ExtraActionSpec;
-
 import java.util.List;
 import java.util.Set;
 
@@ -51,7 +49,7 @@ class ExtraActionUtils {
     }
 
     ImmutableList<Artifact> extraActionArtifacts = ImmutableList.of();
-    NestedSetBuilder<ExtraArtifactSet> builder = NestedSetBuilder.stableOrder();
+    NestedSetBuilder<Artifact> builder = NestedSetBuilder.stableOrder();
 
     List<Label> actionListenerLabels = configuration.getActionListeners();
     if (!actionListenerLabels.isEmpty()
@@ -70,7 +68,7 @@ class ExtraActionUtils {
 
       extraActionArtifacts = visitor.getAndResetExtraArtifacts();
       if (!extraActionArtifacts.isEmpty()) {
-        builder.add(ExtraArtifactSet.of(ruleContext.getLabel(), extraActionArtifacts));
+        builder.addAll(extraActionArtifacts);
       }
     }
 
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();