fix(wasm): resolve nested fn-type aliases transitively for apply_fn lowering (#867)#880
fix(wasm): resolve nested fn-type aliases transitively for apply_fn lowering (#867)#880aallan wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (16)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds shared transitive resolution for function-type aliases, wires it into wasm inference and call handling, expands regression coverage, and updates the async-lowering spec, release notes, and conformance/test count documentation. ChangesAlias resolution fix and coverage
Conformance and test count documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #880 +/- ##
==========================================
- Coverage 93.01% 93.01% -0.01%
==========================================
Files 95 95
Lines 29232 29208 -24
Branches 360 360
==========================================
- Hits 27190 27167 -23
+ Misses 2036 2035 -1
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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: 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 `@vera/wasm/async_fusion.py`:
- Around line 169-236: resolve_fn_type_alias currently drops refinement-wrapped
fn aliases because it only returns a value when the alias is a plain ast.FnType
before any further unwrapping. Update the walker so that after peeling
ast.RefinementType layers from the alias body, it treats a wrapped fn as
terminal and returns that ast.FnType instead of falling through to None. Keep
the existing alias-chain handling in resolve_fn_type_alias and the related
substitution logic intact, but make sure refinement_type cases resolve the same
way as bare fn_type aliases.
🪄 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 Plus
Run ID: b104c382-5790-4264-b2e2-2414c64fe763
⛔ 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/**tests/conformance/ch05_nested_fn_type_alias.verais excluded by!**/*.vera
📒 Files selected for processing (13)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdspec/09-standard-library.mdtests/conformance/manifest.jsontests/test_codegen_closures.pytests/test_codegen_modules.pyvera/wasm/async_fusion.pyvera/wasm/inference.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
…eling; generic-HOF consultors routed through the shared resolver Two findings from the PR #880 review, both the #867 nested-alias class: Finding 1 (CodeRabbit, Major): `resolve_fn_type_alias` peeled `RefinementType` layers but did not treat a bared inline `FnType` as terminal — an alias whose body is a refinement directly wrapping a fn type (`type Foo = { @fn(String -> String) | p };`) fell through the `NamedType` check to None, and the caller fell to the i64 default: check-green, `vera run` trap (`type mismatch: expected i32, found i64`). An Int-returning closure ran by coincidence (the i64 default matches Int) — the String probe is the discriminator. The walk now checks `isinstance(te, ast.FnType)` at the top of each iteration, so a peeled FnType is terminal at any depth. Finding 2 (blast-radius skeptic, pre-existing, same class): the generic higher-order-fn path still did single-hop alias lookups in four places — `Monomorphizer._resolve_arg_fn_shape` / `_infer_fn_alias_type_args` (vera/monomorphize.py, instantiation discovery) and their WASM call-rewrite twins in vera/wasm/calls.py. A two-hop-alias-typed closure slot passed to a generic HOF (`my_map(@MyFn.0, 7)` with `type MyFn = InnerFn;`) failed shape resolution, so a closure-bound type param fell to the phantom-var default: wrong mono suffix (`my_map$Int_Bool`, result i32), check-green, run-trap (`expected i64, found i32`) — while the single-hop control ran. The HOF's own fn param declared through an alias chain (`type MapFn2<X, Y> = MapFn<X, Y>;`) hit the same gap on the param-lookup side. All four sites now route through `resolve_fn_type_alias`; the param side resolves the chain instantiated at the alias's own param names so positional matching survives renaming hops. The resolver moved from vera/wasm/async_fusion.py to the codegen-free vera/monomorphize.py (the #732 relocation precedent): the monomorphizer itself is now a consumer, and async_fusion imports vera.monomorphize, so hosting it in async_fusion would have made that import a cycle. The sweep of remaining `_type_aliases.get` + isinstance-FnType single-hop patterns also routed the fn-typed-slot classifier in `_infer_expr_wasm_type` (vera/wasm/inference.py); the other transitive walkers (`_reaches_fn_type`, `_canonical_named_type`, `_resolve_base_type_name`, registration.py resolvers) were verified already depth-N. Mutation-validated per site: restoring the pre-fix non-terminal peeled-FnType structure flips exactly the two refinement tests; un-routing any one of the four HOF sites flips exactly its discriminating test (arg-side x2 -> the two-hop run + mangle tests; param-side x2 -> the param-chain test); controls stay green throughout. Co-Authored-By: Claude <[email protected]>
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)
TESTING.md (1)
151-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the run-level conformance count here.
Line 151 still says 89 run-level programmes, but Line 180 now says 91. This sentence is stale and leaves the file internally inconsistent.
🤖 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 `@TESTING.md` around lines 151 - 155, Update the stale run-level conformance count in the TESTING.md summary so it matches the current total used elsewhere in the document. Adjust the `test_wasi_target.py` description text that mentions the dual-target conformance differential from 89 to the same value as the later conformance count, keeping the wording consistent and internally aligned.
🤖 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.
Outside diff comments:
In `@TESTING.md`:
- Around line 151-155: Update the stale run-level conformance count in the
TESTING.md summary so it matches the current total used elsewhere in the
document. Adjust the `test_wasi_target.py` description text that mentions the
dual-target conformance differential from 89 to the same value as the later
conformance count, keeping the wording consistent and internally aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d0ca779-17e7-4fbd-bca6-3fec39d5f662
📒 Files selected for processing (10)
CHANGELOG.mdROADMAP.mdTESTING.mdtests/test_codegen_closures.pytests/test_codegen_modules.pytests/test_codegen_monomorphize.pyvera/monomorphize.pyvera/wasm/async_fusion.pyvera/wasm/calls.pyvera/wasm/inference.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
aallan
force-pushed
the
fix/867-transitive-alias-fn
branch
from
4ee579e to
7831e46
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@FAQ.md`:
- Line 177: The FAQ still contains the old conformance-suite program count, so
update every remaining occurrence of the 108 literal to 109 throughout the file,
including the prose in the Conformance Suite section and any other free-form
references; use the existing conformance-suite wording around the Vera
documentation and test-suite descriptions to locate and replace all stale counts
consistently.
In `@TESTING.md`:
- Around line 9-11: The conformance suite count is stale in the prose, so the
document is internally inconsistent. Update every occurrence of the old 108
literal in TESTING.md, including the free-form paragraph under the Conformance
Suite section, so it matches the current 109 count shown in the overview table.
Use the Conformance Suite heading and the surrounding prose to locate and
replace any remaining old-count references.
🪄 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 Plus
Run ID: 519f9288-b69b-48c2-a993-b1fe5c978418
⛔ 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/**tests/conformance/ch05_nested_fn_type_alias.verais excluded by!**/*.vera
📒 Files selected for processing (16)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdspec/09-standard-library.mdtests/conformance/manifest.jsontests/test_codegen_closures.pytests/test_codegen_modules.pytests/test_codegen_monomorphize.pyvera/monomorphize.pyvera/wasm/async_fusion.pyvera/wasm/calls.pyvera/wasm/inference.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
|
@coderabbitai review |
✅ Action performedReview finished.
|
aallan
force-pushed
the
fix/867-transitive-alias-fn
branch
from
7831e46 to
60d398d
Compare
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)
1743-1771: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRefinement-wrapped
FnTypealiases still need the shared resolver
vera/wasm/context.pyandvera/wasm/data.pyboth feed annotated slot types into_slot_name_to_wasm_type, so a binding likelet@foo= ...withtype Foo = {@fn(String -> String) effects(pure) | true };still reaches thename in self._type_aliasesbranch, misses the bareast.FnTypecheck, and falls through toNone. That rejects a valid closure-typed slot. Route this branch throughresolve_fn_type_alias(...)so refinement-wrapped and chained aliases map toi32too.🤖 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 `@vera/wasm/inference.py` around lines 1743 - 1771, The slot type mapper in _slot_name_to_wasm_type is treating only bare ast.FnType aliases as closures, so refinement-wrapped or chained function aliases can fall through to None. Update the self._type_aliases branch to use resolve_fn_type_alias(...) before deciding the WASM type, and keep returning i32 for any alias that resolves to a function type. This lets annotated slots coming from context.py and data.py map correctly for cases like Foo-style alias refinements.
🤖 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.
Outside diff comments:
In `@vera/wasm/inference.py`:
- Around line 1743-1771: The slot type mapper in _slot_name_to_wasm_type is
treating only bare ast.FnType aliases as closures, so refinement-wrapped or
chained function aliases can fall through to None. Update the self._type_aliases
branch to use resolve_fn_type_alias(...) before deciding the WASM type, and keep
returning i32 for any alias that resolves to a function type. This lets
annotated slots coming from context.py and data.py map correctly for cases like
Foo-style alias refinements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3b0113b0-d931-4ab7-b411-1a40b9ba133a
⛔ 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/**tests/conformance/ch05_nested_fn_type_alias.verais excluded by!**/*.vera
📒 Files selected for processing (16)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdspec/09-standard-library.mdtests/conformance/manifest.jsontests/test_codegen_closures.pytests/test_codegen_modules.pytests/test_codegen_monomorphize.pyvera/monomorphize.pyvera/wasm/async_fusion.pyvera/wasm/calls.pyvera/wasm/inference.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
…ering, generic-HOF consultors, refinement peeling (#867) One shared cycle-guarded resolver (resolve_fn_type_alias, relocated to vera/monomorphize.py to avoid an import cycle): apply_fn closure-return consultors, the generic higher-order-function monomorphization sites (calls.py + monomorphize.py twins), and FnType-terminal after refinement unwrapping. Flips the pre-existing still-fails-loud #867 pin to awaited-correctly. PR #880 review, 4th single-hop site: _slot_name_to_wasm_type (vera/wasm/inference.py) classified only a BARE FnType alias as an i32 closure pointer, so a let/param slot annotated with a refinement-wrapped fn alias (type Foo = { @fn(...) | p };) or one chained through a refinement resolved to None — the binding was rejected with CodegenSkip ("has no WASM representation") and its whole function dropped, on a check/verify-green program. A plain NamedType chain was already handled by the _resolve_base_type_name prefix, so only the refinement-involving shapes remained. Routed through the same shared resolver; mutation-kill: un-routing it flips exactly its two refinement-slot tests, all others green. Co-Authored-By: Claude <[email protected]>
aallan
force-pushed
the
fix/867-transitive-alias-fn
branch
from
60d398d to
4327251
Compare
|
@coderabbitai On the outside-diff |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5 4327251 2>/dev/null || echo "commit not found locally"
git show 4327251 --stat 2>/dev/null | head -30
echo "---"
rg -n "_slot_name_to_wasm_type|resolve_fn_type_alias" vera/wasm/inference.py -A5 -B5 | head -100Length of output: 7552 Confirmed — commit 4327251 is present on the branch and
|
1 participant
Summary
A nested type-alias chain to a
fntype (type Inner = fn(...); type Mapper = Inner;) used as anapply_fnclosure argument resolved only ONE alias hop in every closure-arg return-type consultor — the chain landed on aNamedType, the consultor bailed to its default, and a check-green program failed at run:call_indirectemitted against the wrong signature (type mismatch: expected i32, found i64trap) on the sync path, and the fused-await classifier silently skippedasync_awaiton the async path (silent wrong value — the #843-family failure at depth ≥ 2).The fix — one shared transitive resolver
resolve_fn_type_alias(te, type_aliases, type_alias_params)invera/wasm/async_fusion.py: walks the alias chain depth-N to the terminalFnType, unwrappingRefinementTypelayers and substituting each generic alias's type params at every hop, with aseencycle guard returningNoneon malformed cyclic aliases (the checker already rejects those; the resolver must not infinite-loop regardless). All three consultors route through it —_infer_apply_fn_return_type,_infer_fncall_vera_type's apply_fn arm, and_apply_fn_closure_ret_type— completing the shared-resolver consolidation scoped on the issue (PR #868 follow-through), so depth-N behaviour cannot drift between sites.Evidence
call_indirecti32/i64 trap; two- and three-hopFuture<Result<String,String>>alias → fused-await classifier emits noasync_awaitimport (silent identity await). Single-hop control runs correctly — the bug is specifically transitivity.ch05_nested_fn_type_aliason main = check exit 0, run exit 1 withtype mismatch: expected i32, found i64; on the branch = run exit 0, correct output.seenguard removed → the 3 cycle-guard tests time out RED under a SIGALRM watchdog._still_fails_loudNested type alias to a fn type breaks apply_fn lowering: single-level alias resolution emits invalid WASM #867 pin test is flipped to_awaited_correctly.Gates
Full non-stress suite 5915 passed / 43 skipped; conformance 109/109 (new run-level
ch05_nested_fn_type_alias); examples 37/37; mypy clean; ruff (incl.--select S) clean; all doc gates + doc counts + site assets regenerated.Docs
CHANGELOG
[Unreleased]; exactly the #867 KNOWN_ISSUES Bugs row removed; spec §9.5.4 + TESTING.md updated. No version bump (rides the v0.1.0 burndown).Closes #867
🤖 Generated with Claude Code
Summary by CodeRabbit
apply_fn/indirect-call handling for nested (transitive) function-type aliases, including generic substitution and cycle-safe resolution, preventing invalid indirect-call closure signatures (e.g., mismatched value widths).fntype aliases, including behaviour through cyclic alias graphs.