-
Notifications
You must be signed in to change notification settings - Fork 345
Add Object.toMixin() method
#1257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this not be done in userland Pkl?
function dynamicToMixin(mix: Dynamic): Mixin = new { ...mix }It's likely that the "deep" variant of this that recursively turns a nested object structure (and not just a Dynamic!) into a Mixin isn't userland-friendly, but I'm not sure there needs to be an API for the shallow implementation.
Missed the implementation detail that makes this work, never mind :)
pkl-core/src/main/java/org/pkl/core/stdlib/base/DynamicNodes.java
Outdated
Show resolved
Hide resolved
|
@HT154 I don't believe so due to how amends vs replacements are constructed. Consider this example: local base = new {
a1 {
b1 = 2
}
a2 {
b1 = 2
}
}
local over = new Mixin {
a1 = new Dynamic {
b2 = 2
}
a2 {
b2 = 2
}
}
local overrideValue = new Dynamic {} |> over
function dynamicToMixin(mix: Dynamic): Mixin = new {
...mix
}
actualValue = base |> dynamicToMixin(overrideValue)
expectedValue = base |> overWhich evaluates to actualValue {
a1 {
b2 = 2
}
a2 { // ❗️This is not correct, the original Mixin has amend semantics here, not replace
b2 = 2
}
}
expectedValue {
a1 {
b2 = 2
}
a2 {
b1 = 2
b2 = 2
}
} |
|
Updated to implement on |
HT154
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
pkl-core/src/main/java/org/pkl/core/stdlib/base/ObjectNodes.java
Outdated
Show resolved
Hide resolved
pkl-core/src/main/java/org/pkl/core/stdlib/base/ObjectNodes.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few un-tested cases I can think of that I'd like to see covered:
- Applying mixins with properties/entries to Listings (and other variants of this wrong-member-type error)
- Apply
Mixin<Listing>to aDynamic(and other variants; exceptDynamic |> Mixin<some Typed>which is covered) - Applying an object's mixin to the original object (
obj |> obj.toMixin()) - Applying a mixin to an object that would induce a stack overflow (
new Dynamic { foo = bar } |> new Dynamic { bar = foo }.toMixin())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Actually caught an issue with Listings where it was incorrectly overwriting keys. Now it correctly amends keys, same as explicit Mixin declaration does.
| @CompilerDirectives.TruffleBoundary | ||
| private static org.graalvm.collections.UnmodifiableEconomicMap<Object, ObjectMember> adjustMemberIndices( | ||
| org.graalvm.collections.UnmodifiableEconomicMap<Object, ObjectMember> members, | ||
| long parentLength) { | ||
| if (parentLength == 0) { | ||
| return members; | ||
| } | ||
|
|
||
| var result = org.pkl.core.util.EconomicMaps.<Object, ObjectMember>create( | ||
| org.pkl.core.util.EconomicMaps.size(members)); | ||
|
|
||
| var cursor = members.getEntries(); | ||
| while (cursor.advance()) { | ||
| var key = cursor.getKey(); | ||
| var member = cursor.getValue(); | ||
|
|
||
| // If this is an element (not an entry with an Int key), offset the index | ||
| if (member.isElement()) { | ||
| // Elements always have Long keys | ||
| var newKey = (Long) key + parentLength; | ||
| org.pkl.core.util.EconomicMaps.put(result, newKey, member); | ||
| } else { | ||
| // Properties and entries are not offset | ||
| org.pkl.core.util.EconomicMaps.put(result, key, member); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more FQCNs still hanging around:
| @CompilerDirectives.TruffleBoundary | |
| private static org.graalvm.collections.UnmodifiableEconomicMap<Object, ObjectMember> adjustMemberIndices( | |
| org.graalvm.collections.UnmodifiableEconomicMap<Object, ObjectMember> members, | |
| long parentLength) { | |
| if (parentLength == 0) { | |
| return members; | |
| } | |
| var result = org.pkl.core.util.EconomicMaps.<Object, ObjectMember>create( | |
| org.pkl.core.util.EconomicMaps.size(members)); | |
| var cursor = members.getEntries(); | |
| while (cursor.advance()) { | |
| var key = cursor.getKey(); | |
| var member = cursor.getValue(); | |
| // If this is an element (not an entry with an Int key), offset the index | |
| if (member.isElement()) { | |
| // Elements always have Long keys | |
| var newKey = (Long) key + parentLength; | |
| org.pkl.core.util.EconomicMaps.put(result, newKey, member); | |
| } else { | |
| // Properties and entries are not offset | |
| org.pkl.core.util.EconomicMaps.put(result, key, member); | |
| @TruffleBoundary | |
| private static UnmodifiableEconomicMap<Object, ObjectMember> adjustMemberIndices( | |
| UnmodifiableEconomicMap<Object, ObjectMember> members, | |
| long parentLength) { | |
| if (parentLength == 0) { | |
| return members; | |
| } | |
| var result = EconomicMaps.<Object, ObjectMember>create(EconomicMaps.size(members)); | |
| var cursor = members.getEntries(); | |
| while (cursor.advance()) { | |
| var key = cursor.getKey(); | |
| var member = cursor.getValue(); | |
| // If this is an element (not an entry with an Int key), offset the index | |
| if (member.isElement()) { | |
| // Elements always have Long keys | |
| var newKey = (Long) key + parentLength; | |
| EconomicMaps.put(result, newKey, member); | |
| } else { | |
| // Properties and entries are not offset | |
| EconomicMaps.put(result, key, member); |
| var sourceLength = getObjectLength(sourceObject); | ||
| var adjustedMembers = adjustMemberIndices(sourceObject.getMembers(), parentLength); | ||
|
|
||
| return new VmDynamic( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return the same type as sourceObject, so you'll need to choose the right VmObject subclass to build here. We definitely want this invariant to hold:
local base = new Bird { name = "Parrot"; age = 20 }
local birthday = new Bird { age = super.age + 1 }.toMixin()
local aged = base |> birthday
invariant = base.getClass() == aged.getClass()|
This behavior seems obviously wrong, and is what this implementation currently does. local foo {
a = 1
}
local bar = (foo) {
b = 2
}
res = new Dynamic {} |> bar.toMixin()Produces: res {
b = 2
}However, respecting the amends chain is somewhat problematic for class MyClass {
a: Int
b: Int
}
res: MyClass = new MyClass { a = 1; b = 2 }
|> new MyClass { a = 1 }.toMixin() // would throw "cannot read property `a`" if respecting the amends chainThis feature sounds good in theory, but has some nuances that are hard to resolve. Also, this definitely needs a SPICE to guide the design! |
Adds a toMixin() method to the Object class that converts an Object into a Mixin function applicable via the pipe operator (|>). A generic toMixin() method cannot be implemented in user land, so this implementation provides a native method that properly handles: - Property merging and overriding - Element appending with correct index offsetting - Entry merging with proper key handling - Nested object replacement vs amendment semantics Implementation uses the source Object's enclosing frame to ensure proper module context for type resolution during member evaluation.
Adds a
toMixin()method to theObjectclass that converts anObjectinto a
Mixinfunction applicable via the pipe operator (|>).A generic
toMixin()method cannot be implemented in user land, so thisimplementation provides a native method that properly handles:
Implementation uses the source
Object's enclosing frame to ensureproper module context for type resolution during member evaluation.
edit: updated from
DynamictoObject