Clean up a few Skylark-Skyframe structures

Add value-class methods to SkylarkFileDependency and SkylarkImportLookupValue. Remove Java serialization hack from Extension.

RELNOTES: None
PiperOrigin-RevId: 162383283

3 files changed, 73 insertions, 35 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkFileDependency.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkFileDependency.java
index 36a7516e2f..0167aea98e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkFileDependency.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkFileDependency.java
@@ -16,19 +16,20 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.cmdline.Label;
-
-import java.io.Serializable;
+import java.util.Objects;
 
 /**
- * A simple value class to store the direct Skylark file dependencies of a Skylark
- * extension file. It also contains a Label identifying the extension file.
+ * A simple value class to store the direct Skylark file dependencies of a Skylark extension file.
+ * It also contains a Label identifying the extension file.
+ *
+ * <p>The dependency structure must be acyclic.
  */
-class SkylarkFileDependency implements Serializable {
+public class SkylarkFileDependency {
 
   private final Label label;
   private final ImmutableList<SkylarkFileDependency> dependencies;
 
-  SkylarkFileDependency(Label label, ImmutableList<SkylarkFileDependency> dependencies) {
+  public SkylarkFileDependency(Label label, ImmutableList<SkylarkFileDependency> dependencies) {
     this.label = label;
     this.dependencies = dependencies;
   }
@@ -37,14 +38,34 @@ class SkylarkFileDependency implements Serializable {
    * Returns the list of direct Skylark file dependencies of the Skylark extension file
    * corresponding to this object.
    */
-  ImmutableList<SkylarkFileDependency> getDependencies() {
+  public ImmutableList<SkylarkFileDependency> getDependencies() {
     return dependencies;
   }
 
   /**
    * Returns the Label of the Skylark extension file corresponding to this object.
    */
-  Label getLabel() {
+  public Label getLabel() {
     return label;
   }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj) {
+      return true;
+    }
+    if (!(obj instanceof SkylarkFileDependency)) {
+      return false;
+    }
+    SkylarkFileDependency other = (SkylarkFileDependency) obj;
+    if (!label.equals(other.getLabel())) {
+      return false;
+    }
+    return dependencies.equals(other.getDependencies());
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(label, dependencies);
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
index 583e1558dc..4d63f1bdce 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
@@ -102,4 +102,22 @@ public class SkylarkImportLookupValue implements SkyValue {
     return LegacySkyKey.create(
         SkyFunctions.SKYLARK_IMPORTS_LOOKUP, new SkylarkImportLookupKey(importLabel, inWorkspace));
   }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj) {
+      return true;
+    }
+    if (!(obj instanceof SkylarkImportLookupValue)) {
+      return false;
+    }
+    SkylarkImportLookupValue other = (SkylarkImportLookupValue) 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/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 38cbb7c35f..4bec1c6012 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
@@ -29,12 +29,12 @@ import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.SpellChecker;
 import com.google.devtools.common.options.Options;
-import java.io.Serializable;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.TreeSet;
 import javax.annotation.Nullable;
@@ -295,38 +295,20 @@ public final class Environment implements Freezable {
     }
   }
 
-  // TODO(bazel-team): Eliminate this hack around Java serialization. The bindings are currently
-  // factored out into BaseExtension, which is non-Serializable, and which has a default constructor
-  // that does not initialize any bindings. This means that when Extension is Java-serialized, all
-  // the bindings are simply lost.
-  private static class BaseExtension {
-
-    protected final ImmutableMap<String, Object> bindings;
-
-    BaseExtension(Map<String, Object> bindings) {
-      this.bindings = ImmutableMap.copyOf(bindings);
-    }
-
-    // Hack to "allow" java serialization.
-    BaseExtension() {
-      this.bindings = ImmutableMap.of();
-    }
-  }
-
   /** An Extension to be imported with load() into a BUILD or .bzl file. */
   @Immutable
-  public static final class Extension extends BaseExtension implements Serializable {
+  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.
      */
     private final String transitiveContentHashCode;
 
-    /**
-     * Constructs with the given hash code and bindings.
-     */
-    public Extension(Map<String, Object> bindings, String transitiveContentHashCode) {
-      super(bindings);
+    /** Constructs with the given hash code and bindings. */
+    public Extension(ImmutableMap<String, Object> bindings, String transitiveContentHashCode) {
+      this.bindings = bindings;
       this.transitiveContentHashCode = transitiveContentHashCode;
     }
 
@@ -335,8 +317,7 @@ public final class Environment implements Freezable {
      * and that {@code Environment}'s transitive hash code.
      */
     public Extension(Environment env) {
-      super(env.globalFrame.bindings);
-      this.transitiveContentHashCode = env.getTransitiveContentHashCode();
+      this(ImmutableMap.copyOf(env.globalFrame.bindings), env.getTransitiveContentHashCode());
     }
 
     public String getTransitiveContentHashCode() {
@@ -346,6 +327,24 @@ public final class Environment implements Freezable {
     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());
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(bindings, transitiveContentHashCode);
+    }
   }
 
   /**