refactor(transform/hir): de-duplicate HIR-walking helpers (#5293)#5305
Conversation
Collapse copy-pasted HIR/IR walkers whose copies must each be hand-updated when a new Expr variant is added — a missed copy is a silent miscompile, not a build error (the same bug-class as #5143). - compute_max_local_id / compute_max_func_id: delete the copies in finally_inline, async_to_generator (incl. compute_max_func_id_module), state_desugar, and unroll; route all callers through the canonical, already-`pub`, exhaustive-walker-backed implementations in generator::id_scan. First the canonical scanners are made a true superset of every copy: they now also scan switch `case.test` exprs and module-global initializers (a higher max id is always safe — fresh ids just start higher; a missed id collides). - class_computed_member_registration_expr: drop the 3 byte-identical copies in lower/{lower_expr,module_decl,stmt}.rs and re-export the canonical lower_decl::class_computed one through lower_decl::mod. - collect_assigned_locals_expr: replace the ~1120-line hand-rolled per-variant descent (ending in a `_ => {}` catch-all that silently skipped newly-added variants) with the exhaustive walk_expr_children for child descent, keeping only the 7 assignment-recording arms and the intentional don't-recurse-into-closures arm custom. Pure refactor, no behavior change. Verified: perry-hir (158) and perry-transform (34) lib tests pass in release; an end-to-end smoke program covering switch case-tests, module-global closures, async, generators, finally, computed class members, and unrolled loops matches `node --experimental-strip-types` byte-for-byte. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo independent deduplication refactors from issue ChangesHIR: class_computed helper consolidation and analysis refactor
Transform: canonical id_scan extensions and duplicate removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 |
cargo fmt wants the walk_expr_children closure body wrapped in a block since the single-line form exceeds the width limit. Fixes the lint CI failure on this branch; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0161bea2ibAsgFkKE2w7F3Ei
Closes #5293.
An external audit flagged several HIR/IR-walking helpers copy-pasted across files. Each copy must be hand-updated whenever a new
Expr/HIR node variant is added; a missed copy is a silent miscompile, not a build error — the same bug-class behind #5143 (FuncId-collision from a scan missing class field-init closures).What changed
1.
compute_max_local_id/compute_max_func_id(perry-transform)Deleted the copies in
finally_inline,async_to_generator(includingcompute_max_func_id_module),state_desugar, andunroll, plus their privatescan_*helpers. All callers now use the canonical, already-pub, exhaustive-walk_expr_children-backed implementations ingenerator::id_scan.Before routing, the canonical scanners were made a true superset of every copy so no caller loses coverage:
case.testexpressions (some copies did, the canonical didn't),async_to_generator's copy did, the canonical didn't).A higher max id is always safe — fresh ids just start higher; a missed id is what collides.
2.
class_computed_member_registration_expr(perry-hir)Dropped the 3 byte-identical copies in
lower/{lower_expr,module_decl,stmt}.rsand re-exported the canonicallower_decl::class_computedone throughlower_decl::mod(thelower/*files already pull it in viause super::*).3.
collect_assigned_locals_expr(perry-hir)Replaced the ~1120-line hand-rolled per-variant descent — which ended in a
_ => {}catch-all that silently skipped any newly-added variant nesting aLocalSet/Update— with the exhaustivewalk_expr_childrenfor child descent. Only the 7 assignment-recording arms (LocalSet,Update, array push/splice/copyWithin/reverse,SetAdd) and the intentional don't-recurse-into-closures arm remain custom. Net: same recorded assignments on every currently-handled variant, plus coverage for variants the catch-all used to drop.The optional codegen
for_each_body_in_moduleextraction (item 4 in the issue) is not included here — left for a follow-up to keep this diff a pure mechanical dedup.Validation
Pure refactor, no behavior change. Net −2176 lines.
perry-hirlib (158) andperry-transformlib (34) tests pass in release (includes theid_scanunit tests and the lower: deeply-nested expressions (a+b+c+…, o.a.a.…) overflow the stack → fatal abort instead of a diagnostic #5259 deep-chain guard tests).case-tests, module-global closures, async/await, generators,try/finally, computed class members, and unrolled loops compiles and matchesnode --experimental-strip-typesbyte-for-byte.🤖 Generated with Claude Code
Summary by CodeRabbit
LocalId/FuncIdallocation collisions by expanding max-id scanning to cover additional expression locations (including global initializers and switch case tests) and by using shared canonical ID computation helpers across transformation passes.