Skip to content

Commit

Permalink
Fix apple#823
Browse files Browse the repository at this point in the history
When a Pkl object is instantiated, it captures the current frame's receiver and owner
by storing `frame.materialize()`. Because a materialized frame is a reference to, not a snapshot of,
a virtual frame, this way of capturing receiver and owner is only safe if the virtual frame's
receiver and owner are never mutated.
However, TypeAliasTypeNode does mutate receiver and owner (3d1db25), which is why a
VmListingOrMapping instantiated by ListingOrMappingTypeNode must store them individually
instead of storing `frame.materialize()`.

To rule out that the same problem surfaces elsewhere, it might be a good idea to do one of the following:
* stop mutating receiver and owner
* change the way Pkl objects capture receiver and owner
  • Loading branch information
odenix committed Nov 28, 2024
1 parent fa73895 commit bc10eba
Show file tree
Hide file tree
Showing 9 changed files with 74 additions and 11 deletions.
13 changes: 10 additions & 3 deletions pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -1432,11 +1432,13 @@ public Object execute(VirtualFrame frame, Object value) {
return vmListing;
}
return new VmListing(
frame.materialize(),
vmListing.getEnclosingFrame(),
vmListing,
EconomicMaps.emptyMap(),
vmListing.getLength(),
getValueTypeCastNode());
getValueTypeCastNode(),
VmUtils.getReceiver(frame),
VmUtils.getOwner(frame));
}

@Override
Expand Down Expand Up @@ -1498,7 +1500,12 @@ public Object execute(VirtualFrame frame, Object value) {
return vmMapping;
}
return new VmMapping(
frame.materialize(), vmMapping, EconomicMaps.emptyMap(), getValueTypeCastNode());
vmMapping.getEnclosingFrame(),
vmMapping,
EconomicMaps.emptyMap(),
getValueTypeCastNode(),
VmUtils.getReceiver(frame),
VmUtils.getOwner(frame));
}

@Override
Expand Down
8 changes: 5 additions & 3 deletions pkl-core/src/main/java/org/pkl/core/runtime/VmListing.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public VmListing(
VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members,
int length) {
super(enclosingFrame, parent, members, null);
super(enclosingFrame, parent, members);
this.length = length;
}

Expand All @@ -55,8 +55,10 @@ public VmListing(
VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members,
int length,
ListingOrMappingTypeCastNode typeCastNode) {
super(enclosingFrame, parent, members, typeCastNode);
ListingOrMappingTypeCastNode typeCastNode,
Object typeCheckReceiver,
VmObjectLike typeCheckOwner) {
super(enclosingFrame, parent, members, typeCastNode, typeCheckReceiver, typeCheckOwner);
this.length = length;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,30 @@
public abstract class VmListingOrMapping extends VmObject {
// reified type of listing elements and mapping values
private final @Nullable ListingOrMappingTypeCastNode typeCastNode;
private final @Nullable Object typeCheckReceiver;
private final @Nullable VmObjectLike typeCheckOwner;

public VmListingOrMapping(
MaterializedFrame enclosingFrame,
@Nullable VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members) {
super(enclosingFrame, parent, members);
typeCastNode = null;
typeCheckReceiver = null;
typeCheckOwner = null;
}

public VmListingOrMapping(
MaterializedFrame enclosingFrame,
@Nullable VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members,
@Nullable ListingOrMappingTypeCastNode typeCastNode) {
ListingOrMappingTypeCastNode typeCastNode,
Object typeCheckReceiver,
VmObjectLike typeCheckOwner) {
super(enclosingFrame, parent, members);
this.typeCastNode = typeCastNode;
this.typeCheckReceiver = typeCheckReceiver;
this.typeCheckOwner = typeCheckOwner;
}

public final Object doTypeCast(
Expand All @@ -58,7 +74,7 @@ public final Object doTypeCast(
if (typeCastNode == null || typeCastNode == nextTypeCastNode) return result;
var callTarget = typeCastNode.getCallTarget();
try {
return callNode.call(callTarget, getEnclosingReceiver(), getEnclosingOwner(), result);
return callNode.call(callTarget, typeCheckReceiver, typeCheckOwner, result);
} catch (VmException e) {
CompilerDirectives.transferToInterpreter();
if (member != null) {
Expand Down
8 changes: 5 additions & 3 deletions pkl-core/src/main/java/org/pkl/core/runtime/VmMapping.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,17 @@ public VmMapping(
MaterializedFrame enclosingFrame,
VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members) {
super(enclosingFrame, parent, members, null);
super(enclosingFrame, parent, members);
}

public VmMapping(
MaterializedFrame enclosingFrame,
VmObject parent,
UnmodifiableEconomicMap<Object, ObjectMember> members,
ListingOrMappingTypeCastNode typeCastNode) {
super(enclosingFrame, parent, members, typeCastNode);
ListingOrMappingTypeCastNode typeCastNode,
Object typeCheckReceiver,
VmObjectLike typeCheckOwner) {
super(enclosingFrame, parent, members, typeCastNode, typeCheckReceiver, typeCheckOwner);
}

public static boolean isDefaultProperty(Object propertyKey) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const local lastName = "Birdo"

typealias Birds = Listing<String(endsWith(lastName))>

typealias Birds2 = Pair<Listing<String(endsWith(lastName))>, Int>
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import "pkl:test"

import "helpers/originalTypealias.pkl"

typealias Simple = String
const function simple(arg: Simple): Simple = arg

Expand Down Expand Up @@ -105,3 +107,8 @@ res19: LocalTypeAlias = "abc"
typealias VeryComposite = Pair<Composite, Composite>

res20: VeryComposite = Pair(Map("abc", List("def")), Map("abc", List("def")))

// typealiases should be executed in their original context
res21: originalTypealias.Birds = new { "John Birdo" }

res22: originalTypealias.Birds2 = Pair(new Listing { "John Birdo" }, 0)
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import "helpers/originalTypealias.pkl"

res: originalTypealias.Birds = new { "Jimmy Bird" }
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,9 @@ res18 = "Expected value of type `Duration`, but got type `DataSize`. Value: 5.mb
res18b = "Expected value of type `Duration`, but got type `DataSize`. Value: 5.mb"
res19 = "abc"
res20 = Pair(Map("abc", List("def")), Map("abc", List("def")))
res21 {
"John Birdo"
}
res22 = Pair(new {
"John Birdo"
}, 0)
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
–– Pkl Error ––
Type constraint `endsWith(lastName)` violated.
Value: "Jimmy Bird"

x | typealias Birds = Listing<String(endsWith(lastName))>
^^^^^^^^^^^^^^^^^^
at typeAliasContext#res (file:///$snippetsDir/input/types/helpers/originalTypealias.pkl)

x | res: originalTypealias.Birds = new { "Jimmy Bird" }
^^^^^^^^^^^^
at typeAliasContext#res[#1] (file:///$snippetsDir/input/types/typeAliasContext.pkl)

xxx | text = renderer.renderDocument(value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at pkl.base#Module.output.text (pkl:base)

0 comments on commit bc10eba

Please sign in to comment.