fix(wasm): fused-async await classifies indirectly-called closures by declared return type#868
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR fixes fused-async ChangesIndirect closure await classification fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CallsMarkup as calls_markup._translate_await
participant Compilability as compilability._scan_io_ops
participant AsyncFusion as async_fusion.await_needs_check
participant ClosureResolver as _apply_fn_closure_ret_type
CallsMarkup->>AsyncFusion: await_needs_check(arg, future_ret_fns, future_ret_module_fns, type_aliases)
Compilability->>AsyncFusion: await_needs_check(arg, future_ret_fns, future_ret_module_fns, type_aliases)
AsyncFusion->>ClosureResolver: resolve apply_fn closure declared return type
ClosureResolver-->>AsyncFusion: TypeExpr or None
AsyncFusion-->>AsyncFusion: apply_fn_awaits_fused_future checks Future<Result<String,String>>
AsyncFusion-->>CallsMarkup: bool needs fused-handle check
AsyncFusion-->>Compilability: bool registers async_await op
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ 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
🤖 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 168-231: The closure-arg shape handling in
_apply_fn_closure_ret_type is duplicated from vera/wasm/inference.py, which
risks fused-await classification drifting from apply_fn lowering if supported
shapes change. Factor the shared “resolve declared return TypeExpr from closure
arg” logic into one canonical helper and have both _apply_fn_closure_ret_type
and the inference path reuse it, while keeping each caller’s own
translation/classification logic separate.
🪄 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: 4035b8de-d7ed-4e5f-aeab-8c9632bd3da2
📒 Files selected for processing (9)
CHANGELOG.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdspec/09-standard-library.mdtests/test_codegen_modules.pyvera/codegen/compilability.pyvera/wasm/async_fusion.pyvera/wasm/calls_markup.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…lassification (PR #868 panel) The PR #868 review panel found a live silent wrong value on the #843 fix: a GENERIC FnType alias instantiated to the fused type at the slot — `type Producer<T> = fn(String -> T) effects(<Http, Async>);` awaited as `await(apply_fn(@Producer<Future<Result<String, String>>>.0, url))` — classified as unresolvable because `_apply_fn_closure_ret_type` returned the alias's RAW bare-`T` return with no type-arg substitution, while `_infer_apply_fn_return_type` DOES substitute via alias_map and builds a VALID call_indirect signature. Result: no E616, no WASM trap, identity await, wrapper read as the ADT — check+verify+run all green, returning 0 where the non-generic equivalent returns 1, with zero `async_await` references in the WAT instead of the import + probe call. Fix: `_substitute_type_params` (the TypeExpr-level twin of the inference's alias_map substitution, same guard) applied in `_apply_fn_closure_ret_type`; `type_alias_params` threaded through `await_needs_check` from both call sites. The two closure-type consultors now genuinely match — which IS the floor invariant. Also per the panel: - The #867 nested-alias shape is pinned still-loud (compiles, traps at WASM validation — the substitution must not resolve it into a silent path), with docstrings/spec §9.5.4 corrected to state BOTH backstops precisely: [E616] for FnCall-shaped closure args, the #867 call_indirect width-mismatch validation trap for alias-of-alias slots (the previous wording over-claimed E616 for all unresolvable shapes). - Ok-path pinned: a local-HTTP-server test requires the response payload byte-exact through the indirect await (the Err-substring tests could not discriminate Ok-corruption from the same 0). - A comment on the compilability pre-scan threading notes it is redundant-but-benign next to calls_markup's single registration site. Mutation-validated: disabling the generic substitution flips exactly the generic-alias WAT pin RED; disabling the whole apply_fn arm flips the slot, anon, and Ok-path tests RED. Co-Authored-By: Claude <[email protected]>
aallan
force-pushed
the
fix/843-fused-await-loud
branch
from
840a770 to
79ccb42
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
==========================================
- Coverage 92.96% 92.96% -0.01%
==========================================
Files 95 95
Lines 28914 28945 +31
Branches 360 360
==========================================
+ Hits 26881 26909 +28
- Misses 2027 2030 +3
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 review |
✅ Action performedReview finished.
|
… declared return type (#843) Squash of the fix + the #867 discovery row + the panel round (generic-alias substitution restoring the floor invariant, per-shape backstop wording, Ok-path pin). Co-Authored-By: Claude <[email protected]>
aallan
force-pushed
the
fix/843-fused-await-loud
branch
from
79ccb42 to
edb962b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Burndown PR for #843 (promoted limitation→bug: silent wrong value). Under the #841 fused-async lowering, an inline
awaitaround an indirectly-called closure returningFuture<Result<String,String>>lowered to identity — the fused wrapper was read as the ADT, theErrpayload came out as a zero-length string, and the request outcome was silently discarded.Shipped the ceiling (correct classification), with the loud floor proven already-guaranteed:
await_needs_checkgains anapply_fnarm classifying by the closure's declared return type (fn-typed slot through itsFnTypealias, or inlineAnonFn) — the post-apply_fn is unregistered in the checker: spurious E200 on every use, arity/type errors swallowed, failures leak raw WAT assembler errors #854 declared-type precedent. Resolvable indirect awaits now emit the fused-handle check and run correctly._infer_apply_fn_return_type's, so any closure shape it can't resolve is exactly what_translate_apply_fnalready rejects loudly with E616 — verified empirically. No silent path to an identity await remains (differential soundness relationship between the two consultors).await_needs_checkcall sites (calls_markup + compilability) thread type aliases, keeping the two import-emission passes in agreement.0(outcome discarded, noasync_awaitimport in the WAT) on the unfixed tree,1post-fix with the import present; the let-bind workaround control is green on both sides. Mutation kill: disabling the arm flips both classification tests RED with controls green.await(apply_fn(...))shapes anywhere in conformance/examples/docs — purely additive.Discovered during the fix and filed as #867 (row added here as the discovering PR): nested type aliases to fn types break apply_fn lowering (loud WASM validation failure, pre-existing single-level alias resolution). Queued separately.
Orchestrator flip: 2 RED against main's fusion code, 4/4 green on the branch.
Closes #843
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
awaiton indirectly called closures could return the wrong value or fail silently.awaitnow behaves correctly for supported fused-async closure patterns, while unsupported cases are rejected more clearly.Documentation
Tests
awaithandling, runtime checks, and the documented workaround.