fix(ast): include rest properties when using get_binding_identifiers

[camc314]

previously, the rest param of an ArrayPattern​ inside a BindingPattern would never be visited, this meant it had to be manually visited inside the linter. It also meant that the minifier incorrectly transformed { const { a, ...b } = {}; b = {} }​ to { let const { a, ...b } = {} } which is a difference in runtime behvaiour (before is a TypeError​, after is not.

[camc314]

• fix(ast): include rest properties when using get_binding_identifiers #15710 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codspeed-hq]

CodSpeed Performance Report

Merging #15710 will not alter performance

_{Comparing c/11-15-fix_ast-impl_include_rest_properties_when_using_get_binding_identifiers_ (a30af92) with main (56e7e44)¹}

Summary

✅ 37 untouched

Footnotes

1. No successful run was found on main (50f1ec0) during the generation of this report, so 56e7e44 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[Copilot]

Pull Request Overview

This PR fixes the get_binding_identifiers method to correctly include rest properties (...rest) when extracting binding identifiers from array and object destructuring patterns. Previously, rest properties were not being included, which could cause issues in various code analysis scenarios.

• Adds support for rest properties in both ArrayPattern and ObjectPattern destructuring
• Removes redundant BindingRestElement handling from the no_const_assign linter rule (now handled automatically)
• Adds test cases to ensure const-to-let conversion doesn't incorrectly transform const declarations with rest properties that are reassigned

Reviewed Changes

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

File	Description
crates/oxc_ast/src/ast_impl/js.rs	Modified append_binding_identifiers to include rest properties for both array and object patterns
crates/oxc_linter/src/rules/eslint/no_const_assign.rs	Removed explicit BindingRestElement handling, now covered by the fixed get_binding_identifiers
crates/oxc_minifier/src/peephole/normalize.rs	Added test cases for const declarations with rest properties being reassigned

Comments suppressed due to low confidence (1)

crates/oxc_ast/src/ast_impl/js.rs:1351

• The documentation for get_binding_identifiers should be updated to include examples with rest properties, since the method now returns rest bindings. Consider adding examples like let {a, ...rest} = obj would return [a, rest] and let [a, ...rest] = arr would return [a, rest].

    /// Returns the bound identifiers in this binding pattern.
    ///
    /// ## Example
    ///
    /// - `let {} = obj` would return `[]`
    /// - `let {a, b} = obj` would return `[a, b]`
    /// - `let {a = 1, b: c} = obj` would return `[a, c]`

---

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

[overlookmotel]

@camc314 I've only not hit "merge" because it's not assigned to me.

[camc314]

Merge activity

• Nov 15, 7:08 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Nov 15, 7:08 PM UTC: camc314 added this pull request to the Graphite merge queue.
• Nov 15, 7:15 PM UTC: Merged by the Graphite merge queue.