fix(repsel): make scalar replacement's method-call summary independent of PERRY_PTR_SHAPE_LOCALS - #7718
Conversation
…t of PERRY_PTR_SHAPE_LOCALS #6984/#6976: PERRY_PTR_SHAPE_LOCALS=0 crashed test_gap_repsel_ptr_shape_locals with `TypeError: Cannot read properties of undefined`. The kill switch's own eligibility gates (ptr_shape.rs, proven_this.rs) were correctly wired -- the bug was one level down, in a supposedly-unrelated optimization. Scalar replacement's method-call summarizer (lower_call/scalar_method.rs) resolved a scalar-replaced receiver's class via receiver_class_name, whose fallback (reached whenever Phase 3b's Ptr<Shape> proof is unavailable, for any reason) returns the local's DECLARED type. For an interface-typed local with `new` provenance that name is not a registered class, so the summarizer bailed and the caller fell through to ordinary heap-object method dispatch -- which read the receiver from a slot scalar replacement had already turned into a bare, uninitialized alloca. Fixed by resolving the class from ctx.non_escaping_news (the exact map that already gated scalar replacement for that local) before falling back to receiver_class_name. Verified byte-identical LLVM IR for the default (switch-on) build before/after, and the whole test_gap_repsel_* corpus (21 files) PASS under rep_ptr_shape_off with genuine evacuation liveness. Adds gc-ptr-shape-off-witness.yml, a per-PR CI arm exercising the switch's OFF state across the repsel corpus -- rep_ptr_shape_off was previously only in gc_repsel_matrix.sh's --arms all, which runs on push-to-main/schedule, not per PR.
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change fixes class resolution for scalar-replaced method receivers and adds a gated GitHub Actions workflow for the ChangesPtr shape OFF witness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant RelevanceCheck
participant PerryBuild
participant RepselCorpus
participant ArtifactUpload
PullRequest->>RelevanceCheck: provide changed paths
RelevanceCheck->>PerryBuild: enable relevant workflow run
PerryBuild->>RepselCorpus: provide release archives
RepselCorpus->>ArtifactUpload: produce JSON report
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Merging as v0.5.1415The root cause is better than the issue's own hypothesis, and materially wider than a kill-switch bug. It is not in Phase 3b.
The part that matters beyond this ticket: that fallback fires whenever the The fix resolves the class from Diagnosis method worth copying: Verification
CI arm added ( |
Summary
PERRY_PTR_SHAPE_LOCALS=0crashedtest_gap_repsel_ptr_shape_locals.tswithTypeError: Cannot read properties of undefined (reading 'area'), with no GC involved. This is the kill switch for repsel Phase 3b/5a (Ptr<Shape>proven object locals + the proven-thismethod clones it feeds) — an OFF arm is supposed to be the safest cell in the matrix, and it wasn't.collectors/ptr_shape.rsandcollectors/proven_this.rsboth correctly bail to an empty proof set when the switch is off. The bug was one level down, in a completely different, supposedly-orthogonal optimization: scalar replacement ofnewlocals (collectors/escape_news.rs), and specifically its method-call summarizer inlower_call/scalar_method.rs.try_lower_scalar_replaced_method_call) resolved a scalar-replaced receiver's class viatype_analysis::predicates::receiver_class_name. That function's fallback arm — reached whenever Phase 3b'sPtr<Shape>proof is unavailable for any reason, not just the kill switch — returns the local's declared type name.const o: Shaped = new Impl(...)(an interface-typed local withnewprovenance — exactly the "predicates.rs narrowing" scenariotest_gap_repsel_ptr_shape_locals.ts'sifaceLocalwas written to test), that declared name is"Shaped", which is not a registered class.simple_scalar_method_summary(ctx.classes, "Shaped", "area", 0)then fails to find the class, the summarizer gives up (return Ok(None)), and the caller falls through to the ordinary heap-object method-dispatch lowering — which reads the receiver fromctx.locals[receiver_id]. But scalar replacement had already turned that slot into a bare, never-initialized alloca (the heap allocation was elided; seelet_stmt.rs'slet dummy_slot = ctx.func.alloca_entry(DOUBLE);with no store). Reading it produces garbage that happens to decode asundefined, ando.area()throws.ctx.non_escaping_newsfirst — the exact map that already gated scalar replacement for that local inlet_stmt.rs, keyed by thenewexpression's own class name, never the declared annotation — before falling back toreceiver_class_name. This is provably safe:ctx.scalar_replaced.contains_key(receiver_id)being true already certifies (via the escape analysis's ownmark_unstable_scalar_method_receivers/check_escapes_in_expr, both keyed off the samenon_escaping_newsclass name) thatsimple_scalar_method_summarysucceeds for this exact class+method, so the fix can only ever repair a wrong answer, never introduce one.Why this was invisible until now
Two independent reasons (from #6976, which this PR's triage removal and CI arm close out):
#6925shippedtest_gap_repsel_proven_this_frozen.tswithout registering it intest-parity/gc_repsel_corpus.txt, which madescripts/gc_repsel_matrix.shexit 3 before evaluating any cell — the gate was dark, not green. (Already fixed by#6977.)rep_ptr_shape_off— the arm that compiles with the switch off — is not ingc_repsel_matrix.sh's PR-gating subset (PR_ARMS), so no per-PR CI run ever exercised it. It only ran via--arms all, which fires on push-to-main/schedule, not per PR.Decision: fix, not delete
Per CLAUDE.md's GC knob kill-policy ("every GC env knob either has a required CI arm exercising its OFF state, or it is deleted after one release of soak") and "a mode that still exists is a decision that hasn't been made" — the OFF state's failure here was a straightforward, fully-explained codegen defect in an unrelated optimization, not evidence that the representation itself only works with the optimization enabled. The switch has real bisection value (it is what let
#6976's own triage separate "genuine repsel regression" from "everything else"), so fixing it and exercising it in CI is the right call, not deleting it.Verification
mainexactly as described:--trace llvmon a minimal repro (interface + concrete class +forloop + return statement calling.area()twice) — confirmed the default (switch-on) LLVM IR fully scalar-replaces the receiver and inlines.area(), while the switch-off IR reads an alloca (%r4) that is declared but never stored anywhere in the function.test_gap_repsel_ptr_shape_locals.ts.scripts/gc_repsel_matrix.sh --arms rep_ptr_shape_off --filter test_gap_repsel_(the whole representation-selection corpus, 21 files) is 21/21 PASS, 0 FAIL, 0 UNVER, with genuine liveness under the arm's evacuating base (collected 21/21 reclaimed 21/21 moved-objects 21/21 copy-minor 21/21— not a vacuous green from an inert arm).cargo fmt --all -- --checkclean;cargo clippy -p perry-codegen --libshows no new warnings (diffed against the pre-existing baseline); the existingcollectors::scalar_method_dispatchunit tests (9/9) still pass.CI arm
Adds
.github/workflows/gc-ptr-shape-off-witness.yml, a per-PR job that runsscripts/gc_repsel_matrix.sh --arms rep_ptr_shape_off --filter test_gap_repsel_— closing the exact hole described above. Checked against CLAUDE.md's "four ways a gate can be unable to fail":continue-on-error, no|| true, no pipe between the matrix and the shell's exit status.main, and promoting it immediately would block every open PR. Promotion is a maintainer action, not part of this PR.concurrencycancels pull-request runs only;mainruns are keyed on the commit SHA so they queue instead of cancelling each other (mirrorsgc-moving-witnesses.yml's#7205fix).scripts/gc_repsel_matrix.sh's own exit status already folds in the liveness gate (scripts/gc_matrix_liveness_check.py) alongside byte-exactness, so a green run means the arm actually compiled, actually ran under evacuation, and actually matched the oracle — not merely that nothing threw.Also removes the now-obsolete
#6976triage entry fortest_gap_repsel_ptr_shape_locals | rep_ptr_shape_offintest-parity/gc_repsel_triage.txt(per that issue's own "REMOVE THIS ENTRY when #6976 is fixed" instruction), and registers the new workflow in the two places that describetest-parity/gc_repsel_corpus.txt's runners (docs/src/testing/test-registration.md,scripts/check_test_registration.py's dark-test message).Test plan
test_gap_repsel_ptr_shape_locals.tspasses under bothPERRY_PTR_SHAPE_LOCALSon and off, byte-exact vs. the pinned Node oracle.scripts/gc_repsel_matrix.sh --arms rep_ptr_shape_off --filter test_gap_repsel_— 21/21 PASS, live under evacuation.cargo fmt --all -- --check,cargo clippy -p perry-codegen --lib(no new warnings), existingscalar_method_dispatchunit tests.Summary by CodeRabbit
Bug Fixes
Testing
Documentation
Chores