Cleaner separatation between Module and Universe variables.

- In the global frame, rename "parent" into "universe" to clarify what it is.
- Function `moduleLookup` now does the right thing. It doesn't check the universe anymore.

This means that we now forbid (with --incompatible_static_name_resolution) this:
  a = len("abc")
  len = 2

#5827

RELNOTES: None.
PiperOrigin-RevId: 213647873

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> 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());
     }
   }
 

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.
    *
-   * <p>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.
+   * <p>Bindings in a {@link GlobalFrame} may shadow those inherited from its universe.
    *
    * <p>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<String, Object> 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<String, Object> 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.
+     *
+     * <p>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.
-     *
-     * <p>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.
      *
      * <p>The bindings are returned in a deterministic order (for a given sequence of initial values
      * and updates).
@@ -402,17 +404,16 @@ public Map<String, Object> getTransitiveBindings() {
       checkInitialized();
       // Can't use ImmutableMap.Builder because it doesn't allow duplicates.
       LinkedHashMap<String, Object> collectedBindings = new LinkedHashMap<>();
-      accumulateTransitiveBindings(collectedBindings);
+      if (universe != null) {
+        collectedBindings.putAll(universe.getTransitiveBindings());
+      }
+      collectedBindings.putAll(getBindings());
       return collectedBindings;
     }
 
-    private void accumulateTransitiveBindings(LinkedHashMap<String, Object> 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.
+     *
+     * <p>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).
-   *
-   * <p>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) {

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);