fix(deforest): the caller's binding kept a growth-forwarding stub after a deforested call (#7661)

[proggeramlug]

Fixes #7661.

Where the stub comes from

The issue's two candidates were the alwaysinline + $spec_i32_b specialization pair and the return/assignment lowering. It is neither — it is the deforestation pass, and the mechanism is more direct than "a stale head leaks through".

crates/perry-transform/src/deforest/ rewrites an array-producing function to fill a caller-allocated accumulator. Its own module doc, point 7:

Non-consumer call sites (e.g. top-level const all = f(...)): rewrite to const all = []; f(args, all); so callers that need the array as a value still get it.

So the reproducer becomes:

let keep = [];        // caller allocates, capacity 0
build(1000, keep);    // callee grows it 0 -> 1000, relocating repeatedly

js_array_grow does not grow in place. The callee's out.push write-back re-points its own out-param slot; nothing re-points keep. And the producer rewrite dropped the trailing return out (step 2), so there was no value to store back either. Both halves are visible in --trace llvm on the issue's exact reproducer:

; producer — falls through to `return undefined`
for.exit.4:
  ret double 0x7FFC000000000001

; caller — result discarded; `keep`'s slot (%r1) still holds the pre-growth head
%r2 = call i64 @js_array_alloc(i32 0)
store ptr addrspace(1) %rs4gc.s2, ptr %r1
%r8  = load ptr addrspace(1), ptr %r1
%r9  = call double @perry_fn__7661_ts__build$spec_i32_b(i32 1000, double %r8)
%r10 = load ptr addrspace(1), ptr %r1          ; <- reads the stub

That also explains the issue's observation that a module-scope keep.push(x); keep.pop(); changes behaviour: it is the only thing that writes a resolved head back into the global.

The fix

No new HIR node, no codegen change — the producer already holds the correct head in its out-param slot at the point it used to fall off the end.

1. producer_rewrite.rs — KEEP the trailing return out. Step 4 already substitutes out_local_id → out_param, so it becomes return <out_param>: the live head, after every realloc write-back. detect.rs guarantees exactly one top-level Stmt::Return(Some(Expr::LocalGet(out_id))) and that it is the last top-level statement, so there is nothing else this could be keeping.
2. call_sites.rs — all three rewrites store the result back over the caller's binding:
   • non-consumer: keep = f(n, keep)
   • consumer-fuse: outer = f(args, outer)
   • pass-through recursion inside the producer (same code path, so covered by the same change)
3. The seeded binding is emitted mutable, because it is now written twice. Leaving it const while storing through it would make every analysis that trusts mutable: false wrong.

After:

; producer
for.exit.4:
  %r109 = load ptr addrspace(1), ptr %r2
  ret double %r109

; caller
%r9 = call double @perry_fn__7661_ts__build$spec_i32_b(i32 1000, double %r8)
store ptr addrspace(1) %rs4gc.s3, ptr %r1      ; <- keep is re-pointed

Verified in IR that the recursive shape gets it too — both tree(...) self-calls store their result back into the out-param slot before the next use.

This is what the issue asked for: it turns js_array_refresh_local_head from a correctness obligation every future consumer of a raw array head must remember into an optimization.

Coverage is structural, and it was sabotage-checked

Behaviour cannot see this bug. Every runtime entry point resolves the forwarding chain through clean_arr_ptr, so the program prints the right answer either way — which is exactly why it went unnoticed until #7612 dereferenced a head directly and SIGBUSed at N = 17.

So the load-bearing tests assert the transform's output shape, in deforest/tests.rs (--lib, so they run on every PR):

• producer_returns_the_out_param_not_undefined
• plain_call_site_stores_the_returned_head_back_over_the_binding

Both were verified to FAIL against the pre-fix transform — I reverted the two source changes, re-ran, and confirmed 2 failed / 11 passed, then restored. A structural test that has never failed is a test with no subject; these have one.

One existing test needed updating rather than fixing: deforests_producer_called_from_class_method walks for the surviving call under Stmt::Expr, which is now one level deeper inside the LocalSet. Its walk now unwraps, and the comment records why that matters (a non-unwrapping walk would have collected nothing and compared [] == [1] — a failure, but only because the expectation is non-empty).

test-files/test_deforest_growth_forwarding.ts exercises all three call-site shapes end-to-end including N = 17, and its header states plainly that it is a smoke test, not a detector.

Validation

• cargo test -p perry-transform --lib: 58 passed, 0 failed (13 deforest, incl. the 2 new).
• End-to-end vs node 26.5.1 on all three shapes (build(1000), a depth-9 tree with consumer-fuse + recursion, build(17)): byte-identical.
• Full test-files/ parity sweep: run and compared against test-parity/gap_snapshot.json.
• cargo fmt --all --check, scripts/check_file_size.sh: clean.

Cost: one store per deforested call. No allocation, no call, no change to the fusion itself.

No version bump (maintainer bumps at merge).

Summary by CodeRabbit

• Bug Fixes

  • Fixed array growth handling so updated array references are correctly preserved across optimized operations.
  • Improved reliability for recursive processing, direct construction, and boundary-size arrays.
  • Ensured generated bindings remain writable when values need to be updated after growth.

• Tests

  • Added regression and end-to-end coverage for array growth forwarding, including nested and recursive scenarios.

• Documentation

  • Added a changelog entry describing the array growth fix.

• Chores

  • Updated the project version to 0.5.1439.

[coderabbitai]

📝 Walkthrough

Walkthrough

The deforestation transform now preserves producer accumulator returns and assigns relocated array heads back to caller bindings. Structural tests and a runtime smoke test cover direct, recursive, and threshold growth cases. Project version metadata and the changelog were updated.

Changes

Deforestation growth forwarding

Layer / File(s)	Summary
Preserve producer accumulator returns crates/perry-transform/src/deforest/producer_rewrite.rs	Producer rewrites retain the final return out, which becomes a return of the live synthetic accumulator parameter.
Propagate returned accumulator heads crates/perry-transform/src/deforest/call_sites.rs	Consumer-fused and value-bound calls assign returned accumulator heads back to their outer bindings. Generated array bindings are mutable.
Validate forwarding and update release metadata crates/perry-transform/src/deforest/tests.rs, test-files/test_deforest_growth_forwarding.ts, changelog.d/7751-deforest-growth-forwarding.md, Cargo.toml, CLAUDE.md	Structural and runtime tests cover array growth forwarding. The changelog and project version metadata were updated.

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

Possibly related PRs

• PerryTS/perry#7660: Addresses growth-forwarding propagation in a different code-generation subsystem.

Suggested labels: bug

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The Cargo.toml and CLAUDE.md version changes are unrelated to issue #7661 and violate the repository template's explicit merge-time metadata rule.	Remove the Cargo.toml version bump and CLAUDE.md version edit; retain the issue-related source and test changes.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #7661 by returning the live accumulator head and writing it back for all required deforestation call-site forms.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title clearly identifies the deforestation bug and the stale caller binding caused by growth forwarding.
Description check	✅ Passed	The description clearly explains the issue, fix, affected call sites, structural tests, and validation, but it omits the repository template headings and checklist.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/7661-array-head-forwarding

---

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.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/perry-transform/src/deforest/call_sites.rs (1)

249-269: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Mark an immutable fusion target mutable before emitting LocalSet.

Line 268 writes outer_id, but this rewrite does not update the existing Stmt::Let that owns outer_id. If the source uses const outer = [], the transformed HIR contains a write to a binding still marked mutable: false. Lines 432-438 establish that this flag is an analysis contract.

Track fused target IDs and mark their declarations mutable, or rewrite through a mutable compiler-generated binding. Add a regression for const outer = [] followed by a fused producer call.

🤖 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-transform/src/deforest/call_sites.rs` around lines 249 - 269,
Update the fusion rewrite around the LocalSet of outer_id so the binding
declaration for every fused target is marked mutable before emitting the write.
Track the affected target IDs and adjust the existing Stmt::Let metadata, or
route the assignment through a mutable compiler-generated binding, while
preserving the producer call and write-back behavior. Add a regression covering
const outer = [] followed by a fused producer call.

🤖 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 `@Cargo.toml`:
- Line 318: Remove the contributor-managed version updates: restore the
workspace package version at Cargo.toml lines 318-318 and restore the Current
Version value at CLAUDE.md lines 11-11. Keep the existing changelog fragment and
leave release metadata changes to maintainers.

---

Outside diff comments:
In `@crates/perry-transform/src/deforest/call_sites.rs`:
- Around line 249-269: Update the fusion rewrite around the LocalSet of outer_id
so the binding declaration for every fused target is marked mutable before
emitting the write. Track the affected target IDs and adjust the existing
Stmt::Let metadata, or route the assignment through a mutable compiler-generated
binding, while preserving the producer call and write-back behavior. Add a
regression covering const outer = [] followed by a fused producer call.

🪄 Autofix

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: a498b15b-f960-4a87-89ce-6b10d291ca57

📥 Commits

Reviewing files that changed from the base of the PR and between 27d5358 and 18e8bc6.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (7)

• CLAUDE.md
• Cargo.toml
• changelog.d/7751-deforest-growth-forwarding.md
• crates/perry-transform/src/deforest/call_sites.rs
• crates/perry-transform/src/deforest/producer_rewrite.rs
• crates/perry-transform/src/deforest/tests.rs
• test-files/test_deforest_growth_forwarding.ts

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Remove contributor-managed release metadata.

This PR already includes changelog.d/7751-deforest-growth-forwarding.md. Remove these version updates unless this is a maintainer-owned main landing change.

• Cargo.toml#L318-L318: restore the workspace package version.
• CLAUDE.md#L11-L11: restore the Current Version value.

As per coding guidelines, version updates apply only when landing on main. Based on learnings, contributors must use the PR-keyed changelog fragment and leave release metadata to maintainers.

📍 Affects 2 files

• Cargo.toml#L318-L318 (this comment)
• CLAUDE.md#L11-L11

🤖 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 `@Cargo.toml` at line 318, Remove the contributor-managed version updates:
restore the workspace package version at Cargo.toml lines 318-318 and restore
the Current Version value at CLAUDE.md lines 11-11. Keep the existing changelog
fragment and leave release metadata changes to maintainers.

Sources: Coding guidelines, Learnings

[proggeramlug]

Merging as v0.5.1439

The diagnosis rejects both of the issue's own candidates and is better for it. Not the alwaysinline/$spec_i32_b pair, not the return/assignment lowering — the deforestation pass dropped the trailing return out, so the producer fell through to an implicit return undefined and the caller kept the head it had allocated before the call. js_array_grow doesn't grow in place; the callee's write-back re-points its own out-param slot and nothing re-points keep.

The before/after IR makes it unarguable — ret double 0x7FFC000000000001 becoming ret double %r109, and the caller gaining the store that re-points its binding. And it explains the issue's own puzzling observation, that a module-scope keep.push(x); keep.pop(); changed behaviour: that was the only thing writing a resolved head back into the global.

The line that decides how this had to be tested

Behaviour cannot see this bug. Every runtime entry point resolves the forwarding chain through clean_arr_ptr, so the program prints the right answer either way.

That is why it survived until #7612 dereferenced a head directly and SIGBUSed at N = 17, and why an end-to-end test would have been theatre. Asserting the transform's output shape is the only thing that can fail here, and putting those in --lib rather than the tag-gated gap suite means they run on every PR.

I verified they have a subject rather than taking the claim: re-introducing the drop (func.body.retain(|s| !matches!(s, Stmt::Return(_)))) fails producer_returns_the_out_param_not_undefined — and fails exactly one test, because I reverted only the producer half and the call-site test covers the other. That matches your "2 failed / 11 passed" when both are reverted.

Two details that would have been easy to get wrong

The mutable: true change. The seeded binding is now written twice, and leaving it const while storing through it would make every analysis that trusts mutable: false wrong. That is a much worse bug than the one being fixed, and quieter.

The updated existing test, and the reason recorded. deforests_producer_called_from_class_method walks for the call under Stmt::Expr, now one level deeper inside the LocalSet. The note that a non-unwrapping walk would have collected nothing and compared [] == [1] — a failure, but only because the expectation happens to be non-empty — is the kind of thing that saves the next person from "fixing" it by making the expectation empty.

Marking test_deforest_growth_forwarding.ts in its own header as a smoke test and not a detector is the right label, given the first paragraph above.

The framing of the outcome is also right: this turns js_array_refresh_local_head from a correctness obligation every future consumer of a raw array head must remember into an optimization. Fixing the producer once beats fixing consumers one at a time, which is what #7660 had to do.

Cost is one store per deforested call. cargo test -p perry-transform --lib: 58 passed, 0 failed. Gates 21/21.