fix(runtime): finish string coercion operand rooting - #7950
Conversation
📝 WalkthroughWalkthroughThe runtime now roots receivers and operands during ChangesGC-safe runtime operations
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/string/split.rs (1)
483-496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider invalidating the stale
str_dataandsbindings after the array allocation.
str_databorrows the source payload from Line 369. Line 493 allocates the result array, which can move the source. After Line 495 bothstr_dataand the rawsparameter name from-space, yet both remain in scope for the rest of the function. The current code is correct because the loop reads onlys_nowfroms_handleandpart_rangesholds offsets. The risk is a later edit that readsstr_dataagain inside the loop; that is the#5062dangling-source class this file has hit before.Shadowing the stale bindings makes the invalidation explicit at compile time.
♻️ Proposed guard against a future stale read
let n = part_ranges.len(); let (arr, _) = s_handle.across_const::<StringHeader, _>(|| { crate::array::js_array_alloc_pointer_elements(n as u32) }); let arr_handle = scope.root_raw_mut_ptr(arr); + // `str_data` and `s` name pre-allocation addresses from here on. Every + // later read must come from `s_handle`. + #[allow(unused_variables)] + let (str_data, s) = ((), ());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/string/split.rs` around lines 483 - 496, After the result array allocation in the split implementation, explicitly invalidate or shadow the stale str_data and raw s bindings before the loop. Preserve the existing offset-based part_ranges and s_handle/s_now reads, while making any later use of the pre-allocation source bindings fail at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-runtime/src/string/split.rs`:
- Around line 483-496: After the result array allocation in the split
implementation, explicitly invalidate or shadow the stale str_data and raw s
bindings before the loop. Preserve the existing offset-based part_ranges and
s_handle/s_now reads, while making any later use of the pre-allocation source
bindings fail at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7867d243-be3b-4e50-893a-c31989f76835
📒 Files selected for processing (5)
changelog.d/7950-string-coerce-rooting-tail.mdcrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/object/native_call_method/typed_array.rscrates/perry-runtime/src/regex/compile.rscrates/perry-runtime/src/string/split.rs
Summary
Finish the four primary operand-rooting sites left open by #6949 after #7811 and #7815:
splitreceiver and separator across limit/separator coercion, and refresh the source around result allocationsRegExp.prototype.compilereceiver and arguments across source/flags coercion and string allocationRegExppattern (and pending flags value) across the second constructor coerciontoLocaleStringclosure across prototype lookup and every loop allocation/callbackThe separate raw-JSValues-in-Rust-containers shape is now tracked by #7949, so it remains visible without keeping this coercion-specific issue open.
No version bump is included.
Validation
cargo check -p perry-runtime --libcargo test -p perry-runtime --lib string::tests::(33 passed)cargo test -p perry-runtime --lib regex::tests::(27 passed)cargo test -p perry-runtime --lib: 2,152 passed; the two existing Windows-only failures remained (global_sink_isolation::every_covered_clear_helper_is_still_called_by_the_guards,path::value_args::tests::both_operands_survive_the_materialisation_window)scripts/check_file_size.shget_raw_{mut,const}_ptrreadsCloses #6949
Summary by CodeRabbit
Bug Fixes
toLocaleString()operations during memory cleanup.Documentation