Skip to content

Draft: Extractors proposal implementation #924

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Draft: Extractors proposal implementation #924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 53 additions & 5 deletions graal-js/src/com.oracle.js.parser/src/com/oracle/js/parser/Parser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ private Expression verifyAssignment(final long op, final Expression lhs, final E
throw invalidLHSError(lhs);
}
break;
} else if ((opType == ASSIGN || opType == ASSIGN_INIT) && isDestructuringLhs(lhs) && (inPatternPosition || !lhs.isParenthesized())) {
} else if ((opType == ASSIGN || opType == ASSIGN_INIT) && (isDestructuringLhs(lhs) || isExtractorLhs(lhs)) && (inPatternPosition || !lhs.isParenthesized())) {
verifyDestructuringAssignmentPattern(lhs, CONTEXT_ASSIGNMENT_TARGET);
break;
} else {
Expand All @@ -1067,6 +1067,11 @@ private boolean isDestructuringLhs(Expression lhs) {
return false;
}

private boolean isExtractorLhs(Expression lhs) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be merged into isDestructuringLhs, or are there any cases where you don't want to accept both?

In any case, we need to add a feature flag that enables extractors (disabled by default, enabled via an option).

// todo-lw: call node :(
return lhs instanceof CallNode;
}

private void verifyDestructuringAssignmentPattern(Expression pattern, String contextString) {
assert pattern instanceof ObjectNode || pattern instanceof ArrayLiteralNode;
pattern.accept(new VerifyDestructuringPatternNodeVisitor(new LexicalContext()) {
Expand Down Expand Up @@ -1103,6 +1108,26 @@ public boolean enterIndexNode(IndexNode indexNode) {
return false;
}

@Override
public boolean enterCallNode(CallNode callNode) {
if (callNode.isParenthesized()) {
throw error(AbstractParser.message(MSG_INVALID_LVALUE), callNode.getToken());
}
// todo-lw: surely there is a better way to do this
for (final var arg : callNode.getArgs()) {
if (arg instanceof IdentNode) {
enterIdentNode((IdentNode) arg);
} else if (arg instanceof LiteralNode<?>) {
enterLiteralNode((LiteralNode<?>) arg);
} else if (arg instanceof ObjectNode) {
enterObjectNode((ObjectNode) arg);
} else {
enterDefault(arg);
}
}
return false;
}

@Override
protected boolean enterDefault(Node node) {
throw error(String.format("unexpected node in AssignmentPattern: %s", node));
Expand Down Expand Up @@ -2475,9 +2500,12 @@ private ForVariableDeclarationListResult variableDeclarationList(TokenType varTy
final int varLine = line;
final long varToken = Token.recast(token, varType);

// Get name of var.
final Expression binding = bindingIdentifierOrPattern(yield, await, CONTEXT_VARIABLE_NAME);
final boolean isDestructuring = !(binding instanceof IdentNode);
// Get left hand side.
// todo-lw: conditionalExpression feels way too broad here, but binding also uses it so idk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditionalExpression is indeed too broad here. not sure what you mean with "binding also uses it"?

final Expression binding = conditionalExpression(true, yield, await, CoverExpressionError.DENY);

final boolean isExtracting = binding instanceof CallNode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isExtracting seems unnecessary, and extractors may be nested inside

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have a proposed change here? not sure why it seems unnecessary, and don't know why extractors being nested is relevant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something but it seems you could just use isDestructuring and handle extractors in the same code paths, couldn't you?

why extractors being nested is relevant

Unless isExtracting is really supposed to be shallow, it won't be true for extractors nested inside a destructuring pattern. I just don't understand why you'd want such a check?

final boolean isDestructuring = !(binding instanceof IdentNode) && !isExtracting;
if (isDestructuring) {
final int finalVarFlags = varFlags | VarNode.IS_DESTRUCTURING;
verifyDestructuringBindingPattern(binding, new Consumer<IdentNode>() {
Expand Down Expand Up @@ -2525,7 +2553,7 @@ public void accept(IdentNode identNode) {
// else, if we are in a for loop, delay checking until we know the kind of loop
}

if (!isDestructuring) {
if (!isDestructuring && !isExtracting) {
assert init != null || varType != CONST || !isStatement;
final IdentNode ident = (IdentNode) binding;
if (varType != VAR && ident.getName().equals(LET.getName())) {
Expand Down Expand Up @@ -2835,6 +2863,26 @@ public boolean enterIdentNode(IdentNode identNode) {
return false;
}

// todo-lw: this is duplicate code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like it could be moved to VerifyDestructuringPatternNodeVisitor then.

@Override
public boolean enterCallNode(CallNode callNode) {
if (callNode.isParenthesized()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check needs to be stricter; not every non-parenthesized CallNode is valid in this context.

throw error(AbstractParser.message(MSG_INVALID_LVALUE), callNode.getToken());
}
for (final var arg : callNode.getArgs()) {
if (arg instanceof IdentNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could probably just use arg.accept(this)

enterIdentNode((IdentNode) arg);
} else if (arg instanceof LiteralNode<?>) {
enterLiteralNode((LiteralNode<?>) arg);
} else if (arg instanceof ObjectNode) {
enterObjectNode((ObjectNode) arg);
} else {
enterDefault(arg);
}
}
return false;
}

@Override
protected boolean enterDefault(Node node) {
throw error(String.format("unexpected node in BindingPattern: %s", node));
Expand Down
2 changes: 1 addition & 1 deletion graal-js/src/com.oracle.js.parser/src/com/oracle/js/parser/ir/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ public long getToken() {
return token;
}

// on change, we have to replace the entire list, that's we can't simple do ListIterator.set
// on change, we have to replace the entire list, that's we can't simply do ListIterator.set
static <T extends Node> List<T> accept(final NodeVisitor<? extends LexicalContext> visitor, final List<T> list) {
final int size = list.size();
if (size == 0) {
Expand Down
105 changes: 68 additions & 37 deletions .../src/com.oracle.truffle.js.parser/src/com/oracle/truffle/js/parser/GraalJSTranslator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@
import com.oracle.truffle.js.nodes.access.DeclareEvalVariableNode;
import com.oracle.truffle.js.nodes.access.DeclareGlobalNode;
import com.oracle.truffle.js.nodes.access.GetIteratorUnaryNode;
import com.oracle.truffle.js.nodes.access.GetMethodNode;
import com.oracle.truffle.js.nodes.access.GlobalPropertyNode;
import com.oracle.truffle.js.nodes.access.IteratorToArrayNode;
import com.oracle.truffle.js.nodes.access.JSConstantNode;
import com.oracle.truffle.js.nodes.access.JSReadFrameSlotNode;
import com.oracle.truffle.js.nodes.access.JSWriteFrameSlotNode;
Expand Down Expand Up @@ -151,6 +153,7 @@
import com.oracle.truffle.js.nodes.control.SequenceNode;
import com.oracle.truffle.js.nodes.control.StatementNode;
import com.oracle.truffle.js.nodes.control.SuspendNode;
import com.oracle.truffle.js.nodes.extractor.InvokeCustomMatcherOrThrowNode;
import com.oracle.truffle.js.nodes.function.AbstractFunctionArgumentsNode;
import com.oracle.truffle.js.nodes.function.BlockScopeNode;
import com.oracle.truffle.js.nodes.function.EvalNode;
Expand Down Expand Up @@ -2846,7 +2849,12 @@ private JavaScriptNode transformAssignmentImpl(Expression assignmentExpression,
}
// fall through
case IDENT:
assignedNode = transformAssignmentIdent((IdentNode) lhsExpression, assignedValue, binaryOp, returnOldValue, convertLHSToNumeric, initializationAssignment);
// todo-lw: call node :(
if (lhsExpression instanceof CallNode) {
assignedNode = transformAssignmentExtractor((CallNode) lhsExpression, assignedValue, binaryOp, returnOldValue, convertLHSToNumeric, initializationAssignment);
} else {
assignedNode = transformAssignmentIdent((IdentNode) lhsExpression, assignedValue, binaryOp, returnOldValue, convertLHSToNumeric, initializationAssignment);
}
break;
case LBRACKET:
// target[element]
Expand Down Expand Up @@ -2891,40 +2899,38 @@ private JavaScriptNode transformAssignmentIdent(IdentNode identNode, JavaScriptN
rhs = checkMutableBinding(rhs, scopeVar.getName());
}
return scopeVar.createWriteNode(rhs);
} else if (isLogicalOp(binaryOp)) {
assert !convertLHSToNumeric && !returnOldValue && assignedValue != null;
if (constAssignment) {
rhs = checkMutableBinding(rhs, scopeVar.getName());
}
JavaScriptNode readNode = tagExpression(scopeVar.createReadNode(), identNode);
JavaScriptNode writeNode = scopeVar.createWriteNode(rhs);
return factory.createBinary(context, binaryOp, readNode, writeNode);
} else {
if (isLogicalOp(binaryOp)) {
assert !convertLHSToNumeric && !returnOldValue && assignedValue != null;
if (constAssignment) {
rhs = checkMutableBinding(rhs, scopeVar.getName());
}
JavaScriptNode readNode = tagExpression(scopeVar.createReadNode(), identNode);
JavaScriptNode writeNode = scopeVar.createWriteNode(rhs);
return factory.createBinary(context, binaryOp, readNode, writeNode);
// e.g.: lhs *= rhs => lhs = lhs * rhs
// If lhs is a side-effecting getter that deletes lhs, we must not throw a
// ReferenceError at the lhs assignment since the lhs reference is already resolved.
// We also need to ensure that HasBinding is idempotent or evaluated at most once.
Pair<Supplier<JavaScriptNode>, UnaryOperator<JavaScriptNode>> pair = scopeVar.createCompoundAssignNode();
JavaScriptNode readNode = tagExpression(pair.getFirst().get(), identNode);
if (convertLHSToNumeric) {
readNode = factory.createToNumericOperand(readNode);
}
VarRef prevValueTemp = null;
if (returnOldValue) {
prevValueTemp = environment.createTempVar();
readNode = prevValueTemp.createWriteNode(readNode);
}
JavaScriptNode binOpNode = tagExpression(factory.createBinary(context, binaryOp, readNode, rhs), identNode);
if (constAssignment) {
binOpNode = checkMutableBinding(binOpNode, scopeVar.getName());
}
JavaScriptNode writeNode = pair.getSecond().apply(binOpNode);
if (returnOldValue) {
return factory.createDual(context, writeNode, prevValueTemp.createReadNode());
} else {
// e.g.: lhs *= rhs => lhs = lhs * rhs
// If lhs is a side-effecting getter that deletes lhs, we must not throw a
// ReferenceError at the lhs assignment since the lhs reference is already resolved.
// We also need to ensure that HasBinding is idempotent or evaluated at most once.
Pair<Supplier<JavaScriptNode>, UnaryOperator<JavaScriptNode>> pair = scopeVar.createCompoundAssignNode();
JavaScriptNode readNode = tagExpression(pair.getFirst().get(), identNode);
if (convertLHSToNumeric) {
readNode = factory.createToNumericOperand(readNode);
}
VarRef prevValueTemp = null;
if (returnOldValue) {
prevValueTemp = environment.createTempVar();
readNode = prevValueTemp.createWriteNode(readNode);
}
JavaScriptNode binOpNode = tagExpression(factory.createBinary(context, binaryOp, readNode, rhs), identNode);
if (constAssignment) {
binOpNode = checkMutableBinding(binOpNode, scopeVar.getName());
}
JavaScriptNode writeNode = pair.getSecond().apply(binOpNode);
if (returnOldValue) {
return factory.createDual(context, writeNode, prevValueTemp.createReadNode());
} else {
return writeNode;
}
return writeNode;
}
}
}
Expand Down Expand Up @@ -3054,14 +3060,19 @@ private JavaScriptNode transformIndexAssignment(IndexNode indexNode, JavaScriptN
}

private JavaScriptNode transformDestructuringArrayAssignment(Expression lhsExpression, JavaScriptNode assignedValue, boolean initializationAssignment) {
VarRef valueTempVar = environment.createTempVar();
JavaScriptNode initValue = valueTempVar.createWriteNode(assignedValue);
JavaScriptNode getIterator = factory.createGetIterator(initValue);
LiteralNode.ArrayLiteralNode arrayLiteralNode = (LiteralNode.ArrayLiteralNode) lhsExpression;
List<Expression> elementExpressions = arrayLiteralNode.getElementExpressions();

return this.transformDestructuringArrayAssignment(elementExpressions, getIterator, valueTempVar, initializationAssignment);
}

private JavaScriptNode transformDestructuringArrayAssignment(List<Expression> elementExpressions, JavaScriptNode getIterator, VarRef valueTempVar, boolean initializationAssignment) {
JavaScriptNode[] initElements = javaScriptNodeArray(elementExpressions.size());
VarRef iteratorTempVar = environment.createTempVar();
VarRef valueTempVar = environment.createTempVar();
JavaScriptNode initValue = valueTempVar.createWriteNode(assignedValue);
// By default, we use the hint to track the type of iterator.
JavaScriptNode getIterator = factory.createGetIterator(initValue);
JavaScriptNode initIteratorTempVar = iteratorTempVar.createWriteNode(getIterator);

for (int i = 0; i < elementExpressions.size(); i++) {
Expand All @@ -3083,7 +3094,8 @@ private JavaScriptNode transformDestructuringArrayAssignment(Expression lhsExpre
if (init != null) {
rhsNode = factory.createNotUndefinedOr(rhsNode, transform(init));
}
if (lhsExpr != null && lhsExpr.isTokenType(TokenType.SPREAD_ARRAY)) {
// todo-lw: this change is kind of sus
if (lhsExpr != null && (lhsExpr.isTokenType(TokenType.SPREAD_ARRAY) || lhsExpr.isTokenType(TokenType.SPREAD_ARGUMENT))) {
rhsNode = factory.createIteratorToArray(context, iteratorTempVar.createReadNode());
lhsExpr = ((UnaryNode) lhsExpr).getExpression();
}
Expand All @@ -3097,6 +3109,25 @@ private JavaScriptNode transformDestructuringArrayAssignment(Expression lhsExpre
return factory.createExprBlock(initIteratorTempVar, closeIfNotDone, valueTempVar.createReadNode());
}

private JavaScriptNode transformAssignmentExtractor(CallNode fakeCallNode, JavaScriptNode assignedValue, BinaryOperation binaryOp, boolean returnOldValue, boolean convertToNumeric, boolean initializationAssignment) {
// todo-lw: call node :(

final var functionExpr = fakeCallNode.getFunction();
final var function = transform(functionExpr);

var receiver = function;
if (functionExpr instanceof AccessNode) {
final AccessNode accessNode = (AccessNode) functionExpr;
receiver = transform(accessNode.getBase());
}

final var invokeCustomMatcherOrThrowNode = InvokeCustomMatcherOrThrowNode.create(context, function, assignedValue, receiver);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap this in a NodeFactory method.


final var args = fakeCallNode.getArgs();
VarRef valueTempVar = environment.createTempVar();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valueTempVar seems to be not assigned here? I presume this (Extractor() = x) should still return x, right? we should also add a test for this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding is that this is not the case, no. it's just a leftover from when i factored out the destructuring code, will remove it.

return this.transformDestructuringArrayAssignment(args, invokeCustomMatcherOrThrowNode, valueTempVar, initializationAssignment);
}

private JavaScriptNode transformDestructuringObjectAssignment(Expression lhsExpression, JavaScriptNode assignedValue, boolean initializationAssignment) {
ObjectNode objectLiteralNode = (ObjectNode) lhsExpression;
List<PropertyNode> propertyExpressions = objectLiteralNode.getElements();
Expand Down
46 changes: 46 additions & 0 deletions graal-js/src/com.oracle.truffle.js.test/js/extractors/as-default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
load('../assert.js');

const DateExtractor = {
[Symbol.customMatcher](value) {
if (value instanceof Date) {
return [value];
} else if (typeof value === "number") {
return [new Date(value)];
} else if (typeof value === "string") {
return [Date.parse(value)];
}
}
};

class Book {
constructor({
isbn,
title,
// Extract `createdAt` as an Instant
createdAt: DateExtractor(createdAt) = Date.now(),
modifiedAt: DateExtractor(modifiedAt) = createdAt
}) {
this.isbn = isbn;
this.title = title;
this.createdAt = createdAt;
this.modifiedAt = modifiedAt;
}
}

{
const date = Date.parse("1970-01-01T00:00:00Z")
const book = new Book({ isbn: "...", title: "...", createdAt: date });
assertSame(date.valueOf(), book.createdAt.valueOf());
}

{
const msSinceEpoch = 1000;
const book = new Book({ isbn: "...", title: "...", createdAt: msSinceEpoch });
assertSame(msSinceEpoch, book.createdAt.valueOf());
}

{
const createdAt = "1970-01-01T00Z";
const book = new Book({ isbn: "...", title: "...", createdAt });
assertSame(Date.parse(createdAt).valueOf(), book.createdAt.valueOf());
}
Loading