Include the Globber in the PackageFunction AstAfterPreprocessing cache. Otherwise we have potential correctness and performance problems on a missing Skylark import dep.

--
MOS_MIGRATED_REVID=107605200

3 files changed, 21 insertions, 6 deletions

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 d62fe12a03..987b5727a2 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
@@ -1006,8 +1006,8 @@ public final class PackageFactory {
     // show up below.
     BuildFileAST buildFileAST = parseBuildFile(packageId, preprocessingResult.result,
         preludeStatements, localReporterForParsing);
-    AstAfterPreprocessing astAfterPreprocessing =
-        new AstAfterPreprocessing(preprocessingResult, buildFileAST, localReporterForParsing);
+    AstAfterPreprocessing astAfterPreprocessing = new AstAfterPreprocessing(preprocessingResult,
+        buildFileAST, localReporterForParsing, /*globber=*/null);
     return createPackageFromPreprocessingAst(
         externalPkg,
         packageId,
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
index 08bc380524..53e383454c 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Preprocessor.java
@@ -181,15 +181,18 @@ public interface Preprocessor {
     public final BuildFileAST ast;
     public final boolean containsAstParsingErrors;
     public final Iterable<Event> allEvents;
+    @Nullable
+    public final Globber globber;
 
     public AstAfterPreprocessing(Result preprocessingResult, BuildFileAST ast,
-        StoredEventHandler astParsingEventHandler) {
+        StoredEventHandler astParsingEventHandler, @Nullable Globber globber) {
       this.ast = ast;
       this.preprocessed = preprocessingResult.preprocessed;
       this.containsPreprocessingErrors = preprocessingResult.containsErrors;
       this.containsAstParsingErrors = astParsingEventHandler.hasErrors();
       this.allEvents = Iterables.concat(
           preprocessingResult.events, astParsingEventHandler.getEvents());
+      this.globber = globber;
     }
   }
 }
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 ee5ec1a0e4..3829c98f02 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
@@ -867,13 +867,13 @@ public class PackageFunction implements SkyFunction {
     if (pkgBuilder == null) {
       profiler.startTask(ProfilerTask.CREATE_PACKAGE, packageId.toString());
       try {
-        Globber globber = packageFactory.createLegacyGlobber(buildFilePath.getParentDirectory(),
-            packageId, packageLocator);
         AstAfterPreprocessing astAfterPreprocessing = astCache.getIfPresent(packageId);
         if (astAfterPreprocessing == null) {
           if (showLoadingProgress.get()) {
             env.getListener().handle(Event.progress("Loading package: " + packageId));
           }
+          Globber globber = packageFactory.createLegacyGlobber(buildFilePath.getParentDirectory(),
+              packageId, packageLocator);
           Preprocessor.Result preprocessingResult;
           if (replacementContents == null) {
             Preconditions.checkNotNull(buildFileValue, packageId);
@@ -909,8 +909,12 @@ public class PackageFunction implements SkyFunction {
           StoredEventHandler astParsingEventHandler = new StoredEventHandler();
           BuildFileAST ast = PackageFactory.parseBuildFile(packageId, preprocessingResult.result,
               preludeStatements, astParsingEventHandler);
+          // If no globs were fetched during preprocessing, then there's no need to reuse *this
+          // globber instance* during BUILD file evaluation; the correctness and performance
+          // arguments below do not apply.
+          Globber globberToStore = globber.getGlobPatterns().isEmpty() ? null : globber;
           astAfterPreprocessing = new AstAfterPreprocessing(preprocessingResult, ast,
-              astParsingEventHandler);
+              astParsingEventHandler, globberToStore);
           astCache.put(packageId, astAfterPreprocessing);
         }
         SkylarkImportResult importResult;
@@ -929,6 +933,14 @@ public class PackageFunction implements SkyFunction {
           return null;
         }
         astCache.invalidate(packageId);
+        // If the globber was used to evaluate globs during preprocessing, it's important that we
+        // reuse that globber during BUILD file evaluation for two reasons: (i) correctness, since
+        // Skyframe deps are added after the fact (ii) performance, in the case that globs were
+        // fetched lazily during preprocessing.
+        Globber globber = astAfterPreprocessing.globber != null
+            ? astAfterPreprocessing.globber
+            : packageFactory.createLegacyGlobber(buildFilePath.getParentDirectory(),
+                packageId, packageLocator);
         pkgBuilder = packageFactory.createPackageFromPreprocessingAst(externalPkg, packageId,
             buildFilePath, astAfterPreprocessing, importResult.importMap,
             importResult.fileDependencies, defaultVisibility, globber);