Make join call overriden toString() (#1872)

This changes the `join` method in `List`/`Set`/`Listing` to
call the overridden `toString()` method.

This is a breaking change, but is considered a bugfix.

docs/modules/release-notes/pages/0.33.adoc

@@ -82,7 +82,49 @@ bogusMath = (import("pkl:math")) {
 }
 ----
 
-=== XXX
+=== Changes to `join` method on `List`, `Set`, and `Listing`
+
+The `join` method in `List`, `Set`, and `Listing` will now call the `toString()` method if overridden on an object (https://github.com/apple/pkl/pull/1872[#1872]).
+
+This is a breaking change, but is also considered a bugfix.
+
+The following snippet will produce different results:
+
+[source,pkl]
+----
+class Bird {
+  name: String
+
+  function toString() = "{ My name is \(name) }"
+}
+
+hidden birds: Listing<Bird> = new {
+  new { name = "Osprey" }
+  new { name = "Eagle" }
+}
+
+res = birds.join("\n")
+----
+
+Currently, Pkl ignores the `toString()` override, and produces:
+
+[source,pkl]
+----
+res = """
+  new Bird { name = "Osprey" }
+  new Bird { name = "Eagle" }
+  """
+----
+
+In Pkl 0.33, the overridden `toString()` will be called, and produces:
+
+[source,pkl]
+----
+res = """
+  { My name is Osprey }
+  { My name is Eagle }
+  """
+----
 
 == Bug Fixes [small]#🐜#
 

pkl-core/src/main/java/org/pkl/core/ast/internal/ToStringNode.java

@@ -34,6 +34,12 @@ public abstract class ToStringNode extends UnaryExpressionNode {
     super(sourceSection);
   }
 
+  /**
+   * When only using this execute method, pass `null` for `operandNode` to {@link
+   * ToStringNodeGen#create}.
+   */
+  public abstract String executeWith(VirtualFrame frame, Object value);
+
   @Specialization
   protected String evalString(String value) {
     return value;
@@ -74,7 +80,7 @@ public abstract class ToStringNode extends UnaryExpressionNode {
     return (String) callNode.call(value, value.getVmClass().getPrototype());
   }
 
-  @Specialization
+  @Specialization(guards = {"!value.isTyped()", "!value.isReference()"})
   @TruffleBoundary
   protected String evalVmValue(VmValue value) {
     return value.toPklString();
@@ -96,4 +102,9 @@ public abstract class ToStringNode extends UnaryExpressionNode {
     assert toStringMethod != null;
     return DirectCallNode.create(toStringMethod.getCallTarget());
   }
+
+  @Override
+  public final boolean isInstrumentable() {
+    return false;
+  }
 }

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

@@ -160,23 +160,6 @@ public abstract class VmCollection extends VmValue implements Iterable<Object> {
     return builder.build();
   }
 
-  @TruffleBoundary
-  public final String join(String separator) {
-    if (isEmpty()) return "";
-
-    var iter = iterator();
-    var builder = new StringBuilder();
-    // TODO: use ToStringNode
-    builder.append(VmUtils.toPklString(iter.next()));
-
-    while (iter.hasNext()) {
-      builder.append(separator);
-      builder.append(VmUtils.toPklString(iter.next()));
-    }
-
-    return builder.toString();
-  }
-
   @TruffleBoundary
   public final String toPklString() {
     return VmValueRenderer.multiLine(Integer.MAX_VALUE).render(this);

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

@@ -47,6 +47,10 @@ public abstract class VmValue {
     return this instanceof VmTyped;
   }
 
+  public boolean isReference() {
+    return this instanceof VmReference;
+  }
+
   /**
    * Tells if this value is a {@link VmCollection}, {@link VmListing}, or {@link VmDynamic} with
    * {@link VmDynamic#hasElements() elements}.

pkl-core/src/main/java/org/pkl/core/stdlib/base/ListNodes.java

@@ -20,10 +20,13 @@ import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.nodes.LoopNode;
+import com.oracle.truffle.api.profiles.LoopConditionProfile;
 import org.jspecify.annotations.Nullable;
 import org.pkl.core.ast.expression.binary.*;
 import org.pkl.core.ast.internal.IsInstanceOfNode;
 import org.pkl.core.ast.internal.IsInstanceOfNodeGen;
+import org.pkl.core.ast.internal.ToStringNode;
+import org.pkl.core.ast.internal.ToStringNodeGen;
 import org.pkl.core.ast.lambda.*;
 import org.pkl.core.ast.type.TypeNode;
 import org.pkl.core.ast.type.TypeNode.UInt8TypeAliasTypeNode;
@@ -1272,9 +1275,23 @@ public final class ListNodes {
   }
 
   public abstract static class join extends ExternalMethod1Node {
+    @Child ToStringNode toStringNode = ToStringNodeGen.create(sourceSection, null);
+    private final LoopConditionProfile loopConditionProfile = LoopConditionProfile.create();
+
     @Specialization
-    protected String eval(VmList self, String separator) {
-      return self.join(separator);
+    protected String eval(VirtualFrame frame, VmList self, String separator) {
+      if (self.isEmpty()) return "";
+
+      var iter = self.iterator();
+      var builder = VmUtils.createBuilder();
+      VmUtils.appendToBuilder(builder, toStringNode.executeWith(frame, iter.next()));
+
+      while (loopConditionProfile.profile(iter.hasNext())) {
+        VmUtils.appendToBuilder(builder, separator);
+        VmUtils.appendToBuilder(builder, toStringNode.executeWith(frame, iter.next()));
+      }
+
+      return VmUtils.builderToString(builder);
     }
   }
 

pkl-core/src/main/java/org/pkl/core/stdlib/base/ListingNodes.java

@@ -18,9 +18,12 @@ package org.pkl.core.stdlib.base;
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.nodes.IndirectCallNode;
 import com.oracle.truffle.api.nodes.LoopNode;
 import org.pkl.core.ast.PklNode;
+import org.pkl.core.ast.internal.ToStringNode;
+import org.pkl.core.ast.internal.ToStringNodeGen;
 import org.pkl.core.ast.lambda.*;
 import org.pkl.core.runtime.*;
 import org.pkl.core.stdlib.ExternalMethod0Node;
@@ -282,23 +285,30 @@ public final class ListingNodes {
   }
 
   public abstract static class join extends ExternalMethod1Node {
+    @Child ToStringNode toStringNode = ToStringNodeGen.create(sourceSection, null);
+
     @Specialization
-    protected Object eval(VmListing self, String separator) {
+    protected Object eval(VirtualFrame frame, VmListing self, String separator) {
       if (self.isEmpty()) return "";
 
       var builder = new StringBuilder();
+      var materializedFrame = frame.materialize();
       self.forceAndIterateMemberValues(
           (key, member, value) -> {
             if (!key.equals(0L)) {
               builder.append(separator);
             }
-            // TODO: use ToStringNode
-            builder.append(VmUtils.toPklString(value));
+            builder.append(toStringNode.executeWith(materializedFrame, value));
             return true;
           });
       LoopNode.reportLoopCount(this, self.getLength());
       return builder.toString();
     }
+
+    @Override
+    public final boolean isInstrumentable() {
+      return false;
+    }
   }
 
   public abstract static class toList extends ExternalMethod0Node {

pkl-core/src/main/java/org/pkl/core/stdlib/base/SetNodes.java

@@ -20,12 +20,15 @@ import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.nodes.LoopNode;
+import com.oracle.truffle.api.profiles.LoopConditionProfile;
 import org.pkl.core.ast.expression.binary.GreaterThanNode;
 import org.pkl.core.ast.expression.binary.GreaterThanNodeGen;
 import org.pkl.core.ast.expression.binary.LessThanNode;
 import org.pkl.core.ast.expression.binary.LessThanNodeGen;
 import org.pkl.core.ast.internal.IsInstanceOfNode;
 import org.pkl.core.ast.internal.IsInstanceOfNodeGen;
+import org.pkl.core.ast.internal.ToStringNode;
+import org.pkl.core.ast.internal.ToStringNodeGen;
 import org.pkl.core.ast.lambda.*;
 import org.pkl.core.runtime.*;
 import org.pkl.core.stdlib.*;
@@ -1025,9 +1028,23 @@ public final class SetNodes {
   }
 
   public abstract static class join extends ExternalMethod1Node {
+    @Child ToStringNode toStringNode = ToStringNodeGen.create(sourceSection, null);
+    private final LoopConditionProfile loopConditionProfile = LoopConditionProfile.create();
+
     @Specialization
-    protected String eval(VmSet self, String separator) {
-      return self.join(separator);
+    protected String eval(VirtualFrame frame, VmSet self, String separator) {
+      if (self.isEmpty()) return "";
+
+      var iter = self.iterator();
+      var builder = VmUtils.createBuilder();
+      VmUtils.appendToBuilder(builder, toStringNode.executeWith(frame, iter.next()));
+
+      while (loopConditionProfile.profile(iter.hasNext())) {
+        VmUtils.appendToBuilder(builder, separator);
+        VmUtils.appendToBuilder(builder, toStringNode.executeWith(frame, iter.next()));
+      }
+
+      return VmUtils.builderToString(builder);
     }
   }
 

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

@@ -6,6 +6,12 @@ local list3 = List(1, 2, 3, 2, 1)
 local comparator = (x, y) -> x < y
 local comparator1 = (x, y) -> x <= y
 
+local class Bird {
+  name: String
+
+  function toString() = "{ The bird named \(name) }"
+}
+
 facts {
   ["isEmpty"] {
     List().isEmpty
@@ -512,5 +518,7 @@ examples {
   ["join()"] {
     List(1.s, 2.s).join(", ")
     List(1.mb, 2.gb).join(", ")
+    List(new Bird { name = "Sally" }, new Bird { name = "Bob" }).join("\n")
+    List(1, true, 1.ms, new Bird { name = "Sally" }, new Bird { name = "Bob" }).join("\n")
   }
 }

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

@@ -1,6 +1,9 @@
 amends "../snippetTest.pkl"
 
-local class Person { name: String }
+local class Person {
+  name: String
+  function toString() = "{ The person named \(name) }"
+}
 
 local empty = new Listing {}
 
@@ -97,7 +100,7 @@ facts {
     !altered.isDistinctBy((it) -> it.getClass())
     !computedIndex.isDistinctBy((it) -> it.getClass())
   }
-  
+
   ["getOrNull"] {
     empty.getOrNull(-1) == null
     empty.getOrNull(0) == null
@@ -107,7 +110,7 @@ facts {
     }
     base.getOrNull(base.length) == null
   }
-  
+
   ["first"] {
     module.catch(() -> empty.first) == "Expected a non-empty Listing."
     base.first == base[0]
@@ -115,7 +118,7 @@ facts {
     altered.first != base[0]
     computedIndex.first == altered.first
   }
-  
+
   ["firstOrNull"] {
     empty.firstOrNull == null
     base.firstOrNull == base[0]
@@ -157,7 +160,7 @@ facts {
     base.every((it) -> !it.name.isBlank)
     !((base) { new { name = "EEEEE" } }).every((it) -> it.name.contains("rot"))
   }
-  
+
   ["any"] {
     base.any((it) -> it.name.contains("rot"))
     !base.any((it) -> it.name.contains("inch"))
@@ -182,7 +185,7 @@ examples {
     derived.length
     altered.length
     computedIndex.length
-    
+
     local elementsAndEntries = (base) {
       new { name = "" }
       [2] { name = "" }
@@ -265,7 +268,6 @@ examples {
     computedIndex.foldIndexed(List(), (i, l, e) -> l.add(Pair(i, e)))
   }
 
-
   local baseNum = new Listing { 1; 2; 3 }
   local baseString = new Listing { "Pigeon"; "Barn Owl"; "Parrot" }
   local derivedString = (baseString) { "Albatross"; "Elf Owl" }
@@ -284,5 +286,10 @@ examples {
     alteredString.join("\n")
     computedIndexString.join("")
     computedIndexString.join("\n")
+    new Listing {
+      1
+      1.ms
+      new Person { name = "Bob" }
+    }.join("\n")
   }
 }

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

@@ -6,6 +6,12 @@ local set2 = Set(1, 2, 3, 4, 5)
 local comparator = (x, y) -> x < y
 local deprecatedComparator = (x, y) -> if (x < y) -1 else if (x == y) 0 else 1
 
+local class Bird {
+  name: String
+
+  function toString() = "{ The bird named \(name) }"
+}
+
 facts {
   ["isEmpty"] {
     Set().isEmpty
@@ -60,8 +66,8 @@ examples {
 
   ["filter()"] {
     set1.filter((x) -> x > 1)
-    set1. filter((x) -> x > 3)
-    module.catch(() -> set1. filter((x) -> "wrong type"))
+    set1.filter((x) -> x > 3)
+    module.catch(() -> set1.filter((x) -> "wrong type"))
   }
 
   ["map()"] {
@@ -232,10 +238,11 @@ examples {
 
   ["toMap()"] {
     set1.toMap((x) -> x, (x) -> 2 * x)
-    local persons = Set(
-      new Dynamic { name = "Pigeon"; age = 40 },
-      new Dynamic { name = "Parrot"; age = 30 }
-    )
+    local persons =
+      Set(
+        new Dynamic { name = "Pigeon"; age = 40 },
+        new Dynamic { name = "Parrot"; age = 30 },
+      )
     persons.toMap((p) -> p.name, (p) -> p)
   }
 
@@ -287,7 +294,8 @@ examples {
 
   ["mapNonNullIndexed()"] {
     Set(1, 2, 3, 4, 4, 3, 2, 1).mapNonNullIndexed((i, n) -> if (n.isOdd) null else n * i)
-    Set(1, 2, 3, 4, null, 4, 3, 2, 1).mapNonNullIndexed((i, n) -> if (n?.isOdd ?? true) null else n * i)
+    Set(1, 2, 3, 4, null, 4, 3, 2, 1)
+      .mapNonNullIndexed((i, n) -> if (n?.isOdd ?? true) null else n * i)
   }
 
   ["flatMapIndexed()"] {
@@ -444,5 +452,7 @@ examples {
   ["join()"] {
     Set(1.s, 2.s).join(", ")
     Set(1.mb, 2.gb).join(", ")
+    Set(new Bird { name = "Sally" }, new Bird { name = "Bob" }).join("\n")
+    Set(1, true, 1.ms, new Bird { name = "Sally" }, new Bird { name = "Bob" }).join("\n")
   }
 }

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

@@ -441,5 +441,16 @@ examples {
   ["join()"] {
     "1.s, 2.s"
     "1.mb, 2.gb"
+    """
+    { The bird named Sally }
+    { The bird named Bob }
+    """
+    """
+    1
+    true
+    1.ms
+    { The bird named Sally }
+    { The bird named Bob }
+    """
   }
 }

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

@@ -562,5 +562,10 @@ examples {
     Barn Owl
     Parrot
     """
+    """
+    1
+    1.ms
+    { The person named Bob }
+    """
   }
 }

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

@@ -380,5 +380,16 @@ examples {
   ["join()"] {
     "1.s, 2.s"
     "1.mb, 2.gb"
+    """
+    { The bird named Sally }
+    { The bird named Bob }
+    """
+    """
+    1
+    true
+    1.ms
+    { The bird named Sally }
+    { The bird named Bob }
+    """
   }
 }