Fix equals/hashCode/hasCachedValue in delegating listings/mappings (#781)

This fixes issues when a mapping/listing delegates to another mapping/listing.

The core issue is that `cachedValues` is not guaranteed to be the complete set
of the object's members; they may be stored on the delegate instead.

Therefore, it's not correct compute hash code and equality based on `cachedValues`.
Instead, it's better to use the `getCachedValue()` method, which has the correct
logic for looking up the existing cached value.

pkl-core/src/main/java/org/pkl/core/runtime/VmListing.java

@@ -142,14 +142,10 @@ public final class VmListing extends VmListingOrMapping<VmListing> {
     force(false);
     other.force(false);
 
-    var cursor = cachedValues.getEntries();
+    for (var i = 0L; i < length; i++) {
-    while (cursor.advance()) {
+      var value = getCachedValue(i);
-      Object key = cursor.getKey();
-      if (key instanceof Identifier) continue;
-
-      var value = cursor.getValue();
       assert value != null;
-      var otherValue = other.getCachedValue(key);
+      var otherValue = other.getCachedValue(i);
       if (!value.equals(otherValue)) return false;
     }
 
@@ -160,16 +156,14 @@ public final class VmListing extends VmListingOrMapping<VmListing> {
   @TruffleBoundary
   public int hashCode() {
     if (cachedHash != 0) return cachedHash;
+    // It's possible that the delegate has already computed its hash code.
+    // If so, we can go ahead and use it.
+    if (delegate != null && delegate.cachedHash != 0) return delegate.cachedHash;
 
     force(false);
     var result = 0;
-    var cursor = cachedValues.getEntries();
+    for (var i = 0L; i < length; i++) {
-
+      var value = getCachedValue(i);
-    while (cursor.advance()) {
-      var key = cursor.getKey();
-      if (key instanceof Identifier) continue;
-
-      var value = cursor.getValue();
       assert value != null;
       result = 31 * result + value.hashCode();
     }

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

@@ -35,7 +35,7 @@ public abstract class VmListingOrMapping<SELF extends VmListingOrMapping<SELF>>
    * A Listing or Mapping typecast creates a new object that contains a new typecheck node, and
    * delegates member lookups to this delegate.
    */
-  private final @Nullable SELF delegate;
+  protected final @Nullable SELF delegate;
 
   private final @Nullable ListingOrMappingTypeCastNode typeCastNode;
   private final MaterializedFrame typeNodeFrame;
@@ -78,6 +78,11 @@ public abstract class VmListingOrMapping<SELF extends VmListingOrMapping<SELF>>
     EconomicMaps.put(cachedMembers, key, objectMember);
   }
 
+  @Override
+  public boolean hasCachedValue(Object key) {
+    return super.hasCachedValue(key) || delegate != null && delegate.hasCachedValue(key);
+  }
+
   @Override
   public @Nullable Object getCachedValue(Object key) {
     var myCachedValue = super.getCachedValue(key);

pkl-core/src/main/java/org/pkl/core/runtime/VmMapping.java

@@ -81,12 +81,16 @@ public final class VmMapping extends VmListingOrMapping<VmMapping> {
 
   @TruffleBoundary
   public VmSet getAllKeys() {
+    if (delegate != null) {
+      return delegate.getAllKeys();
+    }
     synchronized (this) {
       if (__allKeys == null) {
         // building upon parent's `getAllKeys()` should improve at least worst case efficiency
-        var parentKeys = parent instanceof VmMapping mapping ? mapping.getAllKeys() : VmSet.EMPTY;
+        var parentKeys =
+            getParent() instanceof VmMapping mapping ? mapping.getAllKeys() : VmSet.EMPTY;
         var builder = VmSet.builder(parentKeys);
-        for (var cursor = members.getEntries(); cursor.advance(); ) {
+        for (var cursor = getMembers().getEntries(); cursor.advance(); ) {
           var member = cursor.getValue();
           if (!member.isEntry()) continue;
           builder.add(cursor.getKey());
@@ -113,21 +117,6 @@ public final class VmMapping extends VmListingOrMapping<VmMapping> {
     return properties;
   }
 
-  @TruffleBoundary
-  public Map<Object, Object> toMap() {
-    var properties = CollectionUtils.newLinkedHashMap(EconomicMaps.size(cachedValues));
-
-    forceAndIterateMemberValues(
-        (key, prop, value) -> {
-          if (isDefaultProperty(key)) return true;
-
-          properties.put(key, value);
-          return true;
-        });
-
-    return properties;
-  }
-
   @Override
   public void accept(VmValueVisitor visitor) {
     visitor.visitMapping(this);
@@ -144,17 +133,12 @@ public final class VmMapping extends VmListingOrMapping<VmMapping> {
     if (this == obj) return true;
     if (!(obj instanceof VmMapping other)) return false;
 
+    if (getEntryCount() != other.getEntryCount()) return false;
     // could use shallow force, but deep force is cached
     force(false);
     other.force(false);
-    if (getEntryCount() != other.getEntryCount()) return false;
+    for (var key : getAllKeys()) {
-
+      var value = getCachedValue(key);
-    var cursor = cachedValues.getEntries();
-    while (cursor.advance()) {
-      Object key = cursor.getKey();
-      if (key instanceof Identifier) continue;
-
-      var value = cursor.getValue();
       assert value != null;
       var otherValue = other.getCachedValue(key);
       if (!value.equals(otherValue)) return false;
@@ -167,35 +151,26 @@ public final class VmMapping extends VmListingOrMapping<VmMapping> {
   @TruffleBoundary
   public int hashCode() {
     if (cachedHash != 0) return cachedHash;
+    // It's possible that the delegate has already computed its hash code.
+    // If so, we can go ahead and use it.
+    if (delegate != null && delegate.cachedHash != 0) return delegate.cachedHash;
 
     force(false);
     var result = 0;
-    var cursor = cachedValues.getEntries();
+    for (var key : getAllKeys()) {
-
-    while (cursor.advance()) {
-      var key = cursor.getKey();
       if (key instanceof Identifier) continue;
-
+      var value = getCachedValue(key);
-      var value = cursor.getValue();
       assert value != null;
       result += key.hashCode() ^ value.hashCode();
     }
-
     cachedHash = result;
     return result;
   }
 
-  // assumes mapping has been forced
   public int getEntryCount() {
     if (cachedEntryCount != -1) return cachedEntryCount;
-
+    cachedEntryCount = getAllKeys().getLength();
-    var result = 0;
+    return cachedEntryCount;
-    for (var key : cachedValues.getKeys()) {
-      if (key instanceof Identifier) continue;
-      result += 1;
-    }
-    cachedEntryCount = result;
-    return result;
   }
 
   @Override

pkl-core/src/main/java/org/pkl/core/runtime/VmObject.java

@@ -92,7 +92,7 @@ public abstract class VmObject extends VmObjectLike {
   }
 
   @Override
-  public final boolean hasCachedValue(Object key) {
+  public boolean hasCachedValue(Object key) {
     return EconomicMaps.containsKey(cachedValues, key);
   }
 

pkl-core/src/test/files/LanguageSnippetTests/input/api/mapping.pkl

@@ -34,6 +34,12 @@ local empty2 = (empty) {
   local function `_`() = 42
 }
 
+local typedMapping: Mapping<String, String> = new {
+  ["hello"] = "hellooooo"
+}
+
+local delegatingTypedMapping: Mapping<String, String|Int> = typedMapping
+
 facts {
   ["isEmpty"] {
     !base.isEmpty
@@ -56,6 +62,8 @@ facts {
     !empty.containsKey("Pigeon")
     !empty.containsKey("default")
     !empty2.containsKey("Pigeon")
+
+    delegatingTypedMapping.containsKey("hello")
   }
 
   ["containsValue()"] {

pkl-core/src/test/files/LanguageSnippetTests/input/listings/equality.pkl

@@ -23,3 +23,14 @@ res10 = new Listing { "one" } == new Listing { "one"; default = 9 }
 res11 = new Listing { x; local x = "one" } == new Listing { "one" }
 res12 = new Listing { x; local x = "one"; local `_` = "two" } == new Listing { "one"; default = 9 }
 res13 = new Listing { x; local x = "one" } { y; local y = "two" } == new Listing { "one"; "two" }
+
+local class Bird { name: String }
+
+local l1: Listing<Bird> = new {
+  new { name = "Pigeon" }
+  new { name = "Stork" }
+}
+
+local l2: Listing<Bird|Int> = l1
+
+res14 = l1 == l2

pkl-core/src/test/files/LanguageSnippetTests/input/listings/hashCode.pkl

@@ -1,5 +1,9 @@
 amends "../snippetTest.pkl"
 
+local class Bird {
+  name: String
+}
+
 facts {
   local set: Set<Int> = IntSeq(1, 100).fold(Set(), (res, n) -> res.add(n))
   
@@ -23,3 +27,31 @@ facts {
     set.add(listing1).add(listing2).length == 101
   }
 }
+
+examples {
+  ["delegating listings compute correct hash codes"] {
+    local l1: Listing<Bird> = new {
+      new { name = "Pigeon" }
+      new { name = "Stork" }
+    }
+
+    local l2: Listing<Bird|Int> = l1
+
+    // need to add 6 other elements here; `EconomicMap` will back shorter collections
+    // with an array and not compute hash code
+    List(1, 2, 3, 4, 5, 6, l2, l1).distinct
+  }
+
+  ["delegating listings compute correct hash codes -- re-use hash-code"] {
+    local l1: Listing<Bird> = new {
+      new { name = "Pigeon" }
+      new { name = "Stork" }
+    }
+
+    local l2: Listing<Bird|Int> = l1
+
+    // same as the other test but put `l1` first this time (execute code path where we re-use
+    // already computed hashcode)
+    List(1, 2, 3, 4, 5, 6, l1, l2).distinct
+  }
+}

pkl-core/src/test/files/LanguageSnippetTests/input/mappings/equality.pkl

@@ -24,3 +24,14 @@ res10 = new Mapping { ["one"] = 1 } == new Mapping { ["one"] = 1; default = 1 }
 res11 = new Mapping { ["one"] = x; local x = 1 } == new Mapping { ["one"] = 1 }
 res12 = new Mapping { ["one"] = x; local x = 1; local `_` = "two" } == new Mapping { ["one"] = 1; default = 9 }
 res13 = new Mapping { ["one"] = x; local x = 1 } { ["two"] = y; local y = 2 } == new Mapping { ["one"] = 1; ["two"] = 2 }
+
+class Bird { name: String }
+
+local m1: Mapping<String, Bird> = new {
+  ["Pigeon"] = new { name = "Pigeon" }
+  ["Stork"] = new { name = "Stork" }
+}
+
+local m2: Mapping<String, Bird|Int> = m1
+
+res14 = m1 == m2

pkl-core/src/test/files/LanguageSnippetTests/input/mappings/hashCode.pkl

@@ -1,5 +1,7 @@
 amends "../snippetTest.pkl"
 
+local class Bird { name: String }
+
 facts {
   local set: Set<Int> = IntSeq(1, 100).fold(Set(), (res, n) -> res.add(n))
   
@@ -23,3 +25,31 @@ facts {
     set.add(mapping1).add(mapping2).length == 101
   }
 }
+
+examples {
+  ["delegating mappings produce correct hash codes"] {
+    local m1: Mapping<String, Bird> = new {
+      ["Pigeon"] = new { name = "Pigeon" }
+      ["Stork"] = new { name = "Stork" }
+    }
+
+    local m2: Mapping<String, Bird|Int> = m1
+
+    // need to add 6 other elements here; `EconomicMap` will back shorter collections
+    // with an array and not compute hash code
+    List(1, 2, 3, 4, 5, 6, m2, m1).distinct
+  }
+
+  ["delegating mappings produce correct hash codes -- re-use hash-code"] {
+    local m1: Mapping<String, Bird> = new {
+      ["Pigeon"] = new { name = "Pigeon" }
+      ["Stork"] = new { name = "Stork" }
+    }
+
+    local m2: Mapping<String, Bird|Int> = m1
+
+    // same as the other test but put `m1` first this time (execute code path where we re-use
+    // already computed hashcode)
+    List(1, 2, 3, 4, 5, 6, m1, m2).distinct
+  }
+}

pkl-core/src/test/files/LanguageSnippetTests/output/api/mapping.pcf

@@ -17,6 +17,7 @@ facts {
     true
     true
     true
+    true
   }
   ["containsValue()"] {
     true

pkl-core/src/test/files/LanguageSnippetTests/output/listings/equality.pcf

@@ -10,3 +10,4 @@ res10 = true
 res11 = true
 res12 = true
 res13 = true
+res14 = true

pkl-core/src/test/files/LanguageSnippetTests/output/listings/hashCode.pcf

@@ -11,3 +11,25 @@ facts {
     true
   }
 }
+examples {
+  ["delegating listings compute correct hash codes"] {
+    List(1, 2, 3, 4, 5, 6, new {
+      new {
+        name = "Pigeon"
+      }
+      new {
+        name = "Stork"
+      }
+    })
+  }
+  ["delegating listings compute correct hash codes -- re-use hash-code"] {
+    List(1, 2, 3, 4, 5, 6, new {
+      new {
+        name = "Pigeon"
+      }
+      new {
+        name = "Stork"
+      }
+    })
+  }
+}

pkl-core/src/test/files/LanguageSnippetTests/output/mappings/equality.pcf

@@ -11,3 +11,4 @@ res10 = true
 res11 = true
 res12 = true
 res13 = true
+res14 = true

pkl-core/src/test/files/LanguageSnippetTests/output/mappings/hashCode.pcf

@@ -11,3 +11,25 @@ facts {
     true
   }
 }
+examples {
+  ["delegating mappings produce correct hash codes"] {
+    List(1, 2, 3, 4, 5, 6, new {
+      ["Pigeon"] {
+        name = "Pigeon"
+      }
+      ["Stork"] {
+        name = "Stork"
+      }
+    })
+  }
+  ["delegating mappings produce correct hash codes -- re-use hash-code"] {
+    List(1, 2, 3, 4, 5, 6, new {
+      ["Pigeon"] {
+        name = "Pigeon"
+      }
+      ["Stork"] {
+        name = "Stork"
+      }
+    })
+  }
+}