Remove validate() methods in the AST, use a visitor instead.

This is a simple refactoring, no change in behavior.

RELNOTES: None.
PiperOrigin-RevId: 165572028

26 files changed, 128 insertions, 280 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
index 1e092d1b81..5cdaaa9476 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AbstractComprehension.java
@@ -76,8 +76,6 @@ public abstract class AbstractComprehension extends Expression {
     void eval(Environment env, OutputCollector collector, int step)
         throws EvalException, InterruptedException;
 
-    void validate(ValidationEnvironment env, Location loc) throws EvalException;
-
     /**
      * The LValue defined in Clause, i.e. the loop variables for ForClause and null for
      * IfClause. This is needed for SyntaxTreeVisitor.
@@ -130,12 +128,6 @@ public abstract class AbstractComprehension extends Expression {
     }
 
     @Override
-    public void validate(ValidationEnvironment env, Location loc) throws EvalException {
-      lvalue.validate(env, loc);
-      iterable.validate(env);
-    }
-
-    @Override
     public LValue getLValue() {
       return lvalue;
     }
@@ -190,11 +182,6 @@ public abstract class AbstractComprehension extends Expression {
     }
 
     @Override
-    public void validate(ValidationEnvironment env, Location loc) throws EvalException {
-      condition.validate(env);
-    }
-
-    @Override
     public LValue getLValue() {
       return null;
     }
@@ -307,27 +294,6 @@ public abstract class AbstractComprehension extends Expression {
     return result;
   }
 
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    // Create a new scope so that loop variables do not leak outside the comprehension.
-    if (env.getSemantics().incompatibleComprehensionVariablesDoNotLeak) {
-      env.openScope();
-    }
-
-    for (Clause clause : clauses) {
-      clause.validate(env, getLocation());
-    }
-
-    // Clauses have to be validated before expressions in order to introduce the variable names.
-    for (Expression expr : outputExpressions) {
-      expr.validate(env);
-    }
-
-    if (env.getSemantics().incompatibleComprehensionVariablesDoNotLeak) {
-      env.closeScope();
-    }
-  }
-
   /**
    * Evaluate the clause indexed by step, or elementExpression. When we evaluate the
    * comprehension, step is 0 and we evaluate the first clause. Each clause may
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
index 9b74f30b60..436384a43a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
@@ -66,10 +66,4 @@ public final class AssignmentStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    expression.validate(env);
-    lvalue.validate(env, getLocation());
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java
index 04f54e799b..902b73acdb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java
@@ -67,10 +67,4 @@ public final class AugmentedAssignmentStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    lvalue.validate(env, getLocation());
-    expression.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index 6678978ea2..65565ad7fa 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -259,12 +259,6 @@ public final class BinaryOperatorExpression extends Expression {
     visitor.visit(this);
   }
 
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    lhs.validate(env);
-    rhs.validate(env);
-  }
-
   /** Implements Operator.PLUS. */
   private static Object plus(
       Object lval, Object rval, Environment env, Location location, boolean isAugmented)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
index 4abc12ade7..c2d1c5de95 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ConditionalExpression.java
@@ -65,11 +65,4 @@ public final class ConditionalExpression extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    condition.validate(env);
-    thenCase.validate(env);
-    elseCase.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
index 8795fe0192..7a5abd2818 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DictionaryLiteral.java
@@ -102,12 +102,4 @@ public final class DictionaryLiteral extends Expression {
   public ImmutableList<DictionaryEntryLiteral> getEntries() {
     return entries;
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    for (DictionaryEntryLiteral entry : entries) {
-      entry.key.validate(env);
-      entry.value.validate(env);
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index e04bdf0538..056fca42ca 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -124,9 +124,4 @@ public final class DotExpression extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    object.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
index c92eebbd0f..ae1707c678 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Expression.java
@@ -59,16 +59,6 @@ public abstract class Expression extends ASTNode {
    */
   abstract Object doEval(Environment env) throws EvalException, InterruptedException;
 
-  /**
-   * Returns the inferred type of the result of the Expression.
-   *
-   * <p>Checks the semantics of the Expression using the {@link Environment} according to
-   * the rules of the Skylark language, throws {@link EvalException} in case of a semantical error.
-   *
-   * @see Statement
-   */
-  abstract void validate(ValidationEnvironment env) throws EvalException;
-
   @Override
   public final void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     prettyPrint(buffer);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
index c739dfaf1e..f0a5fa39ad 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ExpressionStatement.java
@@ -47,9 +47,4 @@ public final class ExpressionStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    expression.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
index a1639c95eb..187c4b83e0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
@@ -52,9 +52,6 @@ public final class FlowStatement extends Statement {
   }
 
   @Override
-  void validate(ValidationEnvironment env) throws EvalException {}
-
-  @Override
   public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     printIndent(buffer, indentLevel);
     buffer.append(kind.name);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index bb640ed295..381995d250 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -96,15 +96,4 @@ public final class ForStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    // TODO(bazel-team): validate variable. Maybe make it temporarily readonly.
-    collection.validate(env);
-    variable.validate(env, getLocation());
-
-    for (Statement stmt : block) {
-      stmt.validate(env);
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 55ae49d49b..e034b16bd3 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -761,14 +761,6 @@ public final class FuncallExpression extends Expression {
   }
 
   @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    function.validate(env);
-    for (Argument.Passed arg : arguments) {
-      arg.getValue().validate(env);
-    }
-  }
-
-  @Override
   protected boolean isNewScope() {
     return true;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
index 994f6f2b3d..31b16f0eba 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
@@ -109,37 +109,4 @@ public final class FunctionDefStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(final ValidationEnvironment env) throws EvalException {
-    FunctionSignature sig = signature.getSignature();
-    FunctionSignature.Shape shape = sig.getShape();
-    ImmutableList<String> names = sig.getNames();
-    List<Expression> defaultExpressions = signature.getDefaultValues();
-
-    int positionals = shape.getPositionals();
-    int mandatoryPositionals = shape.getMandatoryPositionals();
-    int namedOnly = shape.getNamedOnly();
-    int mandatoryNamedOnly = shape.getMandatoryNamedOnly();
-    boolean starArg = shape.hasStarArg();
-    boolean kwArg = shape.hasKwArg();
-    int named = positionals + namedOnly;
-    int args = named + (starArg ? 1 : 0) + (kwArg ? 1 : 0);
-    int startOptionals = mandatoryPositionals;
-    int endOptionals = named - mandatoryNamedOnly;
-
-    env.openScope();
-    int j = 0; // index for the defaultExpressions
-    for (int i = 0; i < args; i++) {
-      String name = names.get(i);
-      if (startOptionals <= i && i < endOptionals) {
-        defaultExpressions.get(j++).validate(env);
-      }
-      env.declare(name, getLocation());
-    }
-    for (Statement stmts : statements) {
-      stmts.validate(env);
-    }
-    env.closeScope();
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index a404bfa79f..419427c9b1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -85,14 +85,7 @@ public final class Identifier extends Expression {
     visitor.visit(this);
   }
 
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    if (!env.hasSymbolInEnvironment(name)) {
-      throw createInvalidIdentifierException(env.getAllSymbols());
-    }
-  }
-
-  private EvalException createInvalidIdentifierException(Set<String> symbols) {
+  EvalException createInvalidIdentifierException(Set<String> symbols) {
     if (name.equals("$error$")) {
       return new EvalException(getLocation(), "contains syntax error(s)", true);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
index c96f001c09..089f9c113a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
@@ -26,8 +26,8 @@ public final class IfStatement extends Statement {
   /**
    * Syntax node for an [el]if statement.
    *
-   * <p>This extends Statement because it implements {@code doExec} and {@code validate}, but it
-   * is not actually an independent statement in the grammar.
+   * <p>This extends Statement because it implements {@code doExec}, but it is not actually an
+   * independent statement in the grammar.
    */
   public static final class ConditionalStatements extends Statement {
 
@@ -69,14 +69,6 @@ public final class IfStatement extends Statement {
     public ImmutableList<Statement> getStatements() {
       return statements;
     }
-
-    @Override
-    void validate(ValidationEnvironment env) throws EvalException {
-      condition.validate(env);
-      for (Statement stmt : statements) {
-        stmt.validate(env);
-      }
-    }
   }
 
   /** "if" or "elif" clauses. Must be non-empty. */
@@ -141,14 +133,4 @@ public final class IfStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    for (ConditionalStatements stmts : thenBlocks) {
-      stmts.validate(env);
-    }
-    for (Statement stmt : elseBlock) {
-      stmt.validate(env);
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
index b341b33cc1..6597f9b742 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
@@ -83,9 +83,4 @@ public final class IndexExpression extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    object.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
index b940f50dc2..68192c82c2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IntegerLiteral.java
@@ -43,8 +43,4 @@ public final class IntegerLiteral extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index bd2160e320..b964a2750f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -229,25 +229,6 @@ public final class LValue extends ASTNode {
     visitor.visit(this);
   }
 
-  void validate(ValidationEnvironment env, Location loc) throws EvalException {
-    validate(env, loc, expr);
-  }
-
-  private static void validate(ValidationEnvironment env, Location loc, Expression expr)
-      throws EvalException {
-    if (expr instanceof Identifier) {
-      env.declare(((Identifier) expr).getName(), loc);
-    } else if (expr instanceof IndexExpression) {
-      expr.validate(env);
-    } else if (expr instanceof ListLiteral) {
-      for (Expression e : ((ListLiteral) expr).getElements()) {
-        validate(env, loc, e);
-      }
-    } else {
-      throw new EvalException(loc, "cannot assign to '" + expr + "'");
-    }
-  }
-
   @Override
   public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     expr.prettyPrint(buffer);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
index 9e9b04d136..c1734011ef 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java
@@ -112,11 +112,4 @@ public final class ListLiteral extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    for (Expression element : elements) {
-      element.validate(env);
-    }
-  }
 }
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 d451333501..21c0888200 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
@@ -107,11 +107,4 @@ public final class LoadStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    for (Identifier symbol : symbolMap.keySet()) {
-      env.declare(symbol.getName(), getLocation());
-    }
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index ed5ffabc5a..db4b4de107 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -78,12 +78,4 @@ public final class ReturnStatement extends Statement {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    if (env.isTopLevel()) {
-      throw new EvalException(getLocation(), "Return statements must be inside a function");
-    }
-    returnExpression.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
index 520dcbd954..71b637630c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
@@ -118,9 +118,4 @@ public final class SliceExpression extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    object.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
index cd49a45a0a..58130b6e87 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Statement.java
@@ -44,20 +44,4 @@ public abstract class Statement extends ASTNode {
    * @throws InterruptedException may be thrown in a sub class.
    */
   abstract void doExec(Environment env) throws EvalException, InterruptedException;
-
-  /**
-   * Checks the semantics of the Statement using the Environment according to
-   * the rules of the Skylark language. The Environment can be used e.g. to check
-   * variable type collision, read only variables, detecting recursion, existence of
-   * built-in variables, functions, etc.
-   *
-   * <p>The semantical check should be performed after the Skylark extension is loaded
-   * (i.e. is syntactically correct) and before is executed. The point of the semantical check
-   * is to make sure (as much as possible) that no error can occur during execution (Skylark
-   * programmers get a "compile time" error). It should also check execution branches (e.g. in
-   * if statements) that otherwise might never get executed.
-   *
-   * @throws EvalException if the Statement has a semantical error.
-   */
-  abstract void validate(ValidationEnvironment env) throws EvalException;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
index 19d00b9b8d..61856e2c64 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StringLiteral.java
@@ -43,8 +43,4 @@ public final class StringLiteral extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
index a76abb18d8..e55acdd305 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UnaryOperatorExpression.java
@@ -99,9 +99,4 @@ public final class UnaryOperatorExpression extends Expression {
   public void accept(SyntaxTreeVisitor visitor) {
     visitor.visit(this);
   }
-
-  @Override
-  void validate(ValidationEnvironment env) throws EvalException {
-    operand.validate(env);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
index 68268e0774..8634bcdb08 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -24,14 +24,11 @@ import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
- * An Environment for the semantic checking of Skylark files.
- *
- * @see Statement#validate
- * @see Expression#validate
+ * A class for doing static checks on files, before evaluating them.
  */
-public final class ValidationEnvironment {
+public final class ValidationEnvironment extends SyntaxTreeVisitor {
 
-  private class Scope {
+  private static class Scope {
     private final Set<String> variables = new HashSet<>();
     private final Set<String> readOnlyVariables = new HashSet<>();
     @Nullable private final Scope parent;
@@ -41,6 +38,26 @@ public final class ValidationEnvironment {
     }
   }
 
+  /**
+   * We use an unchecked exception around EvalException because the SyntaxTreeVisitor doesn't let
+   * visit methods throw checked exceptions. We might change that later.
+   */
+  private static class ValidationException extends RuntimeException {
+    EvalException exception;
+
+    ValidationException(EvalException e) {
+      exception = e;
+    }
+
+    ValidationException(Location location, String message, String url) {
+      exception = new EvalException(location, message, url);
+    }
+
+    ValidationException(Location location, String message) {
+      exception = new EvalException(location, message);
+    }
+  }
+
   private final SkylarkSemanticsOptions semantics;
   private Scope scope;
 
@@ -54,17 +71,91 @@ public final class ValidationEnvironment {
     semantics = env.getSemantics();
   }
 
-  /** Returns true if this ValidationEnvironment is top level i.e. has no parent. */
-  boolean isTopLevel() {
-    return scope.parent == null;
+  @Override
+  public void visit(LoadStatement node) {
+    for (Identifier symbol : node.getSymbols()) {
+      declare(symbol.getName(), node.getLocation());
+    }
+  }
+
+  @Override
+  public void visit(Identifier node) {
+    if (!hasSymbolInEnvironment(node.getName())) {
+      throw new ValidationException(node.createInvalidIdentifierException(getAllSymbols()));
+    }
+  }
+
+  private void validateLValue(Location loc, Expression expr) {
+    if (expr instanceof Identifier) {
+      declare(((Identifier) expr).getName(), loc);
+    } else if (expr instanceof IndexExpression) {
+      visit(expr);
+    } else if (expr instanceof ListLiteral) {
+      for (Expression e : ((ListLiteral) expr).getElements()) {
+        validateLValue(loc, e);
+      }
+    } else {
+      throw new ValidationException(loc, "cannot assign to '" + expr + "'");
+    }
   }
 
-  SkylarkSemanticsOptions getSemantics() {
-    return semantics;
+  @Override
+  public void visit(LValue node) {
+    validateLValue(node.getLocation(), node.getExpression());
+  }
+
+  @Override
+  public void visit(ReturnStatement node) {
+    if (isTopLevel()) {
+      throw new ValidationException(
+          node.getLocation(), "Return statements must be inside a function");
+    }
+    super.visit(node);
+  }
+
+  @Override
+  public void visit(DotExpression node) {
+    visit(node.getObject());
+    // Do not visit the field.
+  }
+
+  @Override
+  public void visit(AbstractComprehension node) {
+    if (semantics.incompatibleComprehensionVariablesDoNotLeak) {
+      openScope();
+      super.visit(node);
+      closeScope();
+    } else {
+      super.visit(node);
+    }
+  }
+
+  @Override
+  public void visit(FunctionDefStatement node) {
+    for (Parameter<Expression, Expression> param : node.getParameters()) {
+      if (param.isOptional()) {
+        visit(param.getDefaultValue());
+      }
+    }
+    openScope();
+    for (Parameter<Expression, Expression> param : node.getParameters()) {
+      if (param.hasName()) {
+        declare(param.getName(), param.getLocation());
+      }
+    }
+    for (Statement stmt : node.getStatements()) {
+      visit(stmt);
+    }
+    closeScope();
+  }
+
+  /** Returns true if this ValidationEnvironment is top level i.e. has no parent. */
+  private boolean isTopLevel() {
+    return scope.parent == null;
   }
 
   /** Declare a variable and add it to the environment. */
-  void declare(String varname, Location location) throws EvalException {
+  private void declare(String varname, Location location) {
     checkReadonly(varname, location);
     if (scope.parent == null) {  // top-level values are immutable
       scope.readOnlyVariables.add(varname);
@@ -72,9 +163,9 @@ public final class ValidationEnvironment {
     scope.variables.add(varname);
   }
 
-  private void checkReadonly(String varname, Location location) throws EvalException {
+  private void checkReadonly(String varname, Location location) {
     if (scope.readOnlyVariables.contains(varname)) {
-      throw new EvalException(
+      throw new ValidationException(
           location,
           String.format("Variable %s is read only", varname),
           "https://bazel.build/versions/master/docs/skylark/errors/read-only-variable.html");
@@ -82,7 +173,7 @@ public final class ValidationEnvironment {
   }
 
   /** Returns true if the symbol exists in the validation environment (or a parent). */
-  boolean hasSymbolInEnvironment(String varname) {
+  private boolean hasSymbolInEnvironment(String varname) {
     for (Scope s = scope; s != null; s = s.parent) {
       if (s.variables.contains(varname)) {
         return true;
@@ -92,7 +183,7 @@ public final class ValidationEnvironment {
   }
 
   /** Returns the set of all accessible symbols (both local and global) */
-  Set<String> getAllSymbols() {
+  private Set<String> getAllSymbols() {
     Set<String> all = new HashSet<>();
     for (Scope s = scope; s != null; s = s.parent) {
       all.addAll(s.variables);
@@ -100,8 +191,8 @@ public final class ValidationEnvironment {
     return all;
   }
 
- /** Throws EvalException if a load() appears after another kind of statement. */
-  private static void checkLoadAfterStatement(List<Statement> statements) throws EvalException {
+  /** Throws ValidationException if a load() appears after another kind of statement. */
+  private static void checkLoadAfterStatement(List<Statement> statements) {
     Location firstStatement = null;
 
     for (Statement statement : statements) {
@@ -115,7 +206,7 @@ public final class ValidationEnvironment {
         if (firstStatement == null) {
           continue;
         }
-        throw new EvalException(
+        throw new ValidationException(
             statement.getLocation(),
             "load() statements must be called before any other statement. "
                 + "First non-load() statement appears at "
@@ -130,11 +221,11 @@ public final class ValidationEnvironment {
     }
   }
 
-  /** Throws EvalException if a `if` statement appears at the top level. */
-  private static void checkToplevelIfStatement(List<Statement> statements) throws EvalException {
+  /** Throws ValidationException if a `if` statement appears at the top level. */
+  private static void checkToplevelIfStatement(List<Statement> statements) {
     for (Statement statement : statements) {
       if (statement instanceof IfStatement) {
-        throw new EvalException(
+        throw new ValidationException(
             statement.getLocation(),
             "if statements are not allowed at the top level. You may move it inside a function "
                 + "or use an if expression (x if condition else y). "
@@ -145,7 +236,7 @@ public final class ValidationEnvironment {
   }
 
   /** Validates the AST and runs static checks. */
-  void validateAst(List<Statement> statements) throws EvalException {
+  private void validateAst(List<Statement> statements) {
     // Check that load() statements are on top.
     if (semantics.incompatibleBzlDisallowLoadAfterStatement) {
       checkLoadAfterStatement(statements);
@@ -167,15 +258,19 @@ public final class ValidationEnvironment {
     }
 
     for (Statement statement : statements) {
-      statement.validate(this);
+      this.visit(statement);
     }
   }
 
   public static void validateAst(Environment env, List<Statement> statements) throws EvalException {
-    ValidationEnvironment venv = new ValidationEnvironment(env);
-    venv.validateAst(statements);
-    // Check that no closeScope was forgotten.
-    Preconditions.checkState(venv.scope.parent == null);
+    try {
+      ValidationEnvironment venv = new ValidationEnvironment(env);
+      venv.validateAst(statements);
+      // Check that no closeScope was forgotten.
+      Preconditions.checkState(venv.scope.parent == null);
+    } catch (ValidationException e) {
+      throw e.exception;
+    }
   }
 
   public static boolean validateAst(
@@ -192,12 +287,12 @@ public final class ValidationEnvironment {
   }
 
   /** Open a new scope that will contain the future declarations. */
-  public void openScope() {
+  private void openScope() {
     this.scope = new Scope(this.scope);
   }
 
   /** Close a scope (and lose all declarations it contained). */
-  public void closeScope() {
+  private void closeScope() {
     this.scope = Preconditions.checkNotNull(this.scope.parent);
   }
 }