diff options
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
6 files changed, 129 insertions, 68 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index 7c2b34c7c7..7f8a72f54c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java @@ -762,7 +762,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider { Map<String, Extension> importMap) { return createSkylarkRuleClassEnvironment( mutability, - globals.setLabel(extensionLabel), + globals.withLabel(extensionLabel), skylarkSemantics, eventHandler, astFileContentHashCode, diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java index 03a1050b0e..d26ede30e3 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java @@ -218,7 +218,7 @@ public class WorkspaceFactory { // each workspace file. ImmutableMap.Builder<String, Object> bindingsBuilder = ImmutableMap.builder(); Frame globals = workspaceEnv.getGlobals(); - for (String s : globals.getDirectVariableNames()) { + for (String s : globals.getBindings().keySet()) { Object o = globals.get(s); if (!isAWorkspaceFunction(s, o)) { bindingsBuilder.put(s, o); diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java index 2ba416ff2c..67468cdf21 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java @@ -179,7 +179,7 @@ public final class SkylarkAttr { new SkylarkComputedDefaultTemplate( type, callback.getParameterNames(), callback, ast.getLocation())); } else { - builder.defaultValue(defaultValue, env.getGlobals().label()); + builder.defaultValue(defaultValue, env.getGlobals().getTransitiveLabel()); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index a3bfeeaa6b..e5c33e9140 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java @@ -804,7 +804,7 @@ public class SkylarkRuleClassFunctions { if (relativeToCallerRepository) { parentLabel = env.getCallerLabel(); } else { - parentLabel = env.getGlobals().label(); + parentLabel = env.getGlobals().getTransitiveLabel(); } try { if (parentLabel != null) { 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 3e9ca1583c..e4801bcfb5 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.Preconditions; 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 final class Environment implements Freezable { 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). + private final Frame parent; + + // If this frame is a global frame, the label for the corresponding target, e.g. //foo:bar.bzl. @Nullable - private Label label; + private final Label label; + + private final Map<String, Object> bindings; + + public Frame(Mutability mutability) { + this(mutability, null, null); + } - private Frame(Mutability mutability, Frame parent) { + 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 = parent == null ? null : parent.label; + 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; } + /** Returns the parent {@code Frame}, if it exists. */ + @Nullable + public Frame getParent() { + return parent; + } + /** - * 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. + * 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. */ - public Frame setLabel(Label label) { - Frame result = new Frame(mutability, this); - result.label = label; - return result; + @Nullable + public Label getLabel() { + return label; } /** - * Returns the label for this frame. + * 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 final Label label() { - return label; + 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 frame or if not found its parent. + * 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 @@ public final class Environment implements Freezable { } /** - * 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 @@ public final class Environment implements Freezable { 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 @@ public final class Environment implements Freezable { 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 @@ public final class Environment implements Freezable { * @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 @@ public final class Environment implements Freezable { 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; } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java b/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java index 6c0f523576..ead4df060e 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/Mutability.java @@ -88,7 +88,7 @@ public final class Mutability implements AutoCloseable, Serializable { return new Mutability(Printer.format(pattern, arguments)); } - String getAnnotation() { + public String getAnnotation() { return annotation; } @@ -97,7 +97,7 @@ public final class Mutability implements AutoCloseable, Serializable { return String.format(isFrozen ? "(%s)" : "[%s]", annotation); } - boolean isFrozen() { + public boolean isFrozen() { return isFrozen; } |