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
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). */
