fix(string): root spread character arrays across moving GC (#9983)

[proggeramlug]

Fixes #9983

Lifted from stale draft #9998 (commit 3fd585fc5) onto current main. This is the string-spread half of #9983. The Array.prototype.slice / Symbol.species half already landed on main separately (233514422, "describe species result slots before callbacks").

Bug

js_string_to_char_array ([..."str"]) kept two heap references across the per-character js_string_from_bytes allocations, and each of those allocations can run a moving minor:

1. The raw result arr. After the array was evacuated, the remaining stores went into from-space. The live array was left with unwritten elements, and the side pointer mask no longer matched its contents. That is the UNENUMERATED slot that PERRY_GC_VERIFY_MARK reported.
2. A borrowed &[u8] slice of the source string's payload. js_string_from_bytes allocates its result before it copies the input, so a collection that moves the source string leaves the slice pointing at reset from-space.

Fix

• The payload is copied out of the heap (to_vec()) before the first allocation.
• The result array is kept in a RuntimeHandleScope. Each element allocation runs inside RuntimeHandle::across_mut, and the store/barrier (note_array_slot) uses the head that across_mut returns after the allocation. The return value goes through with_mut_ptr. This follows the gc(layer 3): from-space quarantine catches 55 stale dereferences across the gap suite — the instrument is in CI but aimed at one synthetic fixture #7341 idiom, so the raw_handle_debt ratchet does not change.

Test

gc/tests/string_char_array_roots.rs forces a copying minor before every character allocation, using a test-only pre-allocation hook. It then checks:

• the minors copied at least one object, so the result assertions are not vacuous;
• all 13 elements are "é";
• the array's pointer slots are all enumerated (verify_array_pointer_slots_enumerated_for, unenumerated_slots == 0, checked_pointer_slots == 13).

Sabotage-tested, one half of the fix reverted at a time:

• stale pre-loop array head used for the stores → test process dies with SIGSEGV;
• payload borrowed instead of copied → assertion fails (left: [0, 0], right: [195, 169], i.e. the element bytes were read from reset from-space).

With the fix restored, the test passes.

Validation (macOS arm64)

• RUST_TEST_THREADS=1 cargo test --release -p perry-runtime --lib: 4434 passed, 0 failed, 5 ignored
• Built -p perry -p perry-runtime-static -p perry-stdlib-static and compiled a .ts spread probe (3000 é + 500 ü😀 + suffix, 400 iterations, 20 arrays retained). Output matches node, both plain and under PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_SCHEDULE_SEED=7 PERRY_GC_SCHEDULE_RATE=1 PERRY_GC_VERIFY_MARK=1, with 0 UNENUMERATED reports.
• cargo fmt --all -- --check, check_file_size.sh, gc_runtime_root_holders.py, addr_class_inventory.py, raw_handle_debt.py: pass.
• scripts/run_lint_gates.sh: 91 of 96 pass. The 5 failures are environmental or also fail on unmodified main on the same host: cargo xwin is not installed; public-baseline freshness; binding_governance.py --check; gc_runtime_root_holders.py --self-test; gc_rekeyed_key_tables.py.

Summary by CodeRabbit

• Bug Fixes
  • Fixed an issue that could cause incorrect character-array contents when converting strings containing non-ASCII characters during memory cleanup.
• Tests
  • Added coverage for string-to-character-array conversion when memory cleanup occurs during conversion.

[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: cbd74b9d-e896-46fd-a0c5-19b09eb08412

📥 Commits

Reviewing files that changed from the base of the PR and between 25ef463 and 2922f8d.

📒 Files selected for processing (5)

• changelog.d/11181-string-spread-moving-gc.md
• crates/perry-runtime/src/gc/tests/mod.rs
• crates/perry-runtime/src/gc/tests/string_char_array_roots.rs
• crates/perry-runtime/src/string/char_ops.rs
• crates/perry-runtime/src/string/mod.rs

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

---

📝 Walkthrough

Walkthrough

The string-to-character-array builder now copies source bytes before character allocations and keeps the result array rooted while allocations occur. A regression test forces minor collections during construction and checks the array contents and pointer-slot enumeration.

Changes

String character-array GC safety

Layer / File(s)	Summary
Make character-array construction safe across collections crates/perry-runtime/src/string/char_ops.rs, crates/perry-runtime/src/string/mod.rs	The builder copies the source bytes before allocating character strings, roots the result array, and reacquires its current pointer for each element store. A test-only hook can run before each character allocation.
Exercise moving collections during construction crates/perry-runtime/src/gc/tests/string_char_array_roots.rs, crates/perry-runtime/src/gc/tests/mod.rs, changelog.d/11181-string-spread-moving-gc.md	The registered regression test forces a minor collection before each character allocation. It checks that objects moved, all 13 elements contain "é", and all pointer slots are enumerated. The changelog records the fix and prior verifier report.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 2922f

The string-spread GC fix is mergeable after normal checks; no actionable merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 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 summarizes the main change: rooting string-spread character arrays across moving garbage collection.
Description check	✅ Passed	The description explains the bug, fix, related issue, regression test, and validation results. It does not include the template’s Checklist section or its checkbox responses, but the description is ot…
Linked Issues check	✅ Passed	For the string-spread path in #9983, js_string_to_char_array copies the source payload before allocations, roots the result array, and reacquires its current pointer before each store. The regressio…
Out of Scope Changes check	✅ Passed	The runtime changes and regression test support the #9983 string-spread objective. The test hook is limited to test builds. The changelog entry documents the fix. No unrelated changes are evident in t…

Full details: Docstring Coverage

Explanation

Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 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.}