Draft: Extractors proposal implementation #924
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
the
OCA Required
| // todo-lw: conditionalExpression feels way too broad here, but binding also uses it so idk | ||
| final Expression binding = conditionalExpression(true, yield, await, CoverExpressionError.DENY); | ||
|
|
||
| final boolean isExtracting = binding instanceof CallNode; |
There was a problem hiding this comment.
isExtracting seems unnecessary, and extractors may be nested inside
There was a problem hiding this comment.
do you have a proposed change here? not sure why it seems unnecessary, and don't know why extractors being nested is relevant
There was a problem hiding this comment.
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?
| @@ -2835,6 +2863,26 @@ public boolean enterIdentNode(IdentNode identNode) { | |||
| return false; | |||
| } | |||
|
|
|||
| // todo-lw: this is duplicate code | |||
There was a problem hiding this comment.
seems like it could be moved to VerifyDestructuringPatternNodeVisitor then.
| throw error(AbstractParser.message(MSG_INVALID_LVALUE), callNode.getToken()); | ||
| } | ||
| for (final var arg : callNode.getArgs()) { | ||
| if (arg instanceof IdentNode) { |
There was a problem hiding this comment.
could probably just use arg.accept(this)
| @@ -1067,6 +1067,11 @@ private boolean isDestructuringLhs(Expression lhs) { | |||
| return false; | |||
| } | |||
|
|
|||
| private boolean isExtractorLhs(Expression lhs) { | |||
There was a problem hiding this comment.
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).
|
|
||
| const subject = new C(1, 2, 3); | ||
|
|
||
| const C(x, ...y) = subject; |
There was a problem hiding this comment.
We should have Assignment and Binding patterns for all relevant cases (the file name seems a bit misleading here btw).
Make sure you also test extractors in function/method and arrow function parameters.
| receiver = transform(accessNode.getBase()); | ||
| } | ||
|
|
||
| final var invokeCustomMatcherOrThrowNode = InvokeCustomMatcherOrThrowNode.create(context, function, assignedValue, receiver); |
There was a problem hiding this comment.
Please wrap this in a NodeFactory method.
| final var invokeCustomMatcherOrThrowNode = InvokeCustomMatcherOrThrowNode.create(context, function, assignedValue, receiver); | ||
|
|
||
| final var args = fakeCallNode.getArgs(); | ||
| VarRef valueTempVar = environment.createTempVar(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // todo-lw: this is duplicate code | ||
| @Override | ||
| public boolean enterCallNode(CallNode callNode) { | ||
| if (callNode.isParenthesized()) { |
There was a problem hiding this comment.
This check needs to be stricter; not every non-parenthesized CallNode is valid in this context.
| 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 |
There was a problem hiding this comment.
conditionalExpression is indeed too broad here. not sure what you mean with "binding also uses it"?
|
btw, ExtractorAssignmentPattern and ExtractorBindingPattern are defined as: so IIUC no newline is allowed before the opening parenthesis of an extractor, e.g.: let Extractor(x) = y;is valid but let Extractor
(x) = y;is not. |
https://github.com/tc39/proposal-extractors