fix: closure i32_pair param/return types and host-import propagation (#359)#454
Merged
fix: closure i32_pair param/return types and host-import propagation (#359)#454
Conversation
…359) Four bugs that caused WAT compilation failures when String or Array values appear as closure parameters, return types, or accumulator types: 1. codegen/closures.py: lifted function declarations now emit two consecutive i32 params for i32_pair (String/Array) — (param $p_ptr i32)(param $p_len i32). 2. wasm/closures.py: call_indirect type descriptor now emits two (param i32) entries per i32_pair argument. 3. wasm/inference.py: _infer_apply_fn_return_type handles AnonFn literals directly, returning "i32_pair" for String/Array return types to prevent $closure_sig_N naming collisions. 4. wasm/inference.py: _infer_fncall_vera_type calls _format_named_type instead of ta.name, so parameterised accumulators like Map<String, Int> produce the correct monomorphised call target (array_fold_go$String_Map_String_Int). 5. codegen/closures.py: _compile_lifted_closure propagates all host-import tracking (Map/Set/Decimal/Json/Html) from the closure WasmContext to the module codegen, so imports used inside closure bodies are registered. Fixes: vera run examples/collections.vera now returns 6. Tests: 3 new regression tests in TestClosureI32PairParams. Bumps version to 0.0.109. Co-Authored-By: Claude <noreply@anthropic.invalid>
📝 WalkthroughWalkthroughBumps Vera to v0.0.109 and fixes closure WASM ABI and inference: i32_pair-backed parameters/returns are expanded to two i32 slots in signatures, host-import usage from closures is propagated to module tracking, and apply_fn/closure return-type inference preserves parameterised named types. New regression tests added. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #454 +/- ##
==========================================
+ Coverage 90.32% 90.34% +0.02%
==========================================
Files 50 50
Lines 19459 19508 +49
Branches 225 225
==========================================
+ Hits 17576 17625 +49
Misses 1879 1879
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@HISTORY.md`:
- Line 233: The release summary is stale after adding v0.0.109; update the
global release-total sentence in HISTORY.md to reflect 109 tagged releases
(replace "108 tagged releases" with "109 tagged releases") so the document
matches the new changelog entry and maintain factual accuracy for the release
count.
In `@tests/test_codegen.py`:
- Around line 10306-10329: Update the test_array_fold_with_map_accumulator test
to also exercise the zero-iteration path: add a second assertion that calls
array_fold with an empty `@Array`<String> ([]) and a `@Map`<String, Int> seed from
map_new(), using the same folding closure (map_insert) so the accumulator
monomorphisation path for Map<String, Int> is exercised with an empty input, and
assert the resulting map_size is 0; keep the existing non-empty case intact and
reuse the same symbols (array_fold, map_new, map_insert, map_size, Map<String,
Int>) so the added case lives in the same test method.
In `@vera/wasm/inference.py`:
- Around line 841-849: The anon function return handling (closure_arg of type
ast.AnonFn) currently checks ret as ast.NamedType and calls
_named_type_to_wasm(ret.name), which misses alias/refinement types that
ultimately represent String/Array; update the logic so that when ret is
ast.NamedType you resolve its alias/refinement to the canonical underlying type
(using the project's type-resolution utilities or symbol table) and if that
resolved name is "String" or "Array" return "i32_pair" before falling back to
_named_type_to_wasm; reference closure_arg, ast.AnonFn, ret (ast.NamedType), and
_named_type_to_wasm when making the change.
🪄 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: eb366f4e-bdcc-43b1-80e1-ae2d986d7dff
⛔ Files ignored due to path filters (5)
docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**docs/sitemap.xmlis excluded by!docs/**
📒 Files selected for processing (11)
CHANGELOG.mdHISTORY.mdREADME.mdROADMAP.mdTESTING.mdpyproject.tomltests/test_codegen.pyvera/__init__.pyvera/codegen/closures.pyvera/wasm/closures.pyvera/wasm/inference.py
- Update uv.lock after version bump to 0.0.109 (fixes lint CI) - HISTORY.md: update total release count to 109 - wasm/inference.py: _infer_apply_fn_return_type peels RefinementType and resolves type aliases before checking for String/Array i32_pair - tests/test_codegen.py: extend test_array_fold_with_map_accumulator with zero-iteration (empty array) path Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vera/wasm/inference.py (1)
587-594:⚠️ Potential issue | 🟠 MajorPreserve wrapper type arguments when inferring
apply_fn’s Vera type.Lines 587-594 only keep type arguments when the alias return is a bare type variable. If the closure returns something like
Option<T>,Result<E, T>, or even a concreteMap<String, Int>, theret.name/alias_te.return_type.namefallback still strips the type arguments. Downstream monomorphisation will then resolve againstOption/Result/Mapinstead of the actual specialisation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/wasm/inference.py` around lines 587 - 594, The code currently drops wrapper type arguments by returning ret.name or alias_te.return_type.name; instead, when a NamedType has type arguments preserve them by formatting via the existing helper: when ret is a NamedType look up alias_map and if not substituting, return self._format_named_type(ret) (rather than ret.name); similarly, when alias_te.return_type is a NamedType return self._format_named_type(alias_te.return_type) so wrapper generics like Option<T>, Result<E,T>, Map<String,Int> are retained for downstream monomorphisation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@HISTORY.md`:
- Line 270: Update the summary footer string "Total: **630+ commits, 109 tagged
releases, 31 active development days.**" to reflect the new count of development
days by changing "31 active development days" to "32 active development days" in
HISTORY.md (the line containing that exact summary text updated for v0.0.109).
In `@TESTING.md`:
- Line 9: The Tests headline currently states "| **Tests** | 3,233 across 27
files (~34,000 lines of test code) |" but later mentions 11 skipped tests;
update this headline in TESTING.md to either indicate the number of
executed/passing tests (e.g., "3,222 passed, 11 skipped across 27 files") or
append the skips (e.g., "3,233 across 27 files (11 skipped)"), ensuring the text
in the Tests row and the skipped-tests section are consistent.
In `@tests/test_codegen.py`:
- Around line 10275-10341: Add two tests in TestClosureI32PairParams in
tests/test_codegen.py to cover Array i32_pair ABI: one that passes an `@Array`<T>
as the closure parameter to apply_fn (mirror test_closure_string_param_compiles
but with `@Array`<String> and check the produced value via a consumer like
array_length or element access) and one that has a closure returning `@Array`<T>
used by apply_fn (mirror test_closure_string_return_compiles but returning
`@Array`<String> and then inspect its length/contents), ensuring you assert
_run(...) equals the expected integer result and include an empty-array path to
exercise zero-length handling; locate additions near existing tests (e.g., next
to test_closure_string_param_compiles/test_closure_string_return_compiles) and
keep naming consistent like test_closure_array_param_compiles and
test_closure_array_return_compiles.
In `@vera/wasm/inference.py`:
- Around line 841-859: The bug: named closures returning pair-ABI types
(String/Array) get mapped to a single i32 because the SlotRef path uses
_resolve_generic_fn_return() / _fn_type_return_wasm() which delegates to
_named_type_to_wasm() that currently returns i32 for those types; fix by making
the SlotRef/generic-return path apply the same pair-ABI logic as the
anon-literal branch. Concretely, update either _fn_type_return_wasm() or
_resolve_generic_fn_return() to detect when the resolved return is a NamedType
with name "String" or "Array" and return "i32_pair" (or alter
_named_type_to_wasm() so it returns "i32_pair" for resolved_name == "String" or
"Array"), mirroring the logic in the anon closure branch that peels
RefinementType and checks resolved_name; ensure the same
RefinementType-unwrapping is applied before name checks so named closure returns
use pair-ABI call_indirect signatures.
---
Outside diff comments:
In `@vera/wasm/inference.py`:
- Around line 587-594: The code currently drops wrapper type arguments by
returning ret.name or alias_te.return_type.name; instead, when a NamedType has
type arguments preserve them by formatting via the existing helper: when ret is
a NamedType look up alias_map and if not substituting, return
self._format_named_type(ret) (rather than ret.name); similarly, when
alias_te.return_type is a NamedType return
self._format_named_type(alias_te.return_type) so wrapper generics like
Option<T>, Result<E,T>, Map<String,Int> are retained for downstream
monomorphisation.
🪄 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: 2644e9d4-399a-4ada-92fc-a01af7be03b7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (4)
HISTORY.mdTESTING.mdtests/test_codegen.pyvera/wasm/inference.py
- HISTORY.md: update active development days count 31 → 32 - TESTING.md: clarify test count as "3,235 (3,224 passed, 11 skipped)" and update test_codegen.py row; ROADMAP.md and README.md counts updated - wasm/inference.py: fix SlotRef closure path — _fn_type_return_wasm and _resolve_generic_fn_return now return "i32_pair" for String/Array return types (with RefinementType peeling), matching the AnonFn branch - wasm/inference.py: _infer_fncall_vera_type lines 592/594 now call _format_named_type() instead of .name to preserve type arguments on non-substituted and non-generic return types (e.g. Option<T>, Map<K,V>) - tests/test_codegen.py: add test_closure_array_param_compiles and test_closure_array_return_compiles mirroring the String pair-ABI tests Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vera/wasm/inference.py (1)
841-859:⚠️ Potential issue | 🔴 CriticalResolve aliases before applying the pair-ABI special-case.
These branches still test raw names against
("String", "Array")and otherwise fall through to_named_type_to_wasm(). A direct alias such astype Text = String, or any alias/refinement chain ending inString/Array<T>, will still be inferred as a singlei32. That recreates the invalid closurecall_indirectsignature this PR is fixing.🛠️ Proposed fix
if isinstance(closure_arg, ast.AnonFn): # Closure literal passed directly — infer return type from its # declared return TypeExpr so call_indirect sig matches the # lifted function's actual return (i32_pair for String/Array). ret: ast.TypeExpr = closure_arg.return_type # Peel refinement wrapper to get the underlying named type. if isinstance(ret, ast.RefinementType): ret = ret.base_type if isinstance(ret, ast.NamedType): - # Resolve type aliases (e.g. a refined alias for String). - resolved_name = ret.name - alias = self._type_aliases.get(ret.name) - if isinstance(alias, ast.RefinementType): - inner = alias.base_type - if isinstance(inner, ast.NamedType): - resolved_name = inner.name - if resolved_name in ("String", "Array"): + resolved_name = self._resolve_base_type_name(ret.name) + if self._is_pair_type_name(resolved_name): return "i32_pair" return self._named_type_to_wasm(resolved_name) @@ ret = fn_type.return_type if isinstance(ret, ast.RefinementType): ret = ret.base_type if isinstance(ret, ast.NamedType): # If the return type is a type variable, substitute it - name = subst.get(ret.name, ret.name) - if name in ("String", "Array"): + name = self._resolve_base_type_name(subst.get(ret.name, ret.name)) + if self._is_pair_type_name(name): return "i32_pair" return self._named_type_to_wasm(name) @@ ret = fn_type.return_type if isinstance(ret, ast.RefinementType): ret = ret.base_type if isinstance(ret, ast.NamedType): - if ret.name in ("String", "Array"): + resolved_name = self._resolve_base_type_name(ret.name) + if self._is_pair_type_name(resolved_name): return "i32_pair" - return self._named_type_to_wasm(ret.name) + return self._named_type_to_wasm(resolved_name)Also applies to: 880-887, 906-911
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/wasm/inference.py` around lines 841 - 859, The code checks raw named type names against ("String","Array") before fully resolving type aliases/refinements, so alias chains like type Text = String are treated as scalar i32; update the logic in the closure handling (the branch for isinstance(closure_arg, ast.AnonFn)) to fully resolve aliases/refinements first (follow self._type_aliases for potential nested/refined aliases and peel any ast.RefinementType/ast.NamedType layers to obtain the final underlying named type) and only then apply the special-case that maps "String" or "Array" to "i32_pair" (otherwise call self._named_type_to_wasm); replicate the same alias-resolution-before-check in the analogous blocks referenced around the other occurrences (the similar branches at the later locations noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@HISTORY.md`:
- Line 270: Update the opening summary in HISTORY.md so the active
development-day count matches the new total shown later; replace the old "31"
with "32" in the intro line so it is consistent with the line that now reads
"Total: **630+ commits, 109 tagged releases, 32 active development days.**".
---
Duplicate comments:
In `@vera/wasm/inference.py`:
- Around line 841-859: The code checks raw named type names against
("String","Array") before fully resolving type aliases/refinements, so alias
chains like type Text = String are treated as scalar i32; update the logic in
the closure handling (the branch for isinstance(closure_arg, ast.AnonFn)) to
fully resolve aliases/refinements first (follow self._type_aliases for potential
nested/refined aliases and peel any ast.RefinementType/ast.NamedType layers to
obtain the final underlying named type) and only then apply the special-case
that maps "String" or "Array" to "i32_pair" (otherwise call
self._named_type_to_wasm); replicate the same alias-resolution-before-check in
the analogous blocks referenced around the other occurrences (the similar
branches at the later locations noted).
🪄 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: 58fb6a99-89d4-43db-80a8-d077b997231f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (6)
HISTORY.mdREADME.mdROADMAP.mdTESTING.mdtests/test_codegen.pyvera/wasm/inference.py
- HISTORY.md: update intro line to "32 active development days" (was 31) - wasm/inference.py: add _resolve_type_name_to_wasm_canonical helper that follows alias chains (RefinementType and NamedType) to the canonical type, with a cycle guard; used in AnonFn branch, _fn_type_return_wasm, and _resolve_generic_fn_return to correctly map String/Array aliases to i32_pair - Mark unreachable defensive branches with pragma: no cover Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vera/wasm/inference.py (1)
587-594:⚠️ Potential issue | 🟠 MajorRecursively substitute type variables inside constructed return types.
This only fixes the case where the alias return type is the type variable. If the closure alias returns something like
Result<B, String>orOption<Map<K, V>>, this branch now formats the unresolved template type and still leaksB/K/Vinto the inferred Vera type, which can mis-mangle downstream higher-order call targets.Proposed direction
- if isinstance(ret, ast.NamedType): - if ret.name in alias_map: - ta = alias_map[ret.name] - if isinstance(ta, ast.NamedType): - return self._format_named_type(ta) - return self._format_named_type(ret) + if isinstance(ret, ast.NamedType): + resolved_ret = self._substitute_named_type_vars( + ret, alias_map, + ) + return self._format_named_type(resolved_ret)def _substitute_named_type_vars( self, te: ast.NamedType, alias_map: dict[str, ast.TypeExpr], ) -> ast.NamedType: if te.name in alias_map and isinstance(alias_map[te.name], ast.NamedType): return cast(ast.NamedType, alias_map[te.name]) new_args: list[ast.TypeExpr] = [] for arg in te.type_args: if isinstance(arg, ast.NamedType): new_args.append(self._substitute_named_type_vars(arg, alias_map)) else: new_args.append(arg) return ast.NamedType(te.name, tuple(new_args))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/wasm/inference.py` around lines 587 - 594, The return-type branch currently only substitutes when the alias return is exactly a type variable and fails to recurse into constructed types (e.g., Result<B,String>), so add a helper like _substitute_named_type_vars(self, te: ast.NamedType, alias_map: dict[str, ast.TypeExpr]) that recursively walks te.type_args and replaces any NamedType whose name is in alias_map with the mapped type (and recurses into its type_args), then use this helper when handling both the local ret variable and alias_te.return_type before calling _format_named_type so nested template parameters (B/K/V) are substituted away instead of leaked; reference the existing _format_named_type, alias_map, alias_te.return_type and ret when applying the substitution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@vera/wasm/inference.py`:
- Around line 587-594: The return-type branch currently only substitutes when
the alias return is exactly a type variable and fails to recurse into
constructed types (e.g., Result<B,String>), so add a helper like
_substitute_named_type_vars(self, te: ast.NamedType, alias_map: dict[str,
ast.TypeExpr]) that recursively walks te.type_args and replaces any NamedType
whose name is in alias_map with the mapped type (and recurses into its
type_args), then use this helper when handling both the local ret variable and
alias_te.return_type before calling _format_named_type so nested template
parameters (B/K/V) are substituted away instead of leaked; reference the
existing _format_named_type, alias_map, alias_te.return_type and ret when
applying the substitution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1b59bc9-5b9d-43cb-aa9e-715e73f38560
📒 Files selected for processing (2)
HISTORY.mdvera/wasm/inference.py
This was referenced Apr 17, 2026
6 tasks
This was referenced May 5, 2026
Merged
Merged
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
Fixes five bugs causing WAT compilation failures when
StringorArrayvalues appear as closure parameters, return types, or accumulator types inarray_foldand similar higher-order functions.Root cause: the closure lifting path in
codegen/closures.pyand thecall_indirecttype descriptor inwasm/closures.pyeach treatedi32_pairtypes (String/Array) as a single WASM value slot rather than the required two consecutivei32slots. Additionally, host imports used inside closure bodies were never propagated to the module-level tracker.Observable fix:
vera run examples/collections.veranow returns6correctly.Changes
vera/codegen/closures.py— lifted function declarations emit(param $p_ptr i32)(param $p_len i32)fori32_pairparams;_compile_lifted_closurepropagates all host-import tracking (Map/Set/Decimal/Json/Html) from the closureWasmContextback to the module codegen.vera/wasm/closures.py—call_indirecttype descriptor emits two(param i32)entries peri32_pairargument.vera/wasm/inference.py—_infer_apply_fn_return_typehandlesAnonFnliterals directly to return"i32_pair"forString/Arrayreturn types, preventing$closure_sig_Nnaming collisions;_infer_fncall_vera_typecalls_format_named_typeinstead of returningta.nameso parameterised accumulators likeMap<String, Int>produce the correct monomorphised call target (array_fold_go$String_Map_String_Int).Test plan
TestClosureI32PairParams::test_closure_string_param_compiles— closure withStringparam emits valid(param i32 i32)WATTestClosureI32PairParams::test_closure_string_return_compiles— closure withStringreturn emits valid(result i32 i32)WATTestClosureI32PairParams::test_array_fold_with_map_accumulator—array_foldoverArray<String>withMap<String, Int>accumulator callingmap_insertinside the fold closurevera run examples/collections.vera→6mypy vera/— cleanpython scripts/check_conformance.py— all 73 conformance programs passCloses #359
🤖 Generated with Claude Code
Summary by CodeRabbit
Release v0.0.109
Bug Fixes
Tests
Chores