Add proper serialization constructor and equals/hashCode for EnvironmentLabels.

PiperOrigin-RevId: 188078054

3 files changed, 140 insertions, 23 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java
index c1ce9f0a1a..430078eb62 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.analysis.constraints;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
@@ -89,6 +90,15 @@ public class EnvironmentCollection {
   /** An empty collection. */
   static final EnvironmentCollection EMPTY = new EnvironmentCollection(ImmutableMultimap.of());
 
+  @Override
+  public String toString() {
+    return MoreObjects.toStringHelper(this)
+        .add("size", map.size())
+        .add("hashCode", map.hashCode())
+        .add("map", map)
+        .toString();
+  }
+
   /**
    * Builder for {@link EnvironmentCollection}.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
index a0e8b23ece..34090af5e1 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.packages;
 
 import com.google.common.base.Predicate;
-import com.google.common.base.Verify;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Multimap;
@@ -29,28 +28,26 @@ import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Location;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
 /**
  * Model for the "environment_group' rule: the piece of Bazel's rule constraint system that binds
- * thematically related environments together and determines which environments a rule supports
- * by default. See {@link com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics}
- * for precise semantic details of how this information is used.
+ * thematically related environments together and determines which environments a rule supports by
+ * default. See {@link com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics} for
+ * precise semantic details of how this information is used.
  *
  * <p>Note that "environment_group" is implemented as a loading-time function, not a rule. This is
- * to support proper discovery of defaults: Say rule A has no explicit constraints and depends
- * on rule B, which is explicitly constrained to environment ":bar". Since A declares nothing
- * explicitly, it's implicitly constrained to DEFAULTS (whatever that is). Therefore, the
- * dependency is only allowed if DEFAULTS doesn't include environments beyond ":bar". To figure
- * that out, we need to be able to look up the environment group for ":bar", which is what this
- * class provides.
+ * to support proper discovery of defaults: Say rule A has no explicit constraints and depends on
+ * rule B, which is explicitly constrained to environment ":bar". Since A declares nothing
+ * explicitly, it's implicitly constrained to DEFAULTS (whatever that is). Therefore, the dependency
+ * is only allowed if DEFAULTS doesn't include environments beyond ":bar". To figure that out, we
+ * need to be able to look up the environment group for ":bar", which is what this class provides.
  *
- * <p>If we implemented this as a rule, we'd have to provide that lookup via rule dependencies,
- * e.g. something like:
- *
- * <code>
+ * <p>If we implemented this as a rule, we'd have to provide that lookup via rule dependencies, e.g.
+ * something like: <code>
  *   environment(
  *       name = 'bar',
  *       group = [':sample_environments'],
@@ -62,7 +59,7 @@ import java.util.Set;
  * to determine what other environments belong to the group is to have the group somehow reference
  * them. That would produce circular dependencies in the build graph, which is no good.
  */
-@Immutable
+@Immutable // This is a lie, but this object is only mutable until its containing package is loaded.
 public class EnvironmentGroup implements Target {
   private final EnvironmentLabels environmentLabels;
   private final Location location;
@@ -106,6 +103,7 @@ public class EnvironmentGroup implements Target {
   }
 
   public EnvironmentLabels getEnvironmentLabels() {
+    environmentLabels.checkInitialized();
     return environmentLabels;
   }
 
@@ -167,15 +165,17 @@ public class EnvironmentGroup implements Target {
       }
     }
 
+    Map<Label, NestedSet<Label>> fulfillersMap = new HashMap<>();
     // Now that we know which environments directly fulfill each other, compute which environments
     // transitively fulfill each other. We could alternatively compute this on-demand, but since
     // we don't expect these chains to be very large we opt toward computing them once at package
     // load time.
-    Verify.verify(environmentLabels.fulfillersMap.isEmpty());
+    environmentLabels.assertNotInitialized();
     for (Label envName : environmentLabels.environments) {
-      setTransitiveFulfillers(envName, directFulfillers, environmentLabels.fulfillersMap);
+      setTransitiveFulfillers(envName, directFulfillers, fulfillersMap);
     }
 
+    environmentLabels.setFulfillersMap(fulfillersMap);
     return events;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java
index e756a8e2a9..c375c7a229 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java
@@ -14,19 +14,26 @@
 
 package com.google.devtools.build.lib.packages;
 
-import com.google.common.base.Verify;
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import java.util.Collection;
-import java.util.HashMap;
+import java.util.Collections;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 
 /**
  * Parts of an {@link EnvironmentGroup} that are needed for analysis. Since {@link EnvironmentGroup}
  * keeps a reference to a {@link Package} object, it is too heavyweight to store in analysis.
+ *
+ * <p>Constructor should only be called by {@link EnvironmentGroup}, and this object must never be
+ * accessed externally until after {@link EnvironmentGroup#processMemberEnvironments} is called. The
+ * mutability of fulfillersMap means that we must take care to wait until it is set before doing
+ * anything with this class.
  */
 @AutoCodec
 public class EnvironmentLabels {
@@ -35,22 +42,52 @@ public class EnvironmentLabels {
   final ImmutableSet<Label> defaults;
   /**
    * Maps a member environment to the set of environments that directly fulfill it. Note that we
-   * can't populate this map until all Target instances for member environments have been
-   * initialized, which may occur after group instantiation (this makes the class mutable).
+   * can't set this map until all Target instances for member environments have been initialized,
+   * which occurs after group instantiation (this makes the class mutable).
    */
-  final Map<Label, NestedSet<Label>> fulfillersMap = new HashMap<>();
+  private Map<Label, NestedSet<Label>> fulfillersMap = null;
 
   EnvironmentLabels(Label label, Collection<Label> environments, Collection<Label> defaults) {
+    this(label, environments, defaults, null);
+  }
+
+  /**
+   * Only for use by serialization: the mutable fulfillersMap object is not properly initialized
+   * otherwise during deserialization.
+   */
+  @AutoCodec.VisibleForSerialization
+  @AutoCodec.Instantiator
+  EnvironmentLabels(
+      Label label,
+      Collection<Label> environments,
+      Collection<Label> defaults,
+      Map<Label, NestedSet<Label>> fulfillersMap) {
     this.label = label;
     this.environments = ImmutableSet.copyOf(environments);
     this.defaults = ImmutableSet.copyOf(defaults);
+    this.fulfillersMap = fulfillersMap;
+  }
+
+  void assertNotInitialized() {
+    Preconditions.checkState(fulfillersMap == null, this);
+  }
+
+  void checkInitialized() {
+    Preconditions.checkNotNull(fulfillersMap, this);
+  }
+
+  void setFulfillersMap(Map<Label, NestedSet<Label>> fulfillersMap) {
+    Preconditions.checkState(this.fulfillersMap == null, this);
+    this.fulfillersMap = Collections.unmodifiableMap(fulfillersMap);
   }
 
   public Set<Label> getEnvironments() {
+    checkInitialized();
     return environments;
   }
 
   public Set<Label> getDefaults() {
+    checkInitialized();
     return defaults;
   }
 
@@ -59,6 +96,7 @@ public class EnvironmentLabels {
    * belong to this group.
    */
   public boolean isDefault(Label environment) {
+    checkInitialized();
     return defaults.contains(environment);
   }
 
@@ -73,10 +111,79 @@ public class EnvironmentLabels {
    * <p>If no environments fulfill the input, returns an empty set.
    */
   public Iterable<Label> getFulfillers(Label environment) {
-    return Verify.verifyNotNull(fulfillersMap.get(environment));
+    checkInitialized();
+    return fulfillersMap.get(environment);
   }
 
   public Label getLabel() {
+    checkInitialized();
     return label;
   }
+
+  @Override
+  public String toString() {
+    return MoreObjects.toStringHelper(this)
+        .add("label", label)
+        .add("sizes", environments.size() + ", " + defaults.size() + ", " + fulfillersMap.size())
+        .add("environments", environments)
+        .add("defaults", defaults)
+        .add("fulfillersMap", fulfillersMap)
+        .toString();
+  }
+
+  @Override
+  public int hashCode() {
+    checkInitialized();
+    return Objects.hash(label, environments, defaults, fulfillersMap.keySet());
+  }
+
+  /**
+   * Compares {@code map1} and {@code map2} using deep equality for their values. Should be feasible
+   * because comparison will usually only happen between == objects, so this is hit rarely. If
+   * objects are equal, but have been deserialized separately so not ==, this should still be ok
+   * because these nested sets are not particularly big, and there are very few EnvironmentGroups
+   * (and therefore EnvironmentLabels) in any given build.
+   *
+   * <p>This will have to be revisited if it turns out to be noticeably expensive. It should be
+   * sound to not compare the values of the fulfillerMaps at all, since they are determined from the
+   * package each EnvironmentLabel is associated with, and so as long as EnvironmentLabels from
+   * different source states but the same package are not compared, the values shouldn't be
+   * necessary.
+   */
+  private static boolean fulfillerMapsEqual(
+      Map<Label, NestedSet<Label>> map1, Map<Label, NestedSet<Label>> map2) {
+    if (map1 == map2) {
+      return true;
+    }
+    if (map1.size() != map2.size()) {
+      return false;
+    }
+    for (Map.Entry<Label, NestedSet<Label>> entry : map1.entrySet()) {
+      NestedSet<Label> secondValue = map2.get(entry.getKey());
+      // Do shallowEquals check first for speed.
+      if (secondValue == null
+          || (!entry.getValue().shallowEquals(secondValue)
+              && !entry.getValue().toCollection().equals(secondValue.toCollection()))) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    checkInitialized();
+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+    EnvironmentLabels that = (EnvironmentLabels) o;
+    that.checkInitialized();
+    return label.equals(that.label)
+        && environments.equals(that.environments)
+        && defaults.equals(that.defaults)
+        && fulfillerMapsEqual(this.fulfillersMap, that.fulfillersMap);
+  }
 }