Skip to content

Commit

Permalink
bazel syntax: delete StarlarkThread.Extension
Browse files Browse the repository at this point in the history
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
  • Loading branch information
adonovan authored and Copybara-Service committed May 7, 2020
1 parent 00806c8 commit 1d93d26
Show file tree
Hide file tree
Showing 34 changed files with 286 additions and 531 deletions.
Expand Up @@ -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;
Expand Down Expand Up @@ -764,14 +763,20 @@ public ImmutableMap<String, Object> getEnvironment() {
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);
Expand All @@ -781,8 +786,7 @@ public StarlarkThread createRuleClassStarlarkThread(
StarlarkThread.builder(mutability)
.setGlobals(Module.createForBuiltins(env).withLabel(fileLabel))
.setSemantics(starlarkSemantics)
.setFileContentHashCode(astFileContentHashCode)
.setImportedExtensions(importMap)
.setLoadedModules(loadedModules)
.build();
thread.setPrintHandler(printHandler);

Expand All @@ -792,7 +796,8 @@ public StarlarkThread createRuleClassStarlarkThread(
configurationFragmentMap,
repoMapping,
new SymbolGenerator<>(fileLabel),
/* analysisRuleLabel= */ null)
/*analysisRuleLabel=*/ null,
transitiveDigest)
.storeInThread(thread);

return thread;
Expand Down
Expand Up @@ -344,9 +344,9 @@ public StarlarkCallable rule(
.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));

Expand Down
Expand Up @@ -110,7 +110,8 @@ public static ConfiguredTarget buildRule(
/*fragmentNameToClass=*/ null,
ruleContext.getTarget().getPackage().getRepositoryMapping(),
ruleContext.getSymbolGenerator(),
ruleContext.getLabel())
ruleContext.getLabel(),
/*transitiveDigest=*/ null)
.storeInThread(thread);

RuleClass ruleClass = ruleContext.getRule().getRuleClassObject();
Expand Down
Expand Up @@ -154,7 +154,8 @@ public RepositoryDirectoryValue.Builder fetch(
/*fragmentNameToClass=*/ null,
rule.getPackage().getRepositoryMapping(),
new SymbolGenerator<>(key),
/*analysisRuleLabel=*/ null)
/*analysisRuleLabel=*/ null,
/*transitiveDigest=*/ null)
.storeInThread(thread);

SkylarkRepositoryContext skylarkRepositoryContext =
Expand Down
Expand Up @@ -65,7 +65,8 @@ public StarlarkCallable repositoryRule(
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);

Expand All @@ -90,19 +91,16 @@ public StarlarkCallable repositoryRule(
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);
}
Expand Down
Expand Up @@ -50,6 +50,7 @@ public void storeInThread(StarlarkThread thread) {
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
Expand All @@ -76,13 +77,15 @@ public BazelStarlarkContext(
@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. */
Expand Down Expand Up @@ -124,6 +127,16 @@ public Label getAnalysisRuleLabel() {
return analysisRuleLabel;
}

/**
* 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.
*
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -93,9 +92,8 @@ public final class PackageFactory {
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);
Expand Down Expand Up @@ -414,7 +412,7 @@ public Package.Builder createPackageFromAst(
PackageIdentifier packageId,
RootedPath buildFile,
StarlarkFile file,
Map<String, Extension> imports,
Map<String, Module> loadedModules,
ImmutableList<Label> skylarkFileDependencies,
RuleVisibility defaultVisibility,
StarlarkSemantics starlarkSemantics,
Expand All @@ -431,7 +429,7 @@ public Package.Builder createPackageFromAst(
globber,
defaultVisibility,
starlarkSemantics,
imports,
loadedModules,
skylarkFileDependencies,
repositoryMapping);
} catch (InterruptedException e) {
Expand Down Expand Up @@ -531,7 +529,7 @@ public Package createPackageForTesting(
packageId,
buildFile,
file,
/*imports=*/ ImmutableMap.<String, Extension>of(),
/*loadedModules=*/ ImmutableMap.<String, Module>of(),
/*skylarkFileDependencies=*/ ImmutableList.<Label>of(),
/*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC,
semantics,
Expand Down Expand Up @@ -714,7 +712,7 @@ public Package.Builder evaluateBuildFile(
Globber globber,
RuleVisibility defaultVisibility,
StarlarkSemantics semantics,
Map<String, Extension> imports,
Map<String, Module> loadedModules,
ImmutableList<Label> skylarkFileDependencies,
ImmutableMap<RepositoryName, RepositoryName> repositoryMapping)
throws InterruptedException {
Expand All @@ -741,7 +739,7 @@ public Package.Builder evaluateBuildFile(
packageId,
file,
semantics,
imports,
loadedModules,
new PackageContext(pkgBuilder, globber, eventHandler))) {
pkgBuilder.setContainsErrors();
}
Expand All @@ -757,7 +755,7 @@ private boolean buildPackage(
PackageIdentifier packageId,
StarlarkFile file,
StarlarkSemantics semantics,
Map<String, Extension> imports,
Map<String, Module> loadedModules,
PackageContext pkgContext)
throws InterruptedException {

Expand Down Expand Up @@ -787,7 +785,7 @@ private boolean buildPackage(
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();
Expand Down Expand Up @@ -839,7 +837,8 @@ private boolean buildPackage(
/*fragmentNameToClass=*/ null,
pkgBuilder.getRepositoryMapping(),
new SymbolGenerator<>(packageId),
/*analysisRuleLabel=*/ null)
/*analysisRuleLabel=*/ null,
/*transitiveDigest=*/ null)
.storeInThread(thread);

// TODO(adonovan): save this as a field in BazelSkylarkContext.
Expand Down
29 changes: 16 additions & 13 deletions src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
Expand Up @@ -678,7 +678,7 @@ public String toString() {
/** 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();

Expand Down Expand Up @@ -819,7 +819,7 @@ public RuleClass build(String name, String key) {
Preconditions.checkState(externalBindingsFunction == NO_EXTERNAL_BINDINGS);
}
if (type == RuleClassType.PLACEHOLDER) {
Preconditions.checkNotNull(ruleDefinitionEnvironmentHashCode, this.name);
Preconditions.checkNotNull(ruleDefinitionEnvironmentDigest, this.name);
}

if (buildSetting != null) {
Expand Down Expand Up @@ -865,7 +865,7 @@ public RuleClass build(String name, String key) {
externalBindingsFunction,
optionReferenceFunction,
ruleDefinitionEnvironmentLabel,
ruleDefinitionEnvironmentHashCode,
ruleDefinitionEnvironmentDigest,
configurationFragmentPolicy.build(),
supportsConstraintChecking,
thirdPartyLicenseExistencePolicy,
Expand Down Expand Up @@ -1251,10 +1251,12 @@ public Builder setExternalBindingsFunction(Function<? super Rule, Map<String, La
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;
}

Expand Down Expand Up @@ -1583,7 +1585,7 @@ public Attribute.Builder<?> copy(String name) {
*/
@Nullable private final Label ruleDefinitionEnvironmentLabel;

@Nullable private final String ruleDefinitionEnvironmentHashCode;
@Nullable private final byte[] ruleDefinitionEnvironmentDigest;
private final OutputFile.Kind outputFileKind;

/**
Expand Down Expand Up @@ -1652,7 +1654,7 @@ public Attribute.Builder<?> copy(String name) {
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,
Expand Down Expand Up @@ -1683,7 +1685,7 @@ public Attribute.Builder<?> copy(String name) {
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);
Expand Down Expand Up @@ -2548,12 +2550,13 @@ public Label getRuleDefinitionEnvironmentLabel() {
}

/**
* 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. */
Expand Down
Expand Up @@ -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
Expand Down Expand Up @@ -61,8 +60,8 @@ public interface RuleClassProvider extends RuleDefinitionContext {
* @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
Expand All @@ -72,8 +71,8 @@ StarlarkThread createRuleClassStarlarkThread(
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);

Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -43,8 +44,12 @@ public static Build.Rule.Builder serializeRule(Rule rule) {

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);
Expand Down

0 comments on commit 1d93d26

Please sign in to comment.