SkyframeBuildView: move action conflict checking

Bazel will now delay checking for action conflicts
until it has collected "good" Aspects and CTs and
has created the PackageRoots, because doing so
will let us move the collection and creation logic
into a SkyFunction and so reduce code outside of
Skyframe between Analysis and Execution, and
thereby allow us to maybe merge the two phases
some time in the future.

I believe the collection and creation logic does
not actually depend on the result of action
conflict checking, making this change safe.

I believe this because the collection logic does
not actually use the 'badActions' map, and while
it looks up SkyValues from the EvaluationResult,
those are supposed to be immutable, so the
conflict checker could not be storing anything in
them, therefore the collection logic cannot
possibly be filtering CTs with bad actions.

Therefore "good" in the variable names "goodCts"
and "goodAspects" is misleading, and also the
comment that claimed the "goodCts" collection
logic was filtering out CTs with bad actions was
wrong, so I renamed these variables and I removed
the comment.

I also fixed a minor bug: the 'aspects' list
(previously: 'goodAspects') was initialized with
an expected size of 'ctKeys.size()', now it is
initialized with the 'aspectKeys.size()'.

PiperOrigin-RevId: 288687661

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java

@@ -368,7 +368,7 @@ public void clearAnalysisCache(
    */
   public SkyframeAnalysisResult configureTargets(
       ExtendedEventHandler eventHandler,
-      List<ConfiguredTargetKey> values,
+      List<ConfiguredTargetKey> ctKeys,
       List<AspectValueKey> aspectKeys,
       Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier,
       EventBus eventBus,
@@ -380,35 +380,12 @@ public SkyframeAnalysisResult configureTargets(
     try (SilentCloseable c = Profiler.instance().profile("skyframeExecutor.configureTargets")) {
       result =
           skyframeExecutor.configureTargets(
-              eventHandler, values, aspectKeys, keepGoing, numThreads);
+              eventHandler, ctKeys, aspectKeys, keepGoing, numThreads);
     } finally {
       enableAnalysis(false);
     }
-    try (SilentCloseable c =
-        Profiler.instance().profile("skyframeExecutor.findArtifactConflicts")) {
-      ImmutableSet<SkyKey> newKeys =
-          ImmutableSet.<SkyKey>builderWithExpectedSize(values.size() + aspectKeys.size())
-              .addAll(values)
-              .addAll(aspectKeys)
-              .build();
-      if (someConfiguredTargetEvaluated
-          || anyConfiguredTargetDeleted
-          || !dirtiedConfiguredTargetKeys.isEmpty()
-          || !largestTopLevelKeySetCheckedForConflicts.containsAll(newKeys)
-          || !skyframeActionExecutor.badActions().isEmpty()) {
-        largestTopLevelKeySetCheckedForConflicts = newKeys;
-        // This operation is somewhat expensive, so we only do it if the graph might have changed in
-        // some way -- either we analyzed a new target or we invalidated an old one or are building
-        // targets together that haven't been built before.
-        skyframeActionExecutor.findAndStoreArtifactConflicts(
-            skyframeExecutor.getActionLookupValuesInBuild(values, aspectKeys));
-        someConfiguredTargetEvaluated = false;
-      }
-    }
-    ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions =
-        skyframeActionExecutor.badActions();
 
-    Collection<AspectValue> goodAspects = Lists.newArrayListWithCapacity(values.size());
+    Collection<AspectValue> aspects = Lists.newArrayListWithCapacity(aspectKeys.size());
     Root singleSourceRoot = skyframeExecutor.getForcedSingleSourceRootIfNoExecrootSymlinkCreation();
     NestedSetBuilder<Package> packages =
         singleSourceRoot == null ? NestedSetBuilder.stableOrder() : null;
@@ -418,22 +395,19 @@ public SkyframeAnalysisResult configureTargets(
         // Skip aspects that couldn't be applied to targets.
         continue;
       }
-      goodAspects.add(value);
+      aspects.add(value);
       if (packages != null) {
         packages.addTransitive(value.getTransitivePackagesForPackageRootResolution());
       }
     }
 
-    // Filter out all CTs that have a bad action and convert to a list of configured targets. This
-    // code ensures that the resulting list of configured targets has the same order as the incoming
-    // list of values, i.e., that the order is deterministic.
-    Collection<ConfiguredTarget> goodCts = Lists.newArrayListWithCapacity(values.size());
-    for (ConfiguredTargetKey value : values) {
+    Collection<ConfiguredTarget> cts = Lists.newArrayListWithCapacity(ctKeys.size());
+    for (ConfiguredTargetKey value : ctKeys) {
       ConfiguredTargetValue ctValue = (ConfiguredTargetValue) result.get(value);
       if (ctValue == null) {
         continue;
       }
-      goodCts.add(ctValue.getConfiguredTarget());
+      cts.add(ctValue.getConfiguredTarget());
       if (packages != null) {
         packages.addTransitive(ctValue.getTransitivePackagesForPackageRootResolution());
       }
@@ -443,12 +417,36 @@ public SkyframeAnalysisResult configureTargets(
             ? new MapAsPackageRoots(collectPackageRoots(packages.build().toList()))
             : new PackageRootsNoSymlinkCreation(singleSourceRoot);
 
+    try (SilentCloseable c =
+        Profiler.instance().profile("skyframeExecutor.findArtifactConflicts")) {
+      ImmutableSet<SkyKey> newKeys =
+          ImmutableSet.<SkyKey>builderWithExpectedSize(ctKeys.size() + aspectKeys.size())
+              .addAll(ctKeys)
+              .addAll(aspectKeys)
+              .build();
+      if (someConfiguredTargetEvaluated
+          || anyConfiguredTargetDeleted
+          || !dirtiedConfiguredTargetKeys.isEmpty()
+          || !largestTopLevelKeySetCheckedForConflicts.containsAll(newKeys)
+          || !skyframeActionExecutor.badActions().isEmpty()) {
+        largestTopLevelKeySetCheckedForConflicts = newKeys;
+        // This operation is somewhat expensive, so we only do it if the graph might have changed in
+        // some way -- either we analyzed a new target or we invalidated an old one or are building
+        // targets together that haven't been built before.
+        skyframeActionExecutor.findAndStoreArtifactConflicts(
+            skyframeExecutor.getActionLookupValuesInBuild(ctKeys, aspectKeys));
+        someConfiguredTargetEvaluated = false;
+      }
+    }
+    ImmutableMap<ActionAnalysisMetadata, ConflictException> badActions =
+        skyframeActionExecutor.badActions();
     if (!result.hasError() && badActions.isEmpty()) {
       return new SkyframeAnalysisResult(
-          /*hasLoadingError=*/false, /*hasAnalysisError=*/false,
-          ImmutableList.copyOf(goodCts),
+          /*hasLoadingError=*/ false,
+          /*hasAnalysisError=*/ false,
+          ImmutableList.copyOf(cts),
           result.getWalkableGraph(),
-          ImmutableList.copyOf(goodAspects),
+          ImmutableList.copyOf(aspects),
           packageRoots);
     }
 
@@ -496,24 +494,24 @@ public SkyframeAnalysisResult configureTargets(
       // In order to determine the set of configured targets transitively error free from action
       // conflict issues, we run a post-processing update() that uses the bad action map.
       EvaluationResult<PostConfiguredTargetValue> actionConflictResult =
-          skyframeExecutor.postConfigureTargets(eventHandler, values, keepGoing, badActions);
+          skyframeExecutor.postConfigureTargets(eventHandler, ctKeys, keepGoing, badActions);
 
-      goodCts = Lists.newArrayListWithCapacity(values.size());
-      for (ConfiguredTargetKey value : values) {
+      cts = Lists.newArrayListWithCapacity(ctKeys.size());
+      for (ConfiguredTargetKey value : ctKeys) {
         PostConfiguredTargetValue postCt =
             actionConflictResult.get(PostConfiguredTargetValue.key(value));
         if (postCt != null) {
-          goodCts.add(postCt.getCt());
+          cts.add(postCt.getCt());
         }
       }
     }
 
     return new SkyframeAnalysisResult(
         errors.first,
         result.hasError() || !badActions.isEmpty(),
-        ImmutableList.copyOf(goodCts),
+        ImmutableList.copyOf(cts),
         result.getWalkableGraph(),
-        ImmutableList.copyOf(goodAspects),
+        ImmutableList.copyOf(aspects),
         packageRoots);
   }