bazel syntax: delete StarlarkThread.Extension
This change replaces every use of Extension by Module.
Extension was a pair of a globals dictionary, and a digest
of "the thread's source file" (a dubious concept) and all
the Starlark (actually: .bzl) files it transitively loads.
Before this change, construction of StarlarkThread would
combine the file hash (which is only defined if the parseWithDigest
function was used) with the transitive hashes of the imports,
and would save this information in the StarlarkThread.
Now, StarlarkImportLookupFunction does this hashing, and
saves the result in a new field of BazelStarlarkContext,
which is the application-specific state carried by a Starlark
thread created by Bazel. This field is set only in threads
created by StarlarkImportLookupFunction.
SkylarkTestCase.newStarlarkThread sets it to a dummy value,
because execAndExport requires it to be set.
(It was implicitly a dummy value prior to this change: in these tests
source files are not parsed with parseWithDigest, and newStarlarkThread
uses an empty import map.)
Also:
- use byte[] not string for digest.
- StarlarkImportLookupValue
- record the transitive digest alongside the module.
- use == equivalence relation. There is no realistic scenario in which
two distinct SILV instances alive at the same time might be equal.
- terminology:
import -> load
extension -> module
hashCode -> digest
- without Fingerprint, lib.syntax no longer depends on lib.util.
- Extension.checkStateEquals moved to SerializationCheckingGraph.
- Eval: hoist loop-invariant code for LoadStatement
- discard_graph_edges_test: remove assertions on cardinality of Extension.
The cardinality of Module is quite different, and not something that
belongs in this test.
A follow-up change will remove StarlarkFile.getContentHash.
This is a breaking API change for Copybara.
PiperOrigin-RevId: 310441579
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 c4c6ef3..2130274 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
@@ -60,7 +60,6 @@
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionsProvider;
@@ -764,14 +763,20 @@
return environment;
}
+ // TODO(adonovan): all that needs to be in the RuleClassProvider interface is:
+ //
+ // // Returns the BazelStarlarkContext to be associated with this loading-phase thread.
+ // BazelStarlarkContext getThreadContext(repoMapping, fileLabel, transitiveDigest).
+ //
+ // (Alternatively the call could accept the Thread and set its BazelStarlarkContext.)
@Override
public StarlarkThread createRuleClassStarlarkThread(
Label fileLabel,
Mutability mutability,
StarlarkSemantics starlarkSemantics,
StarlarkThread.PrintHandler printHandler,
- String astFileContentHashCode,
- Map<String, Extension> importMap,
+ byte[] transitiveDigest,
+ Map<String, Module> loadedModules,
ClassObject nativeModule,
ImmutableMap<RepositoryName, RepositoryName> repoMapping) {
Map<String, Object> env = new HashMap<>(environment);
@@ -781,8 +786,7 @@
StarlarkThread.builder(mutability)
.setGlobals(Module.createForBuiltins(env).withLabel(fileLabel))
.setSemantics(starlarkSemantics)
- .setFileContentHashCode(astFileContentHashCode)
- .setImportedExtensions(importMap)
+ .setLoadedModules(loadedModules)
.build();
thread.setPrintHandler(printHandler);
@@ -792,7 +796,8 @@
configurationFragmentMap,
repoMapping,
new SymbolGenerator<>(fileLabel),
- /* analysisRuleLabel= */ null)
+ /*analysisRuleLabel=*/ null,
+ transitiveDigest)
.storeInThread(thread);
return thread;
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 74a7a98..3e0e6f1 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
@@ -344,9 +344,9 @@
.requiresHostConfigurationFragmentsByStarlarkBuiltinName(
Sequence.cast(hostFragments, String.class, "host_fragments"));
builder.setConfiguredTargetFunction(implementation);
- builder.setRuleDefinitionEnvironmentLabelAndHashCode(
+ builder.setRuleDefinitionEnvironmentLabelAndDigest(
(Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel(),
- thread.getTransitiveContentHashCode());
+ bazelContext.getTransitiveDigest());
builder.addRequiredToolchains(parseToolchains(toolchains, thread));
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
index 6e699c7..f4275cf 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleConfiguredTargetUtil.java
@@ -110,7 +110,8 @@
/*fragmentNameToClass=*/ null,
ruleContext.getTarget().getPackage().getRepositoryMapping(),
ruleContext.getSymbolGenerator(),
- ruleContext.getLabel())
+ ruleContext.getLabel(),
+ /*transitiveDigest=*/ null)
.storeInThread(thread);
RuleClass ruleClass = ruleContext.getRule().getRuleClassObject();
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
index e33a30c..78fc21a 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
@@ -154,7 +154,8 @@
/*fragmentNameToClass=*/ null,
rule.getPackage().getRepositoryMapping(),
new SymbolGenerator<>(key),
- /*analysisRuleLabel=*/ null)
+ /*analysisRuleLabel=*/ null,
+ /*transitiveDigest=*/ null)
.storeInThread(thread);
SkylarkRepositoryContext skylarkRepositoryContext =
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 e6dd9ca..629c068 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
@@ -65,7 +65,8 @@
String doc,
StarlarkThread thread)
throws EvalException {
- BazelStarlarkContext.from(thread).checkLoadingOrWorkspacePhase("repository_rule");
+ BazelStarlarkContext context = BazelStarlarkContext.from(thread);
+ context.checkLoadingOrWorkspacePhase("repository_rule");
// We'll set the name later, pass the empty string for now.
RuleClass.Builder builder = new RuleClass.Builder("", RuleClassType.WORKSPACE, true);
@@ -90,19 +91,16 @@
AttributeValueSource source = attrDescriptor.getValueSource();
String attrName = source.convertToNativeName(attr.getKey());
if (builder.contains(attrName)) {
- throw new EvalException(
- null,
- String.format(
- "There is already a built-in attribute '%s' which cannot be overridden",
- attrName));
+ throw Starlark.errorf(
+ "There is already a built-in attribute '%s' which cannot be overridden", attrName);
}
builder.addAttribute(attrDescriptor.build(attrName));
}
}
builder.setConfiguredTargetFunction(implementation);
- builder.setRuleDefinitionEnvironmentLabelAndHashCode(
+ builder.setRuleDefinitionEnvironmentLabelAndDigest(
(Label) Module.ofInnermostEnclosingStarlarkFunction(thread).getLabel(),
- thread.getTransitiveContentHashCode());
+ context.getTransitiveDigest());
builder.setWorkspaceOnly();
return new RepositoryRuleFunction(builder, implementation);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java
index df8d5e4..2c79270 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkContext.java
@@ -50,6 +50,7 @@
private final ImmutableMap<RepositoryName, RepositoryName> repoMapping;
private final SymbolGenerator<?> symbolGenerator;
@Nullable private final Label analysisRuleLabel;
+ @Nullable private final byte[] transitiveDigest;
/**
* @param phase the phase to which this Starlark thread belongs
@@ -76,13 +77,15 @@
@Nullable ImmutableMap<String, Class<?>> fragmentNameToClass,
ImmutableMap<RepositoryName, RepositoryName> repoMapping,
SymbolGenerator<?> symbolGenerator,
- @Nullable Label analysisRuleLabel) {
+ @Nullable Label analysisRuleLabel,
+ @Nullable byte[] transitiveDigest) {
this.phase = phase;
this.toolsRepository = toolsRepository;
this.fragmentNameToClass = fragmentNameToClass;
this.repoMapping = repoMapping;
this.symbolGenerator = Preconditions.checkNotNull(symbolGenerator);
this.analysisRuleLabel = analysisRuleLabel;
+ this.transitiveDigest = transitiveDigest;
}
/** Returns the phase to which this Starlark thread belongs. */
@@ -125,6 +128,16 @@
}
/**
+ * Returns the digest of the .bzl file and those it transitively loads. Only defined for .bzl
+ * initialization threads. Returns a dummy value (empty array) for WORKSPACE initialization
+ * threads. Returns null for all other threads.
+ */
+ @Nullable
+ public byte[] getTransitiveDigest() {
+ return transitiveDigest;
+ }
+
+ /**
* Checks that the Starlark thread is in the loading or the workspace phase.
*
* @param function name of a function that requires this check
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index ea538e6..dae3dc6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -62,7 +62,6 @@
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.syntax.StringLiteral;
import com.google.devtools.build.lib.syntax.Tuple;
import com.google.devtools.build.lib.vfs.FileSystem;
@@ -93,9 +92,8 @@
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
/** An extension to the global namespace of the BUILD language. */
- // TODO(bazel-team): this is largely unrelated to syntax.StarlarkThread.Extension,
- // and should probably be renamed PackageFactory.RuntimeExtension, since really,
- // we're extending the Runtime with more classes.
+ // TODO(bazel-team): this should probably be renamed PackageFactory.RuntimeExtension
+ // since really we're extending the Runtime with more classes.
public interface EnvironmentExtension {
/** Update the predeclared environment with the identifiers this extension contributes. */
void update(ImmutableMap.Builder<String, Object> env);
@@ -414,7 +412,7 @@
PackageIdentifier packageId,
RootedPath buildFile,
StarlarkFile file,
- Map<String, Extension> imports,
+ Map<String, Module> loadedModules,
ImmutableList<Label> skylarkFileDependencies,
RuleVisibility defaultVisibility,
StarlarkSemantics starlarkSemantics,
@@ -431,7 +429,7 @@
globber,
defaultVisibility,
starlarkSemantics,
- imports,
+ loadedModules,
skylarkFileDependencies,
repositoryMapping);
} catch (InterruptedException e) {
@@ -531,7 +529,7 @@
packageId,
buildFile,
file,
- /*imports=*/ ImmutableMap.<String, Extension>of(),
+ /*loadedModules=*/ ImmutableMap.<String, Module>of(),
/*skylarkFileDependencies=*/ ImmutableList.<Label>of(),
/*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC,
semantics,
@@ -714,7 +712,7 @@
Globber globber,
RuleVisibility defaultVisibility,
StarlarkSemantics semantics,
- Map<String, Extension> imports,
+ Map<String, Module> loadedModules,
ImmutableList<Label> skylarkFileDependencies,
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
throws InterruptedException {
@@ -741,7 +739,7 @@
packageId,
file,
semantics,
- imports,
+ loadedModules,
new PackageContext(pkgBuilder, globber, eventHandler))) {
pkgBuilder.setContainsErrors();
}
@@ -757,7 +755,7 @@
PackageIdentifier packageId,
StarlarkFile file,
StarlarkSemantics semantics,
- Map<String, Extension> imports,
+ Map<String, Module> loadedModules,
PackageContext pkgContext)
throws InterruptedException {
@@ -787,7 +785,7 @@
StarlarkThread.builder(mutability)
.setGlobals(Module.createForBuiltins(env.build()))
.setSemantics(semantics)
- .setImportedExtensions(imports)
+ .setLoadedModules(loadedModules)
.build();
thread.setPrintHandler(Event.makeDebugPrintHandler(pkgContext.eventHandler));
Module module = thread.getGlobals();
@@ -839,7 +837,8 @@
/*fragmentNameToClass=*/ null,
pkgBuilder.getRepositoryMapping(),
new SymbolGenerator<>(packageId),
- /*analysisRuleLabel=*/ null)
+ /*analysisRuleLabel=*/ null,
+ /*transitiveDigest=*/ null)
.storeInThread(thread);
// TODO(adonovan): save this as a field in BazelSkylarkContext.
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 2e48221..a834483 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -678,7 +678,7 @@
/** This field and the next are null iff the rule is native. */
@Nullable private Label ruleDefinitionEnvironmentLabel;
- @Nullable private String ruleDefinitionEnvironmentHashCode = null;
+ @Nullable private byte[] ruleDefinitionEnvironmentDigest = null;
private ConfigurationFragmentPolicy.Builder configurationFragmentPolicy =
new ConfigurationFragmentPolicy.Builder();
@@ -819,7 +819,7 @@
Preconditions.checkState(externalBindingsFunction == NO_EXTERNAL_BINDINGS);
}
if (type == RuleClassType.PLACEHOLDER) {
- Preconditions.checkNotNull(ruleDefinitionEnvironmentHashCode, this.name);
+ Preconditions.checkNotNull(ruleDefinitionEnvironmentDigest, this.name);
}
if (buildSetting != null) {
@@ -865,7 +865,7 @@
externalBindingsFunction,
optionReferenceFunction,
ruleDefinitionEnvironmentLabel,
- ruleDefinitionEnvironmentHashCode,
+ ruleDefinitionEnvironmentDigest,
configurationFragmentPolicy.build(),
supportsConstraintChecking,
thirdPartyLicenseExistencePolicy,
@@ -1251,10 +1251,12 @@
return this;
}
- /** Sets the rule definition environment label and hash code. Meant for Starlark usage. */
- public Builder setRuleDefinitionEnvironmentLabelAndHashCode(Label label, String hashCode) {
+ /**
+ * Sets the rule definition environment label and transitive digest. Meant for Starlark usage.
+ */
+ public Builder setRuleDefinitionEnvironmentLabelAndDigest(Label label, byte[] digest) {
this.ruleDefinitionEnvironmentLabel = Preconditions.checkNotNull(label, this.name);
- this.ruleDefinitionEnvironmentHashCode = Preconditions.checkNotNull(hashCode, this.name);
+ this.ruleDefinitionEnvironmentDigest = Preconditions.checkNotNull(digest, this.name);
return this;
}
@@ -1583,7 +1585,7 @@
*/
@Nullable private final Label ruleDefinitionEnvironmentLabel;
- @Nullable private final String ruleDefinitionEnvironmentHashCode;
+ @Nullable private final byte[] ruleDefinitionEnvironmentDigest;
private final OutputFile.Kind outputFileKind;
/**
@@ -1652,7 +1654,7 @@
Function<? super Rule, Map<String, Label>> externalBindingsFunction,
Function<? super Rule, ? extends Set<String>> optionReferenceFunction,
@Nullable Label ruleDefinitionEnvironmentLabel,
- String ruleDefinitionEnvironmentHashCode,
+ @Nullable byte[] ruleDefinitionEnvironmentDigest,
ConfigurationFragmentPolicy configurationFragmentPolicy,
boolean supportsConstraintChecking,
ThirdPartyLicenseExistencePolicy thirdPartyLicenseExistencePolicy,
@@ -1683,7 +1685,7 @@
this.externalBindingsFunction = externalBindingsFunction;
this.optionReferenceFunction = optionReferenceFunction;
this.ruleDefinitionEnvironmentLabel = ruleDefinitionEnvironmentLabel;
- this.ruleDefinitionEnvironmentHashCode = ruleDefinitionEnvironmentHashCode;
+ this.ruleDefinitionEnvironmentDigest = ruleDefinitionEnvironmentDigest;
this.outputFileKind = outputFileKind;
validateNoClashInPublicNames(attributes);
this.attributes = ImmutableList.copyOf(attributes);
@@ -2548,12 +2550,13 @@
}
/**
- * Returns the hash code for the RuleClass's rule definition environment. Will be null for native
- * rules' RuleClass objects.
+ * Returns the digest for the RuleClass's rule definition environment, a hash of the .bzl file
+ * defining the rule class and all the .bzl files it transitively loads. Null for native rules'
+ * RuleClass objects.
*/
@Nullable
- public String getRuleDefinitionEnvironmentHashCode() {
- return ruleDefinitionEnvironmentHashCode;
+ public byte[] getRuleDefinitionEnvironmentDigest() {
+ return ruleDefinitionEnvironmentDigest;
}
/** Returns true if this RuleClass is a Starlark-defined RuleClass. */
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java
index 9ec5719..59afcf6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClassProvider.java
@@ -20,12 +20,11 @@
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy;
import com.google.devtools.build.lib.syntax.ClassObject;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import java.util.Map;
-import javax.annotation.Nullable;
/**
* The collection of the supported build rules. Provides an StarlarkThread for Starlark rule
@@ -61,8 +60,8 @@
* @param mutability the Mutability for the .bzl module globals
* @param starlarkSemantics the semantics options that modify the interpreter
* @param printHandler defines the behavior of Starlark print statements
- * @param astFileContentHashCode the hash code identifying this environment.
- * @param importMap map from import string to Extension
+ * @param transitiveDigest digest of the .bzl file and those it transitively loads
+ * @param loadedModules the .bzl modules loaded by each load statement
* @param nativeModule the appropriate {@code native} module for this environment.
* @param repoMapping map of RepositoryNames to be remapped
* @return the StarlarkThread in which to initualize the .bzl module
@@ -72,8 +71,8 @@
Mutability mutability,
StarlarkSemantics starlarkSemantics,
StarlarkThread.PrintHandler printHandler,
- @Nullable String astFileContentHashCode,
- @Nullable Map<String, Extension> importMap,
+ byte[] transitiveDigest,
+ Map<String, Module> loadedModules,
ClassObject nativeModule,
ImmutableMap<RepositoryName, RepositoryName> repoMapping);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleFormatter.java b/src/main/java/com/google/devtools/build/lib/packages/RuleFormatter.java
index c894642..0a3e140 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleFormatter.java
@@ -15,6 +15,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableSet;
+import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.packages.Attribute.ComputedDefault;
import com.google.devtools.build.lib.query2.proto.proto2api.Build;
import javax.annotation.Nullable;
@@ -43,8 +44,12 @@
if (isSkylark) {
builder.setSkylarkEnvironmentHashCode(
- Preconditions.checkNotNull(
- rule.getRuleClassObject().getRuleDefinitionEnvironmentHashCode(), rule));
+ // hexify
+ BaseEncoding.base16()
+ .lowerCase()
+ .encode(
+ Preconditions.checkNotNull(
+ rule.getRuleClassObject().getRuleDefinitionEnvironmentDigest(), rule)));
}
for (Attribute attr : rule.getAttributes()) {
Object rawAttributeValue = rawAttributeMapper.getRawAttributeValue(rule, attr);
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 f2009d0..e2f1cf7 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
@@ -41,7 +41,6 @@
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.syntax.Tuple;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -73,7 +72,7 @@
private final ImmutableList<EnvironmentExtension> environmentExtensions;
// Values accumulated from all previous WORKSPACE file parts.
- private final Map<String, Extension> importMap = new HashMap<>();
+ private final Map<String, Module> loadedModules = new HashMap<>();
private final Map<String, Object> bindings = new HashMap<>();
// TODO(bazel-team): document installDir
@@ -110,10 +109,10 @@
allowOverride, ruleFactory, this.workspaceGlobals, starlarkSemantics);
}
+ // TODO(adonovan): move this into the test. It doesn't need privileged access,
+ // and it's called exactly once (!).
@VisibleForTesting
- void parseForTesting(
- ParserInput source,
- @Nullable StoredEventHandler localReporter)
+ void parseForTesting(ParserInput source, @Nullable StoredEventHandler localReporter)
throws BuildFileContainsErrorsException, InterruptedException {
if (localReporter == null) {
localReporter = new StoredEventHandler();
@@ -127,7 +126,7 @@
}
execute(
file,
- /*importedExtensions=*/ ImmutableMap.of(),
+ /* additionalLoadedModules= */ ImmutableMap.of(),
localReporter,
WorkspaceFileValue.key(
RootedPath.toRootedPath(
@@ -140,21 +139,23 @@
*/
public void execute(
StarlarkFile file,
- Map<String, Extension> importedExtensions,
+ Map<String, Module> loadedModules,
WorkspaceFileValue.WorkspaceFileKey workspaceFileKey)
throws InterruptedException {
Preconditions.checkNotNull(file);
- Preconditions.checkNotNull(importedExtensions);
- execute(file, importedExtensions, new StoredEventHandler(), workspaceFileKey);
+ Preconditions.checkNotNull(loadedModules);
+ // TODO(adonovan): What's up with the transient StoredEventHandler?
+ // Doesn't this discard events, including print statements?
+ execute(file, loadedModules, new StoredEventHandler(), workspaceFileKey);
}
private void execute(
StarlarkFile file,
- Map<String, Extension> importedExtensions,
+ Map<String, Module> additionalLoadedModules,
StoredEventHandler localReporter,
WorkspaceFileValue.WorkspaceFileKey workspaceFileKey)
throws InterruptedException {
- importMap.putAll(importedExtensions);
+ loadedModules.putAll(additionalLoadedModules);
// environment
HashMap<String, Object> env = new HashMap<>();
@@ -165,7 +166,7 @@
StarlarkThread.builder(this.mutability)
.setSemantics(this.starlarkSemantics)
.setGlobals(Module.createForBuiltins(env))
- .setImportedExtensions(importMap)
+ .setLoadedModules(loadedModules)
.build();
thread.setPrintHandler(Event.makeDebugPrintHandler(localReporter));
thread.setThreadLocal(
@@ -179,11 +180,12 @@
// are, by definition, not in an external repository and so they don't need the mapping
new BazelStarlarkContext(
BazelStarlarkContext.Phase.WORKSPACE,
- /* toolsRepository= */ null,
- /* fragmentNameToClass= */ null,
- /* repoMapping= */ ImmutableMap.of(),
+ /*toolsRepository=*/ null,
+ /*fragmentNameToClass=*/ null,
+ /*repoMapping=*/ ImmutableMap.of(),
new SymbolGenerator<>(workspaceFileKey),
- /* analysisRuleLabel= */ null)
+ /*analysisRuleLabel=*/ null,
+ /*transitiveDigest=*/ new byte[] {}) // dummy value used for repository rules
.storeInThread(thread);
Resolver.resolveFile(file, thread.getGlobals());
@@ -215,15 +217,15 @@
/**
* Adds the various values returned by the parsing of the previous workspace file parts. {@code
- * aPackage} is the package returned by the parent WorkspaceFileFunction, {@code importMap} is the
- * list of load statements imports computed by the parent WorkspaceFileFunction and {@code
+ * aPackage} is the package returned by the parent WorkspaceFileFunction, {@code loadedModules} is
+ * the set of modules loaded by load statements in the parent WorkspaceFileFunction and {@code
* variableBindings} the list of top level variable bindings of that same call.
*/
public void setParent(
- Package aPackage, Map<String, Extension> importMap, Map<String, Object> bindings)
+ Package aPackage, Map<String, Module> loadedModules, Map<String, Object> bindings)
throws NameConflictException, InterruptedException {
this.bindings.putAll(bindings);
- this.importMap.putAll(importMap);
+ this.loadedModules.putAll(loadedModules);
builder.setWorkspaceName(aPackage.getWorkspaceName());
// Transmit the content of the parent package to the new package builder.
builder.addPosts(aPackage.getPosts());
@@ -412,8 +414,8 @@
version);
}
- public Map<String, Extension> getImportMap() {
- return importMap;
+ public Map<String, Module> getLoadedModules() {
+ return loadedModules;
}
public Map<String, Object> getVariableBindings() {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFileValue.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFileValue.java
index f88c582..43c7867 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFileValue.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFileValue.java
@@ -22,7 +22,7 @@
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -101,7 +101,7 @@
private final RootedPath path;
private final boolean hasNext;
private final ImmutableMap<String, Object> bindings;
- private final ImmutableMap<String, Extension> importMap;
+ private final ImmutableMap<String, Module> loadedModules;
private final ImmutableMap<String, Integer> importToChunkMap;
private final ImmutableMap<RepositoryName, ImmutableMap<RepositoryName, RepositoryName>>
repositoryMapping;
@@ -116,8 +116,8 @@
* WORKSPACE file.
*
* @param pkg Package built by agreggating all parts of the split WORKSPACE file up to this one.
- * @param importMap List of imports (i.e., load statements) present in all parts of the split
- * WORKSPACE file up to this one.
+ * @param loadedModules modules loaded by load statements in chunks of the WORKSPACE file up to
+ * this one.
* @param importToChunkMap Map of all load statements encountered so far to the chunk they
* initially appeared in.
* @param bindings List of top-level variable bindings from the all parts of the split WORKSPACE
@@ -132,7 +132,7 @@
*/
public WorkspaceFileValue(
Package pkg,
- Map<String, Extension> importMap,
+ Map<String, Module> loadedModules,
Map<String, Integer> importToChunkMap,
Map<String, Object> bindings,
RootedPath path,
@@ -145,7 +145,7 @@
this.path = path;
this.hasNext = hasNext;
this.bindings = ImmutableMap.copyOf(bindings);
- this.importMap = ImmutableMap.copyOf(importMap);
+ this.loadedModules = ImmutableMap.copyOf(loadedModules);
this.importToChunkMap = ImmutableMap.copyOf(importToChunkMap);
this.repositoryMapping = pkg.getExternalPackageRepositoryMappings();
this.managedDirectories = managedDirectories;
@@ -219,8 +219,8 @@
return bindings;
}
- public ImmutableMap<String, Extension> getImportMap() {
- return importMap;
+ public ImmutableMap<String, Module> getLoadedModules() {
+ return loadedModules;
}
public ImmutableMap<String, Integer> getImportToChunkMap() {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
index d5db2fb..b024d21 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
@@ -29,6 +29,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
+import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
@@ -87,11 +88,16 @@
private static final Comparator<Build.Attribute> ATTRIBUTE_NAME =
Comparator.comparing(Build.Attribute::getName);
- @SuppressWarnings("unchecked")
private static final ImmutableSet<Type<?>> SCALAR_TYPES =
ImmutableSet.<Type<?>>of(
- Type.INTEGER, Type.STRING, BuildType.LABEL, BuildType.NODEP_LABEL, BuildType.OUTPUT,
- Type.BOOLEAN, BuildType.TRISTATE, BuildType.LICENSE);
+ Type.INTEGER,
+ Type.STRING,
+ BuildType.LABEL,
+ BuildType.NODEP_LABEL,
+ BuildType.OUTPUT,
+ Type.BOOLEAN,
+ BuildType.TRISTATE,
+ BuildType.LICENSE);
private AspectResolver aspectResolver;
private DependencyFilter dependencyFilter;
@@ -188,15 +194,16 @@
rulePb.setLocation(FormatUtils.getLocation(target, relativeLocations));
}
addAttributes(rulePb, rule, extraDataForAttrHash);
- String transitiveHashCode = rule.getRuleClassObject().getRuleDefinitionEnvironmentHashCode();
- if (transitiveHashCode != null && includeRuleDefinitionEnvironment()) {
+ byte[] transitiveDigest = rule.getRuleClassObject().getRuleDefinitionEnvironmentDigest();
+ if (transitiveDigest != null && includeRuleDefinitionEnvironment()) {
// The RuleDefinitionEnvironment is always defined for Starlark rules and
// always null for non Starlark rules.
rulePb.addAttribute(
Build.Attribute.newBuilder()
.setName(RULE_IMPLEMENTATION_HASH_ATTR_NAME)
.setType(ProtoUtils.getDiscriminatorFromType(Type.STRING))
- .setStringValue(transitiveHashCode));
+ .setStringValue(
+ BaseEncoding.base16().lowerCase().encode(transitiveDigest))); // hexify
}
ImmutableMultimap<Attribute, Label> aspectsDependencies =
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java
index 3a4477e..c03e43b 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/SyntheticAttributeHashCalculator.java
@@ -65,8 +65,8 @@
RuleClass ruleClass = rule.getRuleClassObject();
if (ruleClass.isStarlark()) {
try {
- codedOut.writeStringNoTag(
- Preconditions.checkNotNull(ruleClass.getRuleDefinitionEnvironmentHashCode(), rule));
+ codedOut.writeByteArrayNoTag(
+ Preconditions.checkNotNull(ruleClass.getRuleDefinitionEnvironmentDigest(), rule));
} catch (IOException e) {
throw new IllegalStateException("Unexpected IO failure writing to digest stream", e);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index a15c5a1..85c75ac 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -166,7 +166,7 @@
}
Object skylarkValue =
- starlarkImportLookupValue.getEnvironmentExtension().getBindings().get(skylarkValueName);
+ starlarkImportLookupValue.getModule().getBindings().get(skylarkValueName);
if (skylarkValue == null) {
throw new ConversionException(
String.format(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index cbec840..b22f58a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -58,10 +58,10 @@
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.FileOptions;
import com.google.devtools.build.lib.syntax.Location;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.syntax.Statement;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
@@ -636,8 +636,8 @@
return null;
}
- // Process the loaded imports.
- Map<String, Extension> importMap = Maps.newHashMapWithExpectedSize(loadMap.size());
+ // Process the loaded modules.
+ Map<String, Module> loadedModules = Maps.newHashMapWithExpectedSize(loadMap.size());
ImmutableList.Builder<StarlarkFileDependency> fileDependencies = ImmutableList.builder();
for (Map.Entry<String, Label> importEntry : loadMap.entrySet()) {
String importString = importEntry.getKey();
@@ -652,12 +652,12 @@
} else {
keyForLabel = StarlarkImportLookupValue.key(importLabel);
}
- StarlarkImportLookupValue importLookupValue =
- (StarlarkImportLookupValue) starlarkImportMap.get(keyForLabel);
- importMap.put(importString, importLookupValue.getEnvironmentExtension());
- fileDependencies.add(importLookupValue.getDependency());
+ StarlarkImportLookupValue v = (StarlarkImportLookupValue) starlarkImportMap.get(keyForLabel);
+ loadedModules.put(importString, v.getModule());
+ fileDependencies.add(v.getDependency());
}
- return new StarlarkImportResult(importMap, transitiveClosureOfLabels(fileDependencies.build()));
+ return new StarlarkImportResult(
+ loadedModules, transitiveClosureOfLabels(fileDependencies.build()));
}
/**
@@ -1261,7 +1261,7 @@
packageId,
buildFilePath,
file,
- importResult.importMap,
+ importResult.loadedModules,
importResult.fileDependencies,
defaultVisibility,
starlarkSemantics,
@@ -1329,12 +1329,12 @@
/** A simple value class to store the result of the Starlark imports. */
static final class StarlarkImportResult {
- final Map<String, Extension> importMap;
+ final Map<String, Module> loadedModules;
final ImmutableList<Label> fileDependencies;
private StarlarkImportResult(
- Map<String, Extension> importMap, ImmutableList<Label> fileDependencies) {
- this.importMap = importMap;
+ Map<String, Module> loadedModules, ImmutableList<Label> fileDependencies) {
+ this.loadedModules = loadedModules;
this.fileDependencies = fileDependencies;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
index 851fdb8..1ef5f9d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkAspectFactory.java
@@ -79,10 +79,11 @@
new BazelStarlarkContext(
BazelStarlarkContext.Phase.ANALYSIS,
toolsRepository,
- /* fragmentNameToClass=*/ null,
+ /*fragmentNameToClass=*/ null,
ruleContext.getRule().getPackage().getRepositoryMapping(),
ruleContext.getSymbolGenerator(),
- ruleContext.getLabel())
+ ruleContext.getLabel(),
+ /*transitiveDigest=*/ null)
.storeInThread(thread);
try {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
index 11d3099..501cd38 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunction.java
@@ -45,12 +45,13 @@
import com.google.devtools.build.lib.syntax.EvalUtils;
import com.google.devtools.build.lib.syntax.LoadStatement;
import com.google.devtools.build.lib.syntax.Location;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.syntax.Statement;
+import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -68,14 +69,14 @@
import javax.annotation.Nullable;
/**
- * A Skyframe function to look up and import a single Starlark extension.
+ * A Skyframe function to look up and load a single .bzl module.
*
- * <p>Given a {@link Label} referencing a Starlark file, attempts to locate the file and load it.
- * The Label must be absolute, and must not reference the special {@code external} package. If
- * loading is successful, returns a {@link StarlarkImportLookupValue} that encapsulates the loaded
- * {@link Extension} and {@link StarlarkFileDependency} information. If loading is unsuccessful,
- * throws a {@link StarlarkImportLookupFunctionException} that encapsulates the cause of the
- * failure.
+ * <p>Given a {@link Label} referencing a .bzl file, attempts to locate the file and load it. The
+ * Label must be absolute, and must not reference the special {@code external} package. If loading
+ * is successful, returns a {@link StarlarkImportLookupValue} that encapsulates the loaded {@link
+ * Module} and its transitive digest and {@link StarlarkFileDependency} information. If loading is
+ * unsuccessful, throws a {@link StarlarkImportLookupFunctionException} that encapsulates the cause
+ * of the failure.
*/
public class StarlarkImportLookupFunction implements SkyFunction {
@@ -440,7 +441,13 @@
}
// Process the loaded imports.
- Map<String, Extension> extensionsForImports = Maps.newHashMapWithExpectedSize(loadMap.size());
+ //
+ // Compute a digest of the file itself plus the transitive hashes of the modules it directly
+ // loads. Loop iteration order matches the source order of load statements.
+ Fingerprint fp = new Fingerprint();
+ // TODO(adonovan): save file.getContentHashCode in ASTFileLookupValue, not the syntax tree.
+ fp.addBytes(file.getContentHashCode());
+ Map<String, Module> loadedModules = Maps.newHashMapWithExpectedSize(loadMap.size());
ImmutableList.Builder<StarlarkFileDependency> fileDependencies =
ImmutableList.builderWithExpectedSize(loadMap.size());
for (Map.Entry<String, Label> importEntry : loadMap.entrySet()) {
@@ -453,26 +460,30 @@
} else {
keyForLabel = StarlarkImportLookupValue.key(importLabel);
}
- StarlarkImportLookupValue importLookupValue =
- (StarlarkImportLookupValue) starlarkImportMap.get(keyForLabel);
- extensionsForImports.put(importString, importLookupValue.getEnvironmentExtension());
- fileDependencies.add(importLookupValue.getDependency());
+ StarlarkImportLookupValue v = (StarlarkImportLookupValue) starlarkImportMap.get(keyForLabel);
+ loadedModules.put(importString, v.getModule());
+ fileDependencies.add(v.getDependency());
+ fp.addBytes(v.getTransitiveDigest());
}
+ byte[] transitiveDigest = fp.digestAndReset();
- // #createExtension does not request values from the Environment. It may post events to the
+ // executeModule does not request values from the Environment. It may post events to the
// Environment, but events do not matter when caching StarlarkImportLookupValues.
- Extension extension =
- createExtension(
+ Module module =
+ executeModule(
file,
fileLabel,
- extensionsForImports,
+ transitiveDigest,
+ loadedModules,
starlarkSemantics,
env,
inWorkspace,
repoMapping);
StarlarkImportLookupValue result =
new StarlarkImportLookupValue(
- extension, new StarlarkFileDependency(fileLabel, fileDependencies.build()));
+ module,
+ transitiveDigest,
+ new StarlarkFileDependency(fileLabel, fileDependencies.build()));
return result;
}
@@ -514,7 +525,8 @@
/**
* Returns a mapping from each load string in the BUILD or .bzl file to the Label it resolves to.
* Labels are resolved relative to {@code base}, the file's package. If any load statement is
- * malformed, getLoadMap reports one or more errors to the handler and returns null.
+ * malformed, getLoadMap reports one or more errors to the handler and returns null. Iteration
+ * order matches the source.
*/
@Nullable
static Map<String, Label> getLoadMap(
@@ -529,7 +541,7 @@
Label buildLabel = getBUILDLabel(base);
boolean ok = true;
- Map<String, Label> loadMap = Maps.newHashMap();
+ Map<String, Label> loadMap = Maps.newLinkedHashMap();
for (Statement stmt : file.getStatements()) {
if (stmt instanceof LoadStatement) {
LoadStatement load = (LoadStatement) stmt;
@@ -647,11 +659,12 @@
return valuesMissing ? null : starlarkImportMap;
}
- /** Creates the Extension to be imported. */
- private Extension createExtension(
+ /** Executes the .bzl file defining the module to be imported. */
+ private Module executeModule(
StarlarkFile file,
Label extensionLabel,
- Map<String, Extension> importMap,
+ byte[] transitiveDigest,
+ Map<String, Module> loadedModules,
StarlarkSemantics starlarkSemantics,
Environment env,
boolean inWorkspace,
@@ -668,10 +681,11 @@
mutability,
starlarkSemantics,
Event.makeDebugPrintHandler(eventHandler),
- file.getContentHashCode(),
- importMap,
+ transitiveDigest,
+ loadedModules,
packageFactory.getNativeModule(inWorkspace),
repositoryMapping);
+ Module module = thread.getGlobals();
execAndExport(file, extensionLabel, eventHandler, thread);
Event.replayEventsOn(env.getListener(), eventHandler.getEvents());
@@ -681,11 +695,13 @@
if (eventHandler.hasErrors()) {
throw StarlarkImportFailedException.errors(extensionFile);
}
- return new Extension(thread);
+ return module;
}
}
// Precondition: file is validated and error-free.
+ // Precondition: thread has a valid transitiveDigest.
+ // TODO(adonovan): executeModule would make a better public API than this function.
public static void execAndExport(
StarlarkFile file, Label extensionLabel, EventHandler handler, StarlarkThread thread)
throws InterruptedException {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java
index d9ac970..6d37e3b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupValue.java
@@ -20,7 +20,7 @@
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
@@ -35,7 +35,9 @@
@AutoCodec
public class StarlarkImportLookupValue implements SkyValue {
- private final Extension environmentExtension;
+ private final Module module; // .bzl module
+ private final byte[] transitiveDigest; // of .bzl file and load dependencies
+
/**
* The immediate Starlark file dependency descriptor class corresponding to this value. Using this
* reference it's possible to reach the transitive closure of Starlark files on which this
@@ -45,16 +47,20 @@
@VisibleForTesting
public StarlarkImportLookupValue(
- Extension environmentExtension, StarlarkFileDependency dependency) {
- this.environmentExtension = Preconditions.checkNotNull(environmentExtension);
+ Module module, byte[] transitiveDigest, StarlarkFileDependency dependency) {
+ this.module = Preconditions.checkNotNull(module);
+ this.transitiveDigest = Preconditions.checkNotNull(transitiveDigest);
this.dependency = Preconditions.checkNotNull(dependency);
}
- /**
- * Returns the Extension
- */
- public Extension getEnvironmentExtension() {
- return environmentExtension;
+ /** Returns the .bzl module. */
+ public Module getModule() {
+ return module;
+ }
+
+ /** Returns the digest of the .bzl module and its transitive load dependencies. */
+ public byte[] getTransitiveDigest() {
+ return transitiveDigest;
}
/** Returns the immediate Starlark file dependency corresponding to this import lookup value. */
@@ -153,22 +159,4 @@
return Key.create(
importLabel, /* inWorkspace= */ false, /* workspaceChunk= */ -1, /* workspacePath= */ null);
}
-
- @Override
- public boolean equals(Object obj) {
- if (this == obj) {
- return true;
- }
- if (!(obj instanceof StarlarkImportLookupValue)) {
- return false;
- }
- StarlarkImportLookupValue other = (StarlarkImportLookupValue) obj;
- return environmentExtension.equals(other.getEnvironmentExtension())
- && dependency.equals(other.getDependency());
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(environmentExtension, dependency);
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index aec9c46..0edfd38 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -27,10 +27,10 @@
import com.google.devtools.build.lib.packages.WorkspaceFileValue;
import com.google.devtools.build.lib.packages.WorkspaceFileValue.WorkspaceFileKey;
import com.google.devtools.build.lib.skyframe.PackageFunction.StarlarkImportResult;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
@@ -38,9 +38,6 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
-import java.util.Map;
-import java.util.function.Function;
-import java.util.stream.Collectors;
/** A SkyFunction to parse the WORKSPACE file at a given path. */
public class WorkspaceFileFunction implements SkyFunction {
@@ -86,7 +83,7 @@
try {
return new WorkspaceFileValue(
/* pkg = */ builder.build(),
- /* importMap = */ ImmutableMap.<String, Extension>of(),
+ /* loadedModules = */ ImmutableMap.<String, Module>of(),
/* importToChunkMap = */ ImmutableMap.<String, Integer>of(),
/* bindings = */ ImmutableMap.<String, Object>of(),
workspaceFile,
@@ -122,7 +119,8 @@
if (prevValue.next() == null) {
return prevValue;
}
- parser.setParent(prevValue.getPackage(), prevValue.getImportMap(), prevValue.getBindings());
+ parser.setParent(
+ prevValue.getPackage(), prevValue.getLoadedModules(), prevValue.getBindings());
}
StarlarkFile ast = workspaceASTValue.getASTs().get(key.getIndex());
StarlarkImportResult importResult =
@@ -137,7 +135,7 @@
if (importResult == null) {
return null;
}
- parser.execute(ast, importResult.importMap, key);
+ parser.execute(ast, importResult.loadedModules, key);
} catch (NoSuchPackageException e) {
throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
} catch (NameConflictException e) {
@@ -147,7 +145,7 @@
try {
return new WorkspaceFileValue(
builder.build(),
- parser.getImportMap(),
+ parser.getLoadedModules(),
createImportToChunkMap(prevValue, parser, key),
parser.getVariableBindings(),
workspaceFile,
@@ -181,13 +179,12 @@
WorkspaceFileValue prevValue, WorkspaceFactory parser, WorkspaceFileKey key) {
ImmutableMap.Builder<String, Integer> builder = new ImmutableMap.Builder<String, Integer>();
if (prevValue == null) {
- Map<String, Integer> map =
- parser.getImportMap().keySet().stream()
- .collect(Collectors.toMap(Function.identity(), s -> key.getIndex()));
- builder.putAll(map);
+ for (String loadString : parser.getLoadedModules().keySet()) {
+ builder.put(loadString, key.getIndex());
+ }
} else {
builder.putAll(prevValue.getImportToChunkMap());
- for (String label : parser.getImportMap().keySet()) {
+ for (String label : parser.getLoadedModules().keySet()) {
if (!prevValue.getImportToChunkMap().containsKey(label)) {
builder.put(label, key.getIndex());
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BUILD b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
index 48652da..c55309f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BUILD
@@ -130,8 +130,6 @@
"//src/main/java/com/google/devtools/build/lib/profiler",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skylarkinterface",
- "//src/main/java/com/google/devtools/build/lib/util",
- "//src/main/java/com/google/devtools/build/lib/util:string",
"//src/main/java/com/google/devtools/starlark/spelling",
"//third_party:auto_value",
"//third_party:guava",
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index d6c8956..ed6b3c3b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -163,23 +163,23 @@
}
private static void execLoad(StarlarkThread.Frame fr, LoadStatement node) throws EvalException {
+ // Load module.
+ String moduleName = node.getImport().getValue();
+ Module module = fr.thread.getModule(moduleName);
+ if (module == null) {
+ throw new EvalException(
+ node.getStartLocation(),
+ String.format(
+ "file '%s' was not correctly loaded. "
+ + "Make sure the 'load' statement appears in the global scope in your file",
+ moduleName));
+ }
+ Map<String, Object> globals = module.getExportedBindings();
+
for (LoadStatement.Binding binding : node.getBindings()) {
-
- // Load module.
- String moduleName = node.getImport().getValue();
- StarlarkThread.Extension module = fr.thread.getExtension(moduleName);
- if (module == null) {
- throw new EvalException(
- node.getStartLocation(),
- String.format(
- "file '%s' was not correctly loaded. "
- + "Make sure the 'load' statement appears in the global scope in your file",
- moduleName));
- }
-
// Extract symbol.
Identifier orig = binding.getOriginalName();
- Object value = module.getBindings().get(orig.getName());
+ Object value = globals.get(orig.getName());
if (value == null) {
throw new EvalException(
orig.getStartLocation(),
@@ -187,7 +187,7 @@
"file '%s' does not contain symbol '%s'%s",
moduleName,
orig.getName(),
- SpellChecker.didYouMean(orig.getName(), module.getBindings().keySet())));
+ SpellChecker.didYouMean(orig.getName(), globals.keySet())));
}
// Define module-local variable.
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 955f9cd..b2dad37 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
@@ -17,8 +17,8 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableMap;
import java.util.Collections;
-import java.util.HashSet;
import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
@@ -79,7 +79,11 @@
final LinkedHashMap<String, FlagGuardedValue> restrictedBindings;
/** Set of bindings that are exported (can be loaded from other modules). */
- final HashSet<String> exportedBindings;
+ // TODO(adonovan): this linked hash table is unnecessary for this class to do its job. However,
+ // the sky graph SerializationTester breaks the abstraction of this (and every) class by
+ // serializing its fields and then asserting that encoding, decoding, then reencoding yields the
+ // same result, even though this is not necessary and imposes a run-time tax.
+ final LinkedHashSet<String> exportedBindings;
/** Constructs an uninitialized instance; caller must call {@link #initialize} before use. */
public Module() {
@@ -88,7 +92,7 @@
this.label = null;
this.bindings = new LinkedHashMap<>();
this.restrictedBindings = new LinkedHashMap<>();
- this.exportedBindings = new HashSet<>();
+ this.exportedBindings = new LinkedHashSet<>();
}
/**
@@ -134,7 +138,7 @@
if (universe != null) {
this.restrictedBindings.putAll(universe.restrictedBindings);
}
- this.exportedBindings = new HashSet<>();
+ this.exportedBindings = new LinkedHashSet<>();
}
public Module(Mutability mutability) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
index 5a02b94..8fed417 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkFile.java
@@ -14,7 +14,6 @@
package com.google.devtools.build.lib.syntax;
import com.google.common.collect.ImmutableList;
-import com.google.common.hash.HashCode;
import java.io.IOException;
import java.util.Collections;
import java.util.List;
@@ -32,7 +31,7 @@
private final FileOptions options;
private final ImmutableList<Comment> comments;
final List<SyntaxError> errors; // appended to by Resolver
- @Nullable private final String contentHashCode;
+ @Nullable private final byte[] contentHashCode;
// set by resolver
@Nullable Resolver.Function resolved;
@@ -53,7 +52,7 @@
FileOptions options,
ImmutableList<Comment> comments,
List<SyntaxError> errors,
- String contentHashCode) {
+ byte[] contentHashCode) {
super(locs);
this.statements = statements;
this.options = options;
@@ -69,7 +68,7 @@
ImmutableList<Statement> statements,
FileOptions options,
Parser.ParseResult result,
- String contentHashCode) {
+ byte[] contentHashCode) {
return new StarlarkFile(
locs,
statements,
@@ -137,16 +136,11 @@
return create(result.locs, stmts.build(), options, result, /*contentHashCode=*/ null);
}
- // TODO(adonovan): make the digest publicly settable, and delete this.
+ // TODO(adonovan): move the digest into skyframe and delete this.
public static StarlarkFile parseWithDigest(ParserInput input, byte[] digest, FileOptions options)
throws IOException {
Parser.ParseResult result = Parser.parseFile(input, options);
- return create(
- result.locs,
- ImmutableList.copyOf(result.statements),
- options,
- result,
- HashCode.fromBytes(digest).toString());
+ return create(result.locs, ImmutableList.copyOf(result.statements), options, result, digest);
}
/**
@@ -184,9 +178,11 @@
}
/**
- * Returns a hash code calculated from the string content of the source file of this AST.
+ * Returns the digest of the source file, if this StarlarkFile was constructed by parseWithDigest,
+ * null otherwise.
*/
- @Nullable public String getContentHashCode() {
+ @Nullable
+ public byte[] getContentHashCode() {
return contentHashCode;
}
}
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 022c062..865cc1b 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
@@ -14,25 +14,19 @@
package com.google.devtools.build.lib.syntax;
-import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
-import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.util.Fingerprint; // TODO(adonovan): break dependency
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.Map;
-import java.util.Objects;
import java.util.Set;
-import java.util.TreeSet;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Predicate;
import javax.annotation.Nullable;
@@ -175,170 +169,6 @@
}
}
- /** An Extension to be imported with load() into a BUILD or .bzl file. */
- @Immutable
- // TODO(janakr,brandjon): Do Extensions actually have to start their own memoization? Or can we
- // have a node higher up in the hierarchy inject the mutability?
- // TODO(adonovan): identify Extension with Module, abolish hash code, and make loading lazy (a
- // callback not a map) so that clients don't need to preemptively scan the set of load statements.
- @AutoCodec
- public static final class Extension {
-
- private final ImmutableMap<String, Object> bindings;
-
- /**
- * Cached hash code for the transitive content of this {@code Extension} and its dependencies.
- *
- * <p>Note that "content" refers to the AST content, not the evaluated bindings.
- */
- private final String transitiveContentHashCode;
-
- /** Constructs with the given hash code and bindings. */
- @AutoCodec.Instantiator
- public Extension(ImmutableMap<String, Object> bindings, String transitiveContentHashCode) {
- this.bindings = bindings;
- this.transitiveContentHashCode = transitiveContentHashCode;
- }
-
- /**
- * Constructs using the bindings from the global definitions of the given {@link
- * StarlarkThread}, and that {@code StarlarkThread}'s transitive hash code.
- */
- public Extension(StarlarkThread thread) {
- this(
- ImmutableMap.copyOf(thread.module.getExportedBindings()),
- thread.getTransitiveContentHashCode());
- }
-
- private String getTransitiveContentHashCode() {
- return transitiveContentHashCode;
- }
-
- /** Retrieves all bindings, in a deterministic order. */
- public ImmutableMap<String, Object> getBindings() {
- return bindings;
- }
-
- @Override
- public boolean equals(Object obj) {
- if (this == obj) {
- return true;
- }
- if (!(obj instanceof Extension)) {
- return false;
- }
- Extension other = (Extension) obj;
- return transitiveContentHashCode.equals(other.getTransitiveContentHashCode())
- && bindings.equals(other.getBindings());
- }
-
- private static boolean skylarkObjectsProbablyEqual(Object obj1, Object obj2) {
- // TODO(b/76154791): check this more carefully.
- return obj1.equals(obj2)
- || (obj1 instanceof StarlarkValue
- && obj2 instanceof StarlarkValue
- && Starlark.repr(obj1).equals(Starlark.repr(obj2)));
- }
-
- /**
- * Throws {@link IllegalStateException} if this {@link Extension} is not equal to {@code obj}.
- *
- * <p>The exception explains the reason for the inequality, including all unequal bindings.
- */
- // TODO(adonovan): move this function into the relevant test.
- public void checkStateEquals(Object obj) {
- if (this == obj) {
- return;
- }
- if (!(obj instanceof Extension)) {
- throw new IllegalStateException(
- String.format(
- "Expected an equal Extension, but got a %s instead of an Extension",
- obj == null ? "null" : obj.getClass().getName()));
- }
- Extension other = (Extension) obj;
- ImmutableMap<String, Object> otherBindings = other.getBindings();
-
- Set<String> names = bindings.keySet();
- Set<String> otherNames = otherBindings.keySet();
- if (!names.equals(otherNames)) {
- throw new IllegalStateException(
- String.format(
- "Expected Extensions to be equal, but they don't define the same bindings: "
- + "in this one but not given one: [%s]; in given one but not this one: [%s]",
- Joiner.on(", ").join(Sets.difference(names, otherNames)),
- Joiner.on(", ").join(Sets.difference(otherNames, names))));
- }
-
- ArrayList<String> badEntries = new ArrayList<>();
- for (String name : names) {
- Object value = bindings.get(name);
- Object otherValue = otherBindings.get(name);
- if (value.equals(otherValue)) {
- continue;
- }
- if (value.getClass() == otherValue.getClass()
- && value.getClass().getSimpleName().equals("Depset")) {
- // Unsoundly assume all depsets are equal.
- // We can't compare {x,y}.toCollection() without an
- // upwards dependency.
- continue;
- } else if (value instanceof Dict) {
- if (otherValue instanceof Dict) {
- @SuppressWarnings("unchecked")
- Dict<Object, Object> thisDict = (Dict<Object, Object>) value;
- @SuppressWarnings("unchecked")
- Dict<Object, Object> otherDict = (Dict<Object, Object>) otherValue;
- if (thisDict.size() == otherDict.size()
- && thisDict.keySet().equals(otherDict.keySet())) {
- boolean foundProblem = false;
- for (Object key : thisDict.keySet()) {
- if (!skylarkObjectsProbablyEqual(
- Preconditions.checkNotNull(thisDict.get(key), key),
- Preconditions.checkNotNull(otherDict.get(key), key))) {
- foundProblem = true;
- }
- }
- if (!foundProblem) {
- continue;
- }
- }
- }
- } else if (skylarkObjectsProbablyEqual(value, otherValue)) {
- continue;
- }
- badEntries.add(
- String.format(
- "%s: this one has %s (class %s, %s), but given one has %s (class %s, %s)",
- name,
- Starlark.repr(value),
- value.getClass().getName(),
- value,
- Starlark.repr(otherValue),
- otherValue.getClass().getName(),
- otherValue));
- }
- if (!badEntries.isEmpty()) {
- throw new IllegalStateException(
- "Expected Extensions to be equal, but the following bindings are unequal: "
- + Joiner.on("; ").join(badEntries));
- }
-
- if (!transitiveContentHashCode.equals(other.getTransitiveContentHashCode())) {
- throw new IllegalStateException(
- String.format(
- "Expected Extensions to be equal, but transitive content hashes don't match:"
- + " %s != %s",
- transitiveContentHashCode, other.getTransitiveContentHashCode()));
- }
- }
-
- @Override
- public int hashCode() {
- return Objects.hash(bindings, transitiveContentHashCode);
- }
- }
-
// The module initialized by this Starlark thread.
//
// TODO(adonovan): eliminate. First we need to simplify the set-up sequence like so:
@@ -360,10 +190,8 @@
/** PrintHandler for Starlark print statements. */
private PrintHandler printHandler = StarlarkThread::defaultPrintHandler;
- /**
- * For each imported extension, a global Starlark frame from which to load() individual bindings.
- */
- private final Map<String, Extension> importedExtensions;
+ /** Loaded modules, keyed by load string. */
+ private final Map<String, Module> loadedModules;
/** Stack of active function calls. */
private final ArrayList<Frame> callstack = new ArrayList<>();
@@ -446,8 +274,6 @@
}
}
- private final String transitiveHashCode;
-
public Mutability mutability() {
return mutability;
}
@@ -528,22 +354,16 @@
* Constructs a StarlarkThread. This is the main, most basic constructor.
*
* @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
+ * @param semantics the StarlarkSemantics for this thread.
+ * @param loadedModules modules for each load statement in the file
*/
private StarlarkThread(
- Module module,
- StarlarkSemantics semantics,
- Map<String, Extension> importedExtensions,
- @Nullable String fileContentHashCode) {
+ Module module, StarlarkSemantics semantics, Map<String, Module> loadedModules) {
this.module = Preconditions.checkNotNull(module);
this.mutability = module.mutability();
Preconditions.checkArgument(!module.mutability().isFrozen());
this.semantics = semantics;
- this.importedExtensions = importedExtensions;
- this.transitiveHashCode =
- computeTransitiveContentHashCode(fileContentHashCode, importedExtensions);
+ this.loadedModules = loadedModules;
}
/**
@@ -553,15 +373,13 @@
* {@link #useDefaultSemantics}.
*/
// TODO(adonovan): eliminate the builder:
- // - replace importedExtensions by a callback
- // - eliminate fileContentHashCode
+ // - replace loadedModules by a callback, since there's no need to enumerate them now.
// - decouple Module from thread.
public static class Builder {
private final Mutability mutability;
@Nullable private Module parent;
@Nullable private StarlarkSemantics semantics;
- @Nullable private Map<String, Extension> importedExtensions;
- @Nullable private String fileContentHashCode;
+ private Map<String, Module> loadedModules = ImmutableMap.of();
Builder(Mutability mutability) {
this.mutability = mutability;
@@ -588,16 +406,9 @@
return this;
}
- /** Declares imported extensions for load() statements. */
- public Builder setImportedExtensions(Map<String, Extension> importMap) {
- Preconditions.checkState(this.importedExtensions == null);
- this.importedExtensions = importMap;
- return this;
- }
-
- /** Declares content hash for the source file for this StarlarkThread. */
- public Builder setFileContentHashCode(String fileContentHashCode) {
- this.fileContentHashCode = fileContentHashCode;
+ /** Sets the modules to be provided to each load statement. */
+ public Builder setLoadedModules(Map<String, Module> loadedModules) {
+ this.loadedModules = loadedModules;
return this;
}
@@ -615,10 +426,7 @@
parent = Module.filterOutRestrictedBindings(mutability, parent, semantics);
Module module = new Module(mutability, parent);
- if (importedExtensions == null) {
- importedExtensions = ImmutableMap.of();
- }
- return new StarlarkThread(module, semantics, importedExtensions, fileContentHashCode);
+ return new StarlarkThread(module, semantics, loadedModules);
}
}
@@ -775,33 +583,7 @@
return String.format("<StarlarkThread%s>", mutability());
}
- Extension getExtension(String module) {
- return importedExtensions.get(module);
- }
-
- /**
- * Computes a deterministic hash for the given base hash code and extension map (the map's order
- * does not matter).
- */
- private static String computeTransitiveContentHashCode(
- @Nullable String baseHashCode, Map<String, Extension> importedExtensions) {
- // Calculate a new hash from the hash of the loaded Extensions.
- Fingerprint fingerprint = new Fingerprint();
- if (baseHashCode != null) {
- fingerprint.addString(Preconditions.checkNotNull(baseHashCode));
- }
- TreeSet<String> importStrings = new TreeSet<>(importedExtensions.keySet());
- for (String importString : importStrings) {
- fingerprint.addString(importedExtensions.get(importString).getTransitiveContentHashCode());
- }
- return fingerprint.hexDigestAndReset();
- }
-
- /**
- * Returns a hash code calculated from the hash code of this StarlarkThread and the transitive
- * closure of other StarlarkThreads it loads.
- */
- public String getTransitiveContentHashCode() {
- return transitiveHashCode;
+ Module getModule(String module) {
+ return loadedModules.get(module);
}
}
diff --git a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
index 2417f2f..19689e1 100644
--- a/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
+++ b/src/main/java/com/google/devtools/build/skydoc/SkydocMain.java
@@ -74,7 +74,6 @@
import com.google.devtools.build.lib.syntax.StarlarkFunction;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.syntax.StarlarkThread;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.syntax.Statement;
import com.google.devtools.build.lib.syntax.StringLiteral;
import com.google.devtools.build.skydoc.fakebuildapi.FakeActionsInfoProvider;
@@ -444,7 +443,7 @@
moduleDocMap.put(label, getModuleDoc(file));
- Map<String, Extension> imports = new HashMap<>();
+ Map<String, Module> imports = new HashMap<>();
for (Statement stmt : file.getStatements()) {
if (stmt instanceof LoadStatement) {
LoadStatement load = (LoadStatement) stmt;
@@ -459,7 +458,7 @@
providerInfoList,
aspectInfoList,
moduleDocMap);
- imports.put(module, new Extension(importThread));
+ imports.put(module, importThread.getGlobals());
} catch (NoSuchFileException noSuchFileException) {
throw new StarlarkEvaluationException(
String.format(
@@ -504,7 +503,7 @@
private StarlarkThread evalSkylarkBody(
StarlarkSemantics semantics,
StarlarkFile file,
- Map<String, Extension> imports,
+ Map<String, Module> imports,
List<RuleInfoWrapper> ruleInfoList,
List<ProviderInfoWrapper> providerInfoList,
List<AspectInfoWrapper> aspectInfoList)
@@ -715,14 +714,12 @@
}
private static StarlarkThread createStarlarkThread(
- StarlarkSemantics semantics,
- Module globals,
- Map<String, Extension> imports) {
+ StarlarkSemantics semantics, Module globals, Map<String, Module> imports) {
// We use the default print handler, which writes to stderr.
return StarlarkThread.builder(Mutability.create("Skydoc"))
.setSemantics(semantics)
.setGlobals(globals)
- .setImportedExtensions(imports)
+ .setLoadedModules(imports)
.build();
}
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 4e4c25c..b056c67 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
@@ -898,10 +898,17 @@
MissingFragmentPolicy missingFragmentPolicy,
boolean supportsConstraintChecking,
Attribute... attributes) {
- String ruleDefinitionStarlarkThreadHashCode =
+ // Here we dig the digest out of the thread, but alternatively we could stow it
+ // in Module.label (which is a currently a Label, but that could be changed into a tuple).
+ // Module.label is analogous to BazelStarlarkContext in that is a hook for applications
+ // to hang their state in the thread or module without a dependency.
+ // Observe that below, the rule definition starlark label comes out of the
+ // innermost enclosing caller's module, which is potentially inconsistent.
+ // TODO(adonovan): decide whether this is correct, and see below.
+ byte[] ruleDefinitionStarlarkTransitiveDigest =
ruleDefinitionStarlarkThread == null
? null
- : ruleDefinitionStarlarkThread.getTransitiveContentHashCode();
+ : BazelStarlarkContext.from(ruleDefinitionStarlarkThread).getTransitiveDigest();
return new RuleClass(
name,
DUMMY_STACK,
@@ -927,12 +934,17 @@
configuredTargetFunction,
externalBindingsFunction,
/*optionReferenceFunction=*/ RuleClass.NO_OPTION_REFERENCE,
+ // TODO(adonovan): I think this has always been wrong: unlike the RDE digest which
+ // comes from the thread executing a .bzl file's toplevel, the RDE label comes from the
+ // innermost enclosing call. That means if a.bzl calls a function in b.bzl that calls rule
+ // (pretty weird, admittedly), then the RDE will have the digest of a but the label of b.
+ // See other comment above.
ruleDefinitionStarlarkThread == null
? null
: (Label)
Module.ofInnermostEnclosingStarlarkFunction(ruleDefinitionStarlarkThread)
.getLabel(),
- ruleDefinitionStarlarkThreadHashCode,
+ ruleDefinitionStarlarkTransitiveDigest,
new ConfigurationFragmentPolicy.Builder()
.requiresConfigurationFragments(allowedConfigurationFragments)
.setMissingFragmentPolicy(missingFragmentPolicy)
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
index f4c3521..445c890 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -32,10 +32,10 @@
import com.google.devtools.build.lib.packages.PackageValidator;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
+import com.google.devtools.build.lib.syntax.Module;
import com.google.devtools.build.lib.syntax.ParserInput;
import com.google.devtools.build.lib.syntax.StarlarkFile;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.util.Pair;
@@ -185,7 +185,7 @@
globber,
ConstantRuleVisibility.PUBLIC,
StarlarkSemantics.DEFAULT_SEMANTICS,
- ImmutableMap.<String, Extension>of(),
+ ImmutableMap.<String, Module>of(),
ImmutableList.<Label>of(),
/*repositoryMapping=*/ ImmutableMap.of());
Package result;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkFileContentHashTests.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkFileContentHashTests.java
index 2630126..0f2b9ee 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkFileContentHashTests.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkylarkFileContentHashTests.java
@@ -17,6 +17,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.clock.BlazeClock;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -182,7 +183,8 @@
Collection<Target> targets = result.get(pkgLookupKey).getPackage().getTargets().values();
for (Target target : targets) {
if (target.getName().equals(name)) {
- return ((Rule) target).getRuleClassObject().getRuleDefinitionEnvironmentHashCode();
+ byte[] hash = ((Rule) target).getRuleClassObject().getRuleDefinitionEnvironmentDigest();
+ return BaseEncoding.base16().lowerCase().encode(hash); // hexify
}
}
throw new IllegalStateException("target not found: " + name);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java
index 17c3157..840bc1f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/StarlarkImportLookupFunctionTest.java
@@ -407,7 +407,7 @@
SkyframeExecutorTestUtils.evaluate(
getSkyframeExecutor(), starlarkImportLookupKey, /*keepGoing=*/ false, reporter);
- assertThat(result.get(starlarkImportLookupKey).getEnvironmentExtension().getBindings())
+ assertThat(result.get(starlarkImportLookupKey).getModule().getExportedBindings())
.containsEntry("a_symbol", 5);
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java b/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
index c46c706..e9913bf 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/util/SkylarkTestCase.java
@@ -76,7 +76,8 @@
/*fragmentNameToClass=*/ null,
/*repoMapping=*/ ImmutableMap.of(),
new SymbolGenerator<>(new Object()),
- /*analysisRuleLabel=*/ null)
+ /*analysisRuleLabel=*/ null,
+ /*transitiveDigest=*/ new byte[] {}) // dummy value for tests
.storeInThread(thread);
return thread;
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/BUILD b/src/test/java/com/google/devtools/build/lib/syntax/BUILD
index 9602e62..888f96a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/syntax/BUILD
@@ -34,7 +34,6 @@
"//src/main/java/com/google/devtools/build/lib/packages:starlark_semantics_options",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils",
"//src/main/java/com/google/devtools/build/lib/skylarkinterface",
- "//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/build/lib/util:string",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/common/options",
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java
index cc9a2ea..8f8748a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/StarlarkThreadTest.java
@@ -17,9 +17,7 @@
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
-import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
-import com.google.devtools.build.lib.syntax.StarlarkThread.Extension;
import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -196,71 +194,4 @@
.contains("local variable 'global_var' is referenced before assignment.");
}
}
-
- @Test
- public void testTransitiveHashCodeDeterminism() throws Exception {
- // As a proxy for determinism, test that changing the order of imports doesn't change the hash
- // code (within any one execution).
- Extension a = new Extension(ImmutableMap.of(), "a123");
- Extension b = new Extension(ImmutableMap.of(), "b456");
- Extension c = new Extension(ImmutableMap.of(), "c789");
- StarlarkThread thread1 =
- StarlarkThread.builder(Mutability.create("testing1"))
- .useDefaultSemantics()
- .setImportedExtensions(ImmutableMap.of("a", a, "b", b, "c", c))
- .setFileContentHashCode("z")
- .build();
- StarlarkThread thread2 =
- StarlarkThread.builder(Mutability.create("testing2"))
- .useDefaultSemantics()
- .setImportedExtensions(ImmutableMap.of("c", c, "b", b, "a", a))
- .setFileContentHashCode("z")
- .build();
- assertThat(thread1.getTransitiveContentHashCode())
- .isEqualTo(thread2.getTransitiveContentHashCode());
- }
-
- @Test
- public void testExtensionEqualityDebugging_RhsIsNull() {
- assertCheckStateFailsWithMessage(new Extension(ImmutableMap.of(), "abc"), null, "got a null");
- }
-
- @Test
- public void testExtensionEqualityDebugging_RhsHasBadType() {
- assertCheckStateFailsWithMessage(
- new Extension(ImmutableMap.of(), "abc"), 5, "got a java.lang.Integer");
- }
-
- @Test
- public void testExtensionEqualityDebugging_DifferentBindings() {
- assertCheckStateFailsWithMessage(
- new Extension(ImmutableMap.of("w", 1, "x", 2, "y", 3), "abc"),
- new Extension(ImmutableMap.of("y", 3, "z", 4), "abc"),
- "in this one but not given one: [w, x]; in given one but not this one: [z]");
- }
-
- @Test
- public void testExtensionEqualityDebugging_DifferentValues() {
- assertCheckStateFailsWithMessage(
- new Extension(ImmutableMap.of("x", 1, "y", "foo", "z", true), "abc"),
- new Extension(ImmutableMap.of("x", 2.0, "y", "foo", "z", false), "abc"),
- "bindings are unequal: x: this one has 1 (class java.lang.Integer, 1), but given one has "
- + "2.0 (class java.lang.Double, 2.0); z: this one has True (class java.lang.Boolean, "
- + "true), but given one has False (class java.lang.Boolean, false)");
- }
-
- @Test
- public void testExtensionEqualityDebugging_DifferentHashes() {
- assertCheckStateFailsWithMessage(
- new Extension(ImmutableMap.of(), "abc"),
- new Extension(ImmutableMap.of(), "xyz"),
- "transitive content hashes don't match: abc != xyz");
- }
-
- private static void assertCheckStateFailsWithMessage(
- Extension left, Object right, String substring) {
- IllegalStateException expected =
- assertThrows(IllegalStateException.class, () -> left.checkStateEquals(right));
- assertThat(expected).hasMessageThat().contains(substring);
- }
}
diff --git a/src/test/shell/integration/discard_graph_edges_test.sh b/src/test/shell/integration/discard_graph_edges_test.sh
index 35538bb..9016112 100755
--- a/src/test/shell/integration/discard_graph_edges_test.sh
+++ b/src/test/shell/integration/discard_graph_edges_test.sh
@@ -251,10 +251,9 @@
local glob_count="$(extract_histogram_count "$histo_file" "GlobValue$")"
[[ "$glob_count" -ge 2 ]] \
|| fail "glob count $glob_count too low: did you move/rename the class?"
- local ext_count="$(extract_histogram_count "$histo_file" \
- 'StarlarkThread\$Extension$')"
- [[ "$ext_count" -ge 3 ]] \
- || fail "Extension count $ext_count too low: did you move/rename the class?"
+ local module_count="$(extract_histogram_count "$histo_file" 'syntax.Module$')"
+ [[ "$module_count" -gt 25 ]] \
+ || fail "Module count $module_count too low: was the class renamed/moved?" # was 74
local ct_count="$(extract_histogram_count "$histo_file" \
'RuleConfiguredTarget$')"
[[ "$ct_count" -ge 18 ]] \
@@ -276,10 +275,9 @@
glob_count="$(extract_histogram_count "$histo_file" "GlobValue$")"
[[ "$glob_count" -le 1 ]] \
|| fail "glob count $glob_count too high"
- ext_count="$(extract_histogram_count "$histo_file" \
- 'StarlarkThread\$ Extension$')"
- [[ "$ext_count" -le 7 ]] \
- || fail "extension count $ext_count too high"
+ module_count="$(extract_histogram_count "$histo_file" 'syntax.Module$')"
+ [[ "$module_count" -lt 25 ]] \
+ || fail "Module count $module_count too high" # was 22
ct_count="$(extract_histogram_count "$histo_file" \
'RuleConfiguredTarget$')"
[[ "$ct_count" -le 1 ]] \
