Eliminate ContainingPackageLookup node in PackageFunction

`Package` node currently creates `ContainingPackageLookup` node to detect cross subpackage boundary. However CPL's compute logic is re-implemented directly `PackageFunction` so that we do not need to create CPL node anymore.

* Before this change, the dependency relation related to subpackage cross boundary check looks like:

`Package node --> CPL node --> Subpackage PL node`

Each Package node creates CPL nodes. The CPL node will then create subpackage's PL node and check their presence in the dependency graph.

* After the change, CPL node is gone and the logic of checking subpackage presence in dependency graph is implemented directly in the outer Package node, so dependency relationship becomes:

`Package node --> Subpackage PL node`

And thus, each Package node directly creates Subpackage PL nodes.

Similar logic also exists in `BzlLoadFunction`, and we will follow up to retire CPL usage there next.

PiperOrigin-RevId: 532511725
Change-Id: I8efe7fce214a01560553d945dccc3b44d1075105

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

@@ -83,7 +83,6 @@
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -799,85 +798,92 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles
       Root pkgRoot, PackageIdentifier pkgId, Package.Builder pkgBuilder, Environment env)
       throws InternalInconsistentFilesystemException, InterruptedException {
     PathFragment pkgDir = pkgId.getPackageFragment();
-    Set<SkyKey> containingPkgLookupKeys = Sets.newHashSet();
-    Map<Target, SkyKey> targetToKey = new HashMap<>();
+    // 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<>();
+    Set<PackageLookupValue.Key> allPackageLookupKeys = new HashSet<>();
     for (Target target : pkgBuilder.getTargets()) {
-      PathFragment dir = Label.getContainingDirectory(target.getLabel());
+      Label label = target.getLabel();
+      PathFragment dir = Label.getContainingDirectory(label);
       if (dir.equals(pkgDir)) {
         continue;
       }
-      PackageIdentifier dirId = PackageIdentifier.create(pkgId.getRepository(), dir);
-      SkyKey key = ContainingPackageLookupValue.key(dirId);
-      targetToKey.put(target, key);
-      containingPkgLookupKeys.add(key);
-    }
-    SkyframeLookupResult containingPkgLookupValues =
-        env.getValuesAndExceptions(containingPkgLookupKeys);
-    if (env.valuesMissing() || containingPkgLookupKeys.isEmpty()) {
+      String labelName = label.getName();
+      PathFragment labelAsRelativePath = PathFragment.create(labelName).getParentDirectory();
+      PathFragment subpackagePath = pkgDir;
+      for (String segment : labelAsRelativePath.segments()) {
+        // Please note that the order from the shallowest path to the deepest is preserved.
+        subpackagePath = subpackagePath.getRelative(segment);
+        PackageLookupValue.Key currentPackageLookupKey =
+            PackageLookupValue.key(PackageIdentifier.create(pkgId.getRepository(), subpackagePath));
+        targetSubpackagePackageLookupKeyMap
+            .computeIfAbsent(target, t -> new ArrayList<>())
+            .add(currentPackageLookupKey);
+        allPackageLookupKeys.add(currentPackageLookupKey);
+      }
+    }
+
+    if (targetSubpackagePackageLookupKeyMap.isEmpty()) {
       return;
     }
-    for (Iterator<Target> iterator = pkgBuilder.getTargets().iterator(); iterator.hasNext(); ) {
-      Target target = iterator.next();
-      SkyKey key = targetToKey.get(target);
-      if (!containingPkgLookupKeys.contains(key)) {
-        continue;
-      }
-      ContainingPackageLookupValue containingPackageLookupValue;
-      try {
-        containingPackageLookupValue =
-            (ContainingPackageLookupValue)
-                containingPkgLookupValues.getOrThrow(
-                    key, BuildFileNotFoundException.class, InconsistentFilesystemException.class);
-      } catch (BuildFileNotFoundException e) {
-        env.getListener().handle(Event.error(null, e.getMessage()));
-        containingPackageLookupValue = null;
-      } catch (InconsistentFilesystemException e) {
-        throw new InternalInconsistentFilesystemException(pkgId, e);
-      }
-      if (maybeAddEventAboutLabelCrossingSubpackage(
-          pkgBuilder,
-          pkgRoot,
-          target.getLabel(),
-          target.getLocation(),
-          containingPackageLookupValue)) {
-        iterator.remove();
-        pkgBuilder.setContainsErrors();
+
+    SkyframeLookupResult packageLookupResults = env.getValuesAndExceptions(allPackageLookupKeys);
+    if (env.valuesMissing()) {
+      return;
+    }
+
+    for (Map.Entry<Target, List<PackageLookupValue.Key>> entry :
+        targetSubpackagePackageLookupKeyMap.entrySet()) {
+      List<PackageLookupValue.Key> targetPackageLookupKeys = entry.getValue();
+      // 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)) {
+        PackageLookupValue packageLookupValue;
+        try {
+          packageLookupValue =
+              (PackageLookupValue)
+                  packageLookupResults.getOrThrow(
+                      packageLookupKey,
+                      BuildFileNotFoundException.class,
+                      InconsistentFilesystemException.class);
+        } catch (BuildFileNotFoundException e) {
+          env.getListener().handle(Event.error(null, e.getMessage()));
+          packageLookupValue = null;
+        } catch (InconsistentFilesystemException e) {
+          throw new InternalInconsistentFilesystemException(pkgId, e);
+        }
+
+        Target target = entry.getKey();
+        if (maybeAddEventAboutLabelCrossingSubpackage(
+            pkgBuilder, pkgRoot, target, packageLookupKey.argument(), packageLookupValue)) {
+          pkgBuilder.getTargets().remove(entry.getKey());
+          pkgBuilder.setContainsErrors();
+          break;
+        }
       }
     }
   }
 
   private static boolean maybeAddEventAboutLabelCrossingSubpackage(
       Package.Builder pkgBuilder,
       Root pkgRoot,
-      Label label,
-      @Nullable Location location,
-      @Nullable ContainingPackageLookupValue containingPkgLookupValue) {
-    if (containingPkgLookupValue == null) {
+      Target target,
+      PackageIdentifier subpackageIdentifier,
+      @Nullable PackageLookupValue packageLookupValue) {
+    if (packageLookupValue == null) {
       return true;
     }
-    if (!containingPkgLookupValue.hasContainingPackage()) {
-      // The missing package here is a problem, but it's not an error from the perspective of
-      // PackageFunction.
-      return false;
-    }
-    PackageIdentifier containingPkg = containingPkgLookupValue.getContainingPackageName();
-    if (containingPkg.equals(label.getPackageIdentifier())) {
-      // The label does not cross a subpackage boundary.
-      return false;
-    }
-    if (!containingPkg.getSourceRoot().startsWith(label.getPackageIdentifier().getSourceRoot())) {
-      // This label is referencing an imaginary package, because the containing package should
-      // extend the label's package: if the label is //a/b:c/d, the containing package could be
-      // //a/b/c or //a/b, but should never be //a. Usually such errors will be caught earlier, but
-      // in some exceptional cases (such as a Python-aware BUILD file catching its own io
-      // exceptions), it reaches here, and we tolerate it.
+    String errMsg =
+        PackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
+            pkgRoot, target.getLabel(), subpackageIdentifier, packageLookupValue);
+    if (errMsg != null) {
+      Event error =
+          Package.error(target.getLocation(), errMsg, Code.LABEL_CROSSES_PACKAGE_BOUNDARY);
+      pkgBuilder.addEvent(error);
+      return true;
+    } else {
       return false;
     }
-    String message =
-        ContainingPackageLookupValue.getErrorMessageForLabelCrossingPackageBoundary(
-            pkgRoot, label, containingPkgLookupValue);
-    pkgBuilder.addEvent(Package.error(location, message, Code.LABEL_CROSSES_PACKAGE_BOUNDARY));
-    return true;
   }
 
   private interface GlobberWithSkyframeGlobDeps extends Globber {
@@ -1466,7 +1472,7 @@ private CompiledBuildFile compileBuildFile(
         FileOptions.builder()
             .requireLoadStatementsFirst(false)
             // For historical reasons, BUILD files are allowed to load a symbol
-            // and then reassign it later. (It is unclear why this is neccessary).
+            // and then reassign it later. (It is unclear why this is necessary).
             // TODO(adonovan): remove this flag and make loads bind file-locally,
             // as in .bzl files. One can always use a renaming load statement.
             .loadBindsGlobally(true)

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

@@ -15,6 +15,7 @@
 
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.BuildFileName;
 import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
@@ -398,4 +399,59 @@ public String getErrorMsg() {
       return String.format("The repository '%s' could not be resolved: %s", repositoryName, reason);
     }
   }
+
+  /**
+   * Creates the error message for the input {@linkplain Label label} has a subpackage crossing
+   * boundary.
+   *
+   * <p>Returns {@code null} if no subpackage is discovered or the subpackage is marked as DELETED.
+   */
+  @Nullable
+  static String getErrorMessageForLabelCrossingPackageBoundary(
+      Root pkgRoot,
+      Label label,
+      PackageIdentifier subpackageIdentifier,
+      PackageLookupValue packageLookupValue) {
+    String message = null;
+    if (packageLookupValue.packageExists()) {
+      message =
+          String.format(
+              "Label '%s' is invalid because '%s' is a subpackage", label, subpackageIdentifier);
+      Root subPackageRoot = packageLookupValue.getRoot();
+
+      if (pkgRoot.equals(subPackageRoot)) {
+        PathFragment labelRootPathFragment = label.getPackageIdentifier().getSourceRoot();
+        PathFragment subpackagePathFragment = subpackageIdentifier.getSourceRoot();
+        if (subpackagePathFragment.startsWith(labelRootPathFragment)) {
+          PathFragment labelNameInSubpackage =
+              PathFragment.create(label.getName())
+                  .subFragment(
+                      subpackagePathFragment.segmentCount() - labelRootPathFragment.segmentCount());
+          message += "; perhaps you meant to put the" + " colon here: '";
+          if (subpackageIdentifier.getRepository().isMain()) {
+            message += "//";
+          }
+          message += subpackageIdentifier + ":" + labelNameInSubpackage + "'?";
+        } else {
+          // TODO: Is this a valid case? How do we handle this case?
+        }
+      } else {
+        message +=
+            "; have you deleted "
+                + subpackageIdentifier
+                + "/BUILD? "
+                + "If so, use the --deleted_packages="
+                + subpackageIdentifier
+                + " option";
+      }
+    } else if (packageLookupValue instanceof IncorrectRepositoryReferencePackageLookupValue) {
+      message =
+          String.format(
+              "Label '%s' is invalid because '%s' is a subpackage",
+              label,
+              ((IncorrectRepositoryReferencePackageLookupValue) packageLookupValue)
+                  .correctedPackageIdentifier);
+    }
+    return message;
+  }
 }

src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java

@@ -1077,7 +1077,7 @@ public void nonSkyframeGlobbingIOException_andLabelCrossingSubpackageBoundaries_
     Path pkgBUILDPath =
         scratch.file(
             "pkg/BUILD",
-            "exports_files(['sub/blah'])  # label crossing subpackage boundaries", //
+            "exports_files(['sub/blah'])  # label crossing subpackage boundaries",
             "glob(['globcycle/**/foo.txt'])  # triggers non-Skyframe globbing error");
     scratch.file("pkg/sub/BUILD");
     Path pkgGlobcyclePath = pkgBUILDPath.getParentDirectory().getChild("globcycle");