Support packages assembled from package pieces in query and genquery machinery

For a package loaded with lazy macro expansion, Target.getPackage() returns null,
so we now need to use TargetProvider/PackageProvider to retrieve a target's
package's BUILD file target or to iterate sibling targets.

As a follow-up, we'd want to avoid unnecessarily loading full packages in
implementations of TargetProvider.getTarget() and in TargetLoadingUtil -
but that requires another new skyfunction.

Working towards #23852

PiperOrigin-RevId: 792276714
Change-Id: I76266c10d4f5d6e087a6f38c6b6bfe06eb72603b

Author: tetromino

src/main/java/com/google/devtools/build/lib/BUILD

@@ -430,6 +430,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/metrics:garbage-collection-metrics-util",
         "//src/main/java/com/google/devtools/build/lib/metrics/criticalpath",
         "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/packages:package_piece_identifier",
         "//src/main/java/com/google/devtools/build/lib/packages/semantics",
         "//src/main/java/com/google/devtools/build/lib/pkgcache",
         "//src/main/java/com/google/devtools/build/lib/pkgcache:package_options",

src/main/java/com/google/devtools/build/lib/pkgcache/AbstractRecursivePackageProvider.java

@@ -28,6 +28,8 @@ protected AbstractRecursivePackageProvider() {
   @Override
   public Target getTarget(ExtendedEventHandler eventHandler, Label label)
       throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
+    // TODO(https://github.com/bazelbuild/bazel/issues/23852): don't expand the full package if lazy
+    // macro expansion is enabled.
     return getPackage(eventHandler, label.getPackageIdentifier()).getTarget(label.getName());
   }
 

src/main/java/com/google/devtools/build/lib/pkgcache/CompileOneDependencyTransformer.java

@@ -25,6 +25,7 @@
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.FileTarget;
+import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.Package;
@@ -93,8 +94,19 @@ private Target transformCompileOneDependency(ExtendedEventHandler eventHandler,
     }
 
     Rule result = null;
-    Iterable<Rule> orderedRuleList =
-        getOrderedRuleList(packageProvider.getPackage(eventHandler, target.getPackageoid()));
+    Iterable<Rule> orderedRuleList;
+    try {
+      orderedRuleList = getOrderedRuleList(packageProvider.getPackage(eventHandler, target));
+    } catch (NoSuchPackageException e) {
+      // Only possible if lazy macro expansion is enabled, and an error was encountered when loading
+      // a different package piece of this package.
+      throw new TargetParsingException(
+          "Package of --compile_one_dependency target '"
+              + target.getLabel()
+              + "' could not be loaded",
+          e,
+          e.getDetailedExitCode());
+    }
     for (Rule rule : orderedRuleList) {
       Set<Label> labels = getInputLabels(rule);
       if (listContainsFile(eventHandler, labels, target.getLabel(), Sets.<Label>newHashSet())) {

src/main/java/com/google/devtools/build/lib/pkgcache/PackageProvider.java

@@ -14,12 +14,20 @@
 
 package com.google.devtools.build.lib.pkgcache;
 
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.collect.ImmutableCollection;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.io.InconsistentFilesystemException;
+import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
+import com.google.devtools.build.lib.packages.NoSuchPackagePieceException;
 import com.google.devtools.build.lib.packages.Package;
+import com.google.devtools.build.lib.packages.PackagePiece;
 import com.google.devtools.build.lib.packages.Packageoid;
+import com.google.devtools.build.lib.packages.Target;
 
 /**
  * API for retrieving packages. Implementations generally load packages to fulfill requests.
@@ -46,23 +54,21 @@ Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageN
       throws NoSuchPackageException, InterruptedException;
 
   /**
-   * If a {@link Packageoid} is a {@link Package}, returns it; otherwise, loads and returns the full
-   * package containing it.
+   * If a {@link Target} is owned by a monolithic {@link Package}, returns it; otherwise, loads and
+   * returns the full package encompassing the target's package piece.
    *
+   * @throws NoSuchPackageException if target is owned by a {@link PackagePiece}, and the full
+   *     package could not be loaded due to an error while loading a different package piece.
    * @throws InterruptedException if the package loading was interrupted.
    */
-  default Package getPackage(ExtendedEventHandler eventHandler, Packageoid packageoid)
-      throws InterruptedException {
+  default Package getPackage(ExtendedEventHandler eventHandler, Target target)
+      throws NoSuchPackageException, InterruptedException {
+    Packageoid packageoid = target.getPackageoid();
     if (packageoid instanceof Package pkg) {
+      // Monolithic package.
       return pkg;
     }
-    try {
-      return getPackage(eventHandler, packageoid.getPackageIdentifier());
-    } catch (NoSuchPackageException e) {
-      // Cannot happen: for a packageoid to exists, its package's BUILD file must also exist and be
-      // parseable.
-      throw new AssertionError("Failed to load package " + packageoid.getPackageIdentifier(), e);
-    }
+    return getPackage(eventHandler, packageoid.getPackageIdentifier());
   }
 
   /**
@@ -84,4 +90,63 @@ default Package getPackage(ExtendedEventHandler eventHandler, Packageoid package
    */
   boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
       throws InconsistentFilesystemException, InterruptedException;
+
+  /**
+   * Returns the BUILD file target of the given package, loading, parsing and evaluating either the
+   * full package (if lazy macro expansion is disabled) or just the package piece owning the BUILD
+   * file (if lazy macro expansion is enabled) if it is not already loaded.
+   *
+   * @throws NoSuchPackageException if the package could not be found
+   * @throws NoSuchPackagePieceException if lazy macro expansion is enabled, and the package piece
+   *     owning the BUILD file failed validation
+   * @throws InterruptedException if the package loading was interrupted
+   */
+  InputFile getBuildFile(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
+      throws NoSuchPackageException, NoSuchPackagePieceException, InterruptedException;
+
+  @Override
+  default InputFile getBuildFile(Target target) throws InterruptedException {
+    Packageoid packageoid = target.getPackageoid();
+    if (packageoid instanceof Package pkg) {
+      // Monolithic package.
+      return pkg.getBuildFile();
+    } else if (packageoid instanceof PackagePiece.ForBuildFile forBuildFile) {
+      // Lazy macro expansion, target is top-level.
+      return forBuildFile.getBuildFile();
+    } else {
+      // Lazy macro expansion, target is in a PackagePiece.ForMacro, we need to retrieve the
+      // BUILD file from the (already loaded) PackagePiece.ForBuildFile.
+      StoredEventHandler localEventHandler = new StoredEventHandler();
+      InputFile buildFile;
+      try {
+        buildFile =
+            getBuildFile(localEventHandler, target.getPackageMetadata().packageIdentifier());
+      } catch (NoSuchPackageException | NoSuchPackagePieceException e) {
+        // If a PackagePiece.ForMacro exists, its corresponding PackagePiece.ForBuildFile must also
+        // exist (and already be loaded).
+        throw new IllegalStateException(
+            String.format(
+                "Bug in package loading machinery: failed to load package piece for BUILD file of"
+                    + " already-loaded target %s",
+                target),
+            e);
+      }
+      // If PackagePiece.ForMacro was loaded, its corresponding PackagePiece.ForBuildFile could not
+      // be in error.
+      checkState(
+          !localEventHandler.hasErrors(),
+          "Bug in package loading machinery: unexpected error while retrieving package piece for"
+              + " BUILD file of already-loaded target %s: %s",
+          target,
+          localEventHandler.getEvents());
+      return buildFile;
+    }
+  }
+
+  @Override
+  default ImmutableCollection<Target> getSiblingTargetsInPackage(
+      ExtendedEventHandler eventHandler, Target target)
+      throws NoSuchPackageException, InterruptedException {
+    return getPackage(eventHandler, target).getTargets().values();
+  }
 }

src/main/java/com/google/devtools/build/lib/pkgcache/RecursivePackageProvider.java

@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.io.InconsistentFilesystemException;
 import com.google.devtools.build.lib.io.ProcessPackageDirectoryException;
+import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
@@ -108,7 +109,8 @@ default Set<PackageIdentifier> bulkIsPackage(
   }
 
   /**
-   * A {@link RecursivePackageProvider} in terms of a map of pre-fetched packages.
+   * A {@link RecursivePackageProvider} in terms of a map of pre-fetched, fully macro-expanded
+   * packages.
    *
    * <p>Note that this class implements neither {@link #streamPackagesUnderDirectory} nor {@link
    * #bulkGetPackages}, so it can only be used for use cases that do not call either of these
@@ -119,6 +121,8 @@ default Set<PackageIdentifier> bulkIsPackage(
    *
    * @see com.google.devtools.build.lib.cmdline.TargetPattern.Type
    */
+  // TODO(bazel-team): should we avoid forcing symbolic macro expansion, and use a backing map of
+  // packageoids-for-build-file instead?
   class PackageBackedRecursivePackageProvider implements RecursivePackageProvider {
     private final Map<PackageIdentifier, Package> packages;
 
@@ -136,6 +140,12 @@ public Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier p
       return pkg;
     }
 
+    @Override
+    public InputFile getBuildFile(ExtendedEventHandler eventHandler, PackageIdentifier packageName)
+        throws NoSuchPackageException {
+      return getPackage(eventHandler, packageName).getBuildFile();
+    }
+
     @Override
     public boolean isPackage(ExtendedEventHandler eventHandler, PackageIdentifier packageName) {
       return packages.containsKey(packageName);

src/main/java/com/google/devtools/build/lib/pkgcache/TargetProvider.java

@@ -14,8 +14,10 @@
 
 package com.google.devtools.build.lib.pkgcache;
 
+import com.google.common.collect.ImmutableCollection;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Target;
@@ -37,4 +39,18 @@ public interface TargetProvider {
    */
   Target getTarget(ExtendedEventHandler eventHandler, Label label)
       throws NoSuchPackageException, NoSuchTargetException, InterruptedException;
+
+  /** Returns the BUILD file target of the package which contains the given target. */
+  InputFile getBuildFile(Target target) throws InterruptedException;
+
+  /**
+   * Returns all targets in the package which contains the given target.
+   *
+   * @throws NoSuchPackageException if the lazy macro expansion is enabled and some of the pieces of
+   *     the full package could not be loaded.
+   * @throws InterruptedException if the package loading was interrupted
+   */
+  ImmutableCollection<Target> getSiblingTargetsInPackage(
+      ExtendedEventHandler eventHandler, Target target)
+      throws NoSuchPackageException, InterruptedException;
 }

src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java

@@ -988,9 +988,13 @@ public Uniquifier<SkyKey> createSkyKeyUniquifier() {
 
   @ThreadSafe
   @Override
-  public Collection<Target> getSiblingTargetsInPackage(Target target) {
-    // TODO(https://github.com/bazelbuild/bazel/issues/23852): support lazy macro expansion
-    return target.getPackage().getTargets().values();
+  public Collection<Target> getSiblingTargetsInPackage(Target target)
+      throws QueryException, InterruptedException {
+    try {
+      return graphBackedRecursivePackageProvider.getSiblingTargetsInPackage(eventHandler, target);
+    } catch (NoSuchPackageException e) {
+      throw new QueryException(e.getMessage(), e, e.getDetailedExitCode().getFailureDetail());
+    }
   }
 
   @ThreadSafe
@@ -1057,9 +1061,14 @@ public QueryTaskFuture<Void> evalTargetPatternKey(
   public TransitiveLoadFilesHelper<Target> getTransitiveLoadFilesHelper() {
     return new TransitiveLoadFilesHelperForTargets() {
       @Override
-      public Target getLoadFileTarget(Target originalTarget, Label bzlLabel) {
-        // TODO(https://github.com/bazelbuild/bazel/issues/23852): support lazy macro expansion
-        return new FakeLoadTarget(bzlLabel, originalTarget.getPackage());
+      public Target getLoadFileTarget(Target originalTarget, Label bzlLabel)
+          throws InterruptedException {
+        return new FakeLoadTarget(bzlLabel, getBuildFileTarget(originalTarget).getPackageoid());
+      }
+
+      @Override
+      public Target getBuildFileTarget(Target originalTarget) throws InterruptedException {
+        return graphBackedRecursivePackageProvider.getBuildFile(originalTarget);
       }
 
       @Override
@@ -1084,12 +1093,11 @@ public Target maybeGetBuildFileTargetForLoadFileTarget(Target originalTarget, La
                           .build())
                   .build());
         }
-        // TODO(https://github.com/bazelbuild/bazel/issues/23852): support lazy macro expansion
         return new FakeLoadTarget(
             Label.createUnvalidated(
                 packageIdentifier,
                 packageLookupValue.getBuildFileName().getFilenameFragment().getBaseName()),
-            originalTarget.getPackage());
+            getBuildFileTarget(originalTarget).getPackageoid());
       }
     };
   }

src/main/java/com/google/devtools/build/lib/query2/common/AbstractBlazeQueryEnvironment.java

@@ -498,12 +498,6 @@ public PackageIdentifier getPkgId(Target target) {
       return target.getLabel().getPackageIdentifier();
     }
 
-    @Override
-    public Target getBuildFileTarget(Target originalTarget) {
-      // TODO(https://github.com/bazelbuild/bazel/issues/23852): support lazy macro expansion
-      return originalTarget.getPackage().getBuildFile();
-    }
-
     @Override
     public void visitLoads(
         Target originalTarget, LoadGraphVisitor<QueryException, InterruptedException> visitor)

src/main/java/com/google/devtools/build/lib/query2/compat/FakeLoadTarget.java

@@ -34,15 +34,20 @@ public class FakeLoadTarget implements Target {
   private final Label label;
   private final Packageoid pkg;
 
-  public FakeLoadTarget(Label label, Package pkg) {
+  /**
+   * @param packageoidOfBuildFile the {@link Packageoid} owning the package's BUILD file: in other
+   *     words, either a monolithic {@link Package} (under eager symbolic macro expansion), or a
+   *     {@link PackagePiece.ForBuildFile} (under lazy symbolic macro expansion).
+   */
+  public FakeLoadTarget(Label label, Packageoid packageoidOfBuildFile) {
     this.label = Preconditions.checkNotNull(label);
-    this.pkg = Preconditions.checkNotNull(pkg);
-  }
-
-  // Fake load targets should be in the same package piece as the package's BUILD file.
-  public FakeLoadTarget(Label label, PackagePiece.ForBuildFile pkgPiece) {
-    this.label = Preconditions.checkNotNull(label);
-    this.pkg = Preconditions.checkNotNull(pkgPiece);
+    Preconditions.checkNotNull(packageoidOfBuildFile);
+    Preconditions.checkArgument(
+        (packageoidOfBuildFile instanceof Package pkg && pkg.getBuildFile().getPackageoid() == pkg)
+            || packageoidOfBuildFile instanceof PackagePiece.ForBuildFile,
+        "%s must be either a monolithic package or a top-level package piece",
+        packageoidOfBuildFile);
+    this.pkg = packageoidOfBuildFile;
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/query2/engine/QueryEnvironment.java

@@ -240,7 +240,7 @@ void samePkgDirectRdeps(Iterable<T> from, QueryExpression caller, Callback<T> ca
   }
 
   /** Returns all of the targets in <code>target</code>'s package, in some stable order. */
-  Collection<T> getSiblingTargetsInPackage(T target) throws QueryException;
+  Collection<T> getSiblingTargetsInPackage(T target) throws QueryException, InterruptedException;
 
   /**
    * Invokes {@code callback} with the set of target nodes in the graph for the specified target
@@ -570,9 +570,9 @@ void visitLoads(
         T originalTarget, LoadGraphVisitor<QueryException, InterruptedException> visitor)
         throws QueryException, InterruptedException;
 
-    T getBuildFileTarget(T originalTarget);
+    T getBuildFileTarget(T originalTarget) throws InterruptedException;
 
-    T getLoadFileTarget(T originalTarget, Label bzlLabel);
+    T getLoadFileTarget(T originalTarget, Label bzlLabel) throws InterruptedException;
 
     @Nullable
     T maybeGetBuildFileTargetForLoadFileTarget(T originalTarget, Label bzlLabel)