diff --git a/docs/modules/release-notes/pages/0.33.adoc b/docs/modules/release-notes/pages/0.33.adoc index 766e1eb53..75566a830 100644 --- a/docs/modules/release-notes/pages/0.33.adoc +++ b/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 = 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]#🐜# diff --git a/pkl-core/src/main/java/org/pkl/core/ast/internal/ToStringNode.java b/pkl-core/src/main/java/org/pkl/core/ast/internal/ToStringNode.java index da83d23c9..570ca8179 100644 --- a/pkl-core/src/main/java/org/pkl/core/ast/internal/ToStringNode.java +++ b/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; + } } diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmCollection.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmCollection.java index 1639ba6fd..4d4bfbb59 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmCollection.java +++ b/pkl-core/src/main/java/org/pkl/core/runtime/VmCollection.java @@ -160,23 +160,6 @@ public abstract class VmCollection extends VmValue implements Iterable { 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); diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmValue.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmValue.java index 909b889a6..e5002a51c 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmValue.java +++ b/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}. diff --git a/pkl-core/src/main/java/org/pkl/core/stdlib/base/ListNodes.java b/pkl-core/src/main/java/org/pkl/core/stdlib/base/ListNodes.java index c4093fc13..5ff1dd824 100644 --- a/pkl-core/src/main/java/org/pkl/core/stdlib/base/ListNodes.java +++ b/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); } } diff --git a/pkl-core/src/main/java/org/pkl/core/stdlib/base/ListingNodes.java b/pkl-core/src/main/java/org/pkl/core/stdlib/base/ListingNodes.java index 74fc80b34..ad214eb05 100644 --- a/pkl-core/src/main/java/org/pkl/core/stdlib/base/ListingNodes.java +++ b/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 { diff --git a/pkl-core/src/main/java/org/pkl/core/stdlib/base/SetNodes.java b/pkl-core/src/main/java/org/pkl/core/stdlib/base/SetNodes.java index f189fe3fb..ba5b1b98b 100644 --- a/pkl-core/src/main/java/org/pkl/core/stdlib/base/SetNodes.java +++ b/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); } } diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/api/list.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/api/list.pkl index ce7f9df47..c9239572e 100644 --- a/pkl-core/src/test/files/LanguageSnippetTests/input/api/list.pkl +++ b/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") } } diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/api/listing.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/api/listing.pkl index 93ee08257..fc2a2ddad 100644 --- a/pkl-core/src/test/files/LanguageSnippetTests/input/api/listing.pkl +++ b/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") } } diff --git a/pkl-core/src/test/files/LanguageSnippetTests/input/api/set.pkl b/pkl-core/src/test/files/LanguageSnippetTests/input/api/set.pkl index 36b603d17..7321a2939 100644 --- a/pkl-core/src/test/files/LanguageSnippetTests/input/api/set.pkl +++ b/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") } } diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/api/list.pcf b/pkl-core/src/test/files/LanguageSnippetTests/output/api/list.pcf index 1e28f74ae..d930774a2 100644 --- a/pkl-core/src/test/files/LanguageSnippetTests/output/api/list.pcf +++ b/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 } + """ } } diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/api/listing.pcf b/pkl-core/src/test/files/LanguageSnippetTests/output/api/listing.pcf index 3d7845abd..e73846557 100644 --- a/pkl-core/src/test/files/LanguageSnippetTests/output/api/listing.pcf +++ b/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 } + """ } } diff --git a/pkl-core/src/test/files/LanguageSnippetTests/output/api/set.pcf b/pkl-core/src/test/files/LanguageSnippetTests/output/api/set.pcf index d66e6c095..071dc9ead 100644 --- a/pkl-core/src/test/files/LanguageSnippetTests/output/api/set.pcf +++ b/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 } + """ } }