Conversation
Make rest parameters use `parseBindingAtom()` rather than accepting only identifiers. Also allow array and object patterns when converting spread elements into r>
```
function f(...[x, y]) {}
(...{length}) => length
(...[item, ...rest]) => item
```
We add "scoped" handling for `DestructuringErrors``. We also: - Gives each assignment its own error accumulator. - Merges errors only when the child remains part of the potential pattern. - Discards pattern-only errors at binary, conditional, sequence, unary, call, and member-expression boundaries. - Preserves binding errors while consuming expression errors used by valid defaults.
Parse object spread with a local deferred-error context and convert it to object rest only when the surrounding object becomes a pattern.
This distinguishes the following cases:
```
({...obj,}) // valid object expression
({...rest,} = obj) // invalid assignment pattern
const {...rest,} = obj; // invalid binding pattern
```
We also recursively validate rest operands and reject nested object-rest patterns such as `const {...{x}} = obj`.
Thread deferred-error state through parenthesized expression parsing. Since the eventual context is not yet known, we record separate deferred errors for binding and assignment contexts.
MathiasVP
changed the title
MathiasVP
force-pushed
the
fix-js-parser-bug
branch
from
September 19, 2026 17:41
1075bef to
c5415f1
Compare
MathiasVP
marked this pull request as ready for review
September 20, 2026 09:17
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Core grammar and deferred-error control flow changes require human validation.
Review effort: Balanced
Findings: 1
What changed in this PR
Updates the JavaScript extractor to correctly parse destructuring rest patterns while improving validation of invalid rest and parenthesized patterns.
Changes:
- Refactors deferred parser-error propagation and rest-pattern conversion.
- Adds JavaScript and Flow coverage for valid and invalid patterns.
- Updates affected generated expectations and SSA fixtures.
| File | Description |
|---|---|
| javascript/ql/test/library-tests/SSA/GetRhsNode/tst.js | Disables newly rejected assignment cases. |
| javascript/ql/test/library-tests/SSA/GetRhsNode/GetRhsNode.expected | Updates SSA expectations. |
| javascript/extractor/tests/restprops/output/trap/invalid-sibling-rest-errors.js.trap | Generated sibling-rest error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-rest-trailing-comma-binding.js.trap | Generated binding-comma error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-rest-trailing-comma-assignment.js.trap | Generated assignment-comma error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-rest-parameter-trailing-comma.js.trap | Generated parameter-comma error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-rest-parameter-not-last.js.trap | Generated parameter-order error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-rest-not-last.js.trap | Generated object-rest order expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-parenthesized-update.js.trap | Generated parenthesized-update error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-parenthesized-rest-parameter.js.trap | Generated rest-parameter error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-parenthesized-rest-assignment.js.trap | Generated rest-assignment error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-parenthesized-property-binding.js.trap | Generated property-binding error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-nested-object-rest-binding.js.trap | Generated nested-rest error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-member-rest-parameter.js.trap | Generated member-rest parameter expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-member-binding.js.trap | Generated member-binding error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-for-of-rest-trailing-comma.js.trap | Generated for-of error expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-duplicate-rest-binding.js.trap | Generated duplicate-binding expectation. |
| javascript/extractor/tests/restprops/output/trap/invalid-defaulted-rest-parameter.js.trap | Generated defaulted-rest error expectation. |
| javascript/extractor/tests/restprops/input/rest-patterns.js | Adds valid rest-pattern cases. |
| javascript/extractor/tests/restprops/input/invalid-sibling-rest-errors.js | Tests sibling error isolation. |
| javascript/extractor/tests/restprops/input/invalid-rest-trailing-comma-binding.js | Tests invalid binding comma. |
| javascript/extractor/tests/restprops/input/invalid-rest-trailing-comma-assignment.js | Tests invalid assignment comma. |
| javascript/extractor/tests/restprops/input/invalid-rest-parameter-trailing-comma.js | Tests invalid parameter comma. |
| javascript/extractor/tests/restprops/input/invalid-rest-parameter-not-last.js | Tests non-final rest parameters. |
| javascript/extractor/tests/restprops/input/invalid-rest-not-last.js | Tests non-final object rest. |
| javascript/extractor/tests/restprops/input/invalid-parenthesized-update.js | Tests parenthesized update patterns. |
| javascript/extractor/tests/restprops/input/invalid-parenthesized-rest-parameter.js | Tests parenthesized rest parameters. |
| javascript/extractor/tests/restprops/input/invalid-parenthesized-rest-assignment.js | Tests parenthesized rest assignments. |
| javascript/extractor/tests/restprops/input/invalid-parenthesized-property-binding.js | Tests parenthesized property bindings. |
| javascript/extractor/tests/restprops/input/invalid-nested-object-rest-binding.js | Tests nested object rest. |
| javascript/extractor/tests/restprops/input/invalid-member-rest-parameter.js | Tests member rest parameters. |
| javascript/extractor/tests/restprops/input/invalid-member-binding.js | Tests member binding targets. |
| javascript/extractor/tests/restprops/input/invalid-for-of-rest-trailing-comma.js | Tests for-of rest commas. |
| javascript/extractor/tests/restprops/input/invalid-duplicate-rest-binding.js | Tests duplicate rest bindings. |
| javascript/extractor/tests/restprops/input/invalid-defaulted-rest-parameter.js | Tests defaulted rest parameters. |
| javascript/extractor/tests/flow/output/trap/rest-patterns.js.trap | Generated Flow pattern expectation. |
| javascript/extractor/tests/flow/output/trap/invalid-parenthesized-optional-parameter.js.trap | Generated optional-parameter error expectation. |
| javascript/extractor/tests/flow/output/trap/invalid-parenthesized-array-rest.js.trap | Generated Flow array-rest error expectation. |
| javascript/extractor/tests/flow/output/trap/invalid-async-parenthesized-optional-parameter.js.trap | Generated async optional error expectation. |
| javascript/extractor/tests/flow/output/trap/invalid-async-parenthesized-object-rest.js.trap | Generated async object-rest expectation. |
| javascript/extractor/tests/flow/input/rest-patterns.js | Adds valid Flow pattern cases. |
| javascript/extractor/tests/flow/input/invalid-parenthesized-optional-parameter.js | Tests parenthesized optional parameters. |
| javascript/extractor/tests/flow/input/invalid-parenthesized-array-rest.js | Tests parenthesized Flow array rest. |
| javascript/extractor/tests/flow/input/invalid-async-parenthesized-optional-parameter.js | Tests async optional parameters. |
| javascript/extractor/tests/flow/input/invalid-async-parenthesized-object-rest.js | Tests async object rest. |
| javascript/extractor/tests/es2017/output/trap/invalid-async-fn.js.trap | Updates generated async error output. |
| javascript/extractor/tests/es2015/output/trap/restparms2.js.trap | Updates generated rest-parameter output. |
| javascript/extractor/tests/errors/output/trap/invalid-assignment-pattern.js.trap | Updates generated assignment error output. |
| javascript/extractor/src/com/semmle/jcorn/Parser.java | Refactors pattern errors and rest parsing. |
| javascript/extractor/src/com/semmle/jcorn/ESNextParser.java | Validates object spread/rest conversion. |
| javascript/extractor/src/com/semmle/jcorn/CustomParser.java | Propagates destructuring errors through extensions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| else if (this.strict && operator.equals("delete") && argument instanceof Identifier) | ||
| this.checkExpressionErrors(errors, true); | ||
| if (update) { | ||
| this.checkPatternErrors(errors, false); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes extraction of destructuring rest patterns.
Previously, we rejected valid constructs such as:
We also accepted or misdiagnosed invalid patterns involving object rest, trailing commas, nested patterns, and parentheses.
Commit-by-commit review recommended. I have done my best to split the (very LLM-driven) changes into what I believe are reviewable chunks, but I do admit that: