Store the hash code of the Environment in the RuleClass object. When a RuleClass is deserialized as part of a Skylark rule, the Environment is currently not present, but it is needed to detect changes to the rule.
Also precompute and store the Environment's hash code, and do a drive-by clean-up of a bunch of warnings in the Environment code.
--
MOS_MIGRATED_REVID=122838588
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 a9c617e..f01026b 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
@@ -102,8 +102,8 @@
*/
@Immutable
public final class RuleClass {
- public static final Function<? super Rule, Map<String, Label>> NO_EXTERNAL_BINDINGS =
- Functions.<Map<String, Label>>constant(ImmutableMap.<String, Label>of());
+ static final Function<? super Rule, Map<String, Label>> NO_EXTERNAL_BINDINGS =
+ Functions.<Map<String, Label>>constant(ImmutableMap.<String, Label>of());
public static final PathFragment THIRD_PARTY_PREFIX = new PathFragment("third_party");
@@ -481,7 +481,8 @@
private BaseFunction configuredTargetFunction = null;
private Function<? super Rule, Map<String, Label>> externalBindingsFunction =
NO_EXTERNAL_BINDINGS;
- private Environment ruleDefinitionEnvironment = null;
+ @Nullable private Environment ruleDefinitionEnvironment = null;
+ @Nullable private String ruleDefinitionEnvironmentHashCode = null;
private ConfigurationFragmentPolicy.Builder configurationFragmentPolicy =
new ConfigurationFragmentPolicy.Builder();
@@ -503,6 +504,7 @@
this.name = name;
this.skylark = skylark;
this.type = type;
+ Preconditions.checkState(skylark || type != RuleClassType.PLACEHOLDER, name);
this.documented = type != RuleClassType.ABSTRACT;
for (RuleClass parent : parents) {
if (parent.getValidityPredicate() != PredicatesWithMessage.<Rule>alwaysTrue()) {
@@ -569,12 +571,32 @@
Preconditions.checkState(skylarkExecutable == (ruleDefinitionEnvironment != null));
Preconditions.checkState(externalBindingsFunction == NO_EXTERNAL_BINDINGS);
}
- return new RuleClass(name, skylark, skylarkExecutable, documented, publicByDefault,
- binaryOutput, workspaceOnly, outputsDefaultExecutable, implicitOutputsFunction,
- configurator, configuredTargetFactory, validityPredicate, preferredDependencyPredicate,
- ImmutableSet.copyOf(advertisedProviders), canHaveAnyProvider, configuredTargetFunction,
- externalBindingsFunction, ruleDefinitionEnvironment, configurationFragmentPolicy.build(),
- supportsConstraintChecking, attributes.values().toArray(new Attribute[0]));
+ if (type == RuleClassType.PLACEHOLDER) {
+ Preconditions.checkNotNull(ruleDefinitionEnvironmentHashCode, this.name);
+ }
+ return new RuleClass(
+ name,
+ skylark,
+ skylarkExecutable,
+ documented,
+ publicByDefault,
+ binaryOutput,
+ workspaceOnly,
+ outputsDefaultExecutable,
+ implicitOutputsFunction,
+ configurator,
+ configuredTargetFactory,
+ validityPredicate,
+ preferredDependencyPredicate,
+ ImmutableSet.copyOf(advertisedProviders),
+ canHaveAnyProvider,
+ configuredTargetFunction,
+ externalBindingsFunction,
+ ruleDefinitionEnvironment,
+ ruleDefinitionEnvironmentHashCode,
+ configurationFragmentPolicy.build(),
+ supportsConstraintChecking,
+ attributes.values().toArray(new Attribute[0]));
}
/**
@@ -814,11 +836,19 @@
return this;
}
- /**
- * Sets the rule definition environment. Meant for Skylark usage.
- */
+ /** Sets the rule definition environment. Meant for Skylark usage. */
public Builder setRuleDefinitionEnvironment(Environment env) {
- this.ruleDefinitionEnvironment = env;
+ this.ruleDefinitionEnvironment = Preconditions.checkNotNull(env, this.name);
+ this.ruleDefinitionEnvironmentHashCode =
+ this.ruleDefinitionEnvironment.getTransitiveContentHashCode();
+ return this;
+ }
+
+ /** Sets the rule definition environment hash code for deserialized rule classes. */
+ Builder setRuleDefinitionEnvironmentHashCode(String hashCode) {
+ Preconditions.checkState(ruleDefinitionEnvironment == null, ruleDefinitionEnvironment);
+ Preconditions.checkState(type == RuleClassType.PLACEHOLDER, type);
+ this.ruleDefinitionEnvironmentHashCode = Preconditions.checkNotNull(hashCode, this.name);
return this;
}
@@ -900,8 +930,8 @@
}
}
- public static Builder createPlaceholderBuilder(final String name, final Location ruleLocation,
- ImmutableList<RuleClass> parents) {
+ static Builder createPlaceholderBuilder(
+ final String name, final Location ruleLocation, ImmutableList<RuleClass> parents) {
return new Builder(name, RuleClassType.PLACEHOLDER, /*skylark=*/true,
parents.toArray(new RuleClass[parents.size()])).factory(
new ConfiguredTargetFactory<Object, Object>() {
@@ -996,6 +1026,7 @@
* Null for non Skylark executable RuleClasses.
*/
@Nullable private final Environment ruleDefinitionEnvironment;
+ @Nullable private final String ruleDefinitionEnvironmentHashCode;
/**
* The set of configuration fragments which are legal for this rule's implementation to access.
@@ -1029,21 +1060,28 @@
* <p>The {@code depsAllowedRules} predicate should have a {@code toString}
* method which returns a plain English enumeration of the allowed rule class
* names, if it does not allow all rule classes.
- * @param workspaceOnly
*/
@VisibleForTesting
- RuleClass(String name,
- boolean isSkylark, boolean skylarkExecutable, boolean documented, boolean publicByDefault,
- boolean binaryOutput, boolean workspaceOnly, boolean outputsDefaultExecutable,
+ RuleClass(
+ String name,
+ boolean isSkylark,
+ boolean skylarkExecutable,
+ boolean documented,
+ boolean publicByDefault,
+ boolean binaryOutput,
+ boolean workspaceOnly,
+ boolean outputsDefaultExecutable,
ImplicitOutputsFunction implicitOutputsFunction,
Configurator<?, ?> configurator,
ConfiguredTargetFactory<?, ?> configuredTargetFactory,
- PredicateWithMessage<Rule> validityPredicate, Predicate<String> preferredDependencyPredicate,
+ PredicateWithMessage<Rule> validityPredicate,
+ Predicate<String> preferredDependencyPredicate,
ImmutableSet<Class<?>> advertisedProviders,
boolean canHaveAnyProvider,
@Nullable BaseFunction configuredTargetFunction,
Function<? super Rule, Map<String, Label>> externalBindingsFunction,
@Nullable Environment ruleDefinitionEnvironment,
+ String ruleDefinitionEnvironmentHashCode,
ConfigurationFragmentPolicy configurationFragmentPolicy,
boolean supportsConstraintChecking,
Attribute... attributes) {
@@ -1064,6 +1102,7 @@
this.configuredTargetFunction = configuredTargetFunction;
this.externalBindingsFunction = externalBindingsFunction;
this.ruleDefinitionEnvironment = ruleDefinitionEnvironment;
+ this.ruleDefinitionEnvironmentHashCode = ruleDefinitionEnvironmentHashCode;
validateNoClashInPublicNames(attributes);
this.attributes = ImmutableList.copyOf(attributes);
this.workspaceOnly = workspaceOnly;
@@ -1756,12 +1795,25 @@
}
/**
- * Returns this RuleClass's rule definition environment.
+ * Returns this RuleClass's rule definition environment. Is null for native rules' RuleClass
+ * objects and deserialized Skylark rules. Deserialized rules do provide a hash code encapsulating
+ * their behavior, available at {@link #getRuleDefinitionEnvironmentHashCode}.
*/
- @Nullable public Environment getRuleDefinitionEnvironment() {
+ @Nullable
+ public Environment getRuleDefinitionEnvironment() {
return ruleDefinitionEnvironment;
}
+ /**
+ * Returns the hash code for the RuleClass's rule definition environment. In deserialization,
+ * this RuleClass may not actually contain its environment, in which case the hash code is all
+ * that is available. Will be null for native rules' RuleClass objects.
+ */
+ @Nullable
+ public String getRuleDefinitionEnvironmentHashCode() {
+ return ruleDefinitionEnvironmentHashCode;
+ }
+
/** Returns true if this RuleClass is a skylark-defined RuleClass. */
public boolean isSkylark() {
return isSkylark;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java
index f7470e9..bfdc858 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleSerializer.java
@@ -40,6 +40,14 @@
RawAttributeMapper rawAttributeMapper = RawAttributeMapper.of(rule);
boolean isSkylark = rule.getRuleClassObject().isSkylark();
+ if (isSkylark) {
+ builder.setSkylarkEnvironmentHashCode(
+ Preconditions.checkNotNull(
+ rule.getRuleClassObject()
+ .getRuleDefinitionEnvironment()
+ .getTransitiveContentHashCode(),
+ rule));
+ }
for (Attribute attr : rule.getAttributes()) {
Object rawAttributeValue = rawAttributeMapper.getRawAttributeValue(rule, attr);
boolean isExplicit = rule.isAttributeValueExplicitlySpecified(attr);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 8ef298a..e26b257 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -176,7 +176,7 @@
* This provides a O(n) way of extracting the list of all variables visible in an Environment.
* @param vars the set of visible variables in the Environment, being computed.
*/
- public void addVariableNamesTo(Set<String> vars) {
+ void addVariableNamesTo(Set<String> vars) {
vars.addAll(bindings.keySet());
if (parent != null) {
parent.addVariableNamesTo(vars);
@@ -224,7 +224,6 @@
FuncallExpression caller,
Frame lexicalFrame,
Frame globalFrame,
- Set<String> knownGlobalVariables,
boolean isSkylark) {
this.continuation = continuation;
this.function = function;
@@ -275,14 +274,7 @@
this.transitiveContentHashCode = env.getTransitiveContentHashCode();
}
- // Hack to allow serialization.
- private Extension() {
- super();
- this.transitiveContentHashCode = null;
- }
-
- @VisibleForTesting // This is only used in one test.
- public String getTransitiveContentHashCode() {
+ String getTransitiveContentHashCode() {
return transitiveContentHashCode;
}
@@ -375,11 +367,11 @@
* @param globals the global Frame that this function closes over from its definition Environment
*/
void enterScope(BaseFunction function, FuncallExpression caller, Frame globals) {
- continuation = new Continuation(
- continuation, function, caller, lexicalFrame, globalFrame, knownGlobalVariables, isSkylark);
+ continuation =
+ new Continuation(continuation, function, caller, lexicalFrame, globalFrame, isSkylark);
lexicalFrame = new Frame(mutability(), null);
globalFrame = globals;
- knownGlobalVariables = new HashSet<String>();
+ knownGlobalVariables = new HashSet<>();
isSkylark = true;
}
@@ -395,10 +387,7 @@
continuation = continuation.continuation;
}
- /**
- * When evaluating code from a file, this contains a hash of the file.
- */
- @Nullable private String fileContentHashCode;
+ private final String transitiveHashCode;
/**
* Is this Environment being evaluated during the loading phase?
@@ -434,10 +423,8 @@
return dynamicFrame.mutability();
}
- /**
- * @return the current Frame, in which variable side-effects happen.
- */
- public Frame currentFrame() {
+ /** @return the current Frame, in which variable side-effects happen. */
+ private Frame currentFrame() {
return isGlobal() ? globalFrame : lexicalFrame;
}
@@ -457,10 +444,8 @@
return eventHandler;
}
- /**
- * @return the current stack trace as a list of functions.
- */
- public ImmutableList<BaseFunction> getStackTrace() {
+ /** @return the current stack trace as a list of functions. */
+ ImmutableList<BaseFunction> getStackTrace() {
ImmutableList.Builder<BaseFunction> builder = new ImmutableList.Builder<>();
for (Continuation k = continuation; k != null; k = k.continuation) {
builder.add(k.function);
@@ -512,10 +497,11 @@
this.eventHandler = eventHandler;
this.importedExtensions = importedExtensions;
this.isSkylark = isSkylark;
- this.fileContentHashCode = fileContentHashCode;
this.phase = phase;
this.callerLabel = callerLabel;
this.toolsRepository = toolsRepository;
+ this.transitiveHashCode =
+ computeTransitiveContentHashCode(fileContentHashCode, importedExtensions);
}
/**
@@ -783,7 +769,7 @@
return knownGlobalVariables != null && knownGlobalVariables.contains(varname);
}
- public void handleEvent(Event event) {
+ void handleEvent(Event event) {
eventHandler.handle(event);
}
@@ -831,7 +817,7 @@
* An Exception thrown when an attempt is made to import a symbol from a file
* that was not properly loaded.
*/
- public static class LoadFailedException extends Exception {
+ static class LoadFailedException extends Exception {
LoadFailedException(String importString) {
super(String.format("file '%s' was not correctly loaded. "
+ "Make sure the 'load' statement appears in the global scope in your file",
@@ -839,7 +825,7 @@
}
}
- public void importSymbol(String importString, Identifier symbol, String nameInLoadedFile)
+ void importSymbol(String importString, Identifier symbol, String nameInLoadedFile)
throws NoSuchVariableException, LoadFailedException {
Preconditions.checkState(isGlobal()); // loading is only allowed at global scope.
@@ -864,14 +850,13 @@
}
}
- /**
- * Returns a hash code calculated from the hash code of this Environment and the
- * transitive closure of other Environments it loads.
- */
- public String getTransitiveContentHashCode() {
+ private static String computeTransitiveContentHashCode(
+ @Nullable String baseHashCode, Map<String, Extension> importedExtensions) {
// Calculate a new hash from the hash of the loaded Extension-s.
Fingerprint fingerprint = new Fingerprint();
- fingerprint.addString(Preconditions.checkNotNull(fileContentHashCode));
+ 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());
@@ -879,9 +864,16 @@
return fingerprint.hexDigestAndReset();
}
+ /**
+ * Returns a hash code calculated from the hash code of this Environment and the
+ * transitive closure of other Environments it loads.
+ */
+ public String getTransitiveContentHashCode() {
+ return transitiveHashCode;
+ }
/** A read-only Environment.Frame with global constants in it only */
- public static final Frame CONSTANTS_ONLY = createConstantsGlobals();
+ static final Frame CONSTANTS_ONLY = createConstantsGlobals();
/** A read-only Environment.Frame with initial globals for the BUILD language */
public static final Frame BUILD = createBuildGlobals();
