Environment: ensure that the global frame does at most two lookups

When we try to access a variable, we do a lookup in the environment. The globalframe has a parent field, so we have no limit on the number of lookups. It may do multiple lookups until it finds a value or the parent is null.

With this CL:
- globalframe represents the symbols from the file
- globalframe.parent represents the universe (builtins)
- globalframe.parent.parent won't exist

So it's at most two lookups. Later, we should reduce to just one lookup. After that, we can have further optimizations (e.g. array lookup instead of hashmap lookup).

#5637

RELNOTES: None.
PiperOrigin-RevId: 211143657

src/main/java/com/google/devtools/build/lib/syntax/Environment.java

@@ -102,6 +102,9 @@ public enum Phase {
    *
    * <p>A {@link Frame} can have an associated "parent" {@link Frame}, which is used in {@link #get}
    * and {@link #getTransitiveBindings()}
+   *
+   * <p>TODO(laurentlb): "parent" should be named "universe" since it contains only the builtins.
+   * The "get" method shouldn't look at the universe (so that "moduleLookup" works as expected)
    */
   public interface Frame extends Freezable {
     /**
@@ -288,6 +291,7 @@ public GlobalFrame(
         @Nullable GlobalFrame parent,
         @Nullable Label label,
         @Nullable Map<String, Object> bindings) {
+      Preconditions.checkState(parent == null || parent.parent == null); // no more than 1 parent
       this.mutability = Preconditions.checkNotNull(mutability);
       this.parent = parent;
       this.label = label;
@@ -333,12 +337,12 @@ public void initialize(
     }
 
     /**
-     * Returns a new {@link GlobalFrame} that is a descendant of this one with {@link #label} set to
+     * Returns a new {@link GlobalFrame} with the same fields, except that {@link #label} is set to
      * the given value.
      */
     public GlobalFrame withLabel(Label label) {
       checkInitialized();
-      return new GlobalFrame(mutability, this, label);
+      return new GlobalFrame(mutability, /*parent*/ null, label, bindings);
     }
 
     /**
@@ -898,6 +902,8 @@ public Builder setPhase(Phase phase) {
     /** Inherits global bindings from the given parent Frame. */
     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;
     }
@@ -937,6 +943,16 @@ 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.
+
+          // Flatten the frame, ensure all builtins are in the same frame.
+          parent =
+              new GlobalFrame(
+                  parent.mutability(),
+                  null /* parent */,
+                  parent.label,
+                  parent.getTransitiveBindings());
+        }
       }
       GlobalFrame globalFrame = new GlobalFrame(mutability, parent);
       LexicalFrame dynamicFrame = LexicalFrame.create(mutability);
@@ -1082,17 +1098,17 @@ public Object localLookup(String varname) {
   }
 
   /**
-   * Returns the value of a variable defined in the Module scope (e.g. global variables,
-   * functions).
+   * 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);
   }
 
   /** Returns the value of a variable defined in the Universe scope (builtins). */
   public Object universeLookup(String varname) {
-    // TODO(laurentlb): We should distinguish between Module and Universe. Values in Module can
-    // shadow those in Universe.
+    // TODO(laurentlb): look only at globalFrame.parent.
     return globalFrame.get(varname);
   }
 

src/test/java/com/google/devtools/build/lib/syntax/EnvironmentTest.java

@@ -101,67 +101,22 @@ public void testBuilderRequiresSemantics() throws Exception {
 
   @Test
   public void testGetVariableNames() throws Exception {
-    Environment outerEnv;
-    Environment innerEnv;
+    Environment env;
     try (Mutability mut = Mutability.create("outer")) {
-      outerEnv =
+      env =
           Environment.builder(mut)
               .useDefaultSemantics()
               .setGlobals(Environment.DEFAULT_GLOBALS)
               .build()
               .update("foo", "bar")
               .update("wiz", 3);
     }
-    try (Mutability mut = Mutability.create("inner")) {
-      innerEnv =
-          Environment.builder(mut)
-              .useDefaultSemantics()
-              .setGlobals(outerEnv.getGlobals())
-              .build()
-              .update("foo", "bat")
-              .update("quux", 42);
-    }
 
-    assertThat(outerEnv.getVariableNames())
-        .isEqualTo(
-            Sets.newHashSet(
-                "foo",
-                "wiz",
-                "False",
-                "None",
-                "True",
-                "all",
-                "any",
-                "bool",
-                "depset",
-                "dict",
-                "dir",
-                "enumerate",
-                "fail",
-                "getattr",
-                "hasattr",
-                "hash",
-                "int",
-                "len",
-                "list",
-                "max",
-                "min",
-                "print",
-                "range",
-                "repr",
-                "reversed",
-                "select",
-                "sorted",
-                "str",
-                "tuple",
-                "type",
-                "zip"));
-    assertThat(innerEnv.getVariableNames())
+    assertThat(env.getVariableNames())
         .isEqualTo(
             Sets.newHashSet(
                 "foo",
                 "wiz",
-                "quux",
                 "False",
                 "None",
                 "True",

src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java

@@ -1821,6 +1821,13 @@ public void testShadowisNotInitialized() throws Exception {
             "res = foo()");
   }
 
+  @Test
+  public void testLegacyGlobalIsNotInitialized() throws Exception {
+    new SkylarkTest("--incompatible_static_name_resolution=false")
+        .setUp("a = len")
+        .testIfErrorContains("Variable len is read only", "len = 2");
+  }
+
   @Test
   public void testFunctionCallRecursion() throws Exception {
     new SkylarkTest().testIfErrorContains("Recursion was detected when calling 'f' from 'g'",