Skip to content

Fix #648: cyclic type aliases produce [E132] at check time#665

Merged
aallan merged 3 commits into
mainfrom
fix-648-cyclic-aliases
May 12, 2026
Merged

Fix #648: cyclic type aliases produce [E132] at check time#665
aallan merged 3 commits into
mainfrom
fix-648-cyclic-aliases

Conversation

@aallan

@aallan aallan commented May 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • Cyclic type aliases (type A = B; type B = A) now produce a clean [E132] diagnostic at vera check time instead of crashing vera compile with RecursionError deep inside _type_expr_to_wasm_type.
  • New post-registration pass _check_alias_cycles in vera/checker/registration.py walks every alias's AST type_expr chain (mirroring codegen's recursion shape — through RefinementType wrappers and across NamedType-of-alias references) and emits [E132] with the originating decl location, the full cycle path (e.g. A -> B -> C -> A), and a Fix: paragraph pointing at data-declared ADTs as the alternative for self-referential types.
  • 4 new tests in 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 an Array<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 skipped
  • mypy vera/ → clean
  • vera check on the issue's reproducer → emits [E132] with the cycle path, exits non-zero
  • vera check on a parallel acyclic chain (type IntAlias = Int; type Pair = IntAlias) → unchanged (OK)
  • Three new E132 tests pass; one new no-false-positive test passes

Closes #648

Summary by CodeRabbit

  • Bug Fixes
    • Cyclic type aliases now produce a clear [E132] “Cyclic type alias” error at check time, preventing crashes during compilation.
  • Tests
    • Added regression tests for two-way, self, three-way and refinement-wrapped cyclic aliases, plus an acyclic alias case.
  • Documentation & Changelog
    • Updated release and project docs to reflect v0.0.149.
  • Maintenance
    • Version metadata bumped to v0.0.149.

Review Change Stack

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>
@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Implements 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.

Changes

Cyclic Type Alias Detection at Check Time

Layer / File(s) Summary
Error code and cycle detection logic
vera/errors.py, vera/checker/registration.py
Adds ERROR_CODES["E132"] = "Cyclic type alias" and implements _check_alias_cycles plus _alias_chain_target; _register_all now runs the post-registration cycle check.
Cyclic alias regression tests
tests/test_checker.py
Adds tests covering two-way, self, three-way, refinement-wrapped cyclic aliases (expect E132) and one acyclic alias-chain acceptance case.
Defence-in-depth documentation updates
tests/test_wasm_coverage.py, vera/wasm/inference.py
Rewords test docstring and _resolve_base_type_name docstring to note checker-level cycle interception and defensive codegen guards.
Version and release documentation
pyproject.toml, vera/__init__.py, CHANGELOG.md, HISTORY.md, KNOWN_ISSUES.md, ROADMAP.md, TESTING.md, README.md
Bumps project version to 0.0.149, documents the fix in CHANGELOG and HISTORY, removes the cyclic-alias KNOWN_ISSUES entry, updates test counts and compare links, and refreshes README/ROADMAP/TESTING metrics.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • aallan/vera#649: Related defensive cycle guard changes in wasm inference that complement this checker-side cycle detection.

Suggested labels

compiler, tests, docs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly addresses the primary change: detecting cyclic type aliases during type checking (issue #648) and emitting a clean [E132] diagnostic at check time instead of crashing at compile time.
Linked Issues check ✅ Passed All objectives from issue #648 are met: post-registration cycle detection pass implemented in vera/checker/registration.py, [E132] error code added to vera/errors.py, cycle path and fix suggestion in diagnostics, regression tests covering two-way, self, three-way cycles and acyclic chains, and codegen prevented from crashing.
Out of Scope Changes check ✅ Passed All changes are scoped to issue #648: cycle detection in registration, error code registration, test coverage, documentation updates (CHANGELOG, HISTORY, KNOWN_ISSUES, README, ROADMAP, TESTING.md), version bumps in pyproject.toml and vera/init.py, and belt-and-braces documentation in vera/wasm/inference.py.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-648-cyclic-aliases

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.06%. Comparing base (bc365d2) to head (69af2a3).

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              
Flag Coverage Δ
javascript 57.36% <ø> (ø)
python 94.85% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between bc365d2 and f23e261.

⛔ Files ignored due to path filters (5)
  • docs/index.html is excluded by !docs/**
  • docs/index.md is excluded by !docs/**
  • docs/llms-full.txt is excluded by !docs/**
  • docs/llms.txt is excluded by !docs/**
  • uv.lock is excluded by !**/*.lock, !uv.lock
📒 Files selected for processing (12)
  • CHANGELOG.md
  • HISTORY.md
  • KNOWN_ISSUES.md
  • ROADMAP.md
  • TESTING.md
  • pyproject.toml
  • tests/test_checker.py
  • tests/test_wasm_coverage.py
  • vera/__init__.py
  • vera/checker/registration.py
  • vera/errors.py
  • vera/wasm/inference.py
💤 Files with no reviewable changes (1)
  • KNOWN_ISSUES.md

Comment thread tests/test_checker.py
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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between f23e261 and e6f5752.

📒 Files selected for processing (4)
  • README.md
  • ROADMAP.md
  • TESTING.md
  • tests/test_checker.py

Comment thread tests/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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e6f5752 and 69af2a3.

📒 Files selected for processing (2)
  • TESTING.md
  • tests/test_checker.py

Comment thread TESTING.md
@aallan aallan merged commit db4f9bd into main May 12, 2026
23 checks passed
@aallan aallan deleted the fix-648-cyclic-aliases branch May 12, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cyclic type aliases crash codegen with RecursionError instead of clean E1xx diagnostic

1 participant