Skip to content

Comments

fix(es/parser): Reject invalid object rest assignment targets#11555

Closed
kdy1 wants to merge 3 commits intomainfrom
codex/fix-issue-11543-rest-assignment-target
Closed

fix(es/parser): Reject invalid object rest assignment targets#11555
kdy1 wants to merge 3 commits intomainfrom
codex/fix-issue-11543-rest-assignment-target

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Feb 18, 2026

Summary

Testing

  • UPDATE=1 cargo test -p swc_ecma_parser --features verify --test errors -- --ignored issue_11543
  • cargo test -p swc_ecma_parser --features verify --test errors -- --ignored issue_11543 arrow_function_binding_rest_property rest_not_last

Fixes #11543

@changeset-bot
Copy link

changeset-bot bot commented Feb 18, 2026

🦋 Changeset detected

Latest commit: 8019ac8

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kdy1 kdy1 changed the title fix(parser): reject invalid object rest assignment targets fix(es/parser): Reject invalid object rest assignment targets Feb 18, 2026
@kdy1 kdy1 marked this pull request as ready for review February 18, 2026 18:18
@kdy1 kdy1 requested review from a team as code owners February 18, 2026 18:18
Copilot AI review requested due to automatic review settings February 18, 2026 18:18
@kdy1 kdy1 enabled auto-merge (squash) February 18, 2026 18:18
@kdy1 kdy1 added this to the Planned milestone Feb 18, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Feb 18, 2026

Merging this PR will not alter performance

✅ 184 untouched benchmarks


Comparing codex/fix-issue-11543-rest-assignment-target (8019ac8) with main (d5c4ebe)

Open in CodSpeed

@claude

This comment has been minimized.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2746209633

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +262 to +266
if !expr.is_valid_simple_assignment_target(
self.ctx().contains(Context::Strict),
) {
self.emit_err(span, SyntaxError::NotSimpleAssign);
expr.into()

Choose a reason for hiding this comment

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

P2 Badge Keep TS1048 for object rest initializers

In the PatType::AssignElement branch for object rest reparsing, invalid targets now emit NotSimpleAssign and return expr.into() directly, which skips the existing Pat::Assign check that reports TS1048. As a result, inputs like ({...a=1}={}) are no longer diagnosed as “A rest parameter cannot have an initializer” (the behavior covered by tests/errors/rest-initializer), so this change regresses parser diagnostics for rest-initializer cases while fixing issue #11543.

Useful? React with 👍 / 👎.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a parser bug where invalid object rest assignment targets like ({...{}} = {}) were incorrectly accepted. The parser now properly rejects non-assignable expressions used as object rest targets in assignment destructuring contexts.

Changes:

  • Added validation to reject invalid object rest assignment targets (object literals, array literals, etc.)
  • Reports "Cannot assign to this" error for invalid cases
  • Added regression test for issue #11543

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
crates/swc_ecma_parser/src/parser/pat.rs Added validation check for PatType::AssignElement in object rest patterns to ensure the target is a valid simple assignment target
crates/swc_ecma_parser/tests/errors/issue-11543/input.js Test input with invalid object rest assignment ({...{}} = {})
crates/swc_ecma_parser/tests/errors/issue-11543/input.js.swc-stderr Expected error output showing "Cannot assign to this" error
.changeset/chilled-apples-impress.md Changeset documenting the parser fix for release notes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kodiakhq[bot]
kodiakhq bot previously approved these changes Feb 18, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 18, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 28M (28574792 bytes)

Commit: a11603a

@kdy1 kdy1 disabled auto-merge February 18, 2026 19:55
@claude
Copy link
Contributor

claude bot commented Feb 18, 2026

PR Review: fix(es/parser): Reject invalid object rest assignment targets

Summary

This PR addresses issue #11543 where ({...{}} = {}) was not being properly rejected by the parser. The fix adds a validation step for PatType::AssignElement in the object rest/spread element reparsing path, checking is_valid_simple_assignment_target() and emitting NotSimpleAssign when it fails.

Code Quality

Good:

  • The if/else to match refactor is a clear improvement — it makes the exhaustive handling of PatType variants explicit and easier to reason about.
  • The unreachable! arm with a descriptive message is appropriate since element() can only map to BindingElement or AssignElement.
  • The error message and span in the test fixture look correct — the span covers ...{} which accurately points to the invalid rest target.

Observation on expr.into() in the error path (line 266):
When the expression is not a valid simple assignment target, the code does expr.into() which produces Pat::Expr(expr). This is consistent with how the existing AssignElement handling works at lines 150–158 of the same file — it also emits the error and returns the expression as a pattern to allow error recovery. Good consistency here.

Potential Issues

  1. Array rest counterpart (line 352): The array rest element path at line 352 (self.reparse_expr_as_pat(pat_ty.element(), expr)) does not have this same guard. For array destructuring assignment like [...{}] = [], the validation would still go through reparse_expr_as_pat → the AssignElement check at line 132 which handles Expr::Object by falling through to the recursive reparsing rather than rejecting it. This means [...{}] = [] would be reparsed as a valid destructuring pattern (an empty ObjectPat), which is actually spec-correct — [...{}] = [] is valid per the spec because AssignmentRestElement in arrays allows DestructuringAssignmentTarget which includes patterns. So the asymmetry between array and object rest handling is correct per the spec, but it might be worth a brief code comment explaining why object rest targets are restricted to simple assignment targets while array rest targets are not.

  2. No concern with the unreachable! arm: The element() method is exhaustive and deterministic — BindingPat|BindingElement → BindingElement, AssignPat|AssignElement → AssignElement. The unreachable arm correctly won't be hit.

Test Coverage

  • The fixture test issue-11543/input.js covers the exact reported case: ({...{}} = {}).
  • The PR description mentions verifying against existing tests (arrow_function_binding_rest_property, rest_not_last) which is good.
  • Suggestion: Consider adding a few more test cases to the fixture to cover related invalid patterns, such as:
    • ({...1} = {}) — literal as rest target
    • ({...(a + b)} = {}) — binary expression as rest target
    • ({...a.b} = {}) — member expression (this should be valid and not error)
      These would strengthen regression coverage for the new validation path. These can be separate fixture files or added to the same one.

Performance

No concerns. The change adds a single is_valid_simple_assignment_target() call (which is a simple pattern match on Expr variants) only in the error path for object rest targets during assignment reparsing. This has negligible cost and only runs when the parser encounters a spread element.

Security

No concerns. This is a parser validation tightening — it rejects previously-accepted invalid syntax.

Verdict

The fix is correct, well-scoped, and consistent with existing patterns in the codebase. The main suggestion is to consider additional test cases for broader coverage of the new validation path.

@kdy1 kdy1 closed this Feb 23, 2026
@github-actions github-actions bot modified the milestones: Planned, 1.15.13 Feb 23, 2026
@kdy1 kdy1 deleted the codex/fix-issue-11543-rest-assignment-target branch February 24, 2026 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

A binding after a rest element is not allowed within an assignment context.

1 participant