bazel syntax: first steps towards Environmental remediation

- make Frame interface private.
- replace "dynamic frame" by simpler mechanism of "Starlark thread locals".
  These values are carried along by the thread but are no longer part of
  the environment accessible to Starlark programs.
- improve one error message (and update test)
- outline next steps for cleanup.

PiperOrigin-RevId: 265902466

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java

@@ -56,7 +56,6 @@
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SkylarkImplicitOutputsFunctionWithCallback;
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SkylarkImplicitOutputsFunctionWithMap;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
-import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
 import com.google.devtools.build.lib.packages.PredicateWithMessage;
 import com.google.devtools.build.lib.packages.Provider;
@@ -306,7 +305,7 @@ public BaseFunction rule(
     // We'll set the name later, pass the empty string for now.
     RuleClass.Builder builder = new RuleClass.Builder("", type, true, parent);
     ImmutableList<Pair<String, SkylarkAttr.Descriptor>> attributes =
-        attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv);
+        attrObjectToAttributesList(attrs, ast.getLocation());
 
     if (skylarkTestable) {
       builder.setSkylarkTestable();
@@ -425,7 +424,7 @@ private static void checkAttributeName(Location loc, String name) throws EvalExc
   }
 
   private static ImmutableList<Pair<String, Descriptor>> attrObjectToAttributesList(
-      Object attrs, Location loc, Environment env) throws EvalException {
+      Object attrs, Location loc) throws EvalException {
     ImmutableList.Builder<Pair<String, Descriptor>> attributes = ImmutableList.builder();
 
     if (attrs != Runtime.NONE) {
@@ -526,7 +525,7 @@ public SkylarkAspect aspect(
     }
 
     ImmutableList<Pair<String, SkylarkAttr.Descriptor>> descriptors =
-        attrObjectToAttributesList(attrs, ast.getLocation(), funcallEnv);
+        attrObjectToAttributesList(attrs, ast.getLocation());
     ImmutableList.Builder<Attribute> attributes = ImmutableList.builder();
     ImmutableSet.Builder<String> requiredParams = ImmutableSet.builder();
     for (Pair<String, Descriptor> nameDescriptorPair : descriptors) {
@@ -641,8 +640,6 @@ private SkylarkRuleFunction(
     }
 
     @Override
-    @SuppressWarnings("unchecked") // the magic hidden $pkg_context variable is guaranteed
-    // to be a PackageContext
     public Object call(Object[] args, FuncallExpression ast, Environment env)
         throws EvalException, InterruptedException, ConversionException {
       SkylarkUtils.checkLoadingPhase(env, getName(), ast.getLocation());
@@ -671,11 +668,12 @@ public Object call(Object[] args, FuncallExpression ast, Environment env)
       BuildLangTypedAttributeValuesMap attributeValues =
           new BuildLangTypedAttributeValuesMap((Map<String, Object>) args[0]);
       try {
-        PackageContext pkgContext = (PackageContext) env.dynamicLookup(PackageFactory.PKG_CONTEXT);
+        PackageContext pkgContext = env.getThreadLocal(PackageContext.class);
         if (pkgContext == null) {
-          throw new EvalException(ast.getLocation(),
-              "Cannot instantiate a rule when loading a .bzl file. Rules can only be called from "
-                  + "a BUILD file (possibly via a macro).");
+          throw new EvalException(
+              ast.getLocation(),
+              "Cannot instantiate a rule when loading a .bzl file. "
+                  + "Rules may be instantiated only in a BUILD thread.");
         }
         RuleFactory.createAndAddRule(
             pkgContext,

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -275,8 +275,6 @@ protected void process(Package.Builder pkgBuilder, Location location,
     }
   }
 
-  public static final String PKG_CONTEXT = "$pkg_context";
-
   /** {@link Globber} that uses the legacy GlobCache. */
   public static class LegacyGlobber implements Globber {
     private final GlobCache globCache;
@@ -1252,13 +1250,14 @@ public Object call(Object[] arguments, FuncallExpression ast, Environment env)
    */
   public static PackageContext getContext(Environment env, Location location)
       throws EvalException {
-    PackageContext value = (PackageContext) env.dynamicLookup(PKG_CONTEXT);
+    PackageContext value = env.getThreadLocal(PackageContext.class);
     if (value == null) {
-      // if PKG_CONTEXT is missing, we're not called from a BUILD file. This happens if someone
+      // if PackageContext is missing, we're not called from a BUILD file. This happens if someone
       // uses native.some_func() in the wrong place.
-      throw new EvalException(location,
-          "The native module cannot be accessed from here. "
-          + "Wrap the function in a macro and call it from a BUILD file");
+      throw new EvalException(
+          location,
+          "The native module can be accessed only from a BUILD thread. "
+              + "Wrap the function in a macro and call it from a BUILD file");
     }
     return value;
   }
@@ -1566,15 +1565,13 @@ private byte[] maybeGetBuildFileBytes(Path buildFile, ExtendedEventHandler event
   }
 
   /**
-   * This tuple holds the current package builder, current lexer, etc, for the
-   * duration of the evaluation of one BUILD file. (We use a PackageContext
-   * object in preference to storing these values in mutable fields of the
-   * PackageFactory.)
+   * This class holds state associated with the construction of a single package for the duration of
+   * execution of one BUILD file. (We use a PackageContext object in preference to storing these
+   * values in mutable fields of the PackageFactory.)
    *
-   * <p>PLEASE NOTE: references to PackageContext objects are held by many
-   * BaseFunction closures, but should become unreachable once the Environment is
-   * discarded at the end of evaluation.  Please be aware of your memory
-   * footprint when making changes here!
+   * <p>PLEASE NOTE: the PackageContext is referred to by the Environment, but should become
+   * unreachable once the Environment is discarded at the end of evaluation. Please be aware of your
+   * memory footprint when making changes here!
    */
   public static class PackageContext {
     final Package.Builder pkgBuilder;
@@ -1686,7 +1683,7 @@ private void buildPkgEnv(Environment pkgEnv, PackageContext context) {
       extension.update(pkgEnv);
     }
 
-    pkgEnv.setupDynamic(PKG_CONTEXT, context);
+    pkgEnv.setThreadLocal(PackageContext.class, context);
   }
 
   /**

src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java

@@ -67,8 +67,7 @@ public class WorkspaceFactory {
           "workspace",
           "__embedded_dir__", // serializable so optional
           "__workspace_dir__", // serializable so optional
-          "DEFAULT_SYSTEM_JAVABASE", // serializable so optional
-          PackageFactory.PKG_CONTEXT);
+          "DEFAULT_SYSTEM_JAVABASE"); // serializable so optional
 
   private final Package.Builder builder;
 
@@ -368,8 +367,8 @@ private void addWorkspaceFunctions(Environment workspaceEnv, StoredEventHandler
       for (EnvironmentExtension extension : environmentExtensions) {
         extension.updateWorkspace(workspaceEnv);
       }
-      workspaceEnv.setupDynamic(
-          PackageFactory.PKG_CONTEXT,
+      workspaceEnv.setThreadLocal(
+          PackageFactory.PackageContext.class,
           new PackageFactory.PackageContext(builder, null, localReporter, AttributeContainer::new));
     } catch (EvalException e) {
       throw new AssertionError(e);

src/main/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunction.java

@@ -22,7 +22,6 @@
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Mutability;
-import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.StarlarkSemantics;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -115,18 +114,15 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionExcept
       long astFileSize = fileValue.getSize();
       try (Mutability mutability = Mutability.create("validate")) {
         com.google.devtools.build.lib.syntax.Environment validationEnv =
-            ruleClassProvider
-                .createSkylarkRuleClassEnvironment(
-                    fileLabel,
-                    mutability,
-                    starlarkSemantics,
-                    env.getListener(),
-                    // the three below don't matter for extracting the ValidationEnvironment:
-                    /*astFileContentHashCode=*/ null,
-                    /*importMap=*/ null,
-                    /*repoMapping=*/ ImmutableMap.of())
-                .setupDynamic(Runtime.PKG_NAME, Runtime.NONE)
-                .setupDynamic(Runtime.REPOSITORY_NAME, Runtime.NONE);
+            ruleClassProvider.createSkylarkRuleClassEnvironment(
+                fileLabel,
+                mutability,
+                starlarkSemantics,
+                env.getListener(),
+                // the three below don't matter for extracting the ValidationEnvironment:
+                /*astFileContentHashCode=*/ null,
+                /*importMap=*/ null,
+                /*repoMapping=*/ ImmutableMap.of());
         byte[] bytes = FileSystemUtils.readWithKnownFileSize(path, astFileSize);
         ast =
             BuildFileAST.parseSkylarkFile(

src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/TestUtils.java

@@ -24,7 +24,6 @@
 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;
@@ -97,7 +96,7 @@ public static <T> T roundTrip(T value) throws IOException, SerializationExceptio
     return TestUtils.roundTrip(value, ImmutableMap.of());
   }
 
-  public static void assertFramesEqual(Frame frame1, Frame frame2) {
+  public static void assertFramesEqual(GlobalFrame frame1, GlobalFrame frame2) {
     assertThat(frame1.getTransitiveBindings())
         .containsExactlyEntriesIn(frame2.getTransitiveBindings())
         .inOrder();