Emit iterative WASM for array_map (#480 PR 1)#485
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMigrates Changes
Sequence DiagramsequenceDiagram
participant Frontend as Vera Frontend
participant CallDispatch as CallsMixin._translate_call
participant ArrayTranslator as CallsArrays._translate_array_map
participant CodeGen as WASM Emitter
participant Runtime as WASM Runtime
Frontend->>CallDispatch: array_map(arr, closure)
CallDispatch->>ArrayTranslator: recognize array_map (2 args)
ArrayTranslator->>ArrayTranslator: infer A and B types
ArrayTranslator->>ArrayTranslator: allocate dst (len × sizeof(B)), gc_shadow_push roots
ArrayTranslator->>CodeGen: register closure signature for call_indirect
ArrayTranslator->>CodeGen: emit loop i=0..len-1
CodeGen->>CodeGen: load element A from (arr_ptr + i*sizeof(A))
CodeGen->>Runtime: push closure env, call_indirect -> execute closure
Runtime->>CodeGen: return B (scalar or pair)
CodeGen->>CodeGen: store B into dst at i
CodeGen->>ArrayTranslator: return (dst, len)
ArrayTranslator-->>Frontend: compiled WASM loop (iterative array_map)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
========================================
Coverage 90.37% 90.38%
========================================
Files 58 58
Lines 19588 19720 +132
Branches 225 225
========================================
+ Hits 17703 17824 +121
- Misses 1881 1892 +11
Partials 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace the recursive prelude array_map/array_map_go with an iterative WAT loop that drives a call_indirect on the closure. Shadow-stack depth is now O(1) regardless of input size — the old recursion hit the 16K shadow-stack ceiling (post-#464) around 4K elements. Kept the generic forall<A, B> signature via the existing FunctionInfo built-in registration; source code is unchanged. New CallsArraysMixin method _translate_array_map with helper _infer_closure_return_vera_type. Covered by 4 new tests in TestArrayOperations: large input (8K elements, capped by the separate 16-bit GC header size-field — see #484), Int→Bool type change, closure capturing outer variable, and Int→String pair output. Stress test hit a pre-existing GC bug: the object-header size field is masked to 16 bits, so allocations ≥ 65536 bytes make the sweeper interpret middle-of-payload i64 words as fake zero-size headers and link each 8-byte chunk into the free list, shredding the input array. Test capped at 8K elements (64,000 bytes) as a workaround. Full write-up and reproducer filed as #484. Co-Authored-By: Claude <noreply@anthropic.invalid>
Filed while diagnosing the array_map stress test failure — an allocation >65535 bytes gets its size truncated to 16 bits on GC readback, shredding live payloads via the free-list linking path. Out of scope for #480 PR 1 but worth fixing early in Stage 11 so downstream large-allocation tests can exercise realistic sizes. Co-Authored-By: Claude <noreply@anthropic.invalid>
f86de93 to
c400cf3
Compare
There was a problem hiding this comment.
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)
vera/prelude.py (1)
563-571:⚠️ Potential issue | 🟠 MajorKeep
ArrayMapFninjection independent from the recursive helper bundle.Removing
array_mapfromarray_fn_namesmeans any module that defines the filter/fold helpers now skips_ARRAY_TYPE_ALIASESas well, soArrayMapFndisappears even though builtinarray_mapis still available. That is a source-compat break for code that annotates mapper values withArrayMapFn<...>.Suggested fix
- if not array_fn_names.issubset(user_names): - if not array_alias_names.issubset(user_names): - source_parts.append(_ARRAY_TYPE_ALIASES) + if not array_alias_names.issubset(user_names): + source_parts.append(_ARRAY_TYPE_ALIASES) + if not array_fn_names.issubset(user_names): source_parts.append(_ARRAY_COMBINATORS)Based on learnings: the prelude injection uses a deliberate two-layer design, and making bundles fully atomic is intentionally avoided because shadowing one array helper should not suppress unrelated declarations.
Also applies to: 589-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/prelude.py` around lines 563 - 571, The removal of "array_map" from array_fn_names inadvertently causes ArrayMapFn to be omitted because alias injection is tied to that bundle; restore ArrayMapFn injection independently by ensuring array_alias_names (or the alias bundle used by _ARRAY_TYPE_ALIASES) still contains "ArrayMapFn" regardless of whether "array_map" is present in array_fn_names—i.e., decouple the alias set from the recursive helper set so ArrayMapFn remains injected even when array_map was removed (apply the same adjustment wherever the array alias bundle is built, e.g., the other occurrence referenced around the later block).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vera/wasm/calls_arrays.py`:
- Around line 581-595: The current _infer_closure_return_vera_type
implementation only inspects syntax (ast.AnonFn) and returns None for other
valid call shapes; instead, when syntax doesn't expose types, look up the
resolved types produced by the typechecker for the closure and array arguments
and extract the function return type from that resolved function type (e.g., use
the resolved function Type/FuncType for closure_arg and the resolved
ArrayType/ElementType for array args), then map that resolved return/element
type via _resolve_type_name_to_wasm_canonical; apply the same pattern to the
similar helper around lines 633-646 so both element and mapper return types are
derived from resolved function/array types when AST inference is insufficient.
---
Outside diff comments:
In `@vera/prelude.py`:
- Around line 563-571: The removal of "array_map" from array_fn_names
inadvertently causes ArrayMapFn to be omitted because alias injection is tied to
that bundle; restore ArrayMapFn injection independently by ensuring
array_alias_names (or the alias bundle used by _ARRAY_TYPE_ALIASES) still
contains "ArrayMapFn" regardless of whether "array_map" is present in
array_fn_names—i.e., decouple the alias set from the recursive helper set so
ArrayMapFn remains injected even when array_map was removed (apply the same
adjustment wherever the array alias bundle is built, e.g., the other occurrence
referenced around the later block).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cc3a009-bd04-41cc-b69b-ccb223d4f023
📒 Files selected for processing (10)
CHANGELOG.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdtests/test_codegen_monomorphize.pytests/test_prelude.pyvera/codegen/modules.pyvera/prelude.pyvera/wasm/calls.pyvera/wasm/calls_arrays.py
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
vera/wasm/calls_arrays.py (1)
581-595:⚠️ Potential issue | 🟠 MajorDon’t make
array_mapdepend on syntax-only type inference.This still rejects valid, type-checked calls when
AorBis only known from the resolved checker state rather than the raw AST — for examplearray_map([], fn(@int->@Bool) { ... })or a let-bound mapper value. The old prelude path handled those shapes; this built-in path now returnsNoneinstead. Please fall back to the resolved array/closure types before bailing out here.Also applies to: 633-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/wasm/calls_arrays.py` around lines 581 - 595, The helper _infer_closure_return_vera_type currently only inspects raw AST (ast.AnonFn.return_type) and returns None for cases where types are resolved later; update it to fall back to the checker/resolved types when AST info is absent: after the existing AST branch, query the resolved type for the closure node (via the same resolver used elsewhere in this module — e.g., the checker/type_map attached to self or a helper like get_resolved_type(closure_arg)) and, if that resolved type is a function/closure type with a named return type, convert that name with _resolve_type_name_to_wasm_canonical and return it; apply the same fallback pattern to the sibling helper referenced around lines 633-646 (the array/element-type inference helper) so array element types also use resolved types before returning None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_codegen_monomorphize.py`:
- Line 947: The inline comment for the boolean vector example is contradictory
to the asserted outcome: update the comment that currently reads "-- [false,
false, true, true] → sum of true-indices = 2 + 3 = 5" (and the other occurrence
noted) so it matches the actual assertion/result in the test; specifically
ensure the comment for the array [false, false, true, true] correctly states the
sum of true-indices as 2 + 3 = 5 (or adjust to whatever the test asserts) and
make the same correction at the second occurrence referenced, keeping the
comments consistent with the asserted values in the test functions in
tests/test_codegen_monomorphize.py.
- Around line 899-905: Add a zero-length boundary regression to the "array_map —
regression tests for the iterative implementation" block: add a test that calls
the iterative array_map path with an empty input array and asserts an empty
output (covering captured closures, A != B pair-output and scalar Int→Bool
variants if the block tests those cases) so the loop initialisation/termination
is exercised. Locate the existing array_map regression block in
tests/test_codegen_monomorphize.py and insert an empty-array case (e.g.,
array_map([], ...) expecting []) alongside the existing
large/closure/type-change cases to ensure the zero-length edge is validated.
In `@tests/test_prelude.py`:
- Around line 50-53: The test suite removed direct guarding for
array_map/array_map_go by narrowing _ARRAY_FN_NAMES, so update the
test_array_operations_injected test to explicitly assert that "array_map" and
"array_map_go" are not present (i.e., absent from whatever injection registry or
the inspected symbols) to prevent accidental re-injection; locate
test_array_operations_injected and add presence checks for the two names
alongside existing assertions that use _ARRAY_FN_NAMES so the contract is
enforced even if _ARRAY_FN_NAMES changes.
In `@vera/prelude.py`:
- Around line 563-566: Update the public docstring of the function
inject_prelude to reflect current behavior: remove or change the claim that
array_map is "always injected" (array_map is now emitted as iterative WASM and
has no prelude body), and state that array_filter and array_fold remain injected
as recursive helpers until their iterative migration lands; ensure the
description matches the comment above about which helpers are emitted vs
injected and keep the docstring concise and accurate.
In `@vera/wasm/calls_arrays.py`:
- Around line 622-626: The generated WAT calls to $alloc in array_map (and the
similar block later) must be pre-guarded to avoid triggering the 16-bit
GC-header bug: compute the requested allocation size as arr_len * sizeof(B) and
if that value exceeds 65535 (i.e. >= 65536) trap (e.g. unreachable/wasm trap)
before invoking $alloc; update both guarded sites referenced in the diff (the
allocation call in array_map and the analogous allocation at the later block) so
the check uses the same sizeof(B) computation and branches to a trap path rather
than calling $alloc for oversized requests.
---
Duplicate comments:
In `@vera/wasm/calls_arrays.py`:
- Around line 581-595: The helper _infer_closure_return_vera_type currently only
inspects raw AST (ast.AnonFn.return_type) and returns None for cases where types
are resolved later; update it to fall back to the checker/resolved types when
AST info is absent: after the existing AST branch, query the resolved type for
the closure node (via the same resolver used elsewhere in this module — e.g.,
the checker/type_map attached to self or a helper like
get_resolved_type(closure_arg)) and, if that resolved type is a function/closure
type with a named return type, convert that name with
_resolve_type_name_to_wasm_canonical and return it; apply the same fallback
pattern to the sibling helper referenced around lines 633-646 (the
array/element-type inference helper) so array element types also use resolved
types before returning None.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f07d749a-19a9-4103-b8ed-2b7ff742aace
📒 Files selected for processing (10)
CHANGELOG.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdtests/test_codegen_monomorphize.pytests/test_prelude.pyvera/codegen/modules.pyvera/prelude.pyvera/wasm/calls.pyvera/wasm/calls_arrays.py
Owner
Author
|
@coderabbitai Verified — the coupling you describe is pre-existing and doesn't break any current test or example. Traced if not array_fn_names.issubset(user_names): # triggers ~always
if not array_alias_names.issubset(user_names): # triggers unless user shadows aliases
source_parts.append(_ARRAY_TYPE_ALIASES) # injects all 3 aliases
source_parts.append(_ARRAY_COMBINATORS)To skip alias injection, a user program has to shadow every name in Crucially, No current test, example, or conformance program hits that intersection. Decoupling would add a |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
Four small fixes from the review: - test_codegen_monomorphize.py: the Int→Bool test comment claimed "sum of true-indices = 2 + 3 = 5" but the expression sums 1 for each true and asserts 2 — corrected comment to match the actual computation. - test_codegen_monomorphize.py: added `test_array_map_empty_input` covering the n=0 boundary (loop's `idx >= arr_len` guard must fire on the first iteration; closure never invoked; output length 0). - test_prelude.py: `test_array_operations_injected` now explicitly asserts `array_map` and `array_map_go` are NOT in the injected names — regression guard against accidental re-injection if `_ARRAY_FN_NAMES` is ever widened without updating the prelude. - prelude.py: `inject_prelude` docstring updated — array_map is no longer always injected; it's emitted as iterative WASM by codegen (#480). array_filter / array_fold remain as recursive helpers until their own iterative migration lands. The suggestion to pre-guard `$alloc` with a 65535-byte trap in the array_map site was declined: the same silent-corruption risk applies to array_range, array_concat, array_append, array_slice today, so a per-site bandaid wouldn't make the system safer. The root fix is #484 (widen the 16-bit GC-header size field), which cures every call site in one change. Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_codegen_monomorphize.py (1)
974-975:⚠️ Potential issue | 🟡 MinorFix the contradictory expectation comment in the closure-capture test.
Line 974 says
102, but Line 975 correctly asserts103. Please align the comment with the assertion to avoid confusion during failures.Suggested fix
- # Outer `@Int.0` = 100 (captured); element 2 + captured 100 = 102 + # Outer `@Int.0` = 100 (captured); element 3 + captured 100 = 103🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen_monomorphize.py` around lines 974 - 975, The comment in the closure-capture test contradicts the assertion: update the inline comment that currently reads "element 2 + captured 100 = 102" to reflect the actual asserted result 103 so it matches the assertion assert _run(source, fn="main") == 103; locate the comment near the closure-capture check in tests/test_codegen_monomorphize.py (around the assertion that calls _run with fn="main") and change the numeric expectation to 103.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_codegen_monomorphize.py`:
- Around line 974-975: The comment in the closure-capture test contradicts the
assertion: update the inline comment that currently reads "element 2 + captured
100 = 102" to reflect the actual asserted result 103 so it matches the assertion
assert _run(source, fn="main") == 103; locate the comment near the
closure-capture check in tests/test_codegen_monomorphize.py (around the
assertion that calls _run with fn="main") and change the numeric expectation to
103.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4293abd-8e40-4a9a-b622-1397fa692e32
📒 Files selected for processing (5)
ROADMAP.mdTESTING.mdtests/test_codegen_monomorphize.pytests/test_prelude.pyvera/prelude.py
Comment claimed "element 2 + captured 100 = 102" but the test maps over [1, 2, 3] and reads index 2 (= 3), then adds the captured 100 → 103 (which is what the assertion checks). Comment now correctly states "[1, 2, 3][2] = 3; closure returns element + captured = 3 + 100 = 103". Co-Authored-By: Claude <noreply@anthropic.invalid>
Owner
Author
|
@coderabbitai Fixed in 839e41d — the closure-capture test comment now matches the assertion. # Outer @Int.0 = 100 (captured); [1, 2, 3][2] = 3;
# closure returns element + captured = 3 + 100 = 103.
assert _run(source, fn="main") == 103 |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
This was referenced Apr 17, 2026
This was referenced Apr 22, 2026
Merged
7 tasks
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
1 participant
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
array_map/array_map_gowith a single WATloopdriven bycall_indirecton the closure. Shadow-stack usage is now O(1) regardless of input length — the old recursive version hit the 16K ceiling (post-Type-checked pure program produces incorrect runtime output that isolated probes cannot reproduce #464) around 4K elements.forall<A, B>signature through the existingFunctionInfobuilt-in registration; source programs are unchanged.TestArrayOperations: 8K-element stress, Int→Bool type change, closure capturing outer variable, Int→String pair output.Discovered along the way
Filed #484 while debugging the stress test: the GC's object header size field is 16-bit, so allocations ≥65536 bytes make the sweeper link middle-of-payload bytes into the free list and clobber the live object. Test is capped at 8K Int elements (64,000 bytes) until #484 lands; full reproducer and fix options are in the issue.
Part of #480 — PR 2 (
array_filter) and PR 3 (array_fold) to follow.Test plan
mypy vera/clean (57 files)pytest tests/ -q— 3,312 passed, 11 skippedTestArrayOperationstests pass including the 4 new ones🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Known Issues
Tests
Documentation