Remove some code leftover from back when BUILD file preprocessing was a thing. Also remove some TODOs that were leftover from before we added the caches used after a PackageFunction restart.

RELNOTES: None
PiperOrigin-RevId: 179926806

5 files changed, 57 insertions, 99 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/AstAfterPreprocessing.java b/src/main/java/com/google/devtools/build/lib/packages/AstParseResult.java
index dba1ed3788..08d6bb2b5a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/AstAfterPreprocessing.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/AstParseResult.java
@@ -18,13 +18,13 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.events.StoredEventHandler;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 
-/** The result of parsing a preprocessed BUILD file. */
-public class AstAfterPreprocessing {
+/** The result of parsing a BUILD file. */
+public class AstParseResult {
   public final BuildFileAST ast;
   public final Iterable<Event> allEvents;
   public final Iterable<Postable> allPosts;
 
-  public AstAfterPreprocessing(BuildFileAST ast, StoredEventHandler astParsingEventHandler) {
+  public AstParseResult(BuildFileAST ast, StoredEventHandler astParsingEventHandler) {
     this.ast = ast;
     this.allPosts = astParsingEventHandler.getPosts();
     this.allEvents = astParsingEventHandler.getEvents();
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index d652327fd7..6f7ee1e002 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -606,6 +606,7 @@ public final class PackageFactory {
    * PythonPreprocessor. We annotate the package with additional dependencies. (A 'real' subinclude
    * will never be seen by the parser, because the presence of "subinclude" triggers preprocessing.)
    */
+  // TODO(b/35913039): Remove this and all references to 'mocksubinclude'.
   @SkylarkSignature(
     name = "mocksubinclude",
     returnType = Runtime.NoneType.class,
@@ -1307,13 +1308,13 @@ public final class PackageFactory {
     // show up below.
     BuildFileAST buildFileAST =
         parseBuildFile(packageId, input, preludeStatements, localReporterForParsing);
-    AstAfterPreprocessing astAfterPreprocessing =
-        new AstAfterPreprocessing(buildFileAST, localReporterForParsing);
-    return createPackageFromPreprocessingAst(
+    AstParseResult astParseResult =
+        new AstParseResult(buildFileAST, localReporterForParsing);
+    return createPackageFromAst(
         workspaceName,
         packageId,
         buildFile,
-        astAfterPreprocessing,
+        astParseResult,
         imports,
         skylarkFileDependencies,
         defaultVisibility,
@@ -1333,11 +1334,11 @@ public final class PackageFactory {
     return buildFileAST;
   }
 
-  public Package.Builder createPackageFromPreprocessingAst(
+  public Package.Builder createPackageFromAst(
       String workspaceName,
       PackageIdentifier packageId,
       Path buildFile,
-      AstAfterPreprocessing astAfterPreprocessing,
+      AstParseResult astParseResult,
       Map<String, Extension> imports,
       ImmutableList<Label> skylarkFileDependencies,
       RuleVisibility defaultVisibility,
@@ -1354,11 +1355,11 @@ public final class PackageFactory {
       return evaluateBuildFile(
           workspaceName,
           packageId,
-          astAfterPreprocessing.ast,
+          astParseResult.ast,
           buildFile,
           globber,
-          astAfterPreprocessing.allEvents,
-          astAfterPreprocessing.allPosts,
+          astParseResult.allEvents,
+          astParseResult.allPosts,
           defaultVisibility,
           skylarkSemantics,
           false /* containsError */,
@@ -1611,7 +1612,7 @@ public final class PackageFactory {
   }
 
   /**
-   * Called by a caller of {@link #createPackageFromPreprocessingAst} after this caller has fully
+   * Called by a caller of {@link #createPackageFromAst} after this caller has fully
    * loaded the package.
    */
   public void afterDoneLoadingPackage(Package pkg, SkylarkSemantics skylarkSemantics) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index fbd9646a8e..bc44be113b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -33,7 +33,7 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.events.StoredEventHandler;
-import com.google.devtools.build.lib.packages.AstAfterPreprocessing;
+import com.google.devtools.build.lib.packages.AstParseResult;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.packages.CachingPackageLocator;
@@ -92,9 +92,8 @@ public class PackageFunction implements SkyFunction {
 
   private final PackageFactory packageFactory;
   private final CachingPackageLocator packageLocator;
-  private final Cache<PackageIdentifier, CacheEntryWithGlobDeps<Package.Builder>>
-      packageFunctionCache;
-  private final Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache;
+  private final Cache<PackageIdentifier, BuilderAndGlobDeps> packageFunctionCache;
+  private final Cache<PackageIdentifier, AstParseResult> astCache;
   private final AtomicBoolean showLoadingProgress;
   private final AtomicInteger numPackagesLoaded;
   @Nullable private final PackageProgressReceiver packageProgress;
@@ -112,8 +111,8 @@ public class PackageFunction implements SkyFunction {
       PackageFactory packageFactory,
       CachingPackageLocator pkgLocator,
       AtomicBoolean showLoadingProgress,
-      Cache<PackageIdentifier, CacheEntryWithGlobDeps<Package.Builder>> packageFunctionCache,
-      Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache,
+      Cache<PackageIdentifier, BuilderAndGlobDeps> packageFunctionCache,
+      Cache<PackageIdentifier, AstParseResult> astCache,
       AtomicInteger numPackagesLoaded,
       @Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining,
       @Nullable PackageProgressReceiver packageProgress,
@@ -138,8 +137,8 @@ public class PackageFunction implements SkyFunction {
       PackageFactory packageFactory,
       CachingPackageLocator pkgLocator,
       AtomicBoolean showLoadingProgress,
-      Cache<PackageIdentifier, CacheEntryWithGlobDeps<Package.Builder>> packageFunctionCache,
-      Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache,
+      Cache<PackageIdentifier, BuilderAndGlobDeps> packageFunctionCache,
+      Cache<PackageIdentifier, AstParseResult> astCache,
       AtomicInteger numPackagesLoaded,
       @Nullable SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining) {
     this(
@@ -200,18 +199,14 @@ public class PackageFunction implements SkyFunction {
     }
   }
 
-  /** An entry in {@link PackageFunction}'s internal caches. */
-  public static class CacheEntryWithGlobDeps<T> {
-    private final T value;
+  /** An entry in {@link PackageFunction} internal cache. */
+  public static class BuilderAndGlobDeps {
+    private final Package.Builder builder;
     private final Set<SkyKey> globDepKeys;
-    @Nullable
-    private final LegacyGlobber legacyGlobber;
 
-    private CacheEntryWithGlobDeps(T value, Set<SkyKey> globDepKeys,
-        @Nullable LegacyGlobber legacyGlobber) {
-      this.value = value;
+    private BuilderAndGlobDeps(Package.Builder builder, Set<SkyKey> globDepKeys) {
+      this.builder = builder;
       this.globDepKeys = globDepKeys;
-      this.legacyGlobber = legacyGlobber;
     }
   }
 
@@ -341,13 +336,6 @@ public class PackageFunction implements SkyFunction {
       throws InternalInconsistentFilesystemException, InterruptedException {
     boolean packageShouldBeInError = containsErrors;
 
-    // TODO(bazel-team): This means that many packages will have to be preprocessed twice. Ouch!
-    // We need a better continuation mechanism to avoid repeating work. [skyframe-loading]
-
-    // TODO(bazel-team): It would be preferable to perform I/O from the package preprocessor via
-    // Skyframe rather than add (potentially incomplete) dependencies after the fact.
-    // [skyframe-loading]
-
     Set<SkyKey> subincludePackageLookupDepKeys = Sets.newHashSet();
     for (Label label : subincludes.keySet()) {
       // Declare a dependency on the package lookup for the package giving access to the label.
@@ -566,7 +554,7 @@ public class PackageFunction implements SkyFunction {
     List<Statement> preludeStatements =
         astLookupValue.lookupSuccessful()
             ? astLookupValue.getAST().getStatements() : ImmutableList.<Statement>of();
-    CacheEntryWithGlobDeps<Package.Builder> packageBuilderAndGlobDeps =
+    BuilderAndGlobDeps packageBuilderAndGlobDeps =
         loadPackage(
             workspaceName,
             replacementContents,
@@ -581,7 +569,7 @@ public class PackageFunction implements SkyFunction {
     if (packageBuilderAndGlobDeps == null) {
       return null;
     }
-    Package.Builder pkgBuilder = packageBuilderAndGlobDeps.value;
+    Package.Builder pkgBuilder = packageBuilderAndGlobDeps.builder;
     try {
       pkgBuilder.buildPartial();
     } catch (NoSuchPackageException e) {
@@ -1186,11 +1174,11 @@ public class PackageFunction implements SkyFunction {
    *
    * <p>Exactly one of {@code replacementContents} and {@code buildFileValue} will be
    * non-{@code null}. The former indicates that we have a faux BUILD file with the given contents
-   * and the latter indicates that we have a legitimate BUILD file and should actually do
-   * preprocessing.
+   * and the latter indicates that we have a legitimate BUILD file and should actually read its
+   * contents.
    */
   @Nullable
-  private CacheEntryWithGlobDeps<Package.Builder> loadPackage(
+  private BuilderAndGlobDeps loadPackage(
       String workspaceName,
       @Nullable String replacementContents,
       PackageIdentifier packageId,
@@ -1202,26 +1190,18 @@ public class PackageFunction implements SkyFunction {
       Path packageRoot,
       Environment env)
       throws InterruptedException, PackageFunctionException {
-    CacheEntryWithGlobDeps<Package.Builder> packageFunctionCacheEntry =
-        packageFunctionCache.getIfPresent(packageId);
-    if (packageFunctionCacheEntry == null) {
+    BuilderAndGlobDeps builderAndGlobDeps = packageFunctionCache.getIfPresent(packageId);
+    if (builderAndGlobDeps == null) {
       profiler.startTask(ProfilerTask.CREATE_PACKAGE, packageId.toString());
       if (packageProgress != null) {
         packageProgress.startReadPackage(packageId);
       }
       try {
-        CacheEntryWithGlobDeps<AstAfterPreprocessing> astCacheEntry =
-            astCache.getIfPresent(packageId);
-        if (astCacheEntry == null) {
+        AstParseResult astParseResult = astCache.getIfPresent(packageId);
+        if (astParseResult == null) {
           if (showLoadingProgress.get()) {
             env.getListener().handle(Event.progress("Loading package: " + packageId));
           }
-          // We use a LegacyGlobber that doesn't sort the matches for each individual glob pattern,
-          // since we want to sort the final result anyway.
-          LegacyGlobber legacyGlobber = packageFactory.createLegacyGlobberThatDoesntSort(
-              buildFilePath.getParentDirectory(), packageId, packageLocator);
-          SkyframeHybridGlobber skyframeGlobber = new SkyframeHybridGlobber(packageId, packageRoot,
-              env, legacyGlobber);
           ParserInputSource input;
           if (replacementContents == null) {
             Preconditions.checkNotNull(buildFileValue, packageId);
@@ -1255,27 +1235,16 @@ public class PackageFunction implements SkyFunction {
           BuildFileAST ast =
               PackageFactory.parseBuildFile(
                   packageId, input, preludeStatements, astParsingEventHandler);
-          // If no globs were fetched during preprocessing, then there's no need to reuse the
-          // legacy globber instance during BUILD file evaluation since the performance argument
-          // below does not apply.
-          Set<SkyKey> globDepsRequested = skyframeGlobber.getGlobDepsRequested();
-          LegacyGlobber legacyGlobberToStore = globDepsRequested.isEmpty() ? null : legacyGlobber;
-          astCacheEntry =
-              new CacheEntryWithGlobDeps<>(
-                  new AstAfterPreprocessing(ast, astParsingEventHandler),
-                  globDepsRequested,
-                  legacyGlobberToStore);
-          astCache.put(packageId, astCacheEntry);
+          astParseResult = new AstParseResult(ast, astParsingEventHandler);
+          astCache.put(packageId, astParseResult);
         }
-        AstAfterPreprocessing astAfterPreprocessing = astCacheEntry.value;
-        Set<SkyKey> globDepsRequestedDuringPreprocessing = astCacheEntry.globDepKeys;
         SkylarkImportResult importResult;
         try {
           importResult =
               fetchImportsFromBuildFile(
                   buildFilePath,
                   packageId,
-                  astAfterPreprocessing.ast,
+                  astParseResult.ast,
                   env,
                   skylarkImportLookupFunctionForInlining);
         } catch (NoSuchPackageException e) {
@@ -1288,41 +1257,32 @@ public class PackageFunction implements SkyFunction {
           return null;
         }
         astCache.invalidate(packageId);
-        // If a legacy globber was used to evaluate globs during preprocessing, it's important that
-        // we reuse that globber during BUILD file evaluation for performance, in the case that
-        // globs were fetched lazily during preprocessing. See Preprocessor.Factory#considersGlobs.
-        LegacyGlobber legacyGlobber = astCacheEntry.legacyGlobber != null
-            ? astCacheEntry.legacyGlobber
-            : packageFactory.createLegacyGlobber(
+        LegacyGlobber legacyGlobber = packageFactory.createLegacyGlobber(
                 buildFilePath.getParentDirectory(), packageId, packageLocator);
         SkyframeHybridGlobber skyframeGlobber = new SkyframeHybridGlobber(packageId, packageRoot,
             env, legacyGlobber);
-        Package.Builder pkgBuilder = packageFactory.createPackageFromPreprocessingAst(
+        Package.Builder pkgBuilder = packageFactory.createPackageFromAst(
             workspaceName,
             packageId,
             buildFilePath,
-            astAfterPreprocessing,
+            astParseResult,
             importResult.importMap,
             importResult.fileDependencies,
             defaultVisibility,
             skylarkSemantics,
             skyframeGlobber);
-        Set<SkyKey> globDepsRequested = ImmutableSet.<SkyKey>builder()
-            .addAll(globDepsRequestedDuringPreprocessing)
-            .addAll(skyframeGlobber.getGlobDepsRequested())
-            .build();
-        packageFunctionCacheEntry =
-            new CacheEntryWithGlobDeps<>(pkgBuilder, globDepsRequested, null);
+        builderAndGlobDeps =
+            new BuilderAndGlobDeps(pkgBuilder, skyframeGlobber.getGlobDepsRequested());
         numPackagesLoaded.incrementAndGet();
         if (packageProgress != null) {
           packageProgress.doneReadPackage(packageId);
         }
-        packageFunctionCache.put(packageId, packageFunctionCacheEntry);
+        packageFunctionCache.put(packageId, builderAndGlobDeps);
       } finally {
         profiler.completeTask(ProfilerTask.CREATE_PACKAGE);
       }
     }
-    return packageFunctionCacheEntry;
+    return builderAndGlobDeps;
   }
 
   private static class InternalInconsistentFilesystemException extends Exception {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 736b8fd10c..6a0ad1f7b3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -86,7 +86,7 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.packages.AspectDescriptor;
-import com.google.devtools.build.lib.packages.AstAfterPreprocessing;
+import com.google.devtools.build.lib.packages.AstParseResult;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
@@ -117,7 +117,6 @@ import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
 import com.google.devtools.build.lib.skyframe.DirtinessCheckerUtils.FileDirtinessChecker;
 import com.google.devtools.build.lib.skyframe.ExternalFilesHelper.ExternalFileAction;
 import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
-import com.google.devtools.build.lib.skyframe.PackageFunction.CacheEntryWithGlobDeps;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionCompletedReceiver;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ProgressSupplier;
@@ -206,13 +205,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   // Cache of partially constructed Package instances, stored between reruns of the PackageFunction
   // (because of missing dependencies, within the same evaluate() run) to avoid loading the same
-  // package twice (first time loading to find subincludes and declare value dependencies).
+  // package twice (first time loading to find imported bzl files and declare Skyframe
+  // dependencies).
   // TODO(bazel-team): remove this cache once we have skyframe-native package loading
   // [skyframe-loading]
-  private final Cache<PackageIdentifier, CacheEntryWithGlobDeps<Package.Builder>>
+  private final Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps>
       packageFunctionCache = newPkgFunctionCache();
-  private final Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache =
-      newAstCache();
+  private final Cache<PackageIdentifier, AstParseResult> astCache = newAstCache();
 
   private final AtomicInteger numPackagesLoaded = new AtomicInteger(0);
   private final PackageProgressReceiver packageProgress = new PackageProgressReceiver();
@@ -479,8 +478,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       PackageFactory pkgFactory,
       PackageManager packageManager,
       AtomicBoolean showLoadingProgress,
-      Cache<PackageIdentifier, CacheEntryWithGlobDeps<Builder>> packageFunctionCache,
-      Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache,
+      Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps> packageFunctionCache,
+      Cache<PackageIdentifier, AstParseResult> astCache,
       AtomicInteger numPackagesLoaded,
       RuleClassProvider ruleClassProvider,
       PackageProgressReceiver packageProgress) {
@@ -791,12 +790,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     }
   }
 
-  protected Cache<PackageIdentifier, CacheEntryWithGlobDeps<Package.Builder>>
+  protected Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps>
       newPkgFunctionCache() {
     return CacheBuilder.newBuilder().build();
   }
 
-  protected Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> newAstCache() {
+  protected Cache<PackageIdentifier, AstParseResult> newAstCache() {
     return CacheBuilder.newBuilder().build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 90fca80f13..46ba9f5c78 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -28,7 +28,7 @@ import com.google.devtools.build.lib.analysis.ServerDirectories;
 import com.google.devtools.build.lib.clock.BlazeClock;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.Reporter;
-import com.google.devtools.build.lib.packages.AstAfterPreprocessing;
+import com.google.devtools.build.lib.packages.AstParseResult;
 import com.google.devtools.build.lib.packages.AttributeContainer;
 import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
 import com.google.devtools.build.lib.packages.BuildFileName;
@@ -56,7 +56,6 @@ import com.google.devtools.build.lib.skyframe.FileSymlinkInfiniteExpansionUnique
 import com.google.devtools.build.lib.skyframe.GlobFunction;
 import com.google.devtools.build.lib.skyframe.PackageFunction;
 import com.google.devtools.build.lib.skyframe.PackageFunction.ActionOnIOExceptionReadingBuildFile;
-import com.google.devtools.build.lib.skyframe.PackageFunction.CacheEntryWithGlobDeps;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction;
 import com.google.devtools.build.lib.skyframe.PackageLookupFunction.CrossRepositoryLabelViolationStrategy;
 import com.google.devtools.build.lib.skyframe.PackageValue;
@@ -341,10 +340,9 @@ public abstract class AbstractPackageLoader implements PackageLoader {
   protected final ImmutableMap<SkyFunctionName, SkyFunction> makeFreshSkyFunctions() {
     AtomicReference<TimestampGranularityMonitor> tsgm =
         new AtomicReference<>(new TimestampGranularityMonitor(BlazeClock.instance()));
-    Cache<PackageIdentifier, CacheEntryWithGlobDeps<Package.Builder>> packageFunctionCache =
-        CacheBuilder.newBuilder().build();
-    Cache<PackageIdentifier, CacheEntryWithGlobDeps<AstAfterPreprocessing>> astCache =
+    Cache<PackageIdentifier, PackageFunction.BuilderAndGlobDeps> packageFunctionCache =
         CacheBuilder.newBuilder().build();
+    Cache<PackageIdentifier, AstParseResult> astCache = CacheBuilder.newBuilder().build();
     AtomicReference<PerBuildSyscallCache> syscallCacheRef = new AtomicReference<>(
         PerBuildSyscallCache.newBuilder().setConcurrencyLevel(legacyGlobbingThreads).build());
     PackageFactory pkgFactory =