fix: isolate nested handle[State<T>] via push/pop host protocol (#417)#453
Merged
Conversation
Each handle[State<T>] now calls state_push_T on entry and state_pop_T on exit, giving every handler its own independent state cell. Nested handlers of the same type no longer share a global cell. Changes: - vera/codegen/api.py: state_store becomes dict[str, list[int|float]]; get/put operate on top-of-stack; new state_push_T/state_pop_T host fns - vera/codegen/assembly.py: emit state_push_T/state_pop_T WAT imports - vera/wasm/calls.py: emit push before init, pop after body - vera/wasm/context.py: add HandleExpr to _is_void_expr (fixes spurious drop for void-body handlers used as statements) - vera/browser/runtime.mjs: stateCells becomes stacks; add push/pop bindings - tests/test_codegen.py: 3 new nested State<T> isolation tests - KNOWN_ISSUES.md: remove #417 (fixed) - HISTORY.md: add 9 Apr bug-fix entry - TESTING.md / ROADMAP.md: update test counts (3227 → 3230) Co-Authored-By: Claude <noreply@anthropic.invalid>
|
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 (1)
📝 WalkthroughWalkthroughSwitch State from single global cells to per-handler stacks with Changes
Sequence DiagramsequenceDiagram
participant Runtime as Runtime (Browser / Python)
participant Module as WASM Module
participant Stack as State Stack (per-key)
Note over Runtime,Stack: Enter outer handle[State<T>]
Runtime->>Module: vera.state_push_Int()
Module->>Stack: push outer value (stack: [10])
Runtime->>Module: vera.state_get_Int()
Module-->>Runtime: return top -> 10
Note over Runtime,Stack: Enter inner handle[State<T>]
Runtime->>Module: vera.state_push_Int()
Module->>Stack: push inner value (stack: [10,99])
Runtime->>Module: vera.state_put_Int(42)
Module->>Stack: write top -> 42 (stack: [10,42])
Note over Runtime,Stack: Exit inner handler
Runtime->>Module: vera.state_pop_Int()
Module->>Stack: pop -> restores top (stack: [10])
Runtime->>Module: vera.state_get_Int()
Module-->>Runtime: return top -> 10
Note over Runtime,Stack: Exit outer handler
Runtime->>Module: vera.state_pop_Int()
Module->>Stack: pop -> stack restored/default
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
- Coverage 90.32% 90.32% -0.01%
==========================================
Files 50 50
Lines 19419 19459 +40
Branches 220 225 +5
==========================================
+ Hits 17541 17576 +35
- Misses 1874 1879 +5
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:
|
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/wasm/calls.py (1)
8242-8274:⚠️ Potential issue | 🔴 CriticalBracket the pushed state frame around the body, not just the happy path.
Line 8244 pushes the new
State<T>cell beforeinit_expris evaluated, so a nested same-type initialiser such ashandle[State<Int>](@int= get(()))now reads the fresh default cell instead of the outer handler’s state. Then Line 8274 only pops on fall-through, so if the body reuses an outerthrowmapping and unwinds to an enclosinghandle[Exn<_>], the pushed cell leaks and laterget/putcalls keep talking to the wrong frame. Please evaluate the initialiser first, then push+initialise the new cell, and make the pop run on both normal and exceptional exits.
🤖 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.py`:
- Around line 2831-2852: The test test_nested_state_inner_does_not_corrupt_outer
currently returns the inner handler's value (999) and doesn't verify the outer
handler's state; update the src string in this test so that after the inner
handle[State<Int>] block finishes you perform another get(()) in the outer
handler to read the outer state (initialized with `@Int` = 5) and return that
value, then assert _run(src, "test") == 5 to confirm the inner put did not
corrupt the outer state; locate the test by the function name
test_nested_state_inner_does_not_corrupt_outer and the use of _run and modify
the nested handle bodies accordingly.
🪄 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: f1ed239c-7358-4383-93c6-454ce07b2ff2
📒 Files selected for processing (10)
HISTORY.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdtests/test_codegen.pyvera/browser/runtime.mjsvera/codegen/api.pyvera/codegen/assembly.pyvera/wasm/calls.pyvera/wasm/context.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
…r state Inner body now discards its result and outer body reads get(()) after, asserting outer state (5) was not corrupted by inner put(999). Also condense 9 Apr HISTORY.md entries to one line per project convention. Co-Authored-By: Claude <noreply@anthropic.invalid>
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)
tests/test_codegen.py (1)
2799-2805: 🧹 Nitpick | 🔵 TrivialAssert the actual import declarations.
These checks still pass if the names only appear in call sites or symbol references. Please assert the concrete import lines so this test proves the host imports are emitted, not just mentioned.
Suggested tightening
- assert "state_get_Int" in result.wat - assert "state_put_Int" in result.wat - assert "state_push_Int" in result.wat - assert "state_pop_Int" in result.wat + assert 'import "vera" "state_get_Int"' in result.wat + assert 'import "vera" "state_put_Int"' in result.wat + assert 'import "vera" "state_push_Int"' in result.wat + assert 'import "vera" "state_pop_Int"' in result.watBased on learnings:
tests/test_codegen.pyincludes explicit WAT import‑gating coverage for JSON host imports — tests verify that imports are emitted only when referenced and absent when unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 2799 - 2805, The current test only checks that the symbols (state_get_Int, state_put_Int, state_push_Int, state_pop_Int) appear anywhere in result.wat; update the assertions to verify concrete WAT import declarations are emitted by asserting the actual import lines for those hosts instead of mere occurrences — e.g. assert that result.wat contains import declarations that include the import prefix and each function name (look for '(import' plus the host function names) so the test proves the host imports are emitted, not just referenced; update the assertions around result.wat where state_get_Int/state_put_Int/state_push_Int/state_pop_Int are checked to perform string matches for import declarations.
🤖 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.py`:
- Around line 2807-2875: Modify one of the three new tests (e.g.,
test_nested_state_outer_readable_after_inner) so the inner handle[State<Int>]
produces and returns a value that the outer block consumes instead of being used
as a Unit statement; specifically, change the inner in { ... } to evaluate to an
Int (for example return the value put by the inner handler) and have the outer
block both capture that returned Int and then call get(()) to verify the outer
handler's state is restored; update the final assert on _run(src, "test") to
expect the consumed inner result while also ensuring the outer state check still
passes.
---
Outside diff comments:
In `@tests/test_codegen.py`:
- Around line 2799-2805: The current test only checks that the symbols
(state_get_Int, state_put_Int, state_push_Int, state_pop_Int) appear anywhere in
result.wat; update the assertions to verify concrete WAT import declarations are
emitted by asserting the actual import lines for those hosts instead of mere
occurrences — e.g. assert that result.wat contains import declarations that
include the import prefix and each function name (look for '(import' plus the
host function names) so the test proves the host imports are emitted, not just
referenced; update the assertions around result.wat where
state_get_Int/state_put_Int/state_push_Int/state_pop_Int are checked to perform
string matches for import declarations.
🪄 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: a4aa2040-9594-4567-ab18-164446bcd5da
📒 Files selected for processing (3)
HISTORY.mdTESTING.mdtests/test_codegen.py
test_handle_state_wat_has_imports: assert full import declaration strings
('(import "vera" "state_*_Int"') rather than bare symbol occurrence, so
the test proves host imports are emitted, not just referenced in call sites.
test_nested_state_outer_readable_after_inner: make inner body return Int
(put(7); get(())) so state_pop_Int is exercised with a live WASM value on
the stack, verifying it is truly stack-neutral. Outer block captures the
inner result via let, then reads outer state (42) as the return value.
Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TESTING.md`:
- Line 9: Update the "Skipped tests" counts in TESTING.md to match the current
test run (change the value 9 to 11 wherever the skipped-test figure appears),
ensuring both tables/occurrences in the "Skipped tests" section and any summary
table are synchronized so totals remain consistent; locate the "Skipped tests"
heading and the two tables that list test counts and replace the old skipped
count with 11 and verify the derived totals in those tables still add up
correctly.
🪄 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: e376f351-f5f5-4123-943f-d3e5bf36d52a
📒 Files selected for processing (2)
TESTING.mdtests/test_codegen.py
ch03_typed_holes is a check-level program; its verify and run stages are both skipped by the conformance framework. Add both rows to the level-limited skips table and update the summary sentence. Co-Authored-By: Claude <noreply@anthropic.invalid>
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
handle[State<T>]of the same type shared one state cell; inner handler mutations corrupted the outer handler value.state_storeis now a per-type stack.state_push_T(handler entry) pushes a fresh default cell;state_pop_T(handler exit) restores the outer cell. Get/put operate on the top of the stack.HandleExprwas missing from_is_void_expr, causing a spuriousdropfor void-body handlers used as statements (WASM validation error).api.py) and browser/Node runtime (runtime.mjs) both updated.Test plan
TestEffectHandlerspytest tests/— 3,230 passed, 11 skippedmypy vera/— cleanCloses #417
Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Behaviour Changes
Tests
Documentation