Draft: Extractors proposal implementation by fs-c · Pull Request #924 · oracle/graaljs

fs-c · 2025-07-23T15:46:35Z

https://github.com/tc39/proposal-extractors

oracle-contributor-agreement · 2025-07-23T15:46:40Z

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

PR author: fs-c
root@fsoc.space (@fs-c)

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.

woess · 2025-07-28T15:25:34Z

+            // 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;


isExtracting seems unnecessary, and extractors may be nested inside

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

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?

can you give an example that you feel won't work? "extractors nested inside a destructuring pattern" sounds like

const subject = new C(new D("data")); const { a: { b: C(D(z)) } } = { a: { b: subject } }

to me, and that works with the current implementation

woess · 2025-07-28T15:28:44Z

                return false;
            }

+            // todo-lw: this is duplicate code


seems like it could be moved to VerifyDestructuringPatternNodeVisitor then.

woess · 2025-07-28T15:35:04Z

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


could probably just use arg.accept(this)

woess · 2025-07-28T15:40:58Z

        return false;
    }

+    private boolean isExtractorLhs(Expression lhs) {


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).

i merged it but it ended up being used in a bunch of places and i'm a bit worried that there might now be some unintended behavior, so i'd feel safer keeping them separate

introduced a feature flag, disabled by default. i also split the js test cases into a different suite, since i didn't want to enable the flag for the entire js suite.

woess · 2025-07-28T16:42:36Z

+
+    const subject = new C(1, 2, 3);
+
+    const C(x, ...y) = subject;


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.

renamed to assignment-binding.js to hopefully make it clearer, and both should now be checked for each case

woess · 2025-07-28T16:50:50Z

+            receiver = transform(accessNode.getBase());
+        }
+
+        final var invokeCustomMatcherOrThrowNode = InvokeCustomMatcherOrThrowNode.create(context, function, assignedValue, receiver);


Please wrap this in a NodeFactory method.

woess · 2025-07-28T16:59:06Z

+        final var invokeCustomMatcherOrThrowNode = InvokeCustomMatcherOrThrowNode.create(context, function, assignedValue, receiver);
+
+        final var args = fakeCallNode.getArgs();
+        VarRef valueTempVar = environment.createTempVar();


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.

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.

nevermind, i initially misunderstood you, yeah i think that this should still behave like a normal assignment in that regard; will also add tests

woess · 2025-07-28T17:08:02Z

+            // todo-lw: this is duplicate code
+            @Override
+            public boolean enterCallNode(CallNode callNode) {
+                if (callNode.isParenthesized()) {


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

can you give an example of how to produce a non-parenthesized CallNode that wouldn't be valid here? i might simply be missing something obvious here, but i don't see how the parser could do better than this

I forgot what I had in mind when I wrote the comment, but new expressions, import and super calls, and tagged template literals are all represented as CallNode, but not valid here. Also, please check the interaction with isWebCompatAssignmentTargetType() and whether the argument checks are strict enough.

woess · 2025-07-31T00:52:40Z

-            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


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

what i was trying (and failing...) to write is that the assignmentExpression also uses conditionalExpression as the lhs expression (which seems weird), but that's why i used it there

does that chnge your opinion that it's too broad?

i narrowed it down to leftHandSideExpression now and added a check for the remaining case

i don't love it, but i tried to implement it 'properly' and i ran into a bunch of issues. i'd like to discuss that with you before trying again, if you want me to continue refining this.

woess · 2025-08-06T13:06:06Z

btw, ExtractorAssignmentPattern and ExtractorBindingPattern are defined as:
ExtractorMemberExpression [no LineTerminator here] ( … )

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.
I don't see this case being handled currently.

fs-c · 2025-10-13T20:06:33Z

btw: I signed the contributor agreement, but it's pending confirmation

woess · 2025-10-20T01:43:59Z

btw, I see some merge conflicts; you will need to rebase the PR on latest master.

- add feature flag - restructure tests accordingly - add factory method for InvokeCustomMatcherOrThrowNode - resolve some todos (better error msgs, ...) - ...

…gets" see tc39/proposal-extractors#34

… suite

oracle-contributor-agreement Bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Jul 23, 2025

woess reviewed Jul 28, 2025

View reviewed changes

woess reviewed Jul 31, 2025

View reviewed changes

fs-c force-pushed the lw/extractors branch from bdd4888 to 6f13441 Compare October 13, 2025 20:04

fs-c force-pushed the lw/extractors branch from 6f13441 to b9537d3 Compare November 8, 2025 13:39

fs-c added 6 commits November 24, 2025 18:06

Initial extractors proposal implementation

548ae96

Address some MR comments

7445aa2

- add feature flag - restructure tests accordingly - add factory method for InvokeCustomMatcherOrThrowNode - resolve some todos (better error msgs, ...) - ...

Address overlap with "Runtime Errors for Function Call Assignment Tar…

80eb7a9

…gets" see tc39/proposal-extractors#34

Introduce extractors context option, move tests back into standard js…

c1e9097

… suite

Fix Extractors in regular function parameters

b05296a

Restructure tests to better fit thesis descriptions, fix found bugs

92779df

fs-c force-pushed the lw/extractors branch from b9537d3 to 92779df Compare December 6, 2025 22:36

Conversation

fs-c commented Jul 23, 2025

Uh oh!

oracle-contributor-agreement Bot commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fs-c Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woess commented Aug 6, 2025

Uh oh!

fs-c commented Oct 13, 2025

Uh oh!

woess commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fs-c Oct 12, 2025 •

edited

Loading