Clean up Environment.Frame
Make fields visibility/accessors more idiomatic. Prefer accessors that give a full map of the bindings and inherited bindings, rather than just the keys.
Also increase visibility of some accessors on Mutability.
RELNOTES: None
PiperOrigin-RevId: 155393780
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 3e9ca15..e4801bc 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
@@ -29,6 +29,8 @@
import com.google.devtools.build.lib.util.SpellChecker;
import com.google.devtools.common.options.Options;
import java.io.Serializable;
+import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.Map;
@@ -77,70 +79,141 @@
public enum Phase { WORKSPACE, LOADING, ANALYSIS }
/**
- * A Frame is a Map of bindings, plus a {@link Mutability} and a parent Frame
- * from which to inherit bindings.
+ * A mapping of bindings, along with a {@link Mutability} and a parent {@code Frame} from which to
+ * inherit bindings. The order of the bindings within a single {@code Frame} is deterministic but
+ * unspecified.
*
- * <p>A Frame contains bindings mapping variable name to variable value in a given scope.
- * It may also inherit bindings from a parent Frame corresponding to a parent scope,
- * which in turn may inherit bindings from its own parent, etc., transitively.
- * Bindings may shadow bindings from the parent. In Skylark, you may only mutate
- * bindings from the current Frame, which always got its {@link Mutability} with the
- * current {@link Environment}; but future extensions may make it more like Python
- * and allow mutation of bindings in outer Frame-s (or then again may not).
+ * <p>Each {@code Frame} can be thought of as either a lexical scope or a scope containing
+ * predefined variables. Bindings in a {@code Frame} may shadow those inherited from its parents.
+ * Thus, the chain of {@code Frame}s can represent a hierarchy of enclosing scopes, or a
+ * collection of builtin modules with a linear precedence ordering.
*
- * <p>A Frame inherits the {@link Mutability} from the {@link Environment} in which it was
- * originally created. When that {@link Environment} is finalized and its {@link Mutability}
- * is closed, it becomes immutable, including the Frame, which can be shared in other
- * {@link Environment}-s. Indeed, a {@link UserDefinedFunction} will close over the global
- * Frame of its definition {@link Environment}, which will thus be reused (immutably)
- * in any {@link Environment} in which this function is called, so it's important to
- * preserve the {@link Mutability} to make sure no Frame is modified after it's been finalized.
+ * <p>Any non-frozen {@code Frame} must have the same {@code Mutability} as the current {@link
+ * Environment}, to avoid interference from other evaluation contexts. For example, a {@link
+ * UserDefinedFunction} will close over the global frame of the {@code Environment} in which it
+ * was defined. When the function is called from other {@code Environment}s (possibly
+ * simultaneously), that global frame must already be frozen; a new local {@code Frame} is created
+ * to represent the lexical scope of the function.
*/
public static final class Frame implements Freezable {
private final Mutability mutability;
- @Nullable
- final Frame parent;
- final Map<String, Object> bindings = new LinkedHashMap<>();
- // The label for the target this frame is defined in (e.g., //foo:bar.bzl).
- @Nullable
- private Label label;
- private Frame(Mutability mutability, Frame parent) {
- this.mutability = mutability;
- this.parent = parent;
- this.label = parent == null ? null : parent.label;
+ @Nullable
+ private final Frame parent;
+
+ // If this frame is a global frame, the label for the corresponding target, e.g. //foo:bar.bzl.
+ @Nullable
+ private final Label label;
+
+ private final Map<String, Object> bindings;
+
+ public Frame(Mutability mutability) {
+ this(mutability, null, null);
}
+ public Frame(Mutability mutability, Frame parent) {
+ this(mutability, parent, null);
+ }
+
+ public Frame(Mutability mutability, Frame parent, Label label) {
+ this.mutability = mutability;
+ this.parent = parent;
+ this.label = label;
+ this.bindings = new LinkedHashMap<>();
+ }
+
+ public Frame(Mutability mutability, Frame parent, Label label, Map<String, Object> bindings) {
+ this(mutability, parent, label);
+ this.bindings.putAll(bindings);
+ }
+
+ /**
+ * Returns a new {@code Frame} that is a copy of this one, but with {@code label} set to the
+ * given value.
+ */
+ public Frame withLabel(Label label) {
+ return new Frame(mutability, this, label);
+ }
+
+ /**
+ * Returns the {@link Mutability} of this {@code Frame}, which may be different from its
+ * parent's.
+ */
@Override
- public final Mutability mutability() {
+ public Mutability mutability() {
return mutability;
}
- /**
- * Attaches a label to an existing frame. This is used to get the repository a Skylark
- * extension is actually defined in.
- * @param label the label to attach.
- * @return a new Frame with the existing frame's properties plus the label.
- */
- public Frame setLabel(Label label) {
- Frame result = new Frame(mutability, this);
- result.label = label;
- return result;
+ /** Returns the parent {@code Frame}, if it exists. */
+ @Nullable
+ public Frame getParent() {
+ return parent;
}
/**
- * Returns the label for this frame.
+ * 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.
*/
@Nullable
- public final Label label() {
+ public Label getLabel() {
return label;
}
/**
- * Gets a binding from the current frame or if not found its parent.
+ * Walks from this {@code Frame} up through transitive parents, and returns the first non-null
+ * label found, or null if all labels are null.
+ */
+ @Nullable
+ public Label getTransitiveLabel() {
+ if (label != null) {
+ return label;
+ } else if (parent != null) {
+ return parent.getTransitiveLabel();
+ } else {
+ return null;
+ }
+ }
+
+ /**
+ * Returns a map of direct bindings of this {@code Frame}, ignoring parents.
+ *
+ * <p>For efficiency an unmodifiable view is returned. Callers should assume that the view is
+ * invalidated by any subsequent modification to the {@code Frame}'s bindings.
+ */
+ public Map<String, Object> getBindings() {
+ return Collections.unmodifiableMap(bindings);
+ }
+
+ /**
+ * Returns a map containing all bindings of this {@code Frame} and of its transitive parents,
+ * taking into account shadowing precedence.
+ */
+ public Map<String, Object> getTransitiveBindings() {
+ // Can't use ImmutableMap.Builder because it doesn't allow duplicates.
+ HashMap<String, Object> collectedBindings = new HashMap<>();
+ accumulateTransitiveBindings(collectedBindings);
+ return collectedBindings;
+ }
+
+ private void accumulateTransitiveBindings(Map<String, Object> accumulator) {
+ // Put parents first, so child bindings take precedence.
+ if (parent != null) {
+ parent.accumulateTransitiveBindings(accumulator);
+ }
+ accumulator.putAll(bindings);
+ }
+
+ /**
+ * Gets a binding from the current {@code Frame} or one of its transitive parents.
+ *
+ * <p>In case of conflicts, the binding found in the {@code Frame} closest to the current one is
+ * used; the remaining bindings are shadowed.
+ *
* @param varname the name of the variable to be bound
- * @return the value bound to variable
+ * @return the value bound to the variable, or null if no binding is found
*/
public Object get(String varname) {
if (bindings.containsKey(varname)) {
@@ -153,11 +226,12 @@
}
/**
- * Modifies a binding in the current Frame.
- * Does not try to modify an inherited binding.
- * This will shadow any inherited binding, which may be an error
- * that you want to guard against before calling this function.
- * @param env the Environment attempting the mutation
+ * Assigns or reassigns a binding in the current {@code Frame}.
+ *
+ * <p>If the binding has the same name as one in a transitive parent, the parent binding is
+ * shadowed (i.e., the parent is unaffected).
+ *
+ * @param env the {@link Environment} attempting the mutation
* @param varname the name of the variable to be bound
* @param value the value to bind to the variable
*/
@@ -167,22 +241,6 @@
bindings.put(varname, value);
}
- /**
- * Adds the variable names of this Frame and its transitive parents to the given set.
- * This provides a O(n) way of extracting the list of all variables visible in an Environment.
- * @param vars the set of visible variables in the Environment, being computed.
- */
- void addVariableNamesTo(Set<String> vars) {
- vars.addAll(bindings.keySet());
- if (parent != null) {
- parent.addVariableNamesTo(vars);
- }
- }
-
- public Set<String> getDirectVariableNames() {
- return bindings.keySet();
- }
-
@Override
public String toString() {
return String.format("<Frame%s>", mutability());
@@ -363,6 +421,9 @@
continuation =
new Continuation(
continuation, function, caller, lexicalFrame, globalFrame, knownGlobalVariables);
+ // TODO(bazel-team): What if instead of tracking both the lexical and global frames from the
+ // Environment, we instead just tracked the current lexical frame, and made the global frame its
+ // parent?
lexicalFrame = new Frame(mutability(), null);
globalFrame = globals;
knownGlobalVariables = new HashSet<>();
@@ -713,7 +774,7 @@
* @return the value from the environment whose name is "varname" if it exists, otherwise null.
*/
public Object lookup(String varname) {
- // Which Frame to lookup first doesn't matter because update prevents clashes.
+ // Lexical frame takes precedence, then globals, then dynamics.
if (lexicalFrame != null) {
Object lexicalValue = lexicalFrame.get(varname);
if (lexicalValue != null) {
@@ -750,14 +811,14 @@
eventHandler.handle(event);
}
- /** @return the (immutable) set of names of all variables defined in this Environment. */
+ /** Returns a set of all names of variables that are accessible in this {@code Environment}. */
public Set<String> getVariableNames() {
Set<String> vars = new HashSet<>();
if (lexicalFrame != null) {
- lexicalFrame.addVariableNamesTo(vars);
+ vars.addAll(lexicalFrame.getTransitiveBindings().keySet());
}
- globalFrame.addVariableNamesTo(vars);
- dynamicFrame.addVariableNamesTo(vars);
+ vars.addAll(globalFrame.getTransitiveBindings().keySet());
+ vars.addAll(dynamicFrame.getTransitiveBindings().keySet());
return vars;
}
