Distinguish between Module scope and Universe scope in ValidationEnvironment

This is just a refactoring, no behavior change intended. This change makes
obvious a previous bug we want to fix. It is also a prerequisite for other
changes related to name resolution.

https://github.com/bazelbuild/bazel/issues/5637

RELNOTES: None.
PiperOrigin-RevId: 209761775

src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java

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 7021b6a..56eede9 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
@@ -26,12 +26,23 @@
 /** A class for doing static checks on files, before evaluating them. */
 public final class ValidationEnvironment extends SyntaxTreeVisitor {
 
+  private enum Scope {
+    /** Symbols defined inside a function or a comprehension. */
+    Local,
+    /** Symbols defined at a module top-level, e.g. functions, loaded symbols. */
+    Module,
+    /** Predefined symbols (builtins) */
+    Universe,
+  }
+
   private static class Block {
     private final Set<String> variables = new HashSet<>();
     private final Set<String> readOnlyVariables = new HashSet<>();
+    private final Scope scope;
     @Nullable private final Block parent;
 
-    Block(@Nullable Block parent) {
+    Block(Scope scope, @Nullable Block parent) {
+      this.scope = scope;
       this.parent = parent;
     }
   }
@@ -60,10 +71,10 @@
   private Block block;
   private int loopCount;
 
-  /** Create a ValidationEnvironment for a given global Environment. */
+  /** Create a ValidationEnvironment for a given global Environment (containing builtins). */
   ValidationEnvironment(Environment env) {
     Preconditions.checkArgument(env.isGlobal());
-    block = new Block(null);
+    block = new Block(Scope.Universe, null);
     Set<String> builtinVariables = env.getVariableNames();
     block.variables.addAll(builtinVariables);
     block.readOnlyVariables.addAll(builtinVariables);
@@ -105,7 +116,7 @@
 
   @Override
   public void visit(ReturnStatement node) {
-    if (isTopLevel()) {
+    if (block.scope != Scope.Local) {
       throw new ValidationException(
           node.getLocation(), "return statements must be inside a function");
     }
@@ -137,7 +148,7 @@
 
   @Override
   public void visit(AbstractComprehension node) {
-    openBlock();
+    openBlock(Scope.Local);
     super.visit(node);
     closeBlock();
   }
@@ -149,7 +160,7 @@
         visit(param.getDefaultValue());
       }
     }
-    openBlock();
+    openBlock(Scope.Local);
     for (Parameter<Expression, Expression> param : node.getParameters()) {
       if (param.hasName()) {
         declare(param.getName(), param.getLocation());
@@ -161,7 +172,7 @@
 
   @Override
   public void visit(IfStatement node) {
-    if (isTopLevel()) {
+    if (block.scope != Scope.Local) {
       throw new ValidationException(
           node.getLocation(),
           "if statements are not allowed at the top level. You may move it inside a function "
@@ -180,20 +191,25 @@
     super.visit(node);
   }
 
-  /** Returns true if the current block is the top level i.e. has no parent. */
-  private boolean isTopLevel() {
-    return block.parent == null;
-  }
-
   /** Declare a variable and add it to the environment. */
   private void declare(String varname, Location location) {
+    boolean readOnlyViolation = false;
     if (block.readOnlyVariables.contains(varname)) {
+      readOnlyViolation = true;
+    }
+    if (block.scope == Scope.Module && block.parent.readOnlyVariables.contains(varname)) {
+      // TODO(laurentlb): This behavior is buggy. Symbols in the module scope should shadow symbols
+      // from the universe. https://github.com/bazelbuild/bazel/issues/5637
+      readOnlyViolation = true;
+    }
+    if (readOnlyViolation) {
       throw new ValidationException(
           location,
           String.format("Variable %s is read only", varname),
           "https://bazel.build/versions/master/docs/skylark/errors/read-only-variable.html");
     }
-    if (isTopLevel()) {  // top-level values are immutable
+    if (block.scope == Scope.Module) {
+      // Symbols defined in the module scope cannot be reassigned.
       block.readOnlyVariables.add(varname);
     }
     block.variables.add(varname);
@@ -255,6 +271,8 @@
       checkLoadAfterStatement(statements);
     }
 
+    openBlock(Scope.Module);
+
     // Add every function in the environment before validating. This is
     // necessary because functions may call other functions defined
     // later in the file.
@@ -266,6 +284,7 @@
     }
 
     this.visitAll(statements);
+    closeBlock();
   }
 
   public static void validateAst(Environment env, List<Statement> statements) throws EvalException {
@@ -293,8 +312,8 @@
   }
 
   /** Open a new lexical block that will contain the future declarations. */
-  private void openBlock() {
-    block = new Block(block);
+  private void openBlock(Scope scope) {
+    block = new Block(scope, block);
   }
 
   /** Close a lexical block (and lose all declarations it contained). */