fix(codegen): don't scalar-replace an instance whose class chain reaches an unmodeled base (#6343)#6357
Merged
Conversation
… unmodeled base (#6343) Escape analysis promoted a non-escaping `new C()` to one alloca per DECLARED field and inlined the constructor stores. That model is only faithful when codegen can see the whole construction — and a native base cannot be seen. `EventEmitter` and the `node:stream` classes install their method surface as OWN PROPERTIES on the instance at subclass-init time (`js_object_set_field_by_name(obj, "emit", <native closure>)`), not on a prototype. A runtime-installed own property has no declared slot, so it was simply absent from the promoted set: class X extends EventEmitter { a = 1 } const x = new X(); x.a // 1 (declared field — promoted) x.emit // undefined (installed own property — no slot) Silent wrong answer, no error. A method call on the receiver forces the heap path and hides it, so the shape that bites is a bare property read next to a field — exactly what `typeof x.emit` does. `class_chain_has_unmodeled_base` walks the `extends` chain and disqualifies a candidate when the chain reaches a base whose construction is opaque: * `native_extends` — events / node:stream / Web Streams / async_hooks / ws; found by walking the CHAIN, so `class Leaf extends Mid extends EventEmitter` is caught too, not just a literal `extends EventEmitter`; * `extends <expr>` (incl. a lexically shadowed heritage name) — an arbitrary runtime parent whose constructor can install anything; * a parent name that resolves to no visible class — a builtin (`Error`, `Map`, `Set`, …) or an import whose stub never landed. A cyclic chain fails closed. This is the third member of the family that already holds #313 (`this`-as-value), #573 (builtin `Error`) and #5872 (dispatch stability), and it is precise for the same reason they are: a chain of ordinary user classes is fully modeled, so the plain non-escaping class keeps its scalar replacement and the #945 IR guard's zero-alloc / zero-dispatch fast path is untouched. Fixture `test_gap_6343_scalar_native_base_surface.ts` covers EventEmitter, `Readable`, `Writable`, an indirect chain, and an Error subclass, with controls for a plain class and a pure user-class chain.
📝 WalkthroughWalkthroughScalar replacement now detects unmodeled inheritance bases, including native, dynamic, unresolved, and cyclic chains, and marks affected allocations as escaped. Unit and TypeScript tests cover native surfaces and user-modeled control cases. ChangesUnmodeled inheritance handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/collectors/this_as_value.rs (1)
574-661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
heritage_lexically_shadowedpath.The
heritage_lexically_shadowedflag is checked at Line 188 alongsideextends_expr, but no test exercises it. This flag guards a distinct scenario: theextends_nameresolves to a visible user class in the map, but the actual parent is a shadowed runtime binding. Without this check, the walk would reach the user class and returnfalse(fully modeled), incorrectly allowing scalar replacement. Removing theheritage_lexically_shadowedcondition would not be caught by any existing test.✅ Proposed test for `heritage_lexically_shadowed`
/// A cyclic chain proves nothing, so it must fail closed (escape). #[test] fn unmodeled_base_fails_closed_on_cyclic_parent_chain() { let child = class("A", Some("B")); let parent = class("B", Some("A")); let mut classes = HashMap::new(); classes.insert(child.name.clone(), &child); classes.insert(parent.name.clone(), &parent); assert!(class_chain_has_unmodeled_base(&child, &classes)); } + + /// `heritage_lexically_shadowed` — the extends name resolves to a visible + /// user class, but the actual parent is a shadowed runtime binding. + /// Must fail closed even though the name is in the `classes` map. + #[test] + fn unmodeled_base_rejects_lexically_shadowed_heritage() { + let mut child = class("X", Some("EventEmitter")); + child.heritage_lexically_shadowed = true; + // Insert a user class named "EventEmitter" so the name resolves — + // without the heritage_lexically_shadowed check, the walk would + // reach this root class and return false (incorrectly modeled). + let emitter = class("EventEmitter", None); + let mut classes = HashMap::new(); + classes.insert(child.name.clone(), &child); + classes.insert(emitter.name.clone(), &emitter); + + assert!(class_chain_has_unmodeled_base(&child, &classes)); + } }🤖 Prompt for 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. In `@crates/perry-codegen/src/collectors/this_as_value.rs` around lines 574 - 661, Add a focused unit test alongside the existing unmodeled-base tests that creates a child whose extends_name resolves to a visible user class while setting heritage_lexically_shadowed, then asserts class_chain_has_unmodeled_base returns true. Ensure the setup distinguishes the shadowed runtime parent from the modeled class so removing the flag check would fail the test.
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/src/collectors/this_as_value.rs`:
- Around line 574-661: Add a focused unit test alongside the existing
unmodeled-base tests that creates a child whose extends_name resolves to a
visible user class while setting heritage_lexically_shadowed, then asserts
class_chain_has_unmodeled_base returns true. Ensure the setup distinguishes the
shadowed runtime parent from the modeled class so removing the flag check would
fail the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38ba325f-181d-4694-bacf-8894b2d38ec6
📒 Files selected for processing (4)
crates/perry-codegen/src/collectors/escape_news.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/collectors/this_as_value.rstest-files/test_gap_6343_scalar_native_base_surface.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Escape analysis promotes a non-escaping
new C()to one alloca per DECLARED field and inlines the constructor stores into them. That model is faithful only when codegen can see the whole construction — and a native base cannot be seen.EventEmitterand thenode:streamclasses install their method surface as own properties on the instance at subclass-init time (js_object_set_field_by_name(obj, "emit", <native closure>)), not on a prototype. A runtime-installed own property has no declared slot, so it was simply absent from the promoted set:field: 1 typeof emit: function typeof on: functionmainfield: 1 typeof emit: undefined typeof on: undefinedfield: 1 typeof emit: function typeof on: functionSilent wrong answer, no error, no diagnostic. Closes #6343.
The trigger is that the instance never escapes. A method call on the receiver (
x.on(…)) forces the heap path and hides the bug entirely — which is why this survived so long. The shape that bites is a bare property read sitting next to a field read, i.e. exactlytypeof x.emit.While writing the fixture I found the issue's "drop the field and the methods reappear" note is a red herring:
class X extends EventEmitter {}with no field is equally broken when its only use is a bare read (the promoted set is then simply empty). What made the methods reappear in the original snippet was the method call, not the missing field.The fix
class_chain_has_unmodeled_base(perry-codegen/src/collectors/this_as_value.rs) walks theextendschain and disqualifies a scalar-replacement candidate when the chain reaches a base whose construction is opaque to codegen:native_extends— events / node:stream / Web Streams / async_hooks / ws. Resolved by walking the chain, soclass Leaf extends Mid,class Mid extends EventEmitteris caught too, not just a literalextends EventEmitter.extends <expr>(including a lexically shadowed heritage name) — an arbitrary runtime parent whose constructor can install anything.Error,Map,Set,Event, …) or an import whose stub never landed. Imported classes do land inclass_tableas stubs, so an ordinary cross-moduleclass Local extends ImportedBasestill resolves and stays on the fast path.A cyclic chain fails closed. It is wired into
collect_non_escaping_newsas one more Pass-3 disqualifier, next to #313 (this-as-value) and #573 (builtinError) — the same family, and the same shape as #5872/#6324's Pass-4 dispatch-stability guard.This generalizes the #573
class_chain_extends_builtin_errorcheck, which stays as-is: it is name-keyed and therefore also fires for a locally shadowedError, which the chain walk would resolve to the user class and let through. Keeping both means the analysis only ever gets more conservative.It did not need #6342's helper
#6342 adds
native_instance_base_in_chaininlower_call/new_helpers.rs, which enumerates named native bases (EventEmitter,Map,Set,Event,CustomEvent) because it has to decide which init to emit. This pass only has to decide whether the base is opaque, which is a strictly weaker question with a name-free answer — "does the chain leave the set of classes codegen can see". So there's no dependency and no duplicated list: the two land independently and can merge in either order. (#6342's own writeup calls this bug out as pre-existing and orthogonal, which matches.)The fast path is preserved — IR evidence
The point of a precise disqualifier is that the real perf win survives. Same two source files compiled by a pristine
origin/mainperryand by this branch'sperry(identicallibperry_runtime.a— this is a codegen-only change, verified by md5, so the archive cannot be a confound):The plain non-escaping class emits byte-identical IR on both sides — zero allocations, zero dispatch. The regression is localized exactly to the class that was miscompiled: on
mainitsprobe()never even calledjs_event_emitter_subclass_init, which is the bug in one line of IR.scripts/run_issue_945_scalar_method_ir_guard.sh→ PASS (both halves: the scalar fast path and the #5872 must-dispatch mirror).test-files/test_issue_945_scalar_method_guards.tsstays byte-identical to Node.Validation
Fixture —
test-files/test_gap_6343_scalar_native_base_surface.ts, byte-identical tonode --experimental-strip-types. It fails on pristinemainon four independent probes (X,Multi,R,W) and passes here. It coversEventEmitter,node:streamReadable+Writable, an indirectLeaf → Mid → EventEmitterchain, and anErrorsubclass, plus controls: a plain class and a pure user-class chain that must stay scalar-replaced, an explicit-super()subclass, and a live emitter (listener actually fires).Unit tests — 7 new cases in
collectors::this_as_valuepin the chain walk: plain class and user-class chain accepted; direct native parent, indirect native parent, unresolvable parent name, dynamicextends <expr>, and a cyclic chain all rejected.cargo test -p perry-codegen --lib: 176 passed, 0 failed.Gap-suite A/B, 298 tests, two genuinely distinct compilers. Baseline = a
perrybuilt from a worktree pinned atorigin/main(be5ed2bfc) and snapshotted before the fix commit (be5ed2bfc; the branch has since been rebased onto currentmain, which added only an unrelated docs commit and #6340); both arms run against the same (unchanged, md5-identical) runtime archives, so the compiler is the only variable.md5(baseline perry) != md5(fixed perry), and the baseline binary demonstrably reproduces the bug while the fixed one does not — so the A/B is not vacuous.295 identical / 3 changed → zero regressions.
test_gap_6343_scalar_native_base_surfacetest_gap_console_methodstest_gap_module_const_local_shadowThe two unchanged-verdict tests are proven noise rather than build differences — re-running the baseline binary alone reproduces each:
test_gap_console_methodsdiverges only on aconsole.timewall-clock line (timer1: 0.010msvs0.009ms); three consecutive runs of the baseline binary print0.001ms/0.002ms/0.002ms.test_gap_module_const_local_shadowis already intest-parity/known_failures.json— it prints uninitialized memory, and three consecutive runs of the baseline binary yieldx+1.116756386944e-311,x+2.613644130718e-311,x+2.771571180747e-311.cargo fmt --all -- --checkclean;scripts/check_file_size.shclean.Summary by CodeRabbit
Bug Fixes
Tests