Fail gracefully on conflicting actions generated by an aspect. These can come from Skylark, so we shouldn't crash. As a safety measure, subclasses of ActionLookupValue are now responsible for detecting action conflicts themselves.

PiperOrigin-RevId: 187095271

11 files changed, 64 insertions, 57 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
index e3081aaf81..d49d533350 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionFunction.java
@@ -73,14 +73,14 @@ public class ActionTemplateExpansionFunction implements SkyFunction {
       return null;
     }
     Iterable<TreeFileArtifact> inputTreeFileArtifacts = treeArtifactValue.getChildren();
-    Iterable<Action> expandedActions;
+    GeneratingActions generatingActions;
     try {
       // Expand the action template using the list of expanded input TreeFileArtifacts.
-      expandedActions = ImmutableList.<Action>copyOf(
-          actionTemplate.generateActionForInputArtifacts(inputTreeFileArtifacts, key));
       // TODO(rduan): Add a check to verify the inputs of expanded actions are subsets of inputs
       // of the ActionTemplate.
-      checkActionAndArtifactConflicts(expandedActions);
+      generatingActions =
+          checkActionAndArtifactConflicts(
+              actionTemplate.generateActionForInputArtifacts(inputTreeFileArtifacts, key));
     } catch (ActionConflictException e) {
       e.reportTo(env.getListener());
       throw new ActionTemplateExpansionFunctionException(e);
@@ -92,8 +92,7 @@ public class ActionTemplateExpansionFunction implements SkyFunction {
       throw new ActionTemplateExpansionFunctionException(e);
     }
 
-    return new ActionTemplateExpansionValue(
-        actionKeyContext, expandedActions, removeActionsAfterEvaluation.get());
+    return new ActionTemplateExpansionValue(generatingActions, removeActionsAfterEvaluation.get());
   }
 
   /** Exception thrown by {@link ActionTemplateExpansionFunction}. */
@@ -111,7 +110,7 @@ public class ActionTemplateExpansionFunction implements SkyFunction {
     }
   }
 
-  private void checkActionAndArtifactConflicts(Iterable<Action> actions)
+  private GeneratingActions checkActionAndArtifactConflicts(Iterable<? extends Action> actions)
       throws ActionConflictException, ArtifactPrefixConflictException {
     GeneratingActions generatingActions =
         Actions.findAndThrowActionConflict(actionKeyContext, ImmutableList.copyOf(actions));
@@ -123,6 +122,7 @@ public class ActionTemplateExpansionFunction implements SkyFunction {
     if (!artifactPrefixConflictMap.isEmpty()) {
       throw artifactPrefixConflictMap.values().iterator().next();
     }
+    return generatingActions;
   }
 
   @Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java
index 8ba11ee73c..0536e8200a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionTemplateExpansionValue.java
@@ -14,11 +14,9 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.MoreObjects;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.actions.Action;
-import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.BlazeInterners;
 import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
@@ -30,10 +28,8 @@ import com.google.devtools.build.skyframe.SkyFunctionName;
  */
 public final class ActionTemplateExpansionValue extends ActionLookupValue {
   ActionTemplateExpansionValue(
-      ActionKeyContext actionKeyContext,
-      Iterable<Action> expandedActions,
-      boolean removeActionsAfterEvaluation) {
-    super(actionKeyContext, ImmutableList.copyOf(expandedActions), removeActionsAfterEvaluation);
+      GeneratingActions generatingActions, boolean removeActionsAfterEvaluation) {
+    super(generatingActions, removeActionsAfterEvaluation);
   }
 
   static ActionTemplateExpansionKey key(ActionLookupKey actionLookupKey, int actionIndex) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 9d3f49cb26..0f3b752d4e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -19,7 +19,9 @@ import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.devtools.build.lib.actions.ActionKeyContext;
+import com.google.devtools.build.lib.actions.Actions;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.AliasProvider;
 import com.google.devtools.build.lib.analysis.AspectResolver;
 import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
@@ -274,7 +276,6 @@ public final class AspectFunction implements SkyFunction {
     if (baseConfiguredTargetValue.getConfiguredTarget().getProvider(AliasProvider.class) != null) {
       return createAliasAspect(
           env,
-          view.getActionKeyContext(),
           associatedConfiguredTargetAndTarget.getTarget(),
           aspect,
           key,
@@ -394,7 +395,6 @@ public final class AspectFunction implements SkyFunction {
 
       return createAspect(
           env,
-          view.getActionKeyContext(),
           key,
           aspectPath,
           aspect,
@@ -478,7 +478,6 @@ public final class AspectFunction implements SkyFunction {
 
   private SkyValue createAliasAspect(
       Environment env,
-      ActionKeyContext actionKeyContext,
       Target originalTarget,
       Aspect aspect,
       AspectKey originalKey,
@@ -513,8 +512,7 @@ public final class AspectFunction implements SkyFunction {
         originalTarget.getLabel(),
         originalTarget.getLocation(),
         ConfiguredAspect.forAlias(real.getConfiguredAspect()),
-        actionKeyContext,
-        ImmutableList.of(),
+        GeneratingActions.EMPTY,
         transitivePackagesForPackageRootResolution,
         removeActionsAfterEvaluation.get());
   }
@@ -522,7 +520,6 @@ public final class AspectFunction implements SkyFunction {
   @Nullable
   private AspectValue createAspect(
       Environment env,
-      ActionKeyContext actionKeyContext,
       AspectKey key,
       ImmutableList<Aspect> aspectPath,
       Aspect aspect,
@@ -587,14 +584,23 @@ public final class AspectFunction implements SkyFunction {
     analysisEnvironment.disable(associatedTarget.getTarget());
     Preconditions.checkNotNull(configuredAspect);
 
+    GeneratingActions generatingActions;
+    // Check for conflicting actions within this aspect (indicates a bug in the implementation).
+    try {
+      generatingActions =
+          Actions.filterSharedActionsAndThrowActionConflict(
+              analysisEnvironment.getActionKeyContext(),
+              analysisEnvironment.getRegisteredActions());
+    } catch (ActionConflictException e) {
+      throw new AspectFunctionException(e);
+    }
     return new AspectValue(
         key,
         aspect,
         associatedTarget.getTarget().getLabel(),
         associatedTarget.getTarget().getLocation(),
         configuredAspect,
-        actionKeyContext,
-        ImmutableList.copyOf(analysisEnvironment.getRegisteredActions()),
+        generatingActions,
         transitivePackagesForPackageRootResolution == null
             ? null
             : transitivePackagesForPackageRootResolution.build(),
@@ -661,7 +667,7 @@ public final class AspectFunction implements SkyFunction {
       super(e, Transience.PERSISTENT);
     }
 
-    public AspectFunctionException(InconsistentAspectOrderException cause) {
+    public AspectFunctionException(ActionConflictException cause) {
       super(cause, Transience.PERSISTENT);
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
index 0186315117..73ce2489c1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java
@@ -19,9 +19,8 @@ import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -39,7 +38,6 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.syntax.SkylarkImport;
 import com.google.devtools.build.skyframe.SkyFunctionName;
-import java.util.List;
 import javax.annotation.Nullable;
 
 /**
@@ -444,11 +442,10 @@ public final class AspectValue extends ActionLookupValue {
       Label label,
       Location location,
       ConfiguredAspect configuredAspect,
-      ActionKeyContext actionKeyContext,
-      List<ActionAnalysisMetadata> actions,
+      GeneratingActions actions,
       NestedSet<Package> transitivePackagesForPackageRootResolution,
       boolean removeActionsAfterEvaluation) {
-    super(actionKeyContext, actions, removeActionsAfterEvaluation);
+    super(actions, removeActionsAfterEvaluation);
     this.label = Preconditions.checkNotNull(label, actions);
     this.aspect = Preconditions.checkNotNull(aspect, label);
     this.location = Preconditions.checkNotNull(location, label);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
index e8230a6b15..0794901435 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionFunction.java
@@ -18,9 +18,13 @@ import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
+import com.google.devtools.build.lib.actions.Actions;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
+import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoCollection;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoContext;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoKey;
@@ -85,9 +89,7 @@ public class BuildInfoCollectionFunction implements SkyFunction {
                 : factory.getDerivedArtifact(rootRelativePath, root, keyAndConfig);
           }
         };
-
-    return new BuildInfoCollectionValue(
-        actionKeyContext,
+    BuildInfoCollection collection =
         buildInfoFactories
             .get(keyAndConfig.getInfoKey())
             .create(
@@ -96,8 +98,17 @@ public class BuildInfoCollectionFunction implements SkyFunction {
                     .getConfiguration(),
                 infoArtifactValue.getStableArtifact(),
                 infoArtifactValue.getVolatileArtifact(),
-                repositoryName),
-        removeActionsAfterEvaluation.get());
+                repositoryName);
+    GeneratingActions generatingActions;
+    try {
+      generatingActions =
+          Actions.filterSharedActionsAndThrowActionConflict(
+              actionKeyContext, collection.getActions());
+    } catch (ActionConflictException e) {
+      throw new IllegalStateException("Action conflicts not expected in build info: " + skyKey, e);
+    }
+    return new BuildInfoCollectionValue(
+        collection, generatingActions, removeActionsAfterEvaluation.get());
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java
index b96e94b32b..7d3fe847eb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildInfoCollectionValue.java
@@ -15,8 +15,8 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoCollection;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -35,10 +35,10 @@ public class BuildInfoCollectionValue extends ActionLookupValue {
   private final BuildInfoCollection collection;
 
   BuildInfoCollectionValue(
-      ActionKeyContext actionKeyContext,
       BuildInfoCollection collection,
+      GeneratingActions generatingActions,
       boolean removeActionsAfterEvaluation) {
-    super(actionKeyContext, collection.getActions(), removeActionsAfterEvaluation);
+    super(generatingActions, removeActionsAfterEvaluation);
     this.collection = collection;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java
index 3fbf2d9e2d..b1e11dc0f1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportFunction.java
@@ -20,7 +20,10 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
+import com.google.devtools.build.lib.actions.Actions;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
@@ -57,7 +60,14 @@ public class CoverageReportFunction implements SkyFunction {
       outputs.addAll(action.getOutputs());
     }
 
-    return new CoverageReportValue(actionKeyContext, actions, removeActionsAfterEvaluation.get());
+    GeneratingActions generatingActions;
+    try {
+      generatingActions =
+          Actions.filterSharedActionsAndThrowActionConflict(actionKeyContext, actions);
+    } catch (ActionConflictException e) {
+      throw new IllegalStateException("Action conflicts not expected in coverage: " + skyKey, e);
+    }
+    return new CoverageReportValue(generatingActions, removeActionsAfterEvaluation.get());
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java
index aa35490f10..edb9226a11 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/CoverageReportValue.java
@@ -14,10 +14,8 @@
 
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
+import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
 import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -30,11 +28,8 @@ public class CoverageReportValue extends ActionLookupValue {
   // There should only ever be one CoverageReportValue value in the graph.
   public static final CoverageReportKey COVERAGE_REPORT_KEY = CoverageReportKey.INSTANCE;
 
-  CoverageReportValue(
-      ActionKeyContext actionKeyContext,
-      ImmutableList<ActionAnalysisMetadata> coverageReportActions,
-      boolean removeActionsAfterEvaluation) {
-    super(actionKeyContext, coverageReportActions, removeActionsAfterEvaluation);
+  CoverageReportValue(GeneratingActions generatingActions, boolean removeActionsAfterEvaluation) {
+    super(generatingActions, removeActionsAfterEvaluation);
   }
 
   @AutoCodec(strategy = AutoCodec.Strategy.SINGLETON)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 0b65ba7e3f..38ad0814f9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -453,8 +453,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
             actionKeyContext, artifactFactory, buildInfoFactories, removeActionsAfterEvaluation));
     map.put(
         SkyFunctions.BUILD_INFO,
-        new WorkspaceStatusFunction(
-            actionKeyContext, removeActionsAfterEvaluation, this::makeWorkspaceStatusAction));
+        new WorkspaceStatusFunction(removeActionsAfterEvaluation, this::makeWorkspaceStatusAction));
     map.put(
         SkyFunctions.COVERAGE_REPORT,
         new CoverageReportFunction(actionKeyContext, removeActionsAfterEvaluation));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
index b231bae76a..68e6550c86 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
@@ -14,7 +14,6 @@
 package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
@@ -27,15 +26,12 @@ public class WorkspaceStatusFunction implements SkyFunction {
     WorkspaceStatusAction create(String workspaceName);
   }
 
-  private final ActionKeyContext actionKeyContext;
   private final Supplier<Boolean> removeActionAfterEvaluation;
   private final WorkspaceStatusActionFactory workspaceStatusActionFactory;
 
   WorkspaceStatusFunction(
-      ActionKeyContext actionKeyContext,
       Supplier<Boolean> removeActionAfterEvaluation,
       WorkspaceStatusActionFactory workspaceStatusActionFactory) {
-    this.actionKeyContext = actionKeyContext;
     this.removeActionAfterEvaluation = Preconditions.checkNotNull(removeActionAfterEvaluation);
     this.workspaceStatusActionFactory = workspaceStatusActionFactory;
   }
@@ -54,7 +50,6 @@ public class WorkspaceStatusFunction implements SkyFunction {
         workspaceStatusActionFactory.create(workspaceNameValue.getName());
 
     return new WorkspaceStatusValue(
-        actionKeyContext,
         action.getStableStatus(),
         action.getVolatileStatus(),
         action,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java
index a628fb653b..02030e45e9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusValue.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupValue;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
@@ -36,12 +35,11 @@ public class WorkspaceStatusValue extends ActionLookupValue {
   public static final BuildInfoKey BUILD_INFO_KEY = BuildInfoKey.INSTANCE;
 
   WorkspaceStatusValue(
-      ActionKeyContext actionKeyContext,
       Artifact stableArtifact,
       Artifact volatileArtifact,
       WorkspaceStatusAction action,
       boolean removeActionAfterEvaluation) {
-    super(actionKeyContext, action, removeActionAfterEvaluation);
+    super(action, removeActionAfterEvaluation);
     this.stableArtifact = stableArtifact;
     this.volatileArtifact = volatileArtifact;
   }