Fix #648: cyclic type aliases produce [E132] at check time#665
Conversation
Pre-fix `vera/checker/registration.py::_register_alias` resolved
aliases one at a time; when `type A = B` was processed before
`B` was registered, the forward-reference fallback in
`_resolve_type` returned a placeholder rather than chasing the
chain. The resolved-type representation reached the
post-registration state with no observable cycle.
Codegen then stored the raw AST `type_expr` and
`vera/codegen/core.py::_type_expr_to_wasm_type` chased the chain
through the AST, blowing the stack with `RecursionError:
maximum recursion depth exceeded` — pointing at codegen
internals rather than the actual program bug.
Post-fix `_register_all` calls a new `_check_alias_cycles` pass
that walks every alias's AST `type_expr` chain (following
`NamedType`-of-alias references through `RefinementType`
wrappers, mirroring codegen's recursion shape) and emits
`[E132]` ("Cyclic type alias") with:
- the originating decl location
- the full cycle path (`A -> B -> C -> A`)
- a `Fix:` paragraph pointing at `data`-declared ADTs as the
alternative for self-referential types
- spec ref to §4.3 "Type Aliases"
The walker mirrors `_type_expr_to_wasm_type`'s recursion shape
deliberately — it follows the same chain that would otherwise
crash codegen, so a cycle the checker detects is exactly a
cycle that would have crashed. Cycles passing *through* an
`Array<T>` / `FnType` / tuple terminate before reaching another
alias (because codegen returns a concrete WASM type at those
nodes without recursing into children), so they aren't the ones
this fix targets — and they don't crash codegen either.
Defensive cycle guards on the alias-walking helpers in
`vera/wasm/inference.py` (closed in #633) remain as
belt-and-braces for the same family.
Tests: 4 new tests in `tests/test_checker.py::TestResolutionCoverage`:
- `test_cyclic_alias_two_way_e132` (canonical case)
- `test_cyclic_alias_self_e132` (`type A = A`)
- `test_cyclic_alias_three_way_e132` (`A -> B -> C -> A`)
- `test_acyclic_alias_chain_ok` (no false-positive on
`type IntAlias = Int; type Pair = IntAlias`)
Validation
- pytest tests/ -q → 3,810 passed, 14 skipped
- mypy vera/ → clean
Closes #648
Co-Authored-By: Claude <noreply@anthropic.invalid>
📝 WalkthroughWalkthroughImplements post-registration detection of cyclic type aliases and emits an [E132] diagnostic at check time; adds regression tests, updates defensive docs and metrics, and bumps the project to v0.0.149. ChangesCyclic Type Alias Detection at Check Time
Sequence Diagram(s)sequenceDiagram
participant CLI as vera check
participant Checker as RegistrationMixin._register_all
participant CycleCheck as _check_alias_cycles
participant Diagnostics as Emit E132
CLI->>Checker: Run registration over program
Checker->>CycleCheck: Invoke post-registration alias checker
CycleCheck->>CycleCheck: Walk alias chains, peel refinements
alt Cycle found
CycleCheck->>Diagnostics: Emit [E132] with location & fix suggestion
Diagnostics->>CLI: Report diagnostic
else No cycle
CycleCheck->>Checker: No errors, continue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #665 +/- ##
==========================================
+ Coverage 91.05% 91.06% +0.01%
==========================================
Files 60 60
Lines 23207 23239 +32
Branches 259 259
==========================================
+ Hits 21131 21163 +32
Misses 2069 2069
Partials 7 7
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
🤖 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 `@tests/test_checker.py`:
- Around line 5508-5575: Add a new test method alongside the other cyclic-alias
tests (e.g., name it test_cyclic_alias_refinement_e132) in tests/test_checker.py
that uses the same _check_err helper to parse code with a refinement-wrapper
cycle like `type A = { `@B` | true }; type B = A;` and assert that at least one
diagnostic has error_code "E132" (mirror the assertion style used in
test_cyclic_alias_two_way_e132 / test_cyclic_alias_self_e132 to locate
failures).
🪄 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: 1dbfbbb9-9229-4344-8e32-e4d96eee3aae
⛔ 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/**uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (12)
CHANGELOG.mdHISTORY.mdKNOWN_ISSUES.mdROADMAP.mdTESTING.mdpyproject.tomltests/test_checker.pytests/test_wasm_coverage.pyvera/__init__.pyvera/checker/registration.pyvera/errors.pyvera/wasm/inference.py
💤 Files with no reviewable changes (1)
- KNOWN_ISSUES.md
CR-1 (`tests/test_checker.py:5508-5575`) — add
`test_cyclic_alias_refinement_e132` covering the refinement-
wrapper cycle `type A = { @b | true }; type B = A`. Pins the
`_alias_chain_target` helper's `RefinementType.base_type`
peeling — codegen's `_type_expr_to_wasm_type` recurses through
refinements unconditionally, so a cycle hidden behind one is
still a codegen-crash cycle and must be intercepted by the
checker. Verified the case fires `[E132]` correctly against
the current walker; this test pins the contract.
CI lint (`scripts/check_version_sync.py`) — `README.md`'s
`v0.0.148`/`148 releases`/`3,820 tests` row was missed in the
version bump. Updated to `v0.0.149` / `149 releases` / `3,825
tests` (the latter accounts for both the four #648 tests
landed in the parent commit and the new refinement-cycle test
in this commit). TESTING.md and ROADMAP.md counts updated
accordingly.
Validation
- pytest tests/test_checker.py -k "cyclic_alias or acyclic_alias" -v
→ 5 passed (was 4)
- python scripts/check_version_sync.py → consistent across 6 files
- python scripts/check_doc_counts.py → 3,825 tests / 29 files (live)
Refs #648 #665
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_checker.py`:
- Around line 5508-5585: The tests only assert that an E132 diagnostic exists
but not its payload; update a representative test (e.g.,
test_cyclic_alias_two_way_e132) that calls _check_err and uses errs/e132 to also
assert the diagnostic description and/or fix_hint contain the expected cycle
path text (e.g., "A -> B -> A" or similar) and the suggested fix hint text so
the cycle-path message and fix hint are pinned; locate the e132 list created
from errs and add a short assertion that checks e132[0].description (and/or
e132[0].fix_hint) includes the canonical cycle string.
🪄 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: 2bcbb29b-1e42-4f8b-bd3a-03ffa3ecd3d4
📒 Files selected for processing (4)
README.mdROADMAP.mdTESTING.mdtests/test_checker.py
CR-2 (`tests/test_checker.py:5508-5585`) — pin diagnostic payload on a representative test. The four cyclic-alias tests landed in `f23e261` asserted only that an `[E132]` diagnostic existed — not its contents. A future refactor that re-routes through a sibling code with matching error_code but different rendering would silently regress the user-facing message. Fix: on `test_cyclic_alias_two_way_e132` (the canonical case), add two payload assertions: 1. `"A -> B -> A" in e132[0].description` — pins that the cycle path is rendered so the user can identify *which* aliases participate. 2. `"data" in e132[0].fix` — pins the `data`-declared ADT suggestion in the fix hint, since `data` is the canonical alternative when self-reference is what the user wanted. The other three E132 tests still check error_code only — the payload contract these assertions embody is uniform across all of them, so duplicating it on every test would be cargo-cult. Documented this scope decision in the canonical test's docstring so a future reader can find the payload assertions without grepping all four. Note on field name: CR's comment referenced `fix_hint`; the actual `Diagnostic` attribute is `fix` (see `vera/errors.py:58`). Intent of the suggestion preserved; field name corrected. Validation - pytest tests/test_checker.py::TestResolutionCoverage::test_cyclic_alias_two_way_e132 -v → 1 passed Refs #648 #665 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@TESTING.md`:
- Line 9: Update the test counts in TESTING.md and the aggregate expected value
in test_checker.py to match the canonical pytest collection output: run `pytest
tests/ --collect-only -q` (or `python scripts/check_doc_counts.py`) in a real
dev environment, use that collected total and passed/skipped numbers (not line
counts or sandbox results), and replace the current table entry (“3,825 total,
3,811 passed, 14 skipped”) with the exact values returned by collection; ensure
both TESTING.md and the `test_checker.py` expected constants/strings are changed
consistently so they match the canonical output.
🪄 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: 59024adf-02a0-49ec-ad10-dd5d30bee79a
📒 Files selected for processing (2)
TESTING.mdtests/test_checker.py
Summary
type A = B; type B = A) now produce a clean[E132]diagnostic atvera checktime instead of crashingvera compilewithRecursionErrordeep inside_type_expr_to_wasm_type._check_alias_cyclesinvera/checker/registration.pywalks every alias's ASTtype_exprchain (mirroring codegen's recursion shape — throughRefinementTypewrappers and acrossNamedType-of-alias references) and emits[E132]with the originating decl location, the full cycle path (e.g.A -> B -> C -> A), and aFix:paragraph pointing atdata-declared ADTs as the alternative for self-referential types.tests/test_checker.py::TestResolutionCoverage: canonical two-way cycle, degenerate self-cycle, three-way cycle, and a no-false-positive case for acyclic alias chains.Why this shape
The walker deliberately mirrors
_type_expr_to_wasm_type's recursion shape — it follows the same chain that would otherwise crash codegen, so a cycle the checker detects is exactly a cycle that would have crashed. Cycles passing through anArray<T>/FnType/ tuple terminate before reaching another alias (codegen returns a concrete WASM type at those nodes without recursing into children), so they aren't the ones this fix targets — and they don't crash codegen either.Defensive cycle guards on the alias-walking helpers in
vera/wasm/inference.py(closed in #633) remain as belt-and-braces.Test plan
pytest tests/ -q→ 3,810 passed, 14 skippedmypy vera/→ cleanvera checkon the issue's reproducer → emits[E132]with the cycle path, exits non-zerovera checkon a parallel acyclic chain (type IntAlias = Int; type Pair = IntAlias) → unchanged (OK)Closes #648
Summary by CodeRabbit