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 c32ab06a4f7775..a8158e3f32f0f2 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 @@ -1112,14 +1112,7 @@ public Object moduleLookup(String varname) { /** Returns the value of a variable defined in the Universe scope (builtins). */ public Object universeLookup(String varname) { // TODO(laurentlb): look only at globalFrame.parent. - Object result = globalFrame.get(varname); - - if (result == null) { - // TODO(laurentlb): Remove once PACKAGE_NAME and REPOSITOYRY_NAME are removed (they are the - // only two user-visible values that use the dynamicFrame). - return dynamicLookup(varname); - } - return result; + return globalFrame.get(varname); } /** Returns the value of a variable defined with setupDynamic. */ 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 84de4925c1c7bd..d212c673982574 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 @@ -107,12 +107,9 @@ Object doEval(Environment env) throws EvalException { if (result == null) { // Since Scope was set, we know that the variable is defined in the scope. // However, the assignment was not yet executed. - EvalException e = getSpecialException(); - throw e != null - ? e - : new EvalException( - getLocation(), - scope.getQualifier() + " variable '" + name + "' is referenced before assignment."); + throw new EvalException( + getLocation(), + scope.getQualifier() + " variable '" + name + "' is referenced before assignment."); } return result; @@ -128,8 +125,11 @@ public Kind kind() { return Kind.IDENTIFIER; } - /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */ - private EvalException getSpecialException() { + EvalException createInvalidIdentifierException(Set symbols) { + if (name.equals("$error$")) { + return new EvalException(getLocation(), "contains syntax error(s)", true); + } + if (name.equals("PACKAGE_NAME")) { return new EvalException( getLocation(), @@ -148,18 +148,6 @@ private EvalException getSpecialException() { + " You can temporarily allow the old name " + "by using --incompatible_package_name_is_a_function=false"); } - return null; - } - - EvalException createInvalidIdentifierException(Set symbols) { - if (name.equals("$error$")) { - return new EvalException(getLocation(), "contains syntax error(s)", true); - } - - EvalException e = getSpecialException(); - if (e != null) { - return e; - } String suggestion = SpellChecker.didYouMean(name, symbols); return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion); 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 8900e5fcf221c5..ec7a9001d7f1af 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,17 @@ /** * A class for doing static checks on files, before evaluating them. * - *

We implement the semantics discussed in + *

The behavior is affected by semantics.incompatibleStaticNameResolution(). When it is set to + * true, we implement the semantics discussed in * https://github.com/bazelbuild/proposals/blob/master/docs/2018-06-18-name-resolution.md * *

When a variable is defined, it is visible in the entire block. For example, a global variable * is visible in the entire file; a variable in a function is visible in the entire function block * (even on the lines before its first assignment). + * + *

The legacy behavior is kept during the transition and will be removed in the future. In the + * legacy behavior, there is no clear separation between the first pass (collect all definitions) + * and the second pass (ensure the symbols can be resolved). */ public final class ValidationEnvironment extends SyntaxTreeVisitor { @@ -56,6 +61,7 @@ public String getQualifier() { private static class Block { private final Set variables = new HashSet<>(); + private final Set readOnlyVariables = new HashSet<>(); private final Scope scope; @Nullable private final Block parent; @@ -96,12 +102,18 @@ private static class ValidationException extends RuntimeException { block = new Block(Scope.Universe, null); Set builtinVariables = env.getVariableNames(); block.variables.addAll(builtinVariables); + if (!semantics.incompatibleStaticNameResolution()) { + block.readOnlyVariables.addAll(builtinVariables); + } } /** * First pass: add all definitions to the current block. This is done because symbols are * sometimes used before their definition point (e.g. a functions are not necessarily declared in * order). + * + *

The old behavior (when incompatibleStaticNameResolution is false) doesn't have this first + * pass. */ private void collectDefinitions(Iterable stmts) { for (Statement stmt : stmts) { @@ -153,23 +165,41 @@ private void collectDefinitions(LValue left) { } } + @Override + public void visit(LoadStatement node) { + if (semantics.incompatibleStaticNameResolution()) { + return; + } + + for (Identifier symbol : node.getSymbols()) { + declare(symbol.getName(), node.getLocation()); + } + } + @Override public void visit(Identifier node) { @Nullable Block b = blockThatDefines(node.getName()); if (b == null) { throw new ValidationException(node.createInvalidIdentifierException(getAllSymbols())); } - node.setScope(b.scope); + if (semantics.incompatibleStaticNameResolution()) { + // The scoping information is reliable only with the new behavior. + node.setScope(b.scope); + } } private void validateLValue(Location loc, Expression expr) { - if (expr instanceof IndexExpression) { + if (expr instanceof Identifier) { + if (!semantics.incompatibleStaticNameResolution()) { + 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 if (!(expr instanceof Identifier)) { + } else { throw new ValidationException(loc, "cannot assign to '" + expr + "'"); } } @@ -214,9 +244,11 @@ public void visit(DotExpression node) { @Override public void visit(AbstractComprehension node) { openBlock(Scope.Local); - for (AbstractComprehension.Clause clause : node.getClauses()) { - if (clause.getLValue() != null) { - collectDefinitions(clause.getLValue()); + if (semantics.incompatibleStaticNameResolution()) { + for (AbstractComprehension.Clause clause : node.getClauses()) { + if (clause.getLValue() != null) { + collectDefinitions(clause.getLValue()); + } } } super.visit(node); @@ -236,7 +268,9 @@ public void visit(FunctionDefStatement node) { declare(param.getName(), param.getLocation()); } } - collectDefinitions(node.getStatements()); + if (semantics.incompatibleStaticNameResolution()) { + collectDefinitions(node.getStatements()); + } visitAll(node.getStatements()); closeBlock(); } @@ -264,13 +298,25 @@ public void visit(AugmentedAssignmentStatement node) { /** Declare a variable and add it to the environment. */ private void declare(String varname, Location location) { - if (block.scope == Scope.Module && block.variables.contains(varname)) { - // Symbols defined in the module scope cannot be reassigned. + 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 (block.scope == Scope.Module) { + // Symbols defined in the module scope cannot be reassigned. + block.readOnlyVariables.add(varname); + } block.variables.add(varname); } @@ -332,9 +378,20 @@ private void validateAst(List statements) { openBlock(Scope.Module); - // Add each variable defined by statements, not including definitions that appear in - // sub-scopes of the given statements (function bodies and comprehensions). - collectDefinitions(statements); + if (semantics.incompatibleStaticNameResolution()) { + // Add each variable defined by statements, not including definitions that appear in + // sub-scopes of the given statements (function bodies and comprehensions). + collectDefinitions(statements); + } else { + // Legacy behavior, to be removed. Add only the functions in the environment before + // validating. + for (Statement statement : statements) { + if (statement instanceof FunctionDefStatement) { + FunctionDefStatement fct = (FunctionDefStatement) statement; + declare(fct.getIdentifier().getName(), fct.getLocation()); + } + } + } // Second pass: ensure that all symbols have been defined. visitAll(statements); diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java index bbad014fca6e62..1b53776bd2ecf3 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java @@ -217,7 +217,7 @@ public void testVariableIsReferencedBeforeAssignment() throws Exception { } catch (EvalExceptionWithStackTrace e) { assertThat(e) .hasMessageThat() - .contains("local variable 'global_var' is referenced before assignment."); + .contains("Variable 'global_var' is referenced before assignment."); } } diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java index c2aa8651bb3abb..1ee0dff026189e 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java @@ -357,15 +357,8 @@ public void testListComprehensionsWithFiltering() throws Exception { @Test public void testListComprehensionDefinitionOrder() throws Exception { - new BuildTest() - .testIfErrorContains("name 'y' is not defined", "[x for x in (1, 2) if y for y in (3, 4)]"); - - // We provide a better error message when we use the ValidationEnvironment. It should be used - // for BUILD files too in the future. - new SkylarkTest() - .testIfErrorContains( - "local variable 'y' is referenced before assignment", - "[x for x in (1, 2) if y for y in (3, 4)]"); + newTest().testIfErrorContains("name 'y' is not defined", + "[x for x in (1, 2) if y for y in (3, 4)]"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java index 95fd03083acdac..f91e2a8a72e020 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/FunctionTest.java @@ -105,8 +105,7 @@ public void testFunctionDefLocalGlobalScope() throws Exception { @Test public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exception { - checkEvalErrorContains( - "local variable 'a' is referenced before assignment.", + checkEvalErrorContains("Variable 'a' is referenced before assignment.", "a = 1", "def func():", " b = a", @@ -117,8 +116,7 @@ public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exce @Test public void testFunctionDefLocalVariableReferencedInCallBeforeAssignment() throws Exception { - checkEvalErrorContains( - "local variable 'a' is referenced before assignment.", + checkEvalErrorContains("Variable 'a' is referenced before assignment.", "def dummy(x):", " pass", "a = 1", diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index 2f5a19ac6f14db..8333245f1e417d 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -1789,17 +1789,15 @@ public void testFunctionCallOrdering() throws Exception { @Test public void testFunctionCallBadOrdering() throws Exception { - new SkylarkTest() - .testIfErrorContains( - "global variable 'foo' is referenced before assignment", - "def func(): return foo() * 2", - "x = func()", - "def foo(): return 2"); + new SkylarkTest().testIfErrorContains("name 'foo' is not defined", + "def func(): return foo() * 2", + "x = func()", + "def foo(): return 2"); } @Test public void testLocalVariableDefinedBelow() throws Exception { - new SkylarkTest() + new SkylarkTest("--incompatible_static_name_resolution=true") .setUp( "def beforeEven(li):", // returns the value before the first even number " for i in li:", @@ -1824,11 +1822,10 @@ public void testShadowisNotInitialized() throws Exception { } @Test - public void testShadowBuiltin() throws Exception { - // TODO(laurentlb): Forbid this. + public void testLegacyGlobalIsNotInitialized() throws Exception { new SkylarkTest("--incompatible_static_name_resolution=false") - .setUp("x = len('abc')", "len = 2", "y = x + len") - .testLookup("y", 5); + .setUp("a = len") + .testIfErrorContains("Variable len is read only", "len = 2"); } @Test @@ -2197,7 +2194,7 @@ public void testStructMethodNotDefined() throws Exception { @Test public void testListComprehensionsDoNotLeakVariables() throws Exception { checkEvalErrorContains( - "local variable 'a' is referenced before assignment", + "name 'a' is not defined", "def foo():", " a = 10", " b = [a for a in range(3)]", diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java index 24179208eaf5b3..403bfa3e4b1e7c 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ValidationTest.java @@ -93,6 +93,7 @@ public void testFunctionParameterDoesNotEffectGlobalValidationEnv() throws Excep @Test public void testDefinitionByItself() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); // Variables are assumed to be statically visible in the block (even if they might not be // initialized). parse("a = a"); @@ -101,13 +102,29 @@ public void testDefinitionByItself() throws Exception { parse("def f():", " for a in a: pass"); } + @Test + public void testDefinitionByItselfLegacy() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=false"); + checkError("name 'a' is not defined", "a = a"); + checkError("name 'a' is not defined", "a += a"); + checkError("name 'a' is not defined", "[[] for a in a]"); + checkError("name 'a' is not defined", "def f():", " for a in a: pass"); + } + @Test public void testLocalValidationEnvironmentsAreSeparated() throws Exception { parse("def func1():", " a = 1", "def func2():", " a = 'abc'\n"); } + @Test + public void testBuiltinSymbolsAreReadOnlyLegacy() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=false"); + checkError("Variable repr is read only", "repr = 1"); + } + @Test public void testBuiltinsCanBeShadowed() throws Exception { + env = newEnvironmentWithSkylarkOptions("--incompatible_static_name_resolution=true"); parse("repr = 1"); }