diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index f8afd1107a2457..942d635549f282 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -1399,8 +1399,6 @@ private CompiledBuildFile compileBuildFile( // Options for processing BUILD files. FileOptions options = FileOptions.builder() - // TODO(adonovan): remove recordScope opt-out. - .recordScope(false) // don't mutate BUILD syntax .requireLoadStatementsFirst(false) .allowToplevelRebinding(true) .restrictStringEscapes( diff --git a/src/main/java/net/starlark/java/eval/Eval.java b/src/main/java/net/starlark/java/eval/Eval.java index 88a26df6c46258..e5fcc83ee472a3 100644 --- a/src/main/java/net/starlark/java/eval/Eval.java +++ b/src/main/java/net/starlark/java/eval/Eval.java @@ -322,25 +322,8 @@ private static void assign(StarlarkThread.Frame fr, Expression lhs, Object value private static void assignIdentifier(StarlarkThread.Frame fr, Identifier id, Object value) throws EvalException { Resolver.Binding bind = id.getBinding(); - // Legacy hack for incomplete identifier resolution. - // In a function, assignments to unresolved identifiers - // update the module, except for load statements and comprehensions, - // which should both be file-local. - // Load statements don't yet use assignIdentifier, - // so we need consider only comprehensions. - // In effect, we do the missing resolution using fr.compcount. - Resolver.Scope scope; - if (bind == null) { - scope = - fn(fr).isToplevel() && fr.compcount == 0 - ? Resolver.Scope.GLOBAL // - : Resolver.Scope.LOCAL; - } else { - scope = bind.getScope(); - } - String name = id.getName(); - switch (scope) { + switch (bind.getScope()) { case LOCAL: fr.locals.put(name, value); break; @@ -353,7 +336,7 @@ private static void assignIdentifier(StarlarkThread.Frame fr, Identifier id, Obj module.exportedGlobals.add(name); break; default: - throw new IllegalStateException(scope.toString()); + throw new IllegalStateException(bind.getScope().toString()); } } @@ -766,7 +749,6 @@ private static Object evalComprehension(StarlarkThread.Frame fr, Comprehension c } } } - fr.compcount++; // The Lambda class serves as a recursive lambda closure. class Lambda { @@ -823,7 +805,6 @@ void execClauses(int index) throws EvalException, InterruptedException { } } new Lambda().execClauses(0); - fr.compcount--; // Restore outer scope variables. // This loop implicitly undefines comprehension variables. diff --git a/src/main/java/net/starlark/java/eval/StarlarkThread.java b/src/main/java/net/starlark/java/eval/StarlarkThread.java index 77378f086ed240..e859ec5d41dad1 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkThread.java +++ b/src/main/java/net/starlark/java/eval/StarlarkThread.java @@ -126,8 +126,6 @@ static final class Frame implements Debug.Frame { @Nullable final Debug.Debugger dbg = Debug.debugger.get(); // the debugger, if active for this frame - int compcount = 0; // number of enclosing comprehensions - Object result = Starlark.NONE; // the operand of a Starlark return statement // Current PC location. Initially fn.getLocation(); for Starlark functions, diff --git a/src/main/java/net/starlark/java/syntax/FileOptions.java b/src/main/java/net/starlark/java/syntax/FileOptions.java index 5301e1d4e0a4bc..7aca0510b6f6b1 100644 --- a/src/main/java/net/starlark/java/syntax/FileOptions.java +++ b/src/main/java/net/starlark/java/syntax/FileOptions.java @@ -76,12 +76,6 @@ public abstract class FileOptions { */ public abstract boolean requireLoadStatementsFirst(); - /** - * Record the results of name resolution in the syntax tree by setting {@code Identifer.scope}. - * (Disabled for Bazel BUILD files, as its prelude's syntax trees are shared.) - */ - public abstract boolean recordScope(); - public static Builder builder() { // These are the DEFAULT values. return new AutoValue_FileOptions.Builder() @@ -89,8 +83,7 @@ public static Builder builder() { .allowLoadPrivateSymbols(false) .allowToplevelRebinding(false) // .loadBindsGlobally(false) - .requireLoadStatementsFirst(true) - .recordScope(true); + .requireLoadStatementsFirst(true); } public abstract Builder toBuilder(); @@ -109,8 +102,6 @@ public abstract static class Builder { public abstract Builder requireLoadStatementsFirst(boolean value); - public abstract Builder recordScope(boolean value); - public abstract FileOptions build(); } } diff --git a/src/main/java/net/starlark/java/syntax/Identifier.java b/src/main/java/net/starlark/java/syntax/Identifier.java index 8299efc8086476..af8188311abcbc 100644 --- a/src/main/java/net/starlark/java/syntax/Identifier.java +++ b/src/main/java/net/starlark/java/syntax/Identifier.java @@ -107,7 +107,7 @@ public static boolean isValid(String name) { // When it works in a single pass, it is more efficient to process bindings in order, // deferring (rare) forward references until the end of the block. // - Eval calls boundIdentifiers for comprehensions. This can be eliminated when - // recordScope is always enabled and variables are assigned frame slot indices. + // variables are assigned frame slot indices. // - Eval calls boundIdentifiers for the 'export' hack. This can be eliminated // when we switch to compilation by emitting EXPORT instructions for the necessary // bindings. (Ideally we would eliminate Bazel's export hack entirely.) diff --git a/src/main/java/net/starlark/java/syntax/Resolver.java b/src/main/java/net/starlark/java/syntax/Resolver.java index a19b4edb7abdeb..6aa7d63f22538c 100644 --- a/src/main/java/net/starlark/java/syntax/Resolver.java +++ b/src/main/java/net/starlark/java/syntax/Resolver.java @@ -191,10 +191,10 @@ public ImmutableList getParameterNames() { /** * isToplevel indicates that this is the function containing top-level statements of - * a file. It causes assignments to unresolved identifiers to update the module, not the lexical - * frame. + * a file. */ - // TODO(adonovan): remove this hack when identifier resolution is accurate. + // TODO(adonovan): remove this when we remove Bazel's "export" hack, + // or switch to a compiled representation of function bodies. public boolean isToplevel() { return isToplevel; } @@ -371,9 +371,7 @@ public void visit(Identifier id) { for (Block b = block; b != null; b = b.parent) { Binding bind = b.bindings.get(id.getName()); if (bind != null) { - if (options.recordScope()) { - id.setBinding(bind); - } + id.setBinding(bind); return; } } @@ -697,9 +695,7 @@ private boolean bind(Identifier id) { } } - if (options.recordScope()) { - id.setBinding(bind); - } + id.setBinding(bind); return true; } @@ -707,9 +703,7 @@ private boolean bind(Identifier id) { // TODO(adonovan): accumulate locals in the enclosing function/file block. bind = new Binding(block.scope, id, block.bindings.size()); block.bindings.put(id.getName(), bind); - if (options.recordScope()) { - id.setBinding(bind); - } + id.setBinding(bind); return false; } diff --git a/src/test/java/net/starlark/java/eval/EvaluationTest.java b/src/test/java/net/starlark/java/eval/EvaluationTest.java index 09cfb44e7b31e2..3d6ff7660ec8a7 100644 --- a/src/test/java/net/starlark/java/eval/EvaluationTest.java +++ b/src/test/java/net/starlark/java/eval/EvaluationTest.java @@ -396,29 +396,10 @@ ev.new Scenario() @Test public void testListComprehensionDefinitionOrder() throws Exception { - // This exercises the .bzl file behavior. This is a dynamic error. - // (The error message for BUILD files is slightly different (no "local") - // because it doesn't record the scope in the syntax tree.) ev.new Scenario() .testIfErrorContains( - "local variable 'y' is referenced before assignment", // + "local variable 'y' is referenced before assignment", "[x for x in (1, 2) if y for y in (3, 4)]"); - - // This is the corresponding test for BUILD files. - EvalException ex = - assertThrows( - EvalException.class, () -> execBUILD("[x for x in (1, 2) if y for y in (3, 4)]")); - assertThat(ex).hasMessageThat().isEqualTo("variable 'y' is referenced before assignment"); - } - - private static void execBUILD(String... lines) - throws SyntaxError.Exception, EvalException, InterruptedException { - ParserInput input = ParserInput.fromLines(lines); - FileOptions options = FileOptions.builder().recordScope(false).build(); - try (Mutability mu = Mutability.create("test")) { - StarlarkThread thread = new StarlarkThread(mu, StarlarkSemantics.DEFAULT); - Starlark.execFile(input, options, Module.create(), thread); - } } @Test