fix(hir): memoize class-mutates-capture recursion to stop exponential HIR lowering - #10801
proggeramlug wants to merge 2 commits into
Conversation
… HIR lowering (#10757) A class whose own methods construct fresh instances of itself while the class also captures an outer local (an everyday shape for arithmetic/ builder classes, e.g. @noble/curves' Point.double()/add() each returning new Point(...)) made for_each_nested_capture misread the class as nested inside itself. class_mutates_capture then recursed back into the same class on every self-constructing method, at every depth up to the hardcoded MAX_NESTED_CLASS_DEPTH cap, recomputing the identical (class, id) subproblem from scratch each time -- exponential in the class's method count, bounded only by that depth cap, so a single ordinary elliptic-curve arithmetic class (weierstrass.js in ethers' @noble/curves dependency) never finished lowering within any practical wait. Memoize class_mutates_capture by (class_name, id), shared across every id detect_shared_in_body asks about, and use an in-progress set to break cycles instead of the depth cap (which could also, in principle, have under-covered a legitimately deep but acyclic chain). Bisected on weierstrass.js confirmed this is a bounded blowup, not a true hang: instrumented call counts fit (B^9-1)/(B-1) almost exactly for branching factors 1, 2, 3 as the class grows by one method at a time. HIR output is byte-identical before/after on every fixture size small enough for the unfixed pass to complete. Also give the parity harness's compile step a timeout (PERRY_COMPILE_TIMEOUT, default 300s) -- it previously had none (only the executed-binary run did), so a compiler hang on any one fixture wedged the whole harness instead of failing that fixture. Add test-files/test_gap_10757_self_referential_class_capture.ts: fails via the harness's compile timeout on unfixed main, passes byte-identical to node with the fix.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe HIR lowering pass now memoizes recursive class-capture analysis and detects cycles. A regression fixture covers self-referential class captures. The parity harness now limits compiler execution with ChangesClass Capture Lowering
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: High 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Queued as the next train, behind #10795 which is validating now. The diagnosis is the strongest part and worth naming precisely: Replacing the depth cap with a termination argument is the right trade and you said why. "Finitely many distinct
Three things I will check when it lands, flagged now:
One note on ordering: |
|
Landed via merge train 241 (#10825) as v0.5.1620 — The diagnosis is what made this fixable, and it is worth restating: 252 → 35,770 → 688,870 fitting Replacing The root cause reads cleanly too — a self-referential #10757 stays open, deliberately. ethers now lowers and codegens — 153 modules, 0 JS fallback, about a minute — but it does not link. Closing #10757 on "compiles" would sever the thread from the original never-finishes report to what actually remains. On that: #10802 is the same defect as #10432, which has had a six-line reproducer since 2026-09-06 — export { createHash } from "crypto"; // dep.ts— confirmed during an audit today with a two-file, no-ethers reproduction producing Your Validation: ten cheap gates, |
Summary
Fixes #10757: compiling
ethers6.17.0 from real source never finished — HIR lowering of@noble/curves'weierstrass.jsspun for 8+ minutes on one core before being killed.It is a bounded superlinear blowup, not a true hang. Bisected
weierstrass.jsdown to a single factory function (weierstrassPoints), then to a package-independent synthetic reproducer. Instrumented call counts toclass_mutates_capture(incrates/perry-hir/src/lower/shared_mutable_capture.rs) grow from 252 (8 methods) → 35,770 (12 methods) → 688,870 (13 methods) as thePointclass gains one arithmetic method at a time — fitting(Bⁿ-1)/(B-1)forn=9(the hardcodedMAX_NESTED_CLASS_DEPTH) almost exactly, at branching factors B=1, 2, 3 respectively. It terminates, but not in any practical time for a realistic method count.Root cause:
Point's methods (double,add,fromAffine,multiplyUnsafe, …) each constructnew Point(...), forwardingPoint's own captured outer context (Fp,CURVE, …) — completely ordinary self-referential-class code.for_each_nested_capturewas written to find a class genuinely nested inside another class's method body (class Outer { make() { return class Inner { ... } } }) by scanning member bodies for capture-forwarding constructions. A self-referentialnew Self(...)matches that same scan, so it misreadsPointas nested insidePointandclass_mutates_capturerecurses back into the class it started from — once per self-constructing method, at every depth up to the cap — redoing the identical(class, id)subproblem from scratch each time.Fix: memoize
class_mutates_captureby(class_name, id), shared across every iddetect_shared_in_bodychecks, with an in-progress set to break cycles (replacing the depth cap, which could in principle also have under-covered a legitimately deep but acyclic chain — the new termination argument is "finitely many distinct(class, id)pairs," not "bounded to 8 levels"). HIR output is byte-identical before/after on every fixture size small enough for the unfixed pass to finish (verified via--print-hirdiff).Instruction-count differential (
perf stat -e instructions,--no-auto-optimize --no-linkto isolate lowering):Real-world acceptance: with the fix, the entire
ethers@6.17.0dependency tree —ethers+@noble/curves+@noble/hashes+@adraffy/ens-normalize+aes-js, 153 modules — lowers and codegens natively (0 JS fallback) in about a minute, versus never finishing onmain. Separate finding, not fixed here: the final link step for that full build fails on undefined references (perry_fn_..._crypto_ts__createHmac/pbkdf2Sync/randomBytes/createHash, and a WebSocket wrapper symbol) — this isethers/src.ts/crypto/crypto.ts'sexport { createHash, createHmac, pbkdf2Sync, randomBytes } from "crypto"re-export shape not resolving against Perry's nativecryptomodule, unrelated to HIR lowering. Reporting this as a candidate follow-up issue, not fixing it in this PR.Also gave the parity harness's compile step a timeout (
PERRY_COMPILE_TIMEOUT, default 300s, inrun_parity_tests.sh) — it had none before (only the executed binary's run did, viaPERRY_RUN_TIMEOUT), so a compiler hang on any one fixture would have wedged the whole harness rather than failing that fixture. The gap test below relies on this.What I did not run
etherslink-step failure described above (crypto/WebSocket native-module wiring) — out of scope for this issue per its own instructions ("if ethers then fails for a different reason, that is a separate finding").run_lint_gates.sh(SKIP_COMPILE_GATES=1; known-red on this Linux host per campaign notes).cargo test --workspace(ran the full script-tierrun_lint_gates.shplus targetedcargo test -p perry-hir, not the whole workspace, given host build-time constraints forperry-ui-*/GTK4 crates unavailable on this Linux box).Test plan
origin/main(1a4fa6507e): isolatedweierstrassPoints()factory (no@noble/curvesdependency, hand-written stubs) hangs; bisected by method count (8→14) showing 0.14s → 1.48s → 31.1s → timeout.test-files/test_gap_10757_self_referential_class_capture.tsadded: fails asCOMPILE_FAILvia the new harness timeout (22s) on pristinemain; passes byte-identical to Node 26.5.1 with the fix (real harness run, not a hand probe).cargo check --workspace --all-targetsclean (0 warnings/errors) on the defaultdevprofile, excluding the UI crates this Linux host can't build (missing systemglib/GTK4 — pre-existing, unrelated).cargo fmt --all -- --checkclean.cargo test -p perry-hir(the touched crate): all tests pass.scripts/run_lint_gates.sh SKIP_COMPILE_GATES=1: 78 of 79 gates passed; the one failure is the pre-existing "Public benchmark evidence freshness" (ci: two reds on main fail every PR — gap-suite shard 5 parity regression (test_gap_10430) and a stale public benchmark baseline #10707), not touched by this change.git diff --statconfirmed clean (no destructive doc regen) after running gates.ethers@6.17.0acceptance: printsethers-version:6.17.0, HIR lowering + codegen succeed for the full 153-module dependency tree; link step fails for the unrelated reason above (reported, not fixed).Summary by CodeRabbit
Bug Fixes
Tests
Chores