Skip to content

Commit

Permalink
Eagerly check listing/mapping in iterables (#752)
Browse files Browse the repository at this point in the history
There is currently a bug around resolving variables within the iterable of a for
generator or spread syntax (#741)

Normally, mappings/listings are type-checked lazily. However, this results in the said
bug getting widened, for any object members declared in the iterable.

As a workaround for now, prevent the bug from being any worse by ensuring that these
object members are eagerly typechecked.
  • Loading branch information
bioball authored Oct 31, 2024
1 parent 1be1fe4 commit 66d751f
Show file tree
Hide file tree
Showing 10 changed files with 126 additions and 11 deletions.
7 changes: 7 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/ast/VmModifier.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ private VmModifier() {}

public static final int GLOB = 0x1000;

// To be removed when https://github.com/apple/pkl/issues/741 is fixed
public static final int IS_IN_ITERABLE = 0x100000;

// modifier sets

public static final int NONE = 0;
Expand Down Expand Up @@ -134,6 +137,10 @@ public static boolean isEntry(int modifiers) {
return (modifiers & ENTRY) != 0;
}

public static boolean isInIterable(int modifiers) {
return (modifiers & IS_IN_ITERABLE) != 0;
}

public static boolean isType(int modifiers) {
return (modifiers & (CLASS | TYPE_ALIAS | IMPORT)) != 0 && (modifiers & GLOB) == 0;
}
Expand Down
32 changes: 24 additions & 8 deletions pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java
Original file line number Diff line number Diff line change
Expand Up @@ -900,8 +900,12 @@ public GeneratorMemberNode visitObjectEntry(ObjectEntryContext ctx) {

@Override
public GeneratorMemberNode visitObjectSpread(ObjectSpreadContext ctx) {
return GeneratorSpreadNodeGen.create(
createSourceSection(ctx), visitExpr(ctx.expr()), ctx.QSPREAD() != null);
var scope = symbolTable.getCurrentScope();
var visitingIterable = scope.isVisitingIterable();
scope.setVisitingIterable(true);
var expr = visitExpr(ctx.expr());
scope.setVisitingIterable(visitingIterable);
return GeneratorSpreadNodeGen.create(createSourceSection(ctx), expr, ctx.QSPREAD() != null);
}

private void insertWriteForGeneratorVarsToFrameSlotsNode(@Nullable MemberNode memberNode) {
Expand Down Expand Up @@ -992,7 +996,11 @@ public GeneratorForNode visitForGenerator(ForGeneratorContext ctx) {
ignoreT1 ? null : visitTypeAnnotation(ctx.t1.typedIdentifier().typeAnnotation());
}

var scope = symbolTable.getCurrentScope();
var visitingIterable = scope.isVisitingIterable();
scope.setVisitingIterable(true);
var iterableNode = visitExpr(ctx.e);
scope.setVisitingIterable(visitingIterable);
var memberNodes = doVisitForWhenBody(ctx.objectBody());
if (keyVariableSlot != -1) {
currentScope.popForGeneratorVariable();
Expand Down Expand Up @@ -1197,11 +1205,15 @@ private ObjectMember doVisitObjectElement(ObjectElementContext ctx) {
scope -> {
var elementNode = visitExpr(ctx.expr());

var modifier =
scope.isVisitingIterable()
? VmModifier.ELEMENT | VmModifier.IS_IN_ITERABLE
: VmModifier.ELEMENT;
var member =
new ObjectMember(
createSourceSection(ctx),
elementNode.getSourceSection(),
VmModifier.ELEMENT,
modifier,
null,
scope.getQualifiedName());

Expand Down Expand Up @@ -1252,13 +1264,13 @@ private Function<EntryScope, Pair<ExpressionNode, ObjectMember>> objectMemberIns
@Nullable ExprContext valueCtx,
List<? extends ObjectBodyContext> objectBodyCtxs) {
return scope -> {
var modifier =
scope.isVisitingIterable()
? VmModifier.ENTRY | VmModifier.IS_IN_ITERABLE
: VmModifier.ENTRY;
var member =
new ObjectMember(
sourceSection,
keyNode.getSourceSection(),
VmModifier.ENTRY,
null,
scope.getQualifiedName());
sourceSection, keyNode.getSourceSection(), modifier, null, scope.getQualifiedName());

if (valueCtx != null) { // ["key"] = value
var valueNode = visitExpr(valueCtx);
Expand Down Expand Up @@ -1338,6 +1350,10 @@ private int doVisitModifiers(
result += modifier;
}

if (symbolTable.getCurrentScope().isVisitingIterable()) {
result += VmModifier.IS_IN_ITERABLE;
}

// flag modifier combinations that are never valid right away

if (VmModifier.isExternal(result) && !ModuleKeys.isStdLibModule(moduleKey)) {
Expand Down
10 changes: 10 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/ast/builder/SymbolTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ public abstract static class Scope {
private int entryCount = 0;
private final FrameDescriptor.Builder frameDescriptorBuilder;
private final ConstLevel constLevel;
private boolean isVisitingIterable;

private Scope(
@Nullable Scope parent,
Expand All @@ -187,6 +188,7 @@ private Scope(
parent != null && parent.constLevel.biggerOrEquals(constLevel)
? parent.constLevel
: constLevel;
this.isVisitingIterable = parent != null && parent.isVisitingIterable;
}

public final @Nullable Scope getParent() {
Expand Down Expand Up @@ -337,6 +339,14 @@ public final boolean isLexicalScope() {
public ConstLevel getConstLevel() {
return constLevel;
}

public void setVisitingIterable(boolean isVisitingIterable) {
this.isVisitingIterable = isVisitingIterable;
}

public boolean isVisitingIterable() {
return isVisitingIterable;
}
}

private interface LexicalScope {}
Expand Down
17 changes: 17 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/ast/member/Member.java
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,21 @@ public final boolean isConstOrFixed() {
public final boolean isLocalOrExternalOrAbstract() {
return VmModifier.isLocalOrExternalOrAbstract(modifiers);
}

/**
* Tells if this member is declared inside the iterable of a for-generator, or an object spread.
* <p>
* This is {@code true} for {@code new {}} within:
*
* <pre>
* {@code
* for (x in new Listing { new {} }) {
* ^^^^^^
* // etc
* }
* </pre>
*/
public boolean isInIterable() {
return VmModifier.isInIterable(modifiers);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.pkl.core.util.Nullable;

public final class ObjectMember extends Member {

@CompilationFinal private @Nullable Object constantValue;
@CompilationFinal private @Nullable MemberNode memberNode;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,25 @@ public String getName() {
return qualifiedPropertyName;
}

private boolean isInIterable(VirtualFrame frame) {
var args = frame.getArguments();
return args.length >= 4 && args[3] instanceof Boolean b && b;
}

@Override
public Object execute(VirtualFrame frame) {
try {
if (isInIterable(frame)) {
// There is currently a bug around resolving variables within the iterable of a for
// generator or spread syntax (https://github.com/apple/pkl/issues/741)
//
// Normally, mappings/listings are type-checked lazily. However, this results in said
// bug getting widened, for any object members declared in the iterable.
//
// As a workaround for now, prevent the bug from being any worse by ensuring that these
// object members are eagerly typechecked.
return typeNode.executeEagerly(frame, frame.getArguments()[2]);
}
return typeNode.execute(frame, frame.getArguments()[2]);
} catch (VmTypeMismatchException e) {
CompilerDirectives.transferToInterpreter();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ protected Object evalTypedObjectCached(

// TODO: propagate SUPER_CALL_MARKER to disable constraint (but not type) check
if (callNode != null && VmUtils.shouldRunTypeCheck(frame)) {
return callNode.call(VmUtils.getReceiverOrNull(frame), property.getOwner(), result);
return callNode.call(
VmUtils.getReceiverOrNull(frame), property.getOwner(), result, member.isInIterable());
}

return result;
Expand All @@ -75,7 +76,8 @@ protected Object eval(
typeAnnNode.getCallTarget(),
VmUtils.getReceiverOrNull(frame),
property.getOwner(),
result);
result,
member.isInIterable());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ public Object execute(VirtualFrame frame) {
var propertyValue = executeBody(frame);
if (VmUtils.shouldRunTypeCheck(frame)) {
return typeCheckCallNode.call(
VmUtils.getReceiver(frame), VmUtils.getOwner(frame), propertyValue);
VmUtils.getReceiver(frame),
VmUtils.getOwner(frame),
propertyValue,
member.isInIterable());
}
return propertyValue;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
class Aviary {
birds: Listing<Bird>
}

class Bird {
age: Int
}

function Bird(_age: Int) = new Bird { age = _age }

res1 {
for (i in IntSeq(1, 1)) {
for (j in
new Aviary {
birds {
Bird(i)
}
}.birds
) {
j
}
}
}

res2 {
for (i in IntSeq(1, 1)) {
...new Aviary {
birds {
Bird(i)
}
}.birds
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
res1 {
new {
age = 1
}
}
res2 {
new {
age = 1
}
}

0 comments on commit 66d751f

Please sign in to comment.