fix(class-refs): static method extracted as a value must not resolve the same-named instance method (#7689) - #7691
Conversation
📝 WalkthroughWalkthroughThe runtime preserves static dispatch when extracted constructor methods share names with instance methods. Prototype extraction keeps instance dispatch. Regression tests cover both paths and the ChangesMethod dispatch
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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.
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 `@crates/perry-runtime/src/object/native_module.rs`:
- Around line 1054-1069: Update js_class_method_bind so constructor static
references that fall through to build_bound_method_closure are stored in and
retrieved from the existing canonical value cache using the static method name.
Preserve the current prototype-reference path and ensure repeated reads such as
C.m === C.m return the same callable value.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bee335bc-4c70-4e57-91e6-a16304606de6
📒 Files selected for processing (4)
changelog.d/7691-static-method-value-name-collision.mdcrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/tests.rstest-files/test_gap_static_method_value_name_collision.ts
| // #7689: a CONSTRUCTOR class-ref receiver (`const f = C.m`) must | ||
| // never canonicalize to the INSTANCE vtable method of the same | ||
| // name — in JS `C.m` sees only statics (`class C { static lex(){} | ||
| // lex(){} }` has `C.lex` === the static; the instance `lex` lives | ||
| // on `C.prototype`). `class_id_from_method_receiver` treats a | ||
| // class ref like an instance, so marked's `const lexer2 = | ||
| // _Lexer.lex; lexer2(src, opt)` extracted the instance `lex`, | ||
| // whose bare invocation read `this.options` off an unconstructed | ||
| // receiver. Fall through to `build_bound_method_closure`: its | ||
| // call-time dispatch (`js_native_call_method`'s 0x7FFE arm) | ||
| // resolves statics-first for constructor refs. PROTOTYPE refs | ||
| // (`C.prototype.m`) keep the canonical path — the instance method | ||
| // is exactly what they name. | ||
| let receiver_is_constructor_ref = | ||
| class_ref_id(instance).is_some() && class_prototype_ref_id(instance).is_none(); | ||
| if !receiver_is_constructor_ref && bound_native_method_length(name).is_none() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'build_bound_method_closure|class_prototype_method_value_for_name|js_class_method_bind|static.*method|method.*cache' \
crates/perry-runtime/src/object/native_module.rs \
crates/perry-runtime/src/object/tests.rsRepository: PerryTS/perry
Length of output: 39242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l crates/perry-runtime/src/object/native_module.rs crates/perry-runtime/src/object/tests.rs
sed -n '1020,1120p' crates/perry-runtime/src/object/native_module.rs
sed -n '1440,1505p' crates/perry-runtime/src/object/tests.rs
sed -n '1136,1175p' crates/perry-runtime/src/object/native_module.rs
rg -n "class_prototype_method_values|class_prototype_method_value_cache_root_store|class.*ref.*method|C\.m|CLASS_PROTOTYPE_METHOD_VALUES|js_native_call_method|0x7FFE|static" crates/perry-runtime/src/object/native_module.rs | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 15970
Cache static method values from constructor refs.
js_class_method_bind falls through to build_bound_method_closure for constructor refs, which allocates a fresh BOUND_METHOD closure on every C.m read. This violates method identity: C.m === C.m becomes false even though static method reads should return one shared callable value. Keep the existing canonical value cache for constructor static refs by adding a static-name cache entry for the built closure instead of minting a new closure each time.
🤖 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-runtime/src/object/native_module.rs` around lines 1054 - 1069,
Update js_class_method_bind so constructor static references that fall through
to build_bound_method_closure are stored in and retrieved from the existing
canonical value cache using the static method name. Preserve the current
prototype-reference path and ensure repeated reads such as C.m === C.m return
the same callable value.
1d6e9f4 to
c2f6e3b
Compare
…esolve the same-named instance method (#7689)
Audit — merging as v0.5.1401Verified independently against node 26.5.1, on a class with the exact
Including the ternary form ( The root cause is the interesting part: #446's method-identity canonicalization resolves the name against the instance vtable, because A static and an instance method sharing a name is unusual enough to have gone unnoticed and common enough that marked does it twice ( Gates: 24/24 via the full |
c2f6e3b to
6309718
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
133-133: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep
CLAUDE.mdchanges limited to the version line.Revert the changes at Lines 133, 154, and 253. They add content outside
**Current Version:**.As per coding guidelines, update only the
**Current Version:**line inCLAUDE.md; do not add changelog entries or detailed history here.Also applies to: 154-154, 253-253
🤖 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 `@CLAUDE.md` at line 133, Revert the added content at the referenced lines in CLAUDE.md, including the escape-hatches text and other changelog/history entries. Keep only the existing **Current Version:** line change, with no additional documentation outside that version line.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@CLAUDE.md`:
- Line 133: Revert the added content at the referenced lines in CLAUDE.md,
including the escape-hatches text and other changelog/history entries. Keep only
the existing **Current Version:** line change, with no additional documentation
outside that version line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 42b8be82-c8a1-432c-9924-c166e38a3fba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CLAUDE.mdCargo.toml
…7664) Re-verifying the checker fix found 9 real+false hits, not the 8 the prior snapshot recorded -- test_gap_static_method_value_name_collision joined the population after #7691 without the budget being re-measured. Of the 9: 4 were the checker's own phi-edge false positives (fixed in the prior commit), 3 were unrooted:global (2 already fixed upstream by #7719, 1 fixed in the prior commit's static_dispatch.rs change), and 2 are unrooted:capture -- real, diagnosed, and tracked as this budget's referent rather than rushed. Measured on the native corpus, both arms of --moving-only, stale still 0.
…--max-unrooted to 2 (#7664) (#7724) * gc: fix the phi-edge checker false positives and the static-dispatch receiver hazard (#7664) scripts/gc_root_dominance_check.py: the native/--statepoints chain treated a phi as unconditionally transparent, so one tainted incoming edge blanket- tainted the phi's result and a downstream use was checked against ANY CFG path between source and use (between_blocks is deliberately path-insensitive, sound for an ordinary register but not for a phi, whose dynamic value depends on which edge was actually taken). All four reported unmasked hits were the same &&/|| short-circuit join: the tainted edge never crosses a safepoint, the OTHER edge does, and the checker reported that. _cast_closure gains phi_all_edges: a phi joins `chain` only once every incoming edge is independently in it. That closes the false positive and deliberately excludes the case of a single tainted edge with its own intervening safepoint before its predecessor's terminator; _phi_edge_hazard covers that separately, checking each edge's own window. Two new self-test fixtures (phi_safe_edge / phi_hazard_edge) pin both directions, each verified against a sabotaged copy of the checker to confirm it can still fail. lower_call/property_get/static_dispatch.rs: (Lexer as any).lex(...) reads a module-global receiver, then held it raw across arg-bundling logic that can allocate (a rest-param bundle always allocates; an object-literal argument can too) before implicit_this_save/js_static_this_arm_value read the stale copy -- the same #6969/#6986 shape #7719 just fixed in lower_call/builtin.rs, here on the receiver. Wrapped it in RootedGroup::adopt/reread. Re-verified against the current corpus: the checker fix eliminates exactly the four phi false positives with nothing else changing. The static-dispatch fix was not yet re-verified against a fresh corpus run after this rebase (disk pressure and box load made prior corpus runs unreliable) -- see the PR description for exactly what is and isn't confirmed. * gate(gc): lower gc-root-dominance-statepoints' --max-unrooted to 2 (#7664) Re-verifying the checker fix found 9 real+false hits, not the 8 the prior snapshot recorded -- test_gap_static_method_value_name_collision joined the population after #7691 without the budget being re-measured. Of the 9: 4 were the checker's own phi-edge false positives (fixed in the prior commit), 3 were unrooted:global (2 already fixed upstream by #7719, 1 fixed in the prior commit's static_dispatch.rs change), and 2 are unrooted:capture -- real, diagnosed, and tracked as this budget's referent rather than rushed. Measured on the native corpus, both arms of --moving-only, stale still 0. * chore: key the changelog fragment on PR #7724 * chore: point the budget referent at the split-out #7725 * chore: bump version to 0.5.1420 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * style: cargo fmt Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Fixes #7689.
What broke
marked.parse("# hi")threwTypeError: Cannot read properties of undefined (reading 'pedantic')on every parse, both with and withoutperry.compilePackages.marked's
parseMarkdownextracts a static method into a variable and calls it unbound:Lexerdeclares bothstatic lex(src, options)and an instance methodlex(src)(Parserhas the sameparsecollision). ReadingC.lexoff the constructor routes throughjs_class_method_bind, whose #446 method-identity canonicalization resolves the name against the instance vtable:class_id_from_method_receivertreats an INT32-tagged constructor ref exactly like an instance receiver, andmethod_owner_class_idonly consults instance methods. The extracted value was therefore the instancelex; invoked bare, itsthis.optionsread producedundefined, andblockTokens' first access (this.options.pedantic) threw.In JS,
C.mnever exposes prototype methods (they live onC.prototype) — the same semantics the NestJS fix already established for the read path inget_field_by_name.rs("Instance (prototype) methods must only resolve when reading off the prototype ref").Fix
In
js_class_method_bind, skip the instance-vtable canonicalization when the receiver is a constructor class ref (class_ref_idmatches,class_prototype_ref_iddoes not). The read then falls through tobuild_bound_method_closure, whose call-time dispatch (js_native_call_method's0x7FFEarm) already resolves statics-first for constructor refs — the exact path that made the same extraction work when no name collision existed. Prototype refs (C.prototype.m) keep the canonical instance-method path unchanged.Validation
constructor_ref_method_value_resolves_static_over_instance_methodregisters a class with both a static and an instancelexand asserts the extracted constructor-ref value dispatches the static while the prototype-ref value still dispatches the instance method. Verified it fails without the fix (dispatches the instance method) and passes with it.test_gap_static_method_value_name_collision.tscovers the marked shape end-to-end (class expression + declaration, ternary + plain extraction, direct calls, prototype read); byte-identical tonode --experimental-strip-types26.5.1. It passes, sogap_snapshot.jsonneeds no entry.mdmin.tsnow prints12(== Node), and the issue'smdapp.tsat 10 documents prints20105 60(== Node).perry-runtime --libsuite: 1931 passed, 0 failed.Follow-up (separate issue)
The full 300-document
mdapp.tsworkload is severely superlinear under Perry: 10 docs run in 0.16 s, but 100 docs did not finish within 5 minutes (Node: ~1 s for 300). That is a scaling defect independent of this correctness fix — I'll file it separately so this comparison workload can actually be run to completion.No version bump per the contribution flow; maintainer bumps at merge.
Summary by CodeRabbit
Bug Fixes
Tests