Remove static checks from the parser.

--
MOS_MIGRATED_REVID=129313959

5 files changed, 62 insertions, 49 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
index 9c28b41faf..7419f31b7e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java
@@ -109,17 +109,20 @@ public class ASTFileLookupFunction implements SkyFunction {
       long astFileSize = fileValue.getSize();
       if (parseAsSkylark) {
         try (Mutability mutability = Mutability.create("validate")) {
-            ast = BuildFileAST.parseSkylarkFile(path, astFileSize, env.getListener(),
-                new ValidationEnvironment(
-                    ruleClassProvider.createSkylarkRuleClassEnvironment(
-                        fileLabel,
-                        mutability,
-                        env.getListener(),
-                        // the two below don't matter for extracting the ValidationEnvironment:
-                        /*astFileContentHashCode=*/null,
-                        /*importMap=*/null)
-                    .setupDynamic(Runtime.PKG_NAME, Runtime.NONE)
-                    .setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE)));
+          ValidationEnvironment validationEnv =
+              new ValidationEnvironment(
+                  ruleClassProvider
+                      .createSkylarkRuleClassEnvironment(
+                          fileLabel,
+                          mutability,
+                          env.getListener(),
+                          // the two below don't matter for extracting the ValidationEnvironment:
+                          /*astFileContentHashCode=*/ null,
+                          /*importMap=*/ null)
+                      .setupDynamic(Runtime.PKG_NAME, Runtime.NONE)
+                      .setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE));
+          ast = BuildFileAST.parseSkylarkFile(path, astFileSize, env.getListener());
+          ast = ast.validate(validationEnv, env.getListener());
         }
       } else {
         ast = BuildFileAST.parseBuildFile(path, astFileSize, env.getListener());
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 c544c08ef8..aa83dbc975 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
@@ -62,11 +62,12 @@ public class BuildFileAST extends ASTNode {
       ImmutableList<Statement> stmts,
       boolean containsErrors,
       String contentHashCode,
-      Location location) {
+      Location location,
+      ImmutableList<Comment> comments) {
     this.stmts = stmts;
     this.containsErrors = containsErrors;
     this.contentHashCode = contentHashCode;
-    this.comments = ImmutableList.of();
+    this.comments = comments;
     this.setLocation(location);
   }
 
@@ -79,7 +80,8 @@ public class BuildFileAST extends ASTNode {
         stmts.subList(firstStatement, lastStatement),
         containsErrors,
         null,
-        stmts.get(firstStatement).getLocation());
+        stmts.get(firstStatement).getLocation(),
+        ImmutableList.of());
   }
 
   /** Collects all load statements */
@@ -213,26 +215,37 @@ public class BuildFileAST extends ASTNode {
   }
 
   /**
-   * Parse the specified Skylark file, returning its AST. All errors during
-   * scanning or parsing will be reported to the reporter.
+   * Parse the specified Skylark file, returning its AST. All errors during scanning or parsing will
+   * be reported to the reporter.
    *
    * @throws IOException if the file cannot not be read.
    */
-  public static BuildFileAST parseSkylarkFile(Path file, EventHandler eventHandler,
-      ValidationEnvironment validationEnvironment) throws IOException {
-    return parseSkylarkFile(file, file.getFileSize(), eventHandler,
-        validationEnvironment);
+  public static BuildFileAST parseSkylarkFile(Path file, EventHandler eventHandler)
+      throws IOException {
+    return parseSkylarkFile(file, file.getFileSize(), eventHandler);
   }
 
-  public static BuildFileAST parseSkylarkFile(Path file, long fileSize, EventHandler eventHandler,
-      ValidationEnvironment validationEnvironment) throws IOException {
+  public static BuildFileAST parseSkylarkFile(Path file, long fileSize, EventHandler eventHandler)
+      throws IOException {
     ParserInputSource input = ParserInputSource.create(file, fileSize);
-    Parser.ParseResult result =
-        Parser.parseFileForSkylark(input, eventHandler, validationEnvironment);
+    Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
     return new BuildFileAST(ImmutableList.<Statement>of(), result,
         HashCode.fromBytes(file.getMD5Digest()).toString());
   }
 
+  /**
+   * Run static checks on the AST.
+   *
+   * @return a new AST (or the same), with the containsErrors flag updated.
+   */
+  public BuildFileAST validate(ValidationEnvironment validationEnv, EventHandler eventHandler) {
+    boolean valid = validationEnv.validateAst(stmts, eventHandler);
+    if (valid || containsErrors) {
+      return this;
+    }
+    return new BuildFileAST(stmts, true, contentHashCode, getLocation(), comments);
+  }
+
   public static BuildFileAST parseBuildString(EventHandler eventHandler, String... content) {
     String str = Joiner.on("\n").join(content);
     ParserInputSource input = ParserInputSource.create(str, null);
@@ -244,7 +257,7 @@ public class BuildFileAST extends ASTNode {
   public static BuildFileAST parseSkylarkString(EventHandler eventHandler, String... content) {
     String str = Joiner.on("\n").join(content);
     ParserInputSource input = ParserInputSource.create(str, null);
-    Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler, null);
+    Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
     return new BuildFileAST(ImmutableList.<Statement>of(), result, null);
   }
 
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 4418167d33..9d6ec1c52b 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
@@ -945,10 +945,14 @@ public final class Environment implements Freezable {
   @VisibleForTesting
   public List<Statement> parseFile(String... inputLines) {
     ParserInputSource input = ParserInputSource.create(Joiner.on("\n").join(inputLines), null);
-    Parser.ParseResult result = isSkylark
-        ? Parser.parseFileForSkylark(input, eventHandler, new ValidationEnvironment(this))
-        : Parser.parseFile(input, eventHandler, /*parsePython=*/false);
-    return result.statements;
+    if (isSkylark) {
+      Parser.ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
+      ValidationEnvironment valid = new ValidationEnvironment(this);
+      valid.validateAst(result.statements, eventHandler);
+      return result.statements;
+    }
+
+    return Parser.parseFile(input, eventHandler, /*parsePython=*/ false).statements;
   }
 
   /**
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 641f7d9bfd..fe9dc53841 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
@@ -31,7 +31,6 @@ import com.google.devtools.build.lib.syntax.DictionaryLiteral.DictionaryEntryLit
 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;
 import java.util.EnumSet;
@@ -40,7 +39,6 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
-import javax.annotation.Nullable;
 
 /**
  * Recursive descent parser for LL(2) BUILD language.
@@ -229,23 +227,20 @@ public class Parser {
   }
 
   /**
-   * Entry-point to parser that parses a build file with comments.  All errors
-   * encountered during parsing are reported via "reporter".  Enable Skylark extensions
-   * that are not part of the core BUILD language.
+   * Entry-point to parser that parses a build file with comments. All errors encountered during
+   * parsing are reported via "reporter". Enable Skylark extensions that are not part of the core
+   * BUILD language.
    */
   public static ParseResult parseFileForSkylark(
-      ParserInputSource input,
-      EventHandler eventHandler,
-      @Nullable ValidationEnvironment validationEnvironment) {
+      ParserInputSource input, EventHandler eventHandler) {
     Lexer lexer = new Lexer(input, eventHandler, false);
     Parser parser = new Parser(lexer, eventHandler, SKYLARK);
     List<Statement> statements = parser.parseFileInput();
-    // TODO(laurentlb): Remove validation from parser
-    boolean hasSemanticalErrors = validationEnvironment == null
-        ? false
-        : !validationEnvironment.validateAst(statements, eventHandler);
-    return new ParseResult(statements, parser.comments, locationFromStatements(lexer, statements),
-        parser.errorsCount > 0 || lexer.containsErrors() || hasSemanticalErrors);
+    return new ParseResult(
+        statements,
+        parser.comments,
+        locationFromStatements(lexer, statements),
+        parser.errorsCount > 0 || lexer.containsErrors());
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java
index 40bb7e58c1..a17cf63097 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitorTest.java
@@ -17,14 +17,12 @@ import static com.google.common.truth.Truth.assertThat;
 
 import com.google.devtools.build.lib.testutil.Scratch;
 import com.google.devtools.build.lib.vfs.Path;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 
 /** Tests for @{code SyntaxTreeVisitor} */
@@ -36,7 +34,7 @@ public class SyntaxTreeVisitorTest {
   /** Parses the contents of the specified string and returns the AST. */
   private BuildFileAST parse(String... lines) throws IOException {
     Path file = scratch.file("/a/build/file/BUILD", lines);
-    return BuildFileAST.parseSkylarkFile(file, null /* reporter */, null /*validationEnvironment*/);
+    return BuildFileAST.parseSkylarkFile(file, null /* reporter */);
   }
 
   @Test