1 files changed, 125 insertions, 30 deletions
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);
   }
 }