fix(hir): bind the value of a mixin-factory result (#5952)#6355
Merged
Conversation
added 3 commits
July 13, 2026 09:46
`const M = Mixin(Base)` where `Mixin` is the pre-scanned
`function Mixin(B) { return class extends B {…} }` shape took the
mixin fast path in `lower/stmt.rs`: it synthesized a real class under the
binding name `M` and then `continue`d, never emitting the value binding
for `M`. `new M()` and `instanceof M` kept working (both key off the
class NAME), but `typeof M` / `M.name` / `String(M)` read an unassigned
local and saw `undefined`.
Emit `emit_class_expression_value_binding` before the `continue`, exactly
as the sibling `const X = class {…}` arm does, so `M` holds
`ClassRef("M")`.
Also register the spec `.name`: the synthesized class is keyed by the
binding name, but a directly-returned class expression gets no
NamedEvaluation, so `M.name` is `""` (or the class expression's own name
when it has one). `pre_scan_mixin_functions` now records that name.
) `js_register_class_name` treated a zero-length name as a no-op, so the synthesized mixin class from `const M = Mixin(Base)` never landed in CLASS_NAMES and `M.name` read `undefined` instead of `""`. An anonymous class expression's `.name` IS the empty string; membership in CLASS_NAMES means 'this id is a real class', which stays true for an anonymous one. Registering an empty name makes the previously-unreachable `!name.is_empty()` filter in `prototype_equality` live and wrong — it would collapse an anonymous class's instances onto Object.prototype. Test the registration, not the name's emptiness.
Byte-compared against node --experimental-strip-types. Covers the direct return, a named returned class expression, return-via-const, return-via-intermediate, two mixins composed, a mixin over a mixin result, and the static-parent captured-param factory whose specialization must survive. Fails on main at the first line (typeof undefined).
📝 WalkthroughWalkthroughThe change introduces structured mixin metadata, binds synthesized mixin classes to their target variables, preserves anonymous class identity, and adds regression coverage for dynamic-parent mixin factories. ChangesMixin factory binding
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 (2)
crates/perry-hir/src/lower/lowering_context.rs (1)
643-649: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment contradicts what this PR actually ships.
The comment says
mixin_funcsis a "Stub field added to satisfy in-tree references; full mixin support is a separate workstream," but per the PR objectives andstmt.rs, this field now drives real synthesized-class binding (typeof M,.name,instanceof, etc.). Leaving "stub"/"separate workstream" language in place will mislead future readers about the feature's completeness.📝 Suggested doc fix
- /// Maps mixin name → (param_name, class-expression's own name, captured - /// class AST). The class-expression name is `None` for the common - /// anonymous `return class extends B {…}` form and drives the synthesized - /// class's user-visible `.name` (issue `#5952`). Stub field added to satisfy - /// in-tree references; full mixin support is a separate workstream. + /// Maps mixin name → `MixinFn { param_name, class_expr_name, class_ast }`. + /// `class_expr_name` is `None` for the common anonymous + /// `return class extends B {…}` form and drives the synthesized class's + /// user-visible `.name` (issue `#5952`).🤖 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-hir/src/lower/lowering_context.rs` around lines 643 - 649, Update the documentation for the mixin_funcs field in LoweringContext to describe its current role in real mixin support and synthesized-class binding, including the class-expression name behavior. Remove the stale “stub field” and “separate workstream” wording while preserving the mapping and anonymous-class name details.crates/perry-runtime/src/object/class_registry/class_meta.rs (1)
33-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused unit test for the new empty-name registration → prototype-identity behavior. Both files change runtime semantics for anonymous class registrations (issue
#5952), but no Rust unit test directly registers a zero-length name and asserts its prototype identity is preserved.
crates/perry-runtime/src/object/class_registry/class_meta.rs#L33-L55: add/extend a test that callsjs_register_class_namewithname_len == 0and assertsclass_name_for_idreturnsSome("".to_string())rather thanNone.crates/perry-runtime/src/builtins/formatting/prototype_equality.rs#L78-L93: extendunregistered_shape_id_normalizes_to_plain_object_prototype(or add a sibling test) to register a class with an empty name and assertprototypes_differtreats its instances as prototype-distinct from a plain object/unregistered shape, mirroring thetest_gap_5952_mixin_factory_binding.tsgap fixture at the Rust unit-test level.🤖 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/class_registry/class_meta.rs` around lines 33 - 55, Add focused Rust unit coverage at both affected sites: in crates/perry-runtime/src/object/class_registry/class_meta.rs lines 33-55, add a test that calls js_register_class_name with class_id nonzero, a valid pointer, and name_len 0, then verifies class_name_for_id returns Some("")；in crates/perry-runtime/src/builtins/formatting/prototype_equality.rs lines 78-93, extend unregistered_shape_id_normalizes_to_plain_object_prototype or add a sibling test to register an empty-name class and verify prototypes_differ distinguishes its instances from a plain object or unregistered shape.
🤖 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-hir/src/lower/lowering_context.rs`:
- Around line 643-649: Update the documentation for the mixin_funcs field in
LoweringContext to describe its current role in real mixin support and
synthesized-class binding, including the class-expression name behavior. Remove
the stale “stub field” and “separate workstream” wording while preserving the
mapping and anonymous-class name details.
In `@crates/perry-runtime/src/object/class_registry/class_meta.rs`:
- Around line 33-55: Add focused Rust unit coverage at both affected sites: in
crates/perry-runtime/src/object/class_registry/class_meta.rs lines 33-55, add a
test that calls js_register_class_name with class_id nonzero, a valid pointer,
and name_len 0, then verifies class_name_for_id returns Some("")；in
crates/perry-runtime/src/builtins/formatting/prototype_equality.rs lines 78-93,
extend unregistered_shape_id_normalizes_to_plain_object_prototype or add a
sibling test to register an empty-name class and verify prototypes_differ
distinguishes its instances from a plain object or unregistered shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d972cfd6-c5a1-4924-805e-e16346ca5d55
📒 Files selected for processing (7)
crates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower/mod.rscrates/perry-hir/src/lower/pre_scan.rscrates/perry-hir/src/lower/stmt.rscrates/perry-runtime/src/builtins/formatting/prototype_equality.rscrates/perry-runtime/src/object/class_registry/class_meta.rstest-files/test_gap_5952_mixin_factory_binding.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.
Fixes #5952.
Symptom
new M()andinstanceof Mworked; only the runtime value ofMwas missing.Root cause — the mixin fast path in HIR lowering, not the factory pass
pre_scan_mixin_functions(crates/perry-hir/src/lower/pre_scan.rs) records everyfunction Mixin(B) { return class extends B { … } }— a single param, a singlestatement, returning a class expression that
extendsthat param. Aconst M = Mixin(Base)call site then takes a fast path incrates/perry-hir/src/lower/stmt.rswhich copies the mixin's class AST, rewritesits
extendsclause to the concrete base, and registers it as a real class underthe binding name
M. That's what makesnew M(),instanceof Mand theinherited field/method work.
The path then
continued straight out of the declaration — it never emitted thevalue binding for
M. The siblingconst X = class {…}arm a few lines aboveends with
emit_class_expression_value_binding(...), which pushesLet { name, init: ClassRef(name) }; the mixin arm simply forgot it. Everyconsumer that keys off the class NAME (
New { class_name: "M" },InstanceOf { ty: "M" }) resolved fine, while every consumer that reads theVALUE (
typeof M,M.name,String(M),M === M) hit an unbound local andfolded to
undefined.--print-hiron the repro shows it exactly — classMis present, theLetis not:This is not
resolve_factory_return_class/specialize_captured_class_factoriesin
perry-transform. That pass never sees this shape: theLetis already gone bythe time the HIR reaches it. I verified the localization by dumping the HIR rather
than by inspection — the transform pass is reached only for the shapes the fast
path declines (see the matrix below), and all of those already worked.
Fix
crates/perry-hir/src/lower/stmt.rs— callemit_class_expression_value_bindingbefore thecontinue, soMholdsClassRef("M"), exactly like the sibling class-expression arm..name— the synthesized class is keyed by the binding name, but adirectly-returned class expression gets no NamedEvaluation from the call
site's
const M =, so per specM.nameis""(or the class expression'sown name for
return class Marker extends B {}).pre_scan_mixin_functionsnow records that name (
MixinFn { param_name, class_expr_name, class_ast })and the call site registers it via the existing
class_display_namesoverride, so codegen emits the spec name rather than the registration key.
crates/perry-runtime/.../class_meta.rs—js_register_class_nametreated a zero-length name as a no-op, so the anonymous class never landed in
CLASS_NAMESand.namestill readundefined. An anonymous class's.nameIS the empty string; membership in
CLASS_NAMESmeans "this id is a realclass", which stays true for an anonymous one. Only the null pointer and the
reserved id 0 are rejected now.
prototype_equality.rs— registering an empty name makes its previouslyunreachable
!name.is_empty()filter live and wrong: it would treat ananonymous class as unregistered and collapse its instances onto
Object.prototype. It now tests registration, not the name's emptiness.The factory-specialization pass is untouched and still fires
specialize_captured_class_factoriesexists to monomorphize a class-returningfactory per call site: it clones the returned class with the factory's captured
params baked in as constants, drops the synthesized
__perry_cap_*ctorparams/fields, and rewrites the
Callto aClassRefof the clone (issue #740 —Effect's
makeLiteralClassshape). Nothing in this PR touches that pass, and themixin fast path it competes with is strictly narrower (single param, single-stmt
return class extends <param>, called with one identifier naming a known class).Every other shape still routes through
Call→factory_specializeunchanged.Its canonical win is covered by the new fixture (shape 7) and verified green
before and after:
Shape matrix vs
node --experimental-strip-typesreturn class extends B {}typeofundefined,.nameTypeErrortypeof,.name === "",new,instanceofself+base,constructor,prototype, inherited field + method)return class Marker extends B {}typeofundefined.name === "Marker")const K = class extends B {}; return KS(G(Base))Top = S(Mid))typeof Midundefined; inherited field;instanceof Midtypeof✅; inherited field /instanceof MidunchangedThe residual "inherited field is
undefined" cells are a separate,pre-existing gap — instance field initializers of the base are not replayed
through a
RegisterClassParentDynamicparent edge (prototype methods resolvefine through it). Identical on main and on this branch, byte for byte; filed
separately rather than folded in here. The fixture asserts only the cells that
match node.
Validation
origin/main(be5ed2bfc) toolchainvs this branch, same
perry-devprofile: 3 output deltas, zero regressions.test_gap_5952_mixin_factory_binding— the new fixture: fails on main at line 1(
typeof undefined), byte-identical to node on this branch.test_gap_console_methods—console.timevalues only (0.003msvs0.014ms);varies run-to-run on the same binary.
test_gap_module_const_local_shadow— a pre-existing uninitialized-memory read(
x+3.06e-311); the value changes on every run of the same binary on both sides.cargo fmt --all -- --checkclean;scripts/check_file_size.shclean.test-files/test_gap_5952_mixin_factory_binding.tscovers the wholeneighbourhood above and is byte-identical to
node --experimental-strip-types.Note on the issue's provenance section
#5952 blames PR #5874. That is stale: the #5874 payload was fully reverted from
mainin5afa08fca(PR #6015) and the bug still reproduces on currentmain.The mixin fast path — and its missing value binding — dates to
1e65d9c22(2026-04-11), long before #5874. #5874 is exonerated.
test_gap_class_advanced,named in the issue as the surfacing gap test, also matches node on main today, so
the suite no longer covered this; the new fixture is the coverage.
Summary by CodeRabbit
Bug Fixes
typeof,.name, string conversion,instanceof, and related runtime checks.Tests