Fix a subtle bug involving action conflicts with extra actions, fix another silly bug, and clean up action conflict handling a fair amount:

1. Add an additional conflict-filtering step after top-level artifacts have been computed (as distinct from top-level configured targets and aspects). This will filter out such artifacts that depend on targets that have action conflicts, preventing those actions from executing.
2. Fix a silly bug where if there was an action conflict, we would filter out all top-level aspects from the set to build, even if they were error-free, because we were checking the wrong type of key.
3. Remove all action conflict checking from the execution phase. It is up to the analysis phase to filter out all targets that depend on such conflicts. Having this belt-and-suspenders style is error-prone.
4. Move action conflict checking into analysis-phase-proper objects. As part of that, move the flag --experimental_strict_conflict_checks to AnalysisOptions. Its prior location actually meant that its stricter action conflict checking would not start on the first build it was specified for: it would have to wait for the subsequent build. There's still a bug here, in that a build with no conflicts if the flag is off might not detect conflicts with the flag on until the server has to analyze a new configured target, but I'll leave it to felly@ to decide how important that is.

I considered unifying the conflict-filtering steps, but decided against it, because I didn't like the idea of computing extra action artifacts for targets that we wouldn't try to build in the end because they depended on action conflicts. Keeping the semantics that only error-free targets can have their extra-action artifacts built seemed like the right call, which means we have to filter twice. I don't expect any performance impact because the Skyframe nodes from the first evaluation will still be there, and this codepath is only hit on rare occasions anyway.

There is talk of deprecating extra actions. If they are ever removed, then potentially this second filtering step could also be removed. Left a TODO for cparsons@ around that.

PiperOrigin-RevId: 306296831
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 28040ba..2737c96 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
@@ -90,6 +90,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
@@ -409,7 +410,8 @@
               topLevelOptions,
               eventBus,
               keepGoing,
-              loadingPhaseThreads);
+              loadingPhaseThreads,
+              viewOptions.strictConflictChecks);
       setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
     } finally {
       skyframeBuildView.clearInvalidatedConfiguredTargets();
@@ -475,7 +477,6 @@
     Collection<Artifact> buildInfoArtifacts =
         skyframeExecutor.getWorkspaceStatusArtifacts(eventHandler);
     Preconditions.checkState(buildInfoArtifacts.size() == 2, buildInfoArtifacts);
-    buildInfoArtifacts.forEach(artifactsToOwnerLabelsBuilder::addArtifact);
 
     // Extra actions
     addExtraActionsIfRequested(
@@ -503,6 +504,20 @@
         actionsWrapper.getCoverageOutputs().forEach(artifactsToOwnerLabelsBuilder::addArtifact);
       }
     }
+    // TODO(cparsons): If extra actions are ever removed, this filtering step can probably be
+    //  removed as well: the only concern would be action conflicts involving coverage artifacts,
+    //  which seems far-fetched.
+    if (skyframeAnalysisResult.hasActionConflicts()) {
+      ArtifactsToOwnerLabels tempOwners = artifactsToOwnerLabelsBuilder.build();
+      // We don't remove the (hopefully unnecessary) guard in SkyframeBuildView that enables/
+      // disables analysis, since no new targets should actually be analyzed.
+      Set<Artifact> artifacts = tempOwners.getArtifacts();
+      Predicate<Artifact> errorFreeArtifacts =
+          skyframeExecutor.filterActionConflictsForTopLevelArtifacts(eventHandler, artifacts);
+      artifactsToOwnerLabelsBuilder = tempOwners.toBuilder().filterArtifacts(errorFreeArtifacts);
+    }
+    // Build-info artifacts are always conflict-free, and can't be checked easily.
+    buildInfoArtifacts.forEach(artifactsToOwnerLabelsBuilder::addArtifact);
 
     // Tests.
     Pair<ImmutableSet<ConfiguredTarget>, ImmutableSet<ConfiguredTarget>> testsPair =