Move skylark import dependency registration to after the preprocessor.

RELNOTES: allow load() in subincluded files.

--
MOS_MIGRATED_REVID=100125415

3 files changed, 37 insertions, 64 deletions

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 5056ac1759..c342eca1a1 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
@@ -458,13 +458,6 @@ public class PackageFunction implements SkyFunction {
           packageId, e.getMessage()), Transience.TRANSIENT);
     }
 
-    SkylarkImportResult importResult =
-        discoverSkylarkImports(
-            buildFilePath, buildFileFragment, packageId, env, inputSource, preludeStatements);
-    if (importResult == null) {
-      return null;
-    }
-
     Package.LegacyBuilder legacyPkgBuilder =
         loadPackage(
             externalPkg,
@@ -472,9 +465,13 @@ public class PackageFunction implements SkyFunction {
             replacementContents,
             packageId,
             buildFilePath,
+            buildFileFragment,
             defaultVisibility,
             preludeStatements,
-            importResult);
+            env);
+    if (legacyPkgBuilder == null) {
+      return null;
+    }
     legacyPkgBuilder.buildPartial();
     try {
       handleLabelsCrossingSubpackagesAndPropagateInconsistentFilesystemExceptions(
@@ -518,6 +515,9 @@ public class PackageFunction implements SkyFunction {
     return new PackageValue(pkg);
   }
 
+  /**
+   * Returns true if includes referencing a different repository have already been computed.
+   */
   private boolean fetchIncludeRepositoryDeps(Environment env, BuildFileAST ast) {
     boolean ok = true;
     for (Label label : ast.getIncludes()) {
@@ -534,7 +534,7 @@ public class PackageFunction implements SkyFunction {
     return ok;
   }
 
-  // Why does this need both buildFilePath and buildFileFragment?
+  // TODO(bazel-team): this should take the AST so we don't parse the file twice.
   @Nullable
   private SkylarkImportResult discoverSkylarkImports(
       Path buildFilePath,
@@ -546,13 +546,15 @@ public class PackageFunction implements SkyFunction {
       throws PackageFunctionException {
     StoredEventHandler eventHandler = new StoredEventHandler();
     BuildFileAST buildFileAST =
-        BuildFileAST.parseBuildFile(inputSource, preludeStatements, eventHandler, null, true);
+        BuildFileAST.parseBuildFile(
+            inputSource,
+            preludeStatements,
+            eventHandler,
+            /* package locator */ null,
+            /* parse python */ false);
     SkylarkImportResult importResult;
     boolean includeRepositoriesFetched;
     if (eventHandler.hasErrors()) {
-      // In case of Python preprocessing, errors have already been reported (see checkSyntax).
-      // In other cases, errors will be reported later.
-      // TODO(bazel-team): maybe we could get rid of checkSyntax and always report errors here?
       importResult =
           new SkylarkImportResult(
               ImmutableMap.<PathFragment, SkylarkEnvironment>of(), ImmutableList.<Label>of());
@@ -570,6 +572,10 @@ public class PackageFunction implements SkyFunction {
     return importResult;
   }
 
+  /**
+   * Fetch the skylark loads for this BUILD file. If any of them haven't been computed yet,
+   * returns null.
+   */
   @Nullable
   private SkylarkImportResult fetchImportsFromBuildFile(
       Path buildFilePath,
@@ -762,16 +768,20 @@ public class PackageFunction implements SkyFunction {
   /**
    * Constructs a {@link Package} object for the given package using legacy package loading.
    * Note that the returned package may be in error.
+   *
+   * <p>May return null if the computation has to be restarted.
    */
+  @Nullable
   private Package.LegacyBuilder loadPackage(
       Package externalPkg,
       ParserInputSource inputSource,
       @Nullable String replacementContents,
       PackageIdentifier packageId,
       Path buildFilePath,
+      PathFragment buildFileFragment,
       RuleVisibility defaultVisibility,
       List<Statement> preludeStatements,
-      SkylarkImportResult importResult)
+      Environment env)
       throws InterruptedException, PackageFunctionException {
     ParserInputSource replacementSource = replacementContents == null ? null
         : ParserInputSource.create(replacementContents, buildFilePath.asFragment());
@@ -786,6 +796,19 @@ public class PackageFunction implements SkyFunction {
             ? packageFactory.preprocess(packageId, buildFilePath, inputSource, globber,
                 localReporter)
                 : Preprocessor.Result.noPreprocessing(replacementSource);
+
+        SkylarkImportResult importResult =
+            discoverSkylarkImports(
+                buildFilePath,
+                buildFileFragment,
+                packageId,
+                env,
+                preprocessingResult.result,
+                preludeStatements);
+        if (importResult == null) {
+          return null;
+        }
+
         pkgBuilder = packageFactory.createPackageFromPreprocessingResult(externalPkg, packageId,
             buildFilePath, preprocessingResult, localReporter.getEvents(), preludeStatements,
             importResult.importMap, importResult.fileDependencies, packageLocator,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
index 35a9af8477..e8f901b72c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
@@ -39,8 +39,6 @@ public class BuildFileAST extends ASTNode {
 
   private ImmutableSet<PathFragment> loads;
 
-  private ImmutableSet<String> subincludes;
-
   private ImmutableSet<Label> includes;
 
   /**
@@ -72,30 +70,6 @@ public class BuildFileAST extends ASTNode {
     }
   }
 
-  /** Collects labels from all subinclude statements */
-  private ImmutableSet<String> fetchSubincludes(List<Statement> stmts) {
-    ImmutableSet.Builder<String> subincludes = new ImmutableSet.Builder<>();
-    for (Statement stmt : stmts) {
-      // The code below matches:  subinclude("literal string")
-      if (!(stmt instanceof ExpressionStatement)) {
-        continue;
-      }
-      Expression expr = ((ExpressionStatement) stmt).getExpression();
-      if (!(expr instanceof FuncallExpression)) {
-        continue;
-      }
-      FuncallExpression call = (FuncallExpression) expr;
-      if (!call.getFunction().getName().equals("subinclude") || call.getArguments().size() != 1) {
-        continue;
-      }
-      Expression arg = call.getArguments().get(0).getValue();
-      if (arg instanceof StringLiteral) {
-        subincludes.add(((StringLiteral) arg).getValue());
-      }
-    }
-    return subincludes.build();
-  }
-
   private ImmutableSet<Label> fetchIncludes(List<Statement> stmts) {
     ImmutableSet.Builder<Label> result = new ImmutableSet.Builder<>();
     for (Statement stmt : stmts) {
@@ -175,16 +149,6 @@ public class BuildFileAST extends ASTNode {
     return loads;
   }
 
-  /**
-   * Returns a set of subincludes in this BUILD file.
-   */
-  public synchronized ImmutableSet<String> getSubincludes() {
-    if (subincludes == null) {
-      subincludes = fetchSubincludes(stmts);
-    }
-    return subincludes;
-  }
-
   public synchronized ImmutableSet<Label> getIncludes() {
     if (includes == null) {
       includes = fetchIncludes(stmts);
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
index 4d9a098a56..b0dd2dee8a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BuildFileASTTest.java
@@ -322,18 +322,4 @@ public class BuildFileASTTest {
     BuildFileAST buildFileAST = parseBuildFile("include('//foo:bar')");
     assertThat(buildFileAST.getStatements()).hasSize(1);
   }
-
-  @Test
-  public void testFetchSubincludes() throws Exception {
-    BuildFileAST buildFileAST =
-        parseBuildFile(
-            "foo('a')\n",
-            "subinclude()\n",
-            "subinclude('hello')\n",
-            "1 + subinclude('ignored')",
-            "var = 'also ignored'",
-            "subinclude(var)\n",
-            "subinclude('world')");
-    assertThat(buildFileAST.getSubincludes()).containsExactly("hello", "world");
-  }
 }