Move SkylarkImport from LoadStatement to BuildFileAST

This allow us to skip the import validation in non-build usages.

--
MOS_MIGRATED_REVID=130936612

5 files changed, 151 insertions, 80 deletions

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 8b58cc7f3e..98ab05583e 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
@@ -13,15 +13,24 @@
 // limitations under the License.
 package com.google.devtools.build.lib.syntax;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableBiMap;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.hash.HashCode;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.syntax.Parser.ParseResult;
+import com.google.devtools.build.lib.syntax.SkylarkImports.SkylarkImportSyntaxException;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import javax.annotation.Nullable;
 
 /**
@@ -33,42 +42,51 @@ public class BuildFileAST extends ASTNode {
 
   private final ImmutableList<Comment> comments;
 
-  private ImmutableList<SkylarkImport> imports;
+  @Nullable private final Map<String, SkylarkImport> imports;
 
   /**
    * Whether any errors were encountered during scanning or parsing.
    */
   private final boolean containsErrors;
 
-  private final String contentHashCode;
-
-  private BuildFileAST(List<Statement> preludeStatements, Parser.ParseResult result) {
-    this(preludeStatements, result, null);
-  }
-
-  private BuildFileAST(List<Statement> preludeStatements,
-      Parser.ParseResult result, String contentHashCode) {
-    this.stmts = ImmutableList.<Statement>builder()
-        .addAll(preludeStatements)
-        .addAll(result.statements)
-        .build();
-    this.comments = ImmutableList.copyOf(result.comments);
-    this.containsErrors = result.containsErrors;
-    this.contentHashCode = contentHashCode;
-    setLocation(result.location);
-  }
+  @Nullable private final String contentHashCode;
 
   private BuildFileAST(
       ImmutableList<Statement> stmts,
       boolean containsErrors,
       String contentHashCode,
       Location location,
-      ImmutableList<Comment> comments) {
+      ImmutableList<Comment> comments,
+      @Nullable Map<String, SkylarkImport> imports) {
     this.stmts = stmts;
     this.containsErrors = containsErrors;
     this.contentHashCode = contentHashCode;
     this.comments = comments;
     this.setLocation(location);
+    this.imports = imports;
+  }
+
+  private static BuildFileAST create(
+      List<Statement> preludeStatements,
+      ParseResult result,
+      String contentHashCode,
+      EventHandler eventHandler) {
+    ImmutableList<Statement> stmts =
+        ImmutableList.<Statement>builder()
+            .addAll(preludeStatements)
+            .addAll(result.statements)
+            .build();
+
+    boolean containsErrors = result.containsErrors;
+    Pair<Boolean, Map<String, SkylarkImport>> skylarkImports = fetchLoads(stmts, eventHandler);
+    containsErrors |= skylarkImports.first;
+    return new BuildFileAST(
+        stmts,
+        containsErrors,
+        contentHashCode,
+        result.location,
+        ImmutableList.copyOf(result.comments),
+        skylarkImports.second);
   }
 
   /**
@@ -76,24 +94,48 @@ public class BuildFileAST extends ASTNode {
    * {@code lastStatement} excluded.
    */
   public BuildFileAST subTree(int firstStatement, int lastStatement) {
+    ImmutableList<Statement> stmts = this.stmts.subList(firstStatement, lastStatement);
+    ImmutableMap.Builder<String, SkylarkImport> imports = ImmutableBiMap.builder();
+    for (Statement stmt : stmts) {
+      if (stmt instanceof LoadStatement) {
+        String str = ((LoadStatement) stmt).getImport();
+        imports.put(
+            str,
+            Preconditions.checkNotNull(
+                this.imports.get(str), "%s cannot be found. This is an internal error.", str));
+      }
+    }
     return new BuildFileAST(
-        stmts.subList(firstStatement, lastStatement),
+        stmts,
         containsErrors,
         null,
-        stmts.get(firstStatement).getLocation(),
-        ImmutableList.<Comment>of());
+        this.stmts.get(firstStatement).getLocation(),
+        ImmutableList.<Comment>of(),
+        imports.build());
   }
 
-  /** Collects all load statements */
-  private ImmutableList<SkylarkImport> fetchLoads(List<Statement> stmts) {
-    ImmutableList.Builder<SkylarkImport> imports = new ImmutableList.Builder<>();
+  /**
+   * Collects all load statements. Returns a pair with a boolean saying if there were errors and the
+   * imports that could be resolved.
+   */
+  @VisibleForTesting
+  static Pair<Boolean, Map<String, SkylarkImport>> fetchLoads(
+      List<Statement> stmts, EventHandler eventHandler) {
+    Map<String, SkylarkImport> imports = new HashMap<>();
+    boolean error = false;
     for (Statement stmt : stmts) {
       if (stmt instanceof LoadStatement) {
-        SkylarkImport imp = ((LoadStatement) stmt).getImport();
-        imports.add(imp);
+        String importString = ((LoadStatement) stmt).getImport();
+        try {
+          SkylarkImport imp = SkylarkImports.create(importString);
+          imports.put(importString, imp);
+        } catch (SkylarkImportSyntaxException e) {
+          eventHandler.handle(Event.error(stmt.getLocation(), e.getMessage()));
+          error = true;
+        }
       }
     }
-    return imports.build();
+    return Pair.of(error, imports);
   }
 
   /**
@@ -119,30 +161,36 @@ public class BuildFileAST extends ASTNode {
     return comments;
   }
 
-  /**
-   * Returns a list of loads in this BUILD file.
-   */
-  public synchronized ImmutableList<SkylarkImport> getImports() {
-    if (imports == null) {
-      imports = fetchLoads(stmts);
-    }
-    return imports;
+  /** Returns a list of loads in this BUILD file. */
+  public ImmutableList<SkylarkImport> getImports() {
+    Preconditions.checkNotNull(imports, "computeImports Should be called in parse* methods");
+    return ImmutableList.copyOf(imports.values());
   }
 
+  /** Returns a list of loads as strings in this BUILD file. */
+  public synchronized ImmutableList<String> getRawImports() {
+    ImmutableList.Builder<String> imports = ImmutableList.builder();
+
+    for (Statement stmt : stmts) {
+      if (stmt instanceof LoadStatement) {
+        imports.add(((LoadStatement) stmt).getImport());
+      }
+    }
+    return imports.build();
+  }
   /**
    * Executes this build file in a given Environment.
    *
-   * <p>If, for any reason, execution of a statement cannot be completed, an
-   * {@link EvalException} is thrown by {@link Statement#exec(Environment)}.
-   * This exception is caught here and reported through reporter and execution
-   * continues on the next statement.  In effect, there is a "try/except" block
-   * around every top level statement.  Such exceptions are not ignored, though:
-   * they are visible via the return value.  Rules declared in a package
-   * containing any error (including loading-phase semantical errors that
-   * cannot be checked here) must also be considered "in error".
+   * <p>If, for any reason, execution of a statement cannot be completed, an {@link EvalException}
+   * is thrown by {@link Statement#exec(Environment)}. This exception is caught here and reported
+   * through reporter and execution continues on the next statement. In effect, there is a
+   * "try/except" block around every top level statement. Such exceptions are not ignored, though:
+   * they are visible via the return value. Rules declared in a package containing any error
+   * (including loading-phase semantical errors that cannot be checked here) must also be considered
+   * "in error".
    *
-   * <p>Note that this method will not affect the value of {@link
-   * #containsErrors()}; that refers only to lexer/parser errors.
+   * <p>Note that this method will not affect the value of {@link #containsErrors()}; that refers
+   * only to lexer/parser errors.
    *
    * @return true if no error occurred during execution.
    */
@@ -206,12 +254,12 @@ public class BuildFileAST extends ASTNode {
                                             List<Statement> preludeStatements,
                                             EventHandler eventHandler) {
     Parser.ParseResult result = Parser.parseFile(input, eventHandler, false);
-    return new BuildFileAST(preludeStatements, result);
+    return create(preludeStatements, result, /*contentHashCode=*/ null, eventHandler);
   }
 
   public static BuildFileAST parseBuildFile(ParserInputSource input, EventHandler eventHandler) {
     Parser.ParseResult result = Parser.parseFile(input, eventHandler, false);
-    return new BuildFileAST(ImmutableList.<Statement>of(), result);
+    return create(ImmutableList.<Statement>of(), result, /*contentHashCode=*/ null, eventHandler);
   }
 
   /**
@@ -229,8 +277,33 @@ public class BuildFileAST extends ASTNode {
       throws IOException {
     ParserInputSource input = ParserInputSource.create(file, fileSize);
     Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
-    return new BuildFileAST(ImmutableList.<Statement>of(), result,
-        HashCode.fromBytes(file.getMD5Digest()).toString());
+    return create(
+        ImmutableList.<Statement>of(), result,
+        HashCode.fromBytes(file.getMD5Digest()).toString(), eventHandler);
+  }
+
+  /**
+   * Parse the specified non-build Skylark file but avoid the validation of the imports, returning
+   * its AST. All errors during scanning or parsing will be reported to the reporter.
+   *
+   * <p>This method should not be used in Bazel code, since it doesn't validate that the imports are
+   * syntactically valid.
+   *
+   * @throws IOException if the file cannot not be read.
+   */
+  public static BuildFileAST parseSkylarkFileWithoutImports(
+      ParserInputSource input, EventHandler eventHandler) throws IOException {
+    ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
+    return new BuildFileAST(
+        ImmutableList.<Statement>builder()
+            .addAll(ImmutableList.<Statement>of())
+            .addAll(result.statements)
+            .build(),
+        result.containsErrors, /*contentHashCode=*/
+        null,
+        result.location,
+        ImmutableList.copyOf(result.comments), /*imports=*/
+        null);
   }
 
   /**
@@ -243,14 +316,14 @@ public class BuildFileAST extends ASTNode {
     if (valid || containsErrors) {
       return this;
     }
-    return new BuildFileAST(stmts, true, contentHashCode, getLocation(), comments);
+    return new BuildFileAST(stmts, true, contentHashCode, getLocation(), comments, imports);
   }
 
   public static BuildFileAST parseBuildString(EventHandler eventHandler, String... content) {
     String str = Joiner.on("\n").join(content);
     ParserInputSource input = ParserInputSource.create(str, null);
     Parser.ParseResult result = Parser.parseFile(input, eventHandler, false);
-    return new BuildFileAST(ImmutableList.<Statement>of(), result, null);
+    return create(ImmutableList.<Statement>of(), result, null, eventHandler);
   }
 
   // TODO(laurentlb): Merge parseSkylarkString and parseBuildString.
@@ -258,7 +331,7 @@ public class BuildFileAST extends ASTNode {
     String str = Joiner.on("\n").join(content);
     ParserInputSource input = ParserInputSource.create(str, null);
     Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
-    return new BuildFileAST(ImmutableList.<Statement>of(), result, null);
+    return create(ImmutableList.<Statement>of(), result, null, eventHandler);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index ec504a06ec..f7ea9f98ca 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -927,14 +927,18 @@ public final class Environment implements Freezable {
   @VisibleForTesting
   public List<Statement> parseFile(String... inputLines) {
     ParserInputSource input = ParserInputSource.create(Joiner.on("\n").join(inputLines), null);
+    List<Statement> statements;
     if (isSkylark) {
       Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
       ValidationEnvironment valid = new ValidationEnvironment(this);
       valid.validateAst(result.statements, eventHandler);
-      return result.statements;
+      statements = result.statements;
+    } else {
+      statements = Parser.parseFile(input, eventHandler, /*parsePython=*/false).statements;
     }
-
-    return Parser.parseFile(input, eventHandler, /*parsePython=*/ false).statements;
+    // Force the validation of imports
+    BuildFileAST.fetchLoads(statements, eventHandler);
+    return statements;
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
index 35fb5d1784..2a6e0fbcab 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java
@@ -33,7 +33,7 @@ public final class LoadStatement extends Statement {
 
   private final ImmutableMap<Identifier, String> symbols;
   private final ImmutableList<Identifier> cachedSymbols; // to save time
-  private final SkylarkImport imp;
+  private final String imp;
   private final Location importLocation;
 
   /**
@@ -43,7 +43,7 @@ public final class LoadStatement extends Statement {
    * the bzl file that should be loaded. If aliasing is used, the value differs from its key's
    * {@code symbol.getName()}. Otherwise, both values are identical.
    */
-  LoadStatement(SkylarkImport imp, Location importLocation, Map<Identifier, String> symbols) {
+  LoadStatement(String imp, Location importLocation, Map<Identifier, String> symbols) {
     this.imp = imp;
     this.importLocation = importLocation;
     this.symbols = ImmutableMap.copyOf(symbols);
@@ -58,14 +58,14 @@ public final class LoadStatement extends Statement {
     return cachedSymbols;
   }
 
-  public SkylarkImport getImport() {
+  public String getImport() {
     return imp;
   }
 
   @Override
   public String toString() {
     return String.format(
-        "load(\"%s\", %s)", imp.getImportString(), Joiner.on(", ").join(cachedSymbols));
+        "load(\"%s\", %s)", imp, Joiner.on(", ").join(cachedSymbols));
   }
 
   @Override
@@ -81,7 +81,7 @@ public final class LoadStatement extends Statement {
         }
         // The key is the original name that was used to define the symbol
         // in the loaded bzl file.
-        env.importSymbol(imp.getImportString(), name, declared.getName());
+        env.importSymbol(imp, name, declared.getName());
       } catch (Environment.LoadFailedException e) {
         throw new EvalException(getLocation(), e.getMessage());
       }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
index fe9dc53841..1e6d9d84ce 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java
@@ -29,7 +29,6 @@ import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLiteral;
 import com.google.devtools.build.lib.syntax.IfStatement.ConditionalStatements;
-import com.google.devtools.build.lib.syntax.SkylarkImports.SkylarkImportSyntaxException;
 import com.google.devtools.build.lib.util.Preconditions;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -1060,16 +1059,9 @@ public class Parser {
     }
     expect(TokenKind.RPAREN);
 
-    SkylarkImport imp;
-    try {
-      imp = SkylarkImports.create(importString.getValue());
-      LoadStatement stmt = new LoadStatement(imp, importString.getLocation(), symbols);
-      list.add(setLocation(stmt, start, token.left));
-    } catch (SkylarkImportSyntaxException e) {
-      String msg = "Load statement parameter '" + importString + "' is invalid. "
-          + e.getMessage();
-      reportError(importString.getLocation(), msg);
-    }
+    LoadStatement stmt = new LoadStatement(
+        importString.getValue(), importString.getLocation(), symbols);
+    list.add(setLocation(stmt, start, token.left));
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
index 3878fc4ea3..d6897dce7d 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.Argument.Passed;
 import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLiteral;
+import com.google.devtools.build.lib.syntax.SkylarkImports.SkylarkImportSyntaxException;
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -1086,12 +1087,12 @@ public class ParserTest extends EvaluationTestCase {
   }
 
   @Test
-  public void testValidAbsoluteImportPath() {
+  public void testValidAbsoluteImportPath() throws SkylarkImportSyntaxException {
     String importString = "/some/skylark/file";
     List<Statement> statements =
         parseFileForSkylark("load('" + importString + "', 'fun_test')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    SkylarkImport imp = stmt.getImport();
+    SkylarkImport imp = SkylarkImports.create(stmt.getImport());
 
     assertThat(imp.getImportString()).named("getImportString()").isEqualTo("/some/skylark/file");
     assertThat(imp.hasAbsolutePath()).named("hasAbsolutePath()").isTrue();
@@ -1106,11 +1107,11 @@ public class ParserTest extends EvaluationTestCase {
   }
 
   private void validNonAbsoluteImportTest(String importString, String containingFileLabelString,
-      String expectedLabelString) {
+      String expectedLabelString) throws SkylarkImportSyntaxException {
     List<Statement> statements =
         parseFileForSkylark("load('" + importString + "', 'fun_test')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    SkylarkImport imp = stmt.getImport();
+    SkylarkImport imp = SkylarkImports.create(stmt.getImport());
 
     assertThat(imp.getImportString()).named("getImportString()").isEqualTo(importString);
     assertThat(imp.hasAbsolutePath()).named("hasAbsolutePath()").isFalse();
@@ -1159,7 +1160,8 @@ public class ParserTest extends EvaluationTestCase {
     invalidImportTest("\tfile", SkylarkImports.INVALID_FILENAME_PREFIX);
   }
 
-  private void validAbsoluteImportLabelTest(String importString) {
+  private void validAbsoluteImportLabelTest(String importString)
+      throws SkylarkImportSyntaxException {
     validNonAbsoluteImportTest(importString, /*irrelevant*/ "//another/path:BUILD",
         /*expected*/ importString);
   }
@@ -1242,7 +1244,7 @@ public class ParserTest extends EvaluationTestCase {
     List<Statement> statements = parseFileForSkylark(
         "load('/foo/bar/file', 'fun_test')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    assertEquals("/foo/bar/file", stmt.getImport().getImportString());
+    assertEquals("/foo/bar/file", stmt.getImport());
     assertThat(stmt.getSymbols()).hasSize(1);
     Identifier sym = stmt.getSymbols().get(0);
     int startOffset = sym.getLocation().getStartOffset();
@@ -1256,7 +1258,7 @@ public class ParserTest extends EvaluationTestCase {
     List<Statement> statements = parseFileForSkylark(
         "load('/foo/bar/file', 'fun_test',)\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    assertEquals("/foo/bar/file", stmt.getImport().getImportString());
+    assertEquals("/foo/bar/file", stmt.getImport());
     assertThat(stmt.getSymbols()).hasSize(1);
   }
 
@@ -1265,7 +1267,7 @@ public class ParserTest extends EvaluationTestCase {
     List<Statement> statements = parseFileForSkylark(
         "load('file', 'foo', 'bar')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    assertEquals("file", stmt.getImport().getImportString());
+    assertEquals("file", stmt.getImport());
     assertThat(stmt.getSymbols()).hasSize(2);
   }