Skip to content

Commit

Permalink
Overhaul implementation of for-generators
Browse files Browse the repository at this point in the history
Motivation:
* fix known bugs and limitations of for-generators
* improve code health by removing complex workarounds

Changes:
* simplify AstBuilder code related to for-generators
  * track for-generators via `SymbolTable.enterForGenerator()`
  * add `RestoreForBindingsNode` during initial AST construction
    instead of calling `MemberNode.replaceBody()` later on
  * simplify some unnecessarily complex code
* remove workarounds and band-aids such as:
  * `isInIterable`
  * `executeAndSetEagerly`
  * adding dummy slots in `AmendFunctionNode`
* overhaul implementation of for-generators
  * store keys and values of for-generator iterations in regular instead of auxiliary frame slots
    * set them via `TypeNode.executeAndSet()`
    * `ResolveVariableNode` no longer needs to search auxiliary slots
    * `Read(Enclosing)AuxiliarySlot` is no longer needed
  * at the start of each for-generator iteration, create a new `VirtualFrame`
    that is a copy of the current frame (arguments + slots)
    and stores the iteration key and value in additional slots.
  * execute for-generator iteration with the newly created frame
    * `childNode.execute(newFrame)`
    * Pkl objects created during the iteration will materialize this frame
  * store newly created frames in `owner.extraStorage`
    if their for-generator slots may be accessed when a generated member is executed
    * resolving variable names to for-generator variables at parse time would make this analysis more precise
  * when a generated member is executed,
	  * retrieve the corresponding frame stored in `owner.extraStorage`
	  * copy the retrieved frame's for-generator slots into slots of the current frame

Result:
* for-generators are implemented in a correct, reasonably simple, and reasonably efficient way
  * complexity is fully contained within package `generator` and `AstBuilder`
* for-generator keys and values can be accessed from all nested scopes:
  * key and value expressions of generated members
  * condition expressions of nested when-generators
  * iterable expressions of nested for-generators
* for-generator keys and values can be accessed from within objects created by the expressions listed above
* sibling for-generators can use the same key/value variable names
* parent/child for-generators can use the same key/value variable names
* fixes apple#741

Limitations not addressed in this PR:
* object spreading is eager in values
  This should be easy to fix.
* for-generators are eager in values
  I think this could be fixed by:
  * resolving variable names to for-generator variables at parse time
  * replacing every access to a for-generator's `value` with `iterable[key]`
* for/when-generator bodies can't have local properties/methods
  I think this could be fixed by:
  * resolving variable names to local properties/methods at parse time
  * internally renaming generated local properties/methods to avoid name clashes
  • Loading branch information
odenix committed Jan 23, 2025
1 parent cdd6d52 commit 43ddb6a
Show file tree
Hide file tree
Showing 57 changed files with 850 additions and 1,086 deletions.
5 changes: 0 additions & 5 deletions pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import com.oracle.truffle.api.frame.FrameDescriptor;
import com.oracle.truffle.api.source.SourceSection;
import java.util.function.Function;
import org.pkl.core.ast.member.DefaultPropertyBodyNode;
import org.pkl.core.runtime.VmExceptionBuilder;
import org.pkl.core.runtime.VmLanguage;
Expand All @@ -43,10 +42,6 @@ public final ExpressionNode getBodyNode() {
return bodyNode;
}

public final void replaceBody(Function<ExpressionNode, ExpressionNode> replacer) {
bodyNode = insert(replacer.apply(bodyNode));
}

protected final VmExceptionBuilder exceptionBuilder() {
return new VmExceptionBuilder().withSourceSection(getHeaderSection());
}
Expand Down
7 changes: 0 additions & 7 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,9 +55,6 @@ 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 @@ -137,10 +134,6 @@ 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
279 changes: 125 additions & 154 deletions pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java

Large diffs are not rendered by default.

115 changes: 72 additions & 43 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 @@ -17,15 +17,14 @@

import com.oracle.truffle.api.frame.FrameDescriptor;
import com.oracle.truffle.api.frame.FrameDescriptor.Builder;
import com.oracle.truffle.api.frame.FrameSlotKind;
import java.util.*;
import java.util.function.Function;
import org.pkl.core.TypeParameter;
import org.pkl.core.ast.ConstantNode;
import org.pkl.core.ast.ExpressionNode;
import org.pkl.core.ast.expression.generator.GeneratorMemberNode;
import org.pkl.core.ast.member.ObjectMember;
import org.pkl.core.parser.Lexer;
import org.pkl.core.parser.antlr.PklParser.ParameterContext;
import org.pkl.core.runtime.Identifier;
import org.pkl.core.runtime.ModuleInfo;
import org.pkl.core.runtime.VmDataSize;
Expand All @@ -35,8 +34,6 @@
public final class SymbolTable {
private Scope currentScope;

public static Object FOR_GENERATOR_VARIABLE = new Object();

public SymbolTable(ModuleInfo moduleInfo) {
currentScope = new ModuleScope(moduleInfo);
}
Expand Down Expand Up @@ -99,6 +96,19 @@ public <T> T enterMethod(
nodeFactory);
}

public <T> T enterForGenerator(
FrameDescriptor.Builder frameDescriptorBuilder,
FrameDescriptor.Builder memberDescriptorBuilder,
Function<ForGeneratorScope, T> nodeFactory) {
return doEnter(
new ForGeneratorScope(
currentScope,
currentScope.qualifiedName,
frameDescriptorBuilder,
memberDescriptorBuilder),
nodeFactory);
}

public <T> T enterLambda(
FrameDescriptor.Builder frameDescriptorBuilder, Function<LambdaScope, T> nodeFactory) {

Expand Down Expand Up @@ -128,9 +138,11 @@ public <T> T enterEntry(
Function<EntryScope, T> nodeFactory) {

var qualifiedName = currentScope.getQualifiedName() + currentScope.getNextEntryName(keyNode);

return doEnter(
new EntryScope(currentScope, qualifiedName, FrameDescriptor.newBuilder()), nodeFactory);
var builder =
currentScope instanceof ForGeneratorScope forScope
? forScope.memberDescriptorBuilder
: FrameDescriptor.newBuilder();
return doEnter(new EntryScope(currentScope, qualifiedName, builder), nodeFactory);
}

public <T> T enterCustomThisScope(Function<CustomThisScope, T> nodeFactory) {
Expand Down Expand Up @@ -166,12 +178,10 @@ public abstract static class Scope {
private final @Nullable Scope parent;
private final @Nullable Identifier name;
private final String qualifiedName;
private final Deque<Identifier> forGeneratorVariables = new ArrayDeque<>();
private int lambdaCount = 0;
private int entryCount = 0;
private final FrameDescriptor.Builder frameDescriptorBuilder;
private final ConstLevel constLevel;
private boolean isVisitingIterable;

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

public final @Nullable Scope getParent() {
Expand All @@ -212,6 +221,30 @@ public FrameDescriptor buildFrameDescriptor() {
return frameDescriptorBuilder.build();
}

/**
* Returns a new descriptor builder that contains the same slots as the current scope's frame
* descriptor.
*/
public FrameDescriptor.Builder newFrameDescriptorBuilder() {
return newFrameDescriptorBuilder(buildFrameDescriptor());
}

/** Returns a new descriptor builder for a {@link GeneratorMemberNode} in the current scope. */
public FrameDescriptor.Builder newForGeneratorMemberDescriptorBuilder() {
return this instanceof ForGeneratorScope forScope
? newFrameDescriptorBuilder(forScope.buildMemberDescriptor())
: FrameDescriptor.newBuilder();
}

private static FrameDescriptor.Builder newFrameDescriptorBuilder(FrameDescriptor descriptor) {
var builder = FrameDescriptor.newBuilder();
for (int i = 0; i < descriptor.getNumberOfSlots(); i++) {
builder.addSlot(
descriptor.getSlotKind(i), descriptor.getSlotName(i), descriptor.getSlotInfo(i));
}
return builder;
}

public @Nullable TypeParameter getTypeParameter(String name) {
return null;
}
Expand Down Expand Up @@ -253,35 +286,11 @@ public int getConstDepth() {
return depth;
}

/**
* Adds the for generator variable to the frame descriptor.
*
* <p>Returns {@code -1} if a for-generator variable already exists with this name.
*/
public int pushForGeneratorVariableContext(ParameterContext ctx) {
var variable = Identifier.localProperty(ctx.typedIdentifier().Identifier().getText());
if (forGeneratorVariables.contains(variable)) {
return -1;
}
var slot =
frameDescriptorBuilder.addSlot(FrameSlotKind.Illegal, variable, FOR_GENERATOR_VARIABLE);
forGeneratorVariables.addLast(variable);
return slot;
}

public void popForGeneratorVariable() {
forGeneratorVariables.removeLast();
}

public Deque<Identifier> getForGeneratorVariables() {
return forGeneratorVariables;
}

private String getNextLambdaName() {
return "<function#" + (++skipLambdaScopes().lambdaCount) + ">";
}

private String getNextEntryName(@Nullable ExpressionNode keyNode) {
protected String getNextEntryName(@Nullable ExpressionNode keyNode) {
if (keyNode instanceof ConstantNode constantNode) {
var value = constantNode.getValue();
if (value instanceof String) {
Expand Down Expand Up @@ -336,16 +345,12 @@ public final boolean isLexicalScope() {
return this instanceof LexicalScope;
}

public ConstLevel getConstLevel() {
return constLevel;
}

public void setVisitingIterable(boolean isVisitingIterable) {
this.isVisitingIterable = isVisitingIterable;
public final boolean isForGeneratorScope() {
return this instanceof ForGeneratorScope;
}

public boolean isVisitingIterable() {
return isVisitingIterable;
public ConstLevel getConstLevel() {
return constLevel;
}
}

Expand Down Expand Up @@ -413,6 +418,30 @@ public LambdaScope(
}
}

public static final class ForGeneratorScope extends Scope implements LexicalScope {
private final FrameDescriptor.Builder memberDescriptorBuilder;

public ForGeneratorScope(
Scope parent,
String qualifiedName,
FrameDescriptor.Builder frameDescriptorBuilder,
FrameDescriptor.Builder memberDescriptorBuilder) {
super(parent, null, qualifiedName, ConstLevel.NONE, frameDescriptorBuilder);
this.memberDescriptorBuilder = memberDescriptorBuilder;
}

public FrameDescriptor buildMemberDescriptor() {
return memberDescriptorBuilder.build();
}

@Override
protected String getNextEntryName(@Nullable ExpressionNode keyNode) {
var parent = getParent();
assert parent != null;
return parent.getNextEntryName(keyNode);
}
}

public static final class PropertyScope extends Scope {
public PropertyScope(
Scope parent,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.oracle.truffle.api.nodes.DirectCallNode;
import com.oracle.truffle.api.source.SourceSection;
import org.pkl.core.ast.ExpressionNode;
import org.pkl.core.ast.builder.SymbolTable.CustomThisScope;
import org.pkl.core.ast.member.FunctionNode;
import org.pkl.core.ast.member.UnresolvedFunctionNode;
import org.pkl.core.runtime.VmFunction;
Expand Down Expand Up @@ -57,7 +56,7 @@ public Object executeGeneric(VirtualFrame frame) {
callNode = insert(DirectCallNode.create(functionNode.getCallTarget()));
if (isCustomThisScope) {
// deferred until execution time s.t. nodes of inlined type aliases get the right frame slot
customThisSlot = VmUtils.findAuxiliarySlot(frame, CustomThisScope.FRAME_SLOT_ID);
customThisSlot = VmUtils.findCustomThisSlot(frame);
}
}

Expand All @@ -71,6 +70,6 @@ public Object executeGeneric(VirtualFrame frame) {

var value = valueNode.executeGeneric(frame);

return callNode.call(function.getThisValue(), function, false, value);
return callNode.call(function.getThisValue(), function, value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,57 +19,50 @@
import com.oracle.truffle.api.dsl.Fallback;
import com.oracle.truffle.api.dsl.ImportStatic;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.VirtualFrame;
import org.pkl.core.ast.member.ObjectMember;
import org.pkl.core.runtime.BaseModule;
import org.pkl.core.runtime.VmClass;
import org.pkl.core.runtime.VmDynamic;
import org.pkl.core.runtime.VmListing;
import org.pkl.core.util.EconomicMaps;

@ImportStatic(BaseModule.class)
public abstract class GeneratorElementNode extends GeneratorMemberNode {
private final ObjectMember element;

protected GeneratorElementNode(ObjectMember element) {
super(element.getSourceSection());
protected GeneratorElementNode(ObjectMember element, boolean needsStoredFrame) {
super(element.getSourceSection(), needsStoredFrame);
this.element = element;
}

@Specialization
@SuppressWarnings("unused")
protected void evalDynamic(VmDynamic parent, ObjectData data) {
addElement(data);
protected void evalDynamic(VirtualFrame frame, VmDynamic parent, ObjectData data) {
data.addElement(frame, element, this);
}

@Specialization
@SuppressWarnings("unused")
protected void evalListing(VmListing parent, ObjectData data) {
addElement(data);
protected void evalListing(VirtualFrame frame, VmListing parent, ObjectData data) {
data.addElement(frame, element, this);
}

@SuppressWarnings("unused")
@Specialization(guards = "parent == getDynamicClass()")
protected void evalDynamicClass(VmClass parent, ObjectData data) {
addElement(data);
protected void evalDynamicClass(VirtualFrame frame, VmClass parent, ObjectData data) {
data.addElement(frame, element, this);
}

@SuppressWarnings("unused")
@Specialization(guards = "parent == getListingClass()")
protected void evalListingClass(VmClass parent, ObjectData data) {
addElement(data);
protected void evalListingClass(VirtualFrame frame, VmClass parent, ObjectData data) {
data.addElement(frame, element, this);
}

@Fallback
@SuppressWarnings("unused")
void fallback(Object parent, ObjectData data) {
void fallback(VirtualFrame frame, Object parent, ObjectData data) {
CompilerDirectives.transferToInterpreter();
throw exceptionBuilder().evalError("objectCannotHaveElement", parent).build();
}

private void addElement(ObjectData data) {
long index = data.length;
EconomicMaps.put(data.members, index, element);
data.length += 1;
data.persistForBindings(index);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@
import org.pkl.core.ast.member.ObjectMember;
import org.pkl.core.runtime.*;
import org.pkl.core.runtime.VmException.ProgramValue;
import org.pkl.core.util.EconomicMaps;

@ImportStatic(BaseModule.class)
public abstract class GeneratorEntryNode extends GeneratorMemberNode {
@Child private ExpressionNode keyNode;
private final ObjectMember member;

protected GeneratorEntryNode(ExpressionNode keyNode, ObjectMember member) {
super(member.getSourceSection());
protected GeneratorEntryNode(
ExpressionNode keyNode, ObjectMember member, boolean needsStoredFrame) {
super(member.getSourceSection(), needsStoredFrame);
this.keyNode = keyNode;
this.member = member;
}
Expand Down Expand Up @@ -84,7 +84,7 @@ void fallback(Object parent, ObjectData data) {

private void addRegularEntry(VirtualFrame frame, ObjectData data) {
var key = keyNode.executeGeneric(frame);
doAdd(key, data);
data.addMember(frame, key, member, this);
}

private void addListingEntry(VirtualFrame frame, ObjectData data, int parentLength) {
Expand All @@ -108,15 +108,6 @@ private void addListingEntry(VirtualFrame frame, ObjectData data, int parentLeng
.build();
}

doAdd(index, data);
}

private void doAdd(Object key, ObjectData data) {
if (EconomicMaps.put(data.members, key, member) != null) {
CompilerDirectives.transferToInterpreter();
throw duplicateDefinition(key, member);
}

data.persistForBindings(key);
data.addMember(frame, index, member, this);
}
}
Loading

0 comments on commit 43ddb6a

Please sign in to comment.