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
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 2d32d5c..a01dd72 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
@@ -38,6 +38,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
@@ -50,13 +51,11 @@
import javax.annotation.Nullable;
/**
- * An {@code Environment} is the main entry point to evaluating Skylark code. It embodies all the
- * state that is required to evaluate such code, except for the current instruction pointer, which
- * is an {@link ASTNode} that is evaluated (for expressions) or executed (for statements) with
- * respect to this {@code Environment}.
+ * An Environment represents a Starlark thread.
*
- * <p>{@link Continuation}-s are explicitly represented, but only partly, with another part being
- * implicit in a series of try-catch statements, to maintain the direct style.
+ * <p>It holds the stack of active Starlark and built-in function calls. In addition, it may hold
+ * per-thread application state (see {@link #setThreadLocal}) that passes through Starlark functions
+ * but does not directly affect them, such as information about the BUILD file being loaded.
*
* <p>Every {@code Environment} has a {@link Mutability} field, and must be used within a function
* that creates and closes this {@link Mutability} with the try-with-resource pattern. This {@link
@@ -69,18 +68,55 @@
* its objects from within a different {@code Environment}.
*
* <p>One creates an Environment using the {@link #builder} function, then populates it with {@link
- * #setup}, {@link #setupDynamic} and sometimes {@link #setupOverride}, before to evaluate code in
- * it with {@link BuildFileAST#eval}, or with {@link BuildFileAST#exec} (where the AST was obtained
- * by passing a {@link ValidationEnvironment} constructed from the Environment to {@link
+ * #setup} and sometimes {@link #setupOverride}, before to evaluate code in it with {@link
+ * BuildFileAST#eval}, or with {@link BuildFileAST#exec} (where the AST was obtained by passing a
+ * {@link ValidationEnvironment} constructed from the Environment to {@link
* BuildFileAST#parseBuildFile} or {@link BuildFileAST#parseSkylarkFile}). When the computation is
* over, the frozen Environment can still be queried with {@link #lookup}.
- *
- * <p>Final fields of an Environment represent its dynamic state, i.e. state that remains the same
- * throughout a given evaluation context, and don't change with source code location, while mutable
- * fields embody its static state, that change with source code location. The seeming paradox is
- * that the words "dynamic" and "static" refer to the point of view of the source code, and here we
- * have a dual point of view.
*/
+// TODO(adonovan): further steps for Environmental remediation:
+// This class should be renamed StarlarkThread, for that is what it is.
+// Its API should expose the following concepts, and no more:
+// 1) "thread local variables": this holds per-thread application
+// state such as the current Label, or BUILD package, for all the
+// native.* built-ins.
+// This may include any thread-specific behaviour relevant to the
+// load and print statements.
+// 2) a stack of call frames, each representing an active function call.
+// Only clients needing debugger-like powers of reflection should need
+// this, such as the debugger itself, and the ill-conceived
+// generator_name attribute. The API for call frames should not
+// expose an object of class CallFrame, because for efficiency we
+// will want to recycle objects in place rather than generate garbage
+// on every call.
+// So the API will look like getCallerLocation(depth),
+// not getCaller(depth).location, with one method per "public" CallFrame
+// attribute, such as location.
+// We must expose these basic CallFrame attributes, for stack traces and errors:
+// - function name
+// - PC location
+// Advanced clients such as the debugger, and the generator_name rule attribute, also need:
+// - the function value (Warning: careless clients can pin closures in memory)
+// - Object getLocalValue(Identifier parameter).
+// 3) Debugging support (thread name, profiling counters, etc).
+// And that is all. See go.starlark.net for the model.
+//
+// The Frame interface should be hidden from clients and then eliminated.
+// The dynamic lookup mechanism should go away.
+// The GlobalFrame class should be redesigned.
+// The concept struggling to get out of it is a Module,
+// which is created before file initialization and
+// populated by execution of the top-level statements in a file;
+// every UserDefinedFunction value should hold a reference to its Module.
+// As best I can tell, all the skyframe serialization
+// as it applies to LexicalFrames is redundant, as these are transient
+// and should not exist after loading.
+// We will remove the FuncallExpression parameter from StarlarkFunction.call.
+// Clients should use getCallerLocation instead.
+// The Continuation class should be deleted.
+// Once the API is small and sound, we can start to represent all
+// the lexical frames within a single function using just an array,
+// indexed by a small integer computed during the validation pass.
public final class Environment implements Freezable, Debuggable {
/**
@@ -101,7 +137,7 @@
* <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 {
+ private interface Frame extends Freezable {
/**
* Gets a binding from this {@link Frame} or one of its transitive parents.
*
@@ -260,7 +296,7 @@
@Nullable private Mutability mutability;
/** Final, except that it may be initialized after instantiation. */
- @Nullable private Frame universe;
+ @Nullable private GlobalFrame universe;
/**
* If this frame is a global frame, the label for the corresponding target, e.g. {@code
@@ -426,7 +462,7 @@
* <p>TODO(laurentlb): Should be called getUniverse.
*/
@Nullable
- public Frame getParent() {
+ public GlobalFrame getParent() {
checkInitialized();
return universe;
}
@@ -521,6 +557,29 @@
}
}
+ // The mutability of the Environment comes from its initial global frame.
+ private final Mutability mutability;
+
+ private final Map<Class<?>, Object> threadLocals = new HashMap<>();
+
+ /**
+ * setThreadLocal saves {@code value} as a thread-local variable of this Starlark thread, keyed by
+ * {@code key}, so that it can later be retrieved by {@code getThreadLocal(key)}.
+ */
+ public <T> void setThreadLocal(Class<T> key, T value) {
+ // The clazz parameter is redundant, but it makes the API clearer.
+ threadLocals.put(key, value);
+ }
+
+ /**
+ * getThreadLocal returns the value {@code v} supplied to the most recent {@code
+ * setThreadLocal(key, v)} call, or null if there was no prior call.
+ */
+ public <T> T getThreadLocal(Class<T> key) {
+ Object v = threadLocals.get(key);
+ return v == null ? null : key.cast(v);
+ }
+
/**
* A Continuation contains data saved during a function call and restored when the function exits.
*/
@@ -735,13 +794,6 @@
*/
private GlobalFrame globalFrame;
- /**
- * Dynamic Frame for variables that are always looked up in the runtime Environment, and never in
- * the lexical or "global" Environment as it was at the time of function definition. For instance,
- * PACKAGE_NAME.
- */
- private final Frame dynamicFrame;
-
/** The semantics options that affect how Skylark code is evaluated. */
private final StarlarkSemantics semantics;
@@ -811,8 +863,7 @@
@Override
public Mutability mutability() {
- // the mutability of the environment is that of its dynamic frame.
- return dynamicFrame.mutability();
+ return mutability;
}
/** Returns the global variables for the Environment (not including dynamic bindings). */
@@ -836,6 +887,8 @@
*/
boolean isRecursiveCall(UserDefinedFunction function) {
for (Continuation k = continuation; k != null; k = k.continuation) {
+ // TODO(adonovan): compare code, not closure values, otherwise
+ // one can defeat this check by writing the Y combinator.
if (k.function.equals(function)) {
return true;
}
@@ -865,7 +918,6 @@
* Constructs an Environment. This is the main, most basic constructor.
*
* @param globalFrame a frame for the global Environment
- * @param dynamicFrame a frame for the dynamic Environment
* @param eventHandler an EventHandler for warnings, errors, etc
* @param importedExtensions Extension-s from which to import bindings with load()
* @param fileContentHashCode a hash for the source file being evaluated, if any
@@ -873,7 +925,6 @@
*/
private Environment(
GlobalFrame globalFrame,
- LexicalFrame dynamicFrame,
StarlarkSemantics semantics,
StarlarkContext starlarkContext,
EventHandler eventHandler,
@@ -882,9 +933,8 @@
@Nullable Label callerLabel) {
this.lexicalFrame = Preconditions.checkNotNull(globalFrame);
this.globalFrame = Preconditions.checkNotNull(globalFrame);
- this.dynamicFrame = Preconditions.checkNotNull(dynamicFrame);
+ this.mutability = globalFrame.mutability();
Preconditions.checkArgument(!globalFrame.mutability().isFrozen());
- Preconditions.checkArgument(!dynamicFrame.mutability().isFrozen());
this.semantics = semantics;
this.starlarkContext = starlarkContext;
this.eventHandler = eventHandler;
@@ -996,13 +1046,11 @@
parent = GlobalFrame.filterOutRestrictedBindings(mutability, parent, semantics);
GlobalFrame globalFrame = new GlobalFrame(mutability, parent);
- LexicalFrame dynamicFrame = LexicalFrame.create(mutability);
if (importedExtensions == null) {
importedExtensions = ImmutableMap.of();
}
return new Environment(
globalFrame,
- dynamicFrame,
semantics,
starlarkContext,
eventHandler,
@@ -1026,32 +1074,6 @@
return callerLabel;
}
- /**
- * Sets a binding for a special dynamic variable in this Environment. This is not for end-users,
- * and will throw an AssertionError in case of conflict.
- *
- * @param varname the name of the dynamic variable to be bound
- * @param value a value to bind to the variable
- * @return this Environment, in fluid style
- */
- public Environment setupDynamic(String varname, Object value) {
- if (lookup(varname) != null) {
- throw new AssertionError(
- String.format("Trying to bind dynamic variable '%s' but it is already bound", varname));
- }
- try {
- dynamicFrame.put(this, varname, value);
- } catch (MutabilityException e) {
- // End users don't have access to setupDynamic, and it is an implementation error
- // if we encounter a mutability exception.
- throw new AssertionError(
- String.format(
- "Trying to bind dynamic variable '%s' in frozen environment %s", varname, this),
- e);
- }
- return this;
- }
-
/** Remove variable from local bindings. */
void removeLocalBinding(String varname) {
try {
@@ -1148,11 +1170,6 @@
return globalFrame.get(varname);
}
- /** Returns the value of a variable defined with setupDynamic. */
- public Object dynamicLookup(String varname) {
- return dynamicFrame.get(varname);
- }
-
/**
* Returns the value from the environment whose name is "varname" if it exists, otherwise null.
*
@@ -1160,7 +1177,7 @@
* the corresponding method (e.g. localLookup or moduleLookup).
*/
Object lookup(String varname) {
- // Lexical frame takes precedence, then globals, then dynamics.
+ // Lexical frame takes precedence, then globals.
Object lexicalValue = lexicalFrame.get(varname);
if (lexicalValue != null) {
return lexicalValue;
@@ -1203,7 +1220,6 @@
vars.addAll(lexicalFrame.getTransitiveBindings().keySet());
// No-op when globalFrame = lexicalFrame
vars.addAll(globalFrame.getTransitiveBindings().keySet());
- vars.addAll(dynamicFrame.getTransitiveBindings().keySet());
return vars;
}
@@ -1328,6 +1344,7 @@
* An Exception thrown when an attempt is made to import a symbol from a file that was not
* properly loaded.
*/
+ // TODO(adonovan): replace with plain EvalException.
static class LoadFailedException extends Exception {
LoadFailedException(String importString) {
super(