From 395fbbdb7f17b97ed140fc0e4a1e061c7bf73843 Mon Sep 17 00:00:00 2001 From: laurentlb Date: Fri, 7 Sep 2018 09:31:51 -0700 Subject: [PATCH] Allow shadowing of builtins in bzl files This was previously allowed only with --incompatible_static_name_resolution (although it was a backward-compatible change). Removing the code path simplifies the code and makes easier to implement the static name resolution proposal. The downside is that it will temporarily allow this corner-case: ``` x = len(...) # reference to the builtin len = ... # shadow the builtin with a new global ``` RELNOTES: None. PiperOrigin-RevId: 211987952 --- .../build/lib/syntax/Environment.java | 9 +- .../devtools/build/lib/syntax/Identifier.java | 28 +++++-- .../lib/syntax/ValidationEnvironment.java | 83 +++---------------- .../build/lib/syntax/EnvironmentTest.java | 2 +- .../build/lib/syntax/EvaluationTest.java | 11 ++- .../build/lib/syntax/FunctionTest.java | 6 +- .../lib/syntax/SkylarkEvaluationTest.java | 21 +++-- .../build/lib/syntax/ValidationTest.java | 17 ---- 8 files changed, 67 insertions(+), 110 deletions(-) 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..84de4925c1c7bd 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,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/EnvironmentTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java index 1b53776bd2ecf3..bbad014fca6e62 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("Variable 'global_var' is referenced before assignment."); + .contains("local 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 1ee0dff026189e..c2aa8651bb3abb 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,8 +357,15 @@ public void testListComprehensionsWithFiltering() throws Exception { @Test public void testListComprehensionDefinitionOrder() throws Exception { - newTest().testIfErrorContains("name 'y' is not defined", - "[x for x in (1, 2) if y for y in (3, 4)]"); + 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)]"); } @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 f91e2a8a72e020..95fd03083acdac 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,7 +105,8 @@ public void testFunctionDefLocalGlobalScope() throws Exception { @Test public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exception { - checkEvalErrorContains("Variable 'a' is referenced before assignment.", + checkEvalErrorContains( + "local variable 'a' is referenced before assignment.", "a = 1", "def func():", " b = a", @@ -116,7 +117,8 @@ public void testFunctionDefLocalVariableReferencedBeforeAssignment() throws Exce @Test public void testFunctionDefLocalVariableReferencedInCallBeforeAssignment() throws Exception { - checkEvalErrorContains("Variable 'a' is referenced before assignment.", + checkEvalErrorContains( + "local 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 8333245f1e417d..2f5a19ac6f14db 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( + "global variable 'foo' is referenced before assignment", + "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:", @@ -1822,10 +1824,11 @@ public void testShadowisNotInitialized() throws Exception { } @Test - public void testLegacyGlobalIsNotInitialized() throws Exception { + 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 @@ -2194,7 +2197,7 @@ public void testStructMethodNotDefined() throws Exception { @Test public void testListComprehensionsDoNotLeakVariables() throws Exception { checkEvalErrorContains( - "name 'a' is not defined", + "local variable 'a' is referenced before assignment", "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 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"); }