Fix non-determinism in the FailureDetail produced for a package with multiple label crosses subpackage boundary errors.

While I'm here, also make the code a tiny bit simpler:
* Explicitly create the empty inner `ArrayList` on each loop iteration. There's no need to use `#computeIfAbsent`; we know no other loop iteration will be processing the same `Target`.
* Use a `List<Pair<Target, List<PackageLookupValue.Key>>>` instead of a `Map<Target, List<PackageLookupValue.Key>>` since we don't need lookup semantics.

PiperOrigin-RevId: 533585562
Change-Id: Iaa3c3d9302a0eef533b55d1f17c33de3ec23666b

Author: haxorz

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

@@ -805,14 +805,16 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
     PathFragment pkgDir = pkgId.getPackageFragment();
     // Contains a key for each package whose label that might have a presence of a subpackage.
     // Values are all potential subpackages of the label.
-    Map<Target, List<PackageLookupValue.Key>> targetSubpackagePackageLookupKeyMap = new HashMap<>();
+    List<Pair<Target, List<PackageLookupValue.Key>>> targetsAndSubpackagePackageLookupKeys =
+        new ArrayList<>();
     Set<PackageLookupValue.Key> allPackageLookupKeys = new HashSet<>();
     for (Target target : pkgBuilder.getTargets()) {
       Label label = target.getLabel();
       PathFragment dir = Label.getContainingDirectory(label);
       if (dir.equals(pkgDir)) {
         continue;
       }
+      List<PackageLookupValue.Key> subpackagePackageLookupKeys = new ArrayList<>();
       String labelName = label.getName();
       PathFragment labelAsRelativePath = PathFragment.create(labelName).getParentDirectory();
       PathFragment subpackagePath = pkgDir;
@@ -821,14 +823,13 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
         subpackagePath = subpackagePath.getRelative(segment);
         PackageLookupValue.Key currentPackageLookupKey =
             PackageLookupValue.key(PackageIdentifier.create(pkgId.getRepository(), subpackagePath));
-        targetSubpackagePackageLookupKeyMap
-            .computeIfAbsent(target, t -> new ArrayList<>())
-            .add(currentPackageLookupKey);
+        subpackagePackageLookupKeys.add(currentPackageLookupKey);
         allPackageLookupKeys.add(currentPackageLookupKey);
       }
+      targetsAndSubpackagePackageLookupKeys.add(Pair.of(target, subpackagePackageLookupKeys));
     }
 
-    if (targetSubpackagePackageLookupKeyMap.isEmpty()) {
+    if (targetsAndSubpackagePackageLookupKeys.isEmpty()) {
       return;
     }
 
@@ -837,9 +838,11 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
       return;
     }
 
-    for (Map.Entry<Target, List<PackageLookupValue.Key>> entry :
-        targetSubpackagePackageLookupKeyMap.entrySet()) {
-      List<PackageLookupValue.Key> targetPackageLookupKeys = entry.getValue();
+    for (Pair<Target, List<PackageLookupValue.Key>> targetAndSubpackagePackageLookupKeys :
+        targetsAndSubpackagePackageLookupKeys) {
+      Target target = targetAndSubpackagePackageLookupKeys.getFirst();
+      List<PackageLookupValue.Key> targetPackageLookupKeys =
+          targetAndSubpackagePackageLookupKeys.getSecond();
       // Iterate from the deepest potential subpackage to the shallowest in that we only want to
       // display the deepest subpackage in the error message for each target.
       for (PackageLookupValue.Key packageLookupKey : Lists.reverse(targetPackageLookupKeys)) {
@@ -858,10 +861,9 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
           throw new InternalInconsistentFilesystemException(pkgId, e);
         }
 
-        Target target = entry.getKey();
         if (maybeAddEventAboutLabelCrossingSubpackage(
             pkgBuilder, pkgRoot, target, packageLookupKey.argument(), packageLookupValue)) {
-          pkgBuilder.getTargets().remove(entry.getKey());
+          pkgBuilder.getTargets().remove(target);
           pkgBuilder.setContainsErrors();
           break;
         }

src/test/java/com/google/devtools/build/lib/pkgcache/BUILD

@@ -181,6 +181,7 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/common/options",
         "//src/main/java/net/starlark/java/syntax",
+        "//src/main/protobuf:failure_details_java_proto",
         "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//src/test/java/com/google/devtools/build/lib/testutil",
         "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",

src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java

@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
 import com.google.devtools.build.lib.runtime.QuiescingExecutorsImpl;
+import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
 import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -578,4 +579,18 @@ public void testDeterminismOfInputFileLocation() throws Exception {
     InputFile f = (InputFile) p.getTarget("f.sh");
     assertThat(f.getLocation().line()).isEqualTo(1);
   }
+
+  @Test
+  public void testDeterminismOfFailureDetailOnMultipleLabelCrossingSubpackageBoundaryErrors()
+      throws Exception {
+    reporter.removeHandler(failFastHandler);
+    scratch.file("p/sub/BUILD");
+    scratch.file("p/BUILD", "sh_library(name = 'sub/a')", "sh_library(name = 'sub/b')");
+    Package p = getPackage("p");
+    assertThat(p.getFailureDetail().getPackageLoading().getCode())
+        .isEqualTo(PackageLoading.Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+    // We used to non-deterministically pick a target whose label crossed a subpackage boundary, but
+    // now we deterministically pick the first one (alphabetically by target name).
+    assertThat(p.getFailureDetail().getMessage()).startsWith("Label '//p:sub/a' is invalid");
+  }
 }