fix: preserve Symbol properties in object rest

[proggeramlug]

Summary

Preserve enumerable own Symbol properties when object destructuring creates a rest object. Symbol accessors are evaluated once, nonenumerable Symbols stay excluded, and Symbol key order matches JavaScript.

Changes

• copy own enumerable Symbol properties after the existing string-key rest copy
• use create-data-property semantics for copied Symbols and keep all intermediate values rooted across getter calls
• add Node parity coverage for retained, omitted, nonenumerable, accessor, and Symbol-only properties

Related issue

Fixes #10815

Test plan

• LLVM_SYS_221_PREFIX=/usr/lib/llvm-22 cargo build --release -p perry -p perry-runtime-static
• RUST_TEST_THREADS=1 LLVM_SYS_221_PREFIX=/usr/lib/llvm-22 cargo test --release -p perry-runtime --lib (4,269 passed; 6 ignored)
• Compiled and ran test_gap_object_destructuring_field_and_rest_guard.ts; output matches Node
• Re-ran the compiled fixture with PERRY_GC_SCAVENGE=1 PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1; output matches the normal run
• cargo fmt --all -- --check
• python3 scripts/check_test_registration.py
• scripts/check_file_size.sh

Checklist

• I have NOT bumped the workspace version or edited CLAUDE.md / CHANGELOG.md (maintainer handles these at merge)
• My commits follow the loose feat: / fix: / docs: / chore: prefix convention used in the log
• I've read CONTRIBUTING.md and agree to the Code of Conduct

Summary by CodeRabbit

• Bug Fixes

  • Object rest destructuring now preserves enumerable own Symbol properties, including accessor values and Symbol-only objects.
  • Excluded, deleted, and non-enumerable Symbol properties continue to be omitted.
  • Rest destructuring remains safe when property access triggers garbage collection.

• Tests

  • Added coverage for computed Symbol exclusions, getters, property enumeration, and Symbol-only objects.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: e1cacf99-914c-4f9c-a4fd-f1d706f99c49

📥 Commits

Reviewing files that changed from the base of the PR and between 57a6d60 and 9a06bb2.

📒 Files selected for processing (3)

• changelog.d/11023-object-rest-symbols.md
• crates/perry-runtime/src/object/delete_rest.rs
• test-files/test_gap_object_destructuring_field_and_rest_guard.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

Object rest destructuring now preserves enumerable own Symbol properties, including accessor values and Symbol-only objects. It excludes named and non-enumerable Symbols. Runtime handles keep operands rooted during getter and conversion activity.

Changes

Object rest Symbol support

Layer / File(s)	Summary
Runtime copying and GC rooting crates/perry-runtime/src/object/delete_rest.rs	js_object_rest copies eligible own Symbol properties, skips excluded or non-enumerable Symbols, and roots runtime operands and result objects during copying.
Regression coverage and changelog test-files/test_gap_object_destructuring_field_and_rest_guard.ts, changelog.d/11023-object-rest-symbols.md	Tests cover omitted, kept, hidden, getter, enumerated, and Symbol-only properties. The changelog records the behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant js_object_rest
  participant copy_rest_symbol_properties
  participant SourceObject
  participant RestObject
  js_object_rest->>copy_rest_symbol_properties: copy eligible Symbol properties
  copy_rest_symbol_properties->>SourceObject: read own symbols and descriptors
  copy_rest_symbol_properties->>RestObject: define enumerable data properties

Loading

Merge Risk: ⚪ Minimal · up to 9a06b

The Symbol rest implementation and its regression coverage have no remaining actionable issue identified in this change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving Symbol properties in object rest destructuring.
Description check	✅ Passed	The description includes all required sections, explains the implementation and scope, references issue #10815, and provides detailed test results with the checklist completed. The optional screenshot…
Linked Issues check	✅ Passed	Issue #10815 requires object rest to copy own enumerable Symbol properties while excluding named properties and nonenumerable properties. The PR updates js_object_rest and adds `copy_rest_symbol_pro…
Out of Scope Changes check	✅ Passed	The changes stay within Issue #10815. The runtime changes implement Symbol-key copying and required GC rooting during getter-related execution. The test changes provide regression coverage. The change…

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[proggeramlug]

Held from merge train 257 (#11039) by the raw-handle ratchet, not by a conflict.

scripts/raw_handle_debt.py on the train reported 930 sites against a baseline of 906, with one per-module violation: crates/perry-runtime/src/object/delete_rest.rs at 26 against its ceiling of 2. The script's own guidance: put the allocating call inside RuntimeHandle::across_{mut,const,nanbox} for a post-call reload, or use with_{mut,const}_ptr for a scoped argument to a non-allocating operation. Empty || () closures still count as debt.

Note #10984 also touches that file and was held for a conflict there, so coordinate if you are both editing it. Ping me when the ratchet is back at baseline.

[proggeramlug]

Held from merge train 257 (#11039) by the raw-handle ratchet, not by a conflict.

scripts/raw_handle_debt.py on the train reported 930 sites against a baseline of 906, with one per-module violation: crates/perry-runtime/src/object/delete_rest.rs at 26 against its ceiling of 2. The script's own guidance: put the allocating call inside RuntimeHandle::across_{mut,const,nanbox} for a post-call reload, or use with_{mut,const}_ptr for a scoped argument to a non-allocating operation. Empty || () closures still count as debt.

Note #10984 also touches that file and was held for a conflict there, so coordinate if you are both editing it. Ping me when the ratchet is back at baseline.

[proggeramlug]

Holding this out of the merge trains for one required lint step. It is not the stale public-baseline red that most open PRs carry — that one is fixed on main now. This is a real ratchet violation on your head (9a06bb2bc1, run 35764589130):

raw-handle debt sites: 930 (baseline 906)
per-module raw-handle rules: 1 violation(s)
  crates/perry-runtime/src/object/delete_rest.rs: raw-handle debt count 26 exceeds its ceiling of 2

scripts/raw_handle_debt_files.txt:93 sets that file's ceiling at 2; the PR takes it to 26. The ratchet's own self-test passed in the same run, so the count is trustworthy.

Reproduce without a build, in a couple of seconds:

python3 scripts/raw_handle_debt.py
python3 scripts/raw_handle_debt.py --no-raise-vs origin/main

The gate's message names the intended fix: put the allocating call inside RuntimeHandle::across_{mut,const,nanbox} so the pointer is reloaded after the call, and use with_{mut,const}_ptr for a scoped argument to a non-allocating operation or a self-rooting runtime entry point. Note the two caveats that bite here — an empty || () closure still counts as debt, and across_* cannot convert a raw-handle read that sits in argument position, so those need restructuring rather than wrapping.

Raising the ceiling instead is not an option I can take: this ratchet is what keeps #7341's cleanup from being undone, and 2 → 26 in one file is the shape it exists to catch.

Everything else on this PR is green — all 6 gap shards, cargo-test, warnings, e2e-scoped, gc-stress. Fix this one step and I'll take it in the next train.

[proggeramlug]

Landed on main in merge train 268 (#11119), released as v0.5.1651 at 36892b7194.

Four of this train's seven PRs — including this one, if it is #11055, #11023, #11012 or #11014 — were repaired here because they were stuck: the fixes were cherry-picked from fix/<PR>-ci branches built in this session, which is also how fork-hosted heads get landed without their authors. CI on the assembled tree was 22/22 green, all 6 gap-suite shards.

A train rebase gives the commits new SHAs, so GitHub cannot auto-close the source PR; closing by hand. Nothing needed from you.