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

5 files changed, 119 insertions, 58 deletions

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 a9c617ea9e..f01026b8da 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 @@ import javax.annotation.concurrent.Immutable;
  */
 @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 @@ public final class RuleClass {
     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 @@ public final class RuleClass {
       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 @@ public final class RuleClass {
         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 @@ public final class RuleClass {
       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 final class RuleClass {
     }
   }
 
-  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 @@ public final class RuleClass {
    * 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 @@ public final class RuleClass {
    * <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 @@ public final class RuleClass {
     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 @@ public final class RuleClass {
   }
 
   /**
-   * 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 f7470e92e6..bfdc858319 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 @@ public class RuleSerializer {
     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 8ef298a697..e26b257af7 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 @@ public final class Environment implements Freezable {
      * 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 @@ public final class Environment implements Freezable {
         FuncallExpression caller,
         Frame lexicalFrame,
         Frame globalFrame,
-        Set<String> knownGlobalVariables,
         boolean isSkylark) {
       this.continuation = continuation;
       this.function = function;
@@ -275,14 +274,7 @@ public final class Environment implements Freezable {
       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 @@ public final class Environment implements Freezable {
    * @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 @@ public final class Environment implements Freezable {
     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 @@ public final class Environment implements Freezable {
     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 @@ public final class Environment implements Freezable {
     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 @@ public final class Environment implements Freezable {
     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 @@ public final class Environment implements Freezable {
     return knownGlobalVariables != null && knownGlobalVariables.contains(varname);
   }
 
-  public void handleEvent(Event event) {
+  void handleEvent(Event event) {
     eventHandler.handle(event);
   }
 
@@ -831,7 +817,7 @@ public final class Environment implements Freezable {
    * 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 final class Environment implements Freezable {
     }
   }
 
-  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 @@ public final class Environment implements Freezable {
     }
   }
 
-  /**
-   * 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 @@ public final class Environment implements Freezable {
     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();
diff --git a/src/main/protobuf/build.proto b/src/main/protobuf/build.proto
index f74f03fb7e..a5c2aa0980 100644
--- a/src/main/protobuf/build.proto
+++ b/src/main/protobuf/build.proto
@@ -306,6 +306,10 @@ message Rule {
 
   // List of Skylark aspects that this rule applies.
   repeated AttributeAspect skylark_attribute_aspects = 11;
+
+  // Hash encapsulating the behavior of this Skylark rule. Any change to this
+  // rule's definition that could change its behavior will be reflected here.
+  optional string skylark_environment_hash_code = 12;
 }
 
 // A pairing of attribute name and a Skylark aspect that is applied to that attribute.
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 e9f9041c4f..6185190035 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
@@ -873,6 +873,10 @@ public class RuleClassTest extends PackageLoadingTestCase {
       MissingFragmentPolicy missingFragmentPolicy,
       boolean supportsConstraintChecking,
       Attribute... attributes) {
+    String ruleDefinitionEnvironmentHashCode =
+        ruleDefinitionEnvironment == null
+            ? null
+            : ruleDefinitionEnvironment.getTransitiveContentHashCode();
     return new RuleClass(
         name,
         /*isSkylark=*/ skylarkExecutable,
@@ -892,6 +896,7 @@ public class RuleClassTest extends PackageLoadingTestCase {
         configuredTargetFunction,
         externalBindingsFunction,
         ruleDefinitionEnvironment,
+        ruleDefinitionEnvironmentHashCode,
         new ConfigurationFragmentPolicy.Builder()
             .requiresConfigurationFragments(allowedConfigurationFragments)
             .setMissingFragmentPolicy(missingFragmentPolicy)