fix(codegen): initialize imported private brands once - #8986
Merged
proggeramlug merged 2 commits intoAug 28, 2026
Conversation
…twice (PerryTS#8962) `import { Hono } from "hono"; new Hono()` compiled and linked, then threw `TypeError: Cannot initialize private elements twice on the same object` during construction. It reduces to two files and no inheritance at all: // base.ts export class BaseX { #m(): number { return 1; } call(): number { return this.#m(); } } // main.ts import { BaseX } from "./base"; new BaseX().call(); The importing module sees the class only as the metadata-only stub `compile_module` synthesizes for an import (`codegen/mod.rs`, "Build a stub Class with the minimum fields the codegen needs"). A stub is a name table: it carries member names so dispatch symbols resolve, and carries no bodies, no initializers and no constructor. Everything construction actually *does* is baked into the defining module's standalone `<prefix>__<class>_constructor` instead — `codegen/method.rs` says so where it emits them, "At the `new ImportedClass(...)` call site, `lower_new` applies initializers against the imported class stub — which has none". That premise held for FIELDS, because the stub flattens every field to `is_private: false` with `init: None`: the worst `apply_field_initializers_ recursive` could do at the `new` site was write `undefined` into a slot the real constructor overwrote moments later. It did not hold for the private BRAND. The stub copies private METHOD and accessor names verbatim, and `has_private_instance_brand` is defined purely over `#`-prefixed member names, so a stub answered `true` and the `new` site emitted `js_private_brand_add` on top of the one the defining module's constructor emits. Installing a class's brand twice on one object is the error PrivateMethodOrAccessorAdd requires, so the runtime threw — correctly, at the second install. Fix: `apply_field_initializers_recursive` skips the private-element decision for a chain entry that is an imported stub. The duplicate check itself is untouched: exactly one `js_private_brand_add` survives, in the defining module's constructor (verified with objdump — the importing module's object now has none, the defining module's still has one). Reached both spellings: the class constructed directly (`new BaseX()`), and the class reached as an ANCESTOR through the `AncestorsOnly` walk, where the leaf is a local subclass. hono hits the second — `class Hono extends HonoBase` with `#path`, `#notFoundHandler`, `#clone`, `#addRoute`, `#dispatch` on the base. Only classes with a private method or accessor were affected; a private field alone never was, since the stub does not mark fields private. Tests: `crates/perry/tests/issue_8962_imported_class_private_brand.rs`. Every case calls the private member after constructing, so a fix that dropped the second install without leaving the first standing fails them too — the brand check throws when no brand is present. Two guard cases pin the boundaries: same-module construction still installs the brand at the `new` site, and a genuine double initialization (a base ctor returning an object the derived class already branded) still throws. Verified: `new Hono()` runs (routing, `route()`, `basePath()`, `fetch`); `cargo test -p perry --bin perry` 1049/1049; `cargo test -p perry-hir -p perry-codegen` all green; mb24's `packages/db/src/migrate.ts` still compiles. Claude-Session: https://claude.ai/code/session_0145yUtx1jiWHf66QEZh6DzY
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Contributor
Author
|
Merged (batched with #8987). Leaving brand installation to the defining module's standalone constructor is the right call — re-branding in the importing module produces a second brand for the same class, so a private access valid through one import path fails through the other, which is exactly the sort of thing that looks like "private fields are broken" rather than "imports are branded twice". The six integration cases are well chosen: direct imports, accessors, local and imported subclasses, same-module branding, and genuine duplicate initialization. That last one matters most — it is the case that distinguishes "installed once" from "never installed again", and without it the fix could silently skip a brand that really was needed. One fix pushed: this PR had no changelog fragment. It touches three Validation — codegen 1341/0, runtime 2781/0, hir 355/0 ( |
proggeramlug
added a commit
that referenced
this pull request
Aug 28, 2026
…8988) * perf(runtime): skip dead feedback observation on the property wrappers Typed-feedback recording is off by default, and guard_observe and record_fallback_call both early-return in that mode — but the property wrappers had already built the whole Observation to hand them, hashing the key and resolving the receiver's shape first. On an isolated property-read loop js_typed_feedback_object_get_field_by_name_f64 was 10% of self time, nearly all of it that dead work. Apply #5094's gate, which the array index wrappers already carry and #8951 gave the fast store path: when recording is off, take the underlying op directly. Behaviour is unchanged in both modes — with recording off guard_observe returns contract_valid and the fallback recorder is a no-op, so the wrapper already reduced to exactly this call. Also: object_live_slot_count reads live_inline_slot_count through the shape table's record instead of lifting the whole ~48-byte descriptor to discard all but four bytes. That bound is consulted on essentially every property operation, and shape_descriptor_by_id was 10.1% of the same loop. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * perf(runtime): megamorphic read stub cache for dynamic string-keyed reads The read twin of the dynamic-write stub, 2-way set-associative from the start (#8977 measured what direct-mapped costs: a colliding pair evicts each other every rotation, so both miss forever). A hit skips js_object_get_field_by_name's fast-lane guard chain — address class, interned-key flag, arena classification, header type/flags/class, keys-array validation — plus the read-plan probe, whose epoch the collector bumps at loop-poll cadence, so on a steady read loop it is repeatedly cold and falls through to a shape-index hash lookup. Safety mirrors the write stub: entries store CONTENT bits, never an address, so a recycled key address cannot produce a false hit, and keys that do not fit the inline form are not cached. Every hit re-validates heap-object type, not-forwarded, blocking flags, class id, and the receiver's current shape token — which pins the exact key set and order, so a match means the cached slot still names this key. The probe sits after the process.env and Proxy arms, which keep their own semantics, and the stub is only primed from inside the lane, once the receiver is proved ordinary. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP * fix(codegen): initialize imported private brands once (#8986) * fix(codegen): an imported class no longer installs its private brand twice (#8962) `import { Hono } from "hono"; new Hono()` compiled and linked, then threw `TypeError: Cannot initialize private elements twice on the same object` during construction. It reduces to two files and no inheritance at all: // base.ts export class BaseX { #m(): number { return 1; } call(): number { return this.#m(); } } // main.ts import { BaseX } from "./base"; new BaseX().call(); The importing module sees the class only as the metadata-only stub `compile_module` synthesizes for an import (`codegen/mod.rs`, "Build a stub Class with the minimum fields the codegen needs"). A stub is a name table: it carries member names so dispatch symbols resolve, and carries no bodies, no initializers and no constructor. Everything construction actually *does* is baked into the defining module's standalone `<prefix>__<class>_constructor` instead — `codegen/method.rs` says so where it emits them, "At the `new ImportedClass(...)` call site, `lower_new` applies initializers against the imported class stub — which has none". That premise held for FIELDS, because the stub flattens every field to `is_private: false` with `init: None`: the worst `apply_field_initializers_ recursive` could do at the `new` site was write `undefined` into a slot the real constructor overwrote moments later. It did not hold for the private BRAND. The stub copies private METHOD and accessor names verbatim, and `has_private_instance_brand` is defined purely over `#`-prefixed member names, so a stub answered `true` and the `new` site emitted `js_private_brand_add` on top of the one the defining module's constructor emits. Installing a class's brand twice on one object is the error PrivateMethodOrAccessorAdd requires, so the runtime threw — correctly, at the second install. Fix: `apply_field_initializers_recursive` skips the private-element decision for a chain entry that is an imported stub. The duplicate check itself is untouched: exactly one `js_private_brand_add` survives, in the defining module's constructor (verified with objdump — the importing module's object now has none, the defining module's still has one). Reached both spellings: the class constructed directly (`new BaseX()`), and the class reached as an ANCESTOR through the `AncestorsOnly` walk, where the leaf is a local subclass. hono hits the second — `class Hono extends HonoBase` with `#path`, `#notFoundHandler`, `#clone`, `#addRoute`, `#dispatch` on the base. Only classes with a private method or accessor were affected; a private field alone never was, since the stub does not mark fields private. Tests: `crates/perry/tests/issue_8962_imported_class_private_brand.rs`. Every case calls the private member after constructing, so a fix that dropped the second install without leaving the first standing fails them too — the brand check throws when no brand is present. Two guard cases pin the boundaries: same-module construction still installs the brand at the `new` site, and a genuine double initialization (a base ctor returning an object the derived class already branded) still throws. Verified: `new Hono()` runs (routing, `route()`, `basePath()`, `fetch`); `cargo test -p perry --bin perry` 1049/1049; `cargo test -p perry-hir -p perry-codegen` all green; mb24's `packages/db/src/migrate.ts` still compiles. Claude-Session: https://claude.ai/code/session_0145yUtx1jiWHf66QEZh6DzY * chore: PR-key the fragment --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> * fix(runtime): inherit Array-subclass fill (#8987) * fix(runtime): inherit Array-subclass fill (#8953) * chore: PR-key the fragment; reuse the shared StringHeader payload helper --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> * chore: PR-key the fragment, drop a duplicate, classify READ_STUB READ_STUB is a new identity-ratcheted thread-local holder; recorded the same not_a_gc_pointer verdict WRITE_STUB carries, since read_stub_key_bits returns short_ascii_sso_bits (content packed inline) and never a heap address. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> Co-authored-by: x <x@x>
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
Testing
cargo test -p perry --test issue_8962_imported_class_private_brand -- --test-threads=1(6 passed)cargo test -p perry-hir -p perry-codegen --lib(1,696 passed, 2 ignored)cargo build --profile perry-dev -p perry -p perry-runtime-static -p perry-stdlib-static./scripts/pre-tag-check.sh --quickperrymaster.skelpo.net(new Hono()plus route registration)Known baseline
The full
perry-codegenintegration run reaches an unrelated stale assertion intemp_root_operand_temporaries: it expectsi64 @js_string_concat_value, while current main emitsdouble @js_string_concat_value_box. The changed files do not touch string concatenation.Fixes #8962