fix(runtime,codegen): demote unique strings stored into array elements#5548
Merged
proggeramlug merged 1 commit intoJun 22, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds ChangesHeap-string aliasing fix for array element stores
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry/tests/string_append_heap_alias.rs (1)
124-125: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression that forces the outlined literal store path (
js_array_from_values).Line 124 explicitly targets the small-literal lowering path, so this set still doesn’t pin the outlined array-literal construction path in this file.
🤖 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/tests/string_append_heap_alias.rs` around lines 124 - 125, The current test targets only the small-literal lowering path that uses js_array_alloc and js_array_push_f64, leaving the outlined array-literal construction path uncovered. Add a new regression test after the existing test that forces the outlined literal store path by using js_array_from_values instead of the small-literal approach. This new test should create a scenario that triggers the outlined array-literal construction behavior to ensure that code path is properly tested and pinned in this test file.
🤖 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.
Inline comments:
In `@crates/perry-codegen/src/stmt/let_stmt.rs`:
- Around line 411-416: The needs_string_demote gate in the let_stmt.rs file only
triggers when ctx.local_types.get(src_id) is exactly perry_types::Type::String,
but LocalGet values typed as Any or other non-exact types can still carry
uniquely-owned heap strings that need demotion to avoid alias corruption.
Broaden the matches! condition in needs_string_demote to include not just exact
String type checks but also type cases like Any that could potentially contain
heap strings, ensuring demote logic applies whenever a LocalGet value could
reference a uniquely-owned heap string regardless of its static type annotation.
---
Nitpick comments:
In `@crates/perry/tests/string_append_heap_alias.rs`:
- Around line 124-125: The current test targets only the small-literal lowering
path that uses js_array_alloc and js_array_push_f64, leaving the outlined
array-literal construction path uncovered. Add a new regression test after the
existing test that forces the outlined literal store path by using
js_array_from_values instead of the small-literal approach. This new test should
create a scenario that triggers the outlined array-literal construction behavior
to ensure that code path is properly tested and pinned in this test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d79a1cf-9327-4cac-b6f9-4d585ebfd17e
📒 Files selected for processing (7)
crates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-runtime/src/array/alloc.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/array/splice_slice.rscrates/perry/tests/string_append_heap_alias.rs
machineloop
force-pushed
the
fix/array-element-string-demote
branch
from
bebc0cb to
cac311a
Compare
Contributor
Author
|
Addressed the outline-path coverage nitpick in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry/tests/string_append_heap_alias.rs`:
- Around line 253-255: The test is not verifying that the compiled binary
executed successfully before attempting to parse its stdout. After calling
Command::new(&output).output() and storing the result in the run variable, add
an assertion to check run.status.success() before accessing run.stdout. This
will ensure that if the binary fails to run or returns a non-zero exit code, the
test will fail with an explicit status error rather than a confusing output
mismatch assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90f04182-5511-4b64-a50c-de39af7b189e
📒 Files selected for processing (7)
crates/perry-codegen/src/expr/write_barrier.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-runtime/src/array/alloc.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/push_pop.rscrates/perry-runtime/src/array/splice_slice.rscrates/perry/tests/string_append_heap_alias.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/perry-runtime/src/array/splice_slice.rs
- crates/perry-runtime/src/array/indexing.rs
- crates/perry-codegen/src/expr/write_barrier.rs
- crates/perry-runtime/src/array/alloc.rs
- crates/perry-runtime/src/array/push_pop.rs
- crates/perry-codegen/src/stmt/let_stmt.rs
Follow-up to PerryTS#5533, which demoted heap-stored unique strings to shared for object-field stores (`runtime_store_jsvalue_slot`) but left array-element stores uncovered. The same aliasing bug applies: a uniquely-owned (refcount==1) string written into an array element is aliased by the array, so a later in-place `+=` on the source local (`js_string_append`'s refcount==1 fast path) mutates the stored element and corrupts it. Array element stores reach the slot through several paths that don't share one choke point, so apply the same tag-checked demote (`js_string_addref_if_heap_string`, added in PerryTS#5533 — a no-op for small-string-optimized / non-string values) at each. Codegen (the inline fast paths that bypass the runtime store helpers): - `emit_jsvalue_slot_store_on_block` (write_barrier.rs): the shared inline element-store emitter for array literals, `push`, and `arr[i] =`. Gated on the existing "value may be a heap pointer" flag, so numeric stores pay nothing. - the scalar-replaced array-literal init in let_stmt.rs (`const a = [s]`): demote the element where it is captured into its slot, before the deferred build — mirrors the object scalar-field demote. Runtime (the paths codegen hands off to a store helper): - `js_array_push_f64` (push realloc / forwarding / proxy paths; the grow path receives the already-demoted value). - `js_array_set_f64` / `js_array_set_f64_extend` (`arr[i] =`). - `js_array_from_values` (the outline array-literal construction path). - the splice inserted-items loop. Internal reshuffles (sort, splice tail shift, slice copy) only move values already stored in an array — already shared — so no demote is needed there. Tests: crates/perry/tests/string_append_heap_alias.rs gains 4 compile-run regression cases (array literal, `arr[i] =`, push, splice). Each stores a non-SSO refcount==1 string then grows the source; all fail without the demotes.
machineloop
force-pushed
the
fix/array-element-string-demote
branch
from
cac311a to
f5a3245
Compare
proggeramlug
added a commit
that referenced
this pull request
Jun 23, 2026
…aths (unshift / fill / with / from_jsvalue) (#5567) * fix(runtime): #5552 demote unique strings in remaining array insert paths Follow-up to #5533 (object fields) and #5548 (the array store paths it enumerated). A uniquely-owned (refcount==1) heap string written into an array element aliases that slot, so a later in-place `s += x` on the source local (`js_string_append`'s refcount==1 fast path) rewrites the stored element and corrupts it. #5548 fixed the push / set / from_values / splice-insert paths; the sibling insert/replace paths below do the same raw element write without the demote. Apply the same tag-checked `js_string_addref_if_heap_string` (no-op for SSO / non-string, idempotent) before the element write at each: - `js_array_unshift_f64` (covers `js_array_unshift_jsvalue` transitively) and the per-item loop in `js_array_unshift_variadic`. - `js_array_fill` / `js_array_fill_range` (demote once before the fill loop — the source aliases every filled slot) and `js_array_fill_generic` (its object-receiver loop writes `value` into each index directly; the array receiver delegates to the two above, so the extra demote is idempotent). - `js_array_with` (the replacement value stored into the new array's slot; the cloned elements are already shared). - `js_array_from_jsvalue` (mixed-type literal construction — the JSValue sibling of the already-covered `js_array_from_values`). Internal reshuffles (sort, splice tail shift, slice copy, copyWithin) only move values already stored in an array — already shared — so no demote is needed. Tests: `string_append_heap_alias.rs` gains compile-run regressions for unshift / fill / with, each confirmed to fail without the demote. `js_array_from_jsvalue` is not emitted by codegen from any TypeScript source, so it gets a runtime unit test (`array/tests.rs`) instead, also confirmed to fail without the demote. Refs #5533, #5548. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(runtime): restore GC_STORE_AUDIT marker on unshift_variadic insert write The #5552 demote line pushed the ptr::write past the proximity window of the existing GC_STORE_AUDIT(BARRIERED) marker, failing the lint job's GC store-site inventory check. Re-annotate the insert write directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Ralph <ralph@skelpo.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #5533. That PR demoted heap-stored unique strings to shared for object-field stores (
runtime_store_jsvalue_slot), but array-element stores reach the slot through several paths that don't share that choke point and were left uncovered. The same aliasing bug applies: a uniquely-owned (refcount==1) string written into an array element is aliased by the array, so a later in-place+=on the source local mutates the stored element and corrupts it.This applies the same tag-checked demote (
js_string_addref_if_heap_string, introduced in #5533 — a no-op for small-string-optimized / non-string values) at each array store site.Changes
Codegen — the inline fast paths that bypass the runtime store helpers:
crates/perry-codegen/src/expr/write_barrier.rs—emit_jsvalue_slot_store_on_block, the shared inline element-store emitter for array literals,push, andarr[i] =. Gated on the existing "value may be a heap pointer" flag, so numeric stores pay nothing.crates/perry-codegen/src/stmt/let_stmt.rs— the scalar-replaced array-literal init (const a = [s]): demote the element where it's captured into its slot, before the deferred build (mirrors the object scalar-field demote).Runtime — the paths codegen hands off to a store helper:
crates/perry-runtime/src/array/push_pop.rs—js_array_push_f64(push realloc / forwarding / proxy paths).crates/perry-runtime/src/array/indexing.rs—js_array_set_f64/js_array_set_f64_extend(arr[i] =).crates/perry-runtime/src/array/alloc.rs—js_array_from_values(outline array-literal construction).crates/perry-runtime/src/array/splice_slice.rs— the splice inserted-items loop.crates/perry/tests/string_append_heap_alias.rsgains 4 compile-run regression tests.Internal reshuffles (sort, splice tail shift, slice copy) only move values already stored in an array — already shared — so no demote is needed there.
Related issue
Follow-up to #5533.
Test plan
Each of the 4 new tests stores a non-SSO
refcount==1string into an array (literal /arr[i] =/ push / splice) then grows the source, asserting the stored element is unchanged. Verified they fail without the demotes (the stored element shows the grown value) and pass with them — 8/8 in the suite, including the object-field cases from #5533.cargo build --releaseclean (affected crates)cargo test -p perry --test string_append_heap_aliaspasses (8/8)docs/src/— n/a (internal store behavior)Checklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit
+=mutations no longer corrupt stored elements.+=on the original string.