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 a8158e3f32f0f2..c32ab06a4f7775 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,7 +1112,14 @@ 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. - return globalFrame.get(varname); + 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; } /** 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 d212c673982574..dcd39b6e65a6f0 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 @@ -81,8 +81,8 @@ void setScope(ValidationEnvironment.Scope scope) { @Override Object doEval(Environment env) throws EvalException { Object result; - if (scope == null) { - // Legacy behavior, in case the AST was not analyzed. + if (scope == null || !env.getSemantics().incompatibleStaticNameResolution()) { + // Legacy behavior, to be removed. result = env.lookup(name); if (result == null) { throw createInvalidIdentifierException(env.getVariableNames()); @@ -107,9 +107,12 @@ 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. - throw new EvalException( - getLocation(), - scope.getQualifier() + " variable '" + name + "' is referenced before assignment."); + EvalException e = getSpecialException(); + throw e != null + ? e + : new EvalException( + getLocation(), + scope.getQualifier() + " variable '" + name + "' is referenced before assignment."); } return result; @@ -125,11 +128,8 @@ public Kind kind() { return Kind.IDENTIFIER; } - EvalException createInvalidIdentifierException(Set symbols) { - if (name.equals("$error$")) { - return new EvalException(getLocation(), "contains syntax error(s)", true); - } - + /** Exception to provide a better error message for using PACKAGE_NAME or REPOSITORY_NAME. */ + private EvalException getSpecialException() { if (name.equals("PACKAGE_NAME")) { return new EvalException( getLocation(), @@ -148,6 +148,18 @@ EvalException createInvalidIdentifierException(Set symbols) { + " 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 ec7a9001d7f1af..8900e5fcf221c5 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,17 +26,12 @@ /** * A class for doing static checks on files, before evaluating them. * - *

The behavior is affected by semantics.incompatibleStaticNameResolution(). When it is set to - * true, we implement the semantics discussed in + *

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 { @@ -61,7 +56,6 @@ 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; @@ -102,18 +96,12 @@ 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) { @@ -165,41 +153,23 @@ 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())); } - if (semantics.incompatibleStaticNameResolution()) { - // The scoping information is reliable only with the new behavior. - node.setScope(b.scope); - } + node.setScope(b.scope); } private void validateLValue(Location loc, Expression expr) { - if (expr instanceof Identifier) { - if (!semantics.incompatibleStaticNameResolution()) { - declare(((Identifier) expr).getName(), loc); - } - } else if (expr instanceof IndexExpression) { + if (expr instanceof IndexExpression) { visit(expr); } else if (expr instanceof ListLiteral) { for (Expression e : ((ListLiteral) expr).getElements()) { validateLValue(loc, e); } - } else { + } else if (!(expr instanceof Identifier)) { throw new ValidationException(loc, "cannot assign to '" + expr + "'"); } } @@ -244,11 +214,9 @@ public void visit(DotExpression node) { @Override public void visit(AbstractComprehension node) { openBlock(Scope.Local); - if (semantics.incompatibleStaticNameResolution()) { - for (AbstractComprehension.Clause clause : node.getClauses()) { - if (clause.getLValue() != null) { - collectDefinitions(clause.getLValue()); - } + for (AbstractComprehension.Clause clause : node.getClauses()) { + if (clause.getLValue() != null) { + collectDefinitions(clause.getLValue()); } } super.visit(node); @@ -268,9 +236,7 @@ public void visit(FunctionDefStatement node) { declare(param.getName(), param.getLocation()); } } - if (semantics.incompatibleStaticNameResolution()) { - collectDefinitions(node.getStatements()); - } + collectDefinitions(node.getStatements()); visitAll(node.getStatements()); closeBlock(); } @@ -298,25 +264,13 @@ public void visit(AugmentedAssignmentStatement node) { /** 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) { + if (block.scope == Scope.Module && block.variables.contains(varname)) { + // Symbols defined in the module scope cannot be reassigned. 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); } @@ -378,20 +332,9 @@ private void validateAst(List statements) { openBlock(Scope.Module); - 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()); - } - } - } + // Add each variable defined by statements, not including definitions that appear in + // sub-scopes of the given statements (function bodies and comprehensions). + collectDefinitions(statements); // Second pass: ensure that all symbols have been defined. visitAll(statements); 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 8333245f1e417d..810aaa219456ba 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,15 +1789,17 @@ public void testFunctionCallOrdering() throws Exception { @Test public void testFunctionCallBadOrdering() throws Exception { - new SkylarkTest().testIfErrorContains("name 'foo' is not defined", - "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("--incompatible_static_name_resolution=true") + new SkylarkTest() .setUp( "def beforeEven(li):", // returns the value before the first even number " for i in li:", @@ -1815,17 +1817,32 @@ public void testShadowisNotInitialized() throws Exception { .testIfErrorContains( /* error message */ "local variable 'gl' is referenced before assignment", "gl = 5", - "def foo():", // returns the value before the first even number + "def foo():", " if False: gl = 2", " return gl", "res = foo()"); } @Test - public void testLegacyGlobalIsNotInitialized() throws Exception { + public void testLegacyGlobalVariableNotShadowed() throws Exception { + new SkylarkTest("--incompatible_static_name_resolution=false") + .setUp( + "gl = 5", + "def foo():", + " if False: gl = 2", + // The legacy behavior is that the global variable is returned. + // With --incompatible_static_name_resolution set to true, this becomes an error. + " return gl", + "res = foo()") + .testLookup("res", 5); + } + + @Test + public void testShadowBuiltin() throws Exception { + // TODO(laurentlb): Forbid this. new SkylarkTest("--incompatible_static_name_resolution=false") - .setUp("a = len") - .testIfErrorContains("Variable len is read only", "len = 2"); + .setUp("x = len('abc')", "len = 2", "y = x + len") + .testLookup("y", 5); } @Test 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 403bfa3e4b1e7c..24179208eaf5b3 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,7 +93,6 @@ 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"); @@ -102,29 +101,13 @@ 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"); }