diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/TestUtils.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/TestUtils.java index 2ed9db42906270..d98f413b56466c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/TestUtils.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/TestUtils.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.skyframe.serialization.ObjectCodecs; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; import com.google.devtools.build.lib.skyframe.serialization.SerializationException; +import com.google.devtools.build.lib.syntax.Environment.Frame; import com.google.devtools.build.lib.syntax.Environment.GlobalFrame; import com.google.devtools.build.lib.syntax.Mutability; import com.google.protobuf.ByteString; @@ -96,6 +97,12 @@ public static T roundTrip(T value) throws IOException, SerializationExceptio return TestUtils.roundTrip(value, ImmutableMap.of()); } + public static void assertFramesEqual(Frame frame1, Frame frame2) { + assertThat(frame1.getTransitiveBindings()) + .containsExactlyEntriesIn(frame2.getTransitiveBindings()) + .inOrder(); + } + /** * Asserts that two {@link GlobalFrame}s have the same structure. Needed because * {@link GlobalFrame} doesn't override {@link Object#equals}. @@ -110,7 +117,7 @@ public static void assertGlobalFramesEqual(GlobalFrame frame1, GlobalFrame frame assertThat(frame1.getParent()).isNull(); assertThat(frame2.getParent()).isNull(); } else { - assertGlobalFramesEqual(frame1.getParent(), frame2.getParent()); + assertFramesEqual(frame1.getParent(), frame2.getParent()); } } 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 ff1df2eb816cab..3a4f0c9c958305 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 @@ -240,10 +240,10 @@ public String toString() { } /** - * A {@link Frame} that can have a parent {@link GlobalFrame} from which it inherits bindings. + * A {@link Frame} that represents the top-level definitions of a file. It contains the + * module-scope variables and has a reference to the universe. * - *

Bindings in a {@link GlobalFrame} may shadow those inherited from its parents. A chain of - * {@link GlobalFrame}s can represent different builtin scopes with a linear precedence ordering. + *

Bindings in a {@link GlobalFrame} may shadow those inherited from its universe. * *

A {@link GlobalFrame} can also be constructed in a two-phase process. To do this, call the * nullary constructor to create an uninitialized {@link GlobalFrame}, then call {@link @@ -258,7 +258,7 @@ public static final class GlobalFrame implements Frame { @Nullable private Mutability mutability; /** Final, except that it may be initialized after instantiation. */ - @Nullable private GlobalFrame parent; + @Nullable private Frame universe; /** * If this frame is a global frame, the label for the corresponding target, e.g. {@code @@ -274,20 +274,26 @@ public static final class GlobalFrame implements Frame { /** Constructs an uninitialized instance; caller must call {@link #initialize} before use. */ public GlobalFrame() { this.mutability = null; - this.parent = null; + this.universe = null; this.label = null; this.bindings = new LinkedHashMap<>(); } public GlobalFrame( Mutability mutability, - @Nullable GlobalFrame parent, + @Nullable GlobalFrame universe, @Nullable Label label, @Nullable Map bindings) { - Preconditions.checkState(parent == null || parent.parent == null); // no more than 1 parent + Preconditions.checkState(universe == null || universe.universe == null); this.mutability = Preconditions.checkNotNull(mutability); - this.parent = parent; - this.label = label; + this.universe = universe; + if (label != null) { + this.label = label; + } else if (universe != null) { + this.label = universe.label; + } else { + this.label = null; + } this.bindings = new LinkedHashMap<>(); if (bindings != null) { this.bindings.putAll(bindings); @@ -298,12 +304,13 @@ public GlobalFrame(Mutability mutability) { this(mutability, null, null, null); } - public GlobalFrame(Mutability mutability, @Nullable GlobalFrame parent) { - this(mutability, parent, null, null); + public GlobalFrame(Mutability mutability, @Nullable GlobalFrame universe) { + this(mutability, universe, null, null); } - public GlobalFrame(Mutability mutability, @Nullable GlobalFrame parent, @Nullable Label label) { - this(mutability, parent, label, null); + public GlobalFrame( + Mutability mutability, @Nullable GlobalFrame universe, @Nullable Label label) { + this(mutability, universe, label, null); } /** Constructs a global frame for the given builtin bindings. */ @@ -318,14 +325,22 @@ private void checkInitialized() { public void initialize( Mutability mutability, - @Nullable GlobalFrame parent, + @Nullable GlobalFrame universe, @Nullable Label label, Map bindings) { + Preconditions.checkState( + universe == null || universe.universe == null); // no more than 1 universe Preconditions.checkState( this.mutability == null, "Attempted to initialize an already initialized Frame"); this.mutability = Preconditions.checkNotNull(mutability); - this.parent = parent; - this.label = label; + this.universe = universe; + if (label != null) { + this.label = label; + } else if (universe != null) { + this.label = universe.label; + } else { + this.label = null; + } this.bindings.putAll(bindings); } @@ -335,56 +350,43 @@ public void initialize( */ public GlobalFrame withLabel(Label label) { checkInitialized(); - return new GlobalFrame(mutability, /*parent*/ null, label, bindings); + return new GlobalFrame(mutability, /*universe*/ null, label, bindings); } - /** - * Returns the {@link Mutability} of this {@link GlobalFrame}, which may be different from its - * parent's. - */ + /** Returns the {@link Mutability} of this {@link GlobalFrame}. */ @Override public Mutability mutability() { checkInitialized(); return mutability; } - /** Returns the parent {@link GlobalFrame}, if it exists. */ + /** + * Returns the parent {@link GlobalFrame}, if it exists. + * + *

TODO(laurentlb): Should be called getUniverse. + */ @Nullable - public GlobalFrame getParent() { + public Frame getParent() { checkInitialized(); - return parent; + return universe; } - /** - * Returns the label of this {@code Frame}, which may be null. Parent labels are not consulted. - * - *

Usually you want to use {@link #getTransitiveLabel}; this is just an accessor for - * completeness. - */ + /** Returns the label of this {@code Frame}, which may be null. */ @Nullable public Label getLabel() { checkInitialized(); return label; } - /** - * Walks from this {@link GlobalFrame} up through transitive parents, and returns the first - * non-null label found, or null if all labels are null. - */ + /** Same as getLabel. */ @Nullable public Label getTransitiveLabel() { checkInitialized(); - if (label != null) { - return label; - } else if (parent != null) { - return parent.getTransitiveLabel(); - } else { - return null; - } + return label; } /** - * Returns a map of direct bindings of this {@link GlobalFrame}, ignoring parents. + * Returns a map of direct bindings of this {@link GlobalFrame}, ignoring universe. * *

The bindings are returned in a deterministic order (for a given sequence of initial values * and updates). @@ -402,17 +404,16 @@ public Map getTransitiveBindings() { checkInitialized(); // Can't use ImmutableMap.Builder because it doesn't allow duplicates. LinkedHashMap collectedBindings = new LinkedHashMap<>(); - accumulateTransitiveBindings(collectedBindings); + if (universe != null) { + collectedBindings.putAll(universe.getTransitiveBindings()); + } + collectedBindings.putAll(getBindings()); return collectedBindings; } - private void accumulateTransitiveBindings(LinkedHashMap accumulator) { + public Object getDirectBindings(String varname) { checkInitialized(); - // Put parents first, so child bindings take precedence. - if (parent != null) { - parent.accumulateTransitiveBindings(accumulator); - } - accumulator.putAll(bindings); + return bindings.get(varname); } @Override @@ -422,8 +423,8 @@ public Object get(String varname) { if (val != null) { return val; } - if (parent != null) { - return parent.get(varname); + if (universe != null) { + return universe.get(varname); } return null; } @@ -855,11 +856,13 @@ public static class Builder { this.mutability = mutability; } - /** Inherits global bindings from the given parent Frame. */ + /** + * Inherits global bindings from the given parent Frame. + * + *

TODO(laurentlb): this should be called setUniverse. + */ public Builder setGlobals(GlobalFrame parent) { Preconditions.checkState(this.parent == null); - // Make sure that the global frame does at most two lookups: one for the module definitions - // and one for the builtins. this.parent = parent; return this; } @@ -899,7 +902,7 @@ public Environment build() { Preconditions.checkArgument(!mutability.isFrozen()); if (parent != null) { Preconditions.checkArgument(parent.mutability().isFrozen(), "parent frame must be frozen"); - if (parent.parent != null) { // This code path doesn't happen in Bazel. + if (parent.universe != null) { // This code path doesn't happen in Bazel. // Flatten the frame, ensure all builtins are in the same frame. parent = @@ -1053,16 +1056,14 @@ public Object localLookup(String varname) { /** * Returns the value of a variable defined in the Module scope (e.g. global variables, functions). - * - *

TODO(laurentlb): This method may also return values from the universe. We should fix that. */ public Object moduleLookup(String varname) { - return globalFrame.get(varname); + return globalFrame.getDirectBindings(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. + // TODO(laurentlb): look only at globalFrame.universe. Object result = globalFrame.get(varname); if (result == null) { 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 810aaa219456ba..3dae2bcd8acdaf 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 @@ -1839,7 +1839,16 @@ public void testLegacyGlobalVariableNotShadowed() throws Exception { @Test public void testShadowBuiltin() throws Exception { - // TODO(laurentlb): Forbid this. + new SkylarkTest("--incompatible_static_name_resolution=true") + .testIfErrorContains( + "global variable 'len' is referenced before assignment", + "x = len('abc')", + "len = 2", + "y = x + len"); + } + + @Test + public void testLegacyShadowBuiltin() throws Exception { new SkylarkTest("--incompatible_static_name_resolution=false") .setUp("x = len('abc')", "len = 2", "y = x + len") .testLookup("y", 5);