bazel syntax: eliminate calls to StarlarkThread.getGlobals during evaluation
Calls during evaluation want the innermost enclosing Starlark source
file on the call stack, which is now provided by the ugly helper function
Module.ofInnermostEnclosingStarlarkFunction.
All remaining calls to getGlobals are called before or after
execution, so StarlarkThread call/return no longer needs to
save/restore this.globalFrame (now called this.module). These calls
will be eliminated once the StarlarkThread and Module construction API
is simplified.
This is a breaking API change for Copybara. References to StarlarkThread
have been replaced by Module where the one appeared to be a proxy for the other.
No behavior change.
PiperOrigin-RevId: 292000083
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
index c8aa38e..91305aa7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkAttr.java
@@ -46,6 +46,7 @@
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.SkylarkType;
@@ -139,7 +140,7 @@
builder.defaultValue(
defaultValue,
new BuildType.LabelConversionContext(
- (Label) thread.getGlobals().getLabel(),
+ (Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel(),
BazelStarlarkContext.from(thread).getRepoMapping()),
DEFAULT_ARG);
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index b07bb56..9fdae97 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -85,6 +85,7 @@
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.FunctionSignature;
import com.google.devtools.build.lib.syntax.Identifier;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.SkylarkType;
@@ -356,7 +357,8 @@
hostFragments.getContents(String.class, "host_fragments"));
builder.setConfiguredTargetFunction(implementation);
builder.setRuleDefinitionEnvironmentLabelAndHashCode(
- (Label) thread.getGlobals().getLabel(), thread.getTransitiveContentHashCode());
+ (Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel(),
+ thread.getTransitiveContentHashCode());
builder.addRequiredToolchains(
collectToolchainLabels(
@@ -836,9 +838,8 @@
// rule or aspect implementation thread, or null otherwise.
parentLabel = context.getAnalysisRuleLabel();
} else {
- // This is the label of the BUILD/.bzl file on the top of the current call stack.
- // (Function enter/exit changes getGlobals.)
- parentLabel = (Label) thread.getGlobals().getLabel();
+ // This is the label of the innermost BUILD/.bzl file on the current call stack.
+ parentLabel = (Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel();
}
try {
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
index 54520ac..8bedec7 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java
@@ -43,6 +43,7 @@
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.FunctionSignature;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.Starlark;
@@ -101,7 +102,8 @@
}
builder.setConfiguredTargetFunction(implementation);
builder.setRuleDefinitionEnvironmentLabelAndHashCode(
- (Label) thread.getGlobals().getLabel(), thread.getTransitiveContentHashCode());
+ (Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel(),
+ thread.getTransitiveContentHashCode());
builder.setWorkspaceOnly();
return new RepositoryRuleFunction(builder, thread.getCallerLocation(), implementation);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
index c7658b4..de1af56 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.skylarkbuildapi.WorkspaceGlobalsApi;
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.NoneType;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.Starlark;
@@ -235,7 +236,8 @@
private static List<String> renamePatterns(
List<String> patterns, Package.Builder builder, StarlarkThread thread) {
- RepositoryName myName = getRepositoryName((Label) thread.getGlobals().getLabel());
+ RepositoryName myName =
+ getRepositoryName((Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel());
Map<RepositoryName, RepositoryName> renaming = builder.getRepositoryMappingFor(myName);
return patterns.stream()
.map(patternEntry -> TargetPattern.renameRepository(patternEntry, renaming))
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationOutputs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationOutputs.java
index a8c1cef..8865004 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationOutputs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationOutputs.java
@@ -23,6 +23,7 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.skylarkbuildapi.cpp.CcCompilationOutputsApi;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.StarlarkList;
import com.google.devtools.build.lib.syntax.StarlarkThread;
@@ -108,7 +109,9 @@
CcCommon.checkLocationWhitelisted(
thread.getSemantics(),
thread.getCallerLocation(),
- ((Label) thread.getGlobals().getLabel()).getPackageIdentifier().toString());
+ ((Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel())
+ .getPackageIdentifier()
+ .toString());
return StarlarkList.immutableCopyOf(getObjectFiles(usePic));
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Module.java b/src/main/java/com/google/devtools/build/lib/syntax/Module.java
index 148afd9..2411fd2 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Module.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Module.java
@@ -93,6 +93,22 @@
this.exportedBindings = new HashSet<>();
}
+ /**
+ * Returns the module (file) of the innermost enclosing Starlark function on the call stack, or
+ * null if none of the active calls are functions defined in Starlark.
+ *
+ * <p>The name of this function is intentionally horrible to make you feel bad for using it.
+ */
+ @Nullable
+ public static Module ofInnermostEnclosingStarlarkFunction(StarlarkThread thread) {
+ for (Debug.Frame fr : thread.getDebugCallStack().reverse()) {
+ if (fr.getFunction() instanceof StarlarkFunction) {
+ return ((StarlarkFunction) fr.getFunction()).getModule();
+ }
+ }
+ return null;
+ }
+
Module(
Mutability mutability,
@Nullable Module universe,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
index 4b093e4..22a3054 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkThread.java
@@ -123,16 +123,9 @@
@Nullable private SilentCloseable profileSpan; // current span of walltime profiler
- // Note that the inherited design is off-by-one:
- // the following fields are logically facts about the _enclosing_ frame.
- // TODO(adonovan): fix that.
-
- private final Module savedModule; // the saved module of the parent (TODO(adonovan): eliminate)
-
- private Frame(StarlarkThread thread, StarlarkCallable fn, Module savedModule) {
+ private Frame(StarlarkThread thread, StarlarkCallable fn) {
this.thread = thread;
this.fn = fn;
- this.savedModule = savedModule;
}
// Updates the PC location in this frame.
@@ -194,7 +187,7 @@
// Legacy behavior: all symbols from the global Frame are exported (including symbols
// introduced by load).
this(
- ImmutableMap.copyOf(thread.globalFrame.getExportedBindings()),
+ ImmutableMap.copyOf(thread.module.getExportedBindings()),
thread.getTransitiveContentHashCode());
}
@@ -325,11 +318,20 @@
}
}
- // Global environment of the current topmost call frame,
- // or of the file about to be initialized if no calls are active.
- // TODO(adonovan): eliminate once we represent even toplevel statements
- // as a StarlarkFunction that closes over its Module.
- private Module globalFrame;
+ // The module initialized by this Starlark thread.
+ //
+ // TODO(adonovan): eliminate. First we need to simplify the set-up sequence like so:
+ //
+ // // Filter predeclaredEnv based on semantics,
+ // // create a mutability, and retain the semantics:
+ // Module module = new Module(semantics, predeclaredEnv);
+ //
+ // // Create a thread that takes its semantics and mutability
+ // // (and only them) from the Module.
+ // StarlarkThread thread = StarlarkThread.toInitializeModule(module);
+ //
+ // Then clients that call thread.getGlobals() should use 'module' directly.
+ private final Module module;
/** The semantics options that affect how Skylark code is evaluated. */
private final StarlarkSemantics semantics;
@@ -350,7 +352,7 @@
/** Pushes a function onto the call stack. */
void push(StarlarkCallable fn) {
- Frame fr = new Frame(this, fn, this.globalFrame);
+ Frame fr = new Frame(this, fn);
callstack.add(fr);
// Push the function onto the allocation tracker's stack.
@@ -363,16 +365,10 @@
if (fn instanceof StarlarkFunction) {
StarlarkFunction sfn = (StarlarkFunction) fn;
fr.locals = Maps.newLinkedHashMapWithExpectedSize(sfn.getSignature().numParameters());
- this.globalFrame = sfn.getModule();
taskKind = ProfilerTask.STARLARK_USER_FN;
} else {
// built-in function
fr.locals = ImmutableMap.of();
- // this.globalFrame is left as is.
- // For built-ins, thread.globals() returns the module
- // of the file from which the built-in was called.
- // Really they have no business knowing about that.
-
taskKind = ProfilerTask.STARLARK_BUILTIN_FN;
}
@@ -390,7 +386,6 @@
int last = callstack.size() - 1;
Frame top = callstack.get(last);
callstack.remove(last); // pop
- this.globalFrame = top.savedModule;
// end profile span
if (top.profileSpan != null) {
@@ -408,11 +403,13 @@
return mutability;
}
- /** Returns the global variables for the StarlarkThread (not including dynamic bindings). */
+ /** Returns the module initialized by this StarlarkThread. */
// TODO(adonovan): get rid of this. Logically, a thread doesn't have module, but every
- // Starlark source function does.
+ // Starlark source function does. If you want to know the module of the innermost
+ // enclosing call from a function defined in Starlark source code, use
+ // Module.ofInnermostEnclosingStarlarkFunction.
public Module getGlobals() {
- return globalFrame;
+ return module;
}
/**
@@ -488,19 +485,19 @@
/**
* Constructs a StarlarkThread. This is the main, most basic constructor.
*
- * @param globalFrame a frame for the global StarlarkThread
+ * @param module the module initialized by this StarlarkThread
* @param eventHandler an EventHandler for warnings, errors, etc
* @param importedExtensions Extensions from which to import bindings with load()
* @param fileContentHashCode a hash for the source file being evaluated, if any
*/
private StarlarkThread(
- Module globalFrame,
+ Module module,
StarlarkSemantics semantics,
Map<String, Extension> importedExtensions,
@Nullable String fileContentHashCode) {
- this.globalFrame = Preconditions.checkNotNull(globalFrame);
- this.mutability = globalFrame.mutability();
- Preconditions.checkArgument(!globalFrame.mutability().isFrozen());
+ this.module = Preconditions.checkNotNull(module);
+ this.mutability = module.mutability();
+ Preconditions.checkArgument(!module.mutability().isFrozen());
this.semantics = semantics;
this.importedExtensions = importedExtensions;
this.transitiveHashCode =
@@ -590,11 +587,11 @@
// filtered out.
parent = Module.filterOutRestrictedBindings(mutability, parent, semantics);
- Module globalFrame = new Module(mutability, parent);
+ Module module = new Module(mutability, parent);
if (importedExtensions == null) {
importedExtensions = ImmutableMap.of();
}
- return new StarlarkThread(globalFrame, semantics, importedExtensions, fileContentHashCode);
+ return new StarlarkThread(module, semantics, importedExtensions, fileContentHashCode);
}
}
@@ -633,7 +630,7 @@
if (!callstack.isEmpty()) {
vars.addAll(frame(0).locals.keySet());
}
- vars.addAll(globalFrame.getTransitiveBindings().keySet());
+ vars.addAll(module.getTransitiveBindings().keySet());
return vars;
}
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAttrApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAttrApi.java
index 545e0da..e999b13 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAttrApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkAttrApi.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.skylarkbuildapi.core.ProviderApi;
import com.google.devtools.build.lib.syntax.Dict;
import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Printer;
import com.google.devtools.build.lib.syntax.Sequence;
import com.google.devtools.build.lib.syntax.StarlarkThread;
@@ -278,12 +279,11 @@
* name will be set to "Unknown Provider".
*/
private static String providerName(ProviderApi provider, StarlarkThread thread) {
- Map<String, Object> bindings = thread.getGlobals().getTransitiveBindings();
- if (bindings.containsValue(provider)) {
- for (Entry<String, Object> envEntry : bindings.entrySet()) {
- if (provider.equals(envEntry.getValue())) {
- return envEntry.getKey();
- }
+ Map<String, Object> bindings =
+ Module.ofInnermostEnclosingStarlarkFunction(thread).getTransitiveBindings();
+ for (Entry<String, Object> envEntry : bindings.entrySet()) {
+ if (provider.equals(envEntry.getValue())) {
+ return envEntry.getKey();
}
}
return "Unknown Provider";
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 0227bb4..fda2f34 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -55,6 +55,7 @@
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory;
import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.StarlarkFunction;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
@@ -908,7 +909,9 @@
/*optionReferenceFunction=*/ RuleClass.NO_OPTION_REFERENCE,
ruleDefinitionStarlarkThread == null
? null
- : (Label) ruleDefinitionStarlarkThread.getGlobals().getLabel(),
+ : (Label)
+ Module.ofInnermostEnclosingStarlarkFunction(ruleDefinitionStarlarkThread)
+ .getLabel(),
ruleDefinitionStarlarkThreadHashCode,
new ConfigurationFragmentPolicy.Builder()
.requiresConfigurationFragments(allowedConfigurationFragments)
