From 84936b345d7e847449bb5ba5e444e5d41af57e4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 28 Aug 2026 15:25:22 +0000 Subject: [PATCH 1/2] fix(codegen): an imported class no longer installs its private brand twice (#8962) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 `___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 --- .../src/lower_call/field_init.rs | 29 ++ crates/perry-hir/src/ir/decl.rs | 21 ++ ...issue_8962_imported_class_private_brand.rs | 284 ++++++++++++++++++ 3 files changed, 334 insertions(+) create mode 100644 crates/perry/tests/issue_8962_imported_class_private_brand.rs diff --git a/crates/perry-codegen/src/lower_call/field_init.rs b/crates/perry-codegen/src/lower_call/field_init.rs index 9090685aaa..6615765a10 100644 --- a/crates/perry-codegen/src/lower_call/field_init.rs +++ b/crates/perry-codegen/src/lower_call/field_init.rs @@ -693,10 +693,39 @@ pub(crate) fn apply_field_initializers_recursive( None => init_pairs.push((field.name.clone(), init, field.is_private)), } } + // #8962: an IMPORTED class installs nothing here. Its whole + // field-initializer phase — public field writes, private-field adds AND + // the shared private brand — is baked into the defining module's + // standalone `___constructor`, which `codegen/method.rs` + // emits for exactly that reason ("At the `new ImportedClass(...)` call + // site, `lower_new` applies initializers against the imported class + // stub — which has none"). That premise holds for FIELDS because the + // stub flattens every field to `is_private: false` with `init: None`, + // so the worst this loop could do was write `undefined` into a slot the + // real constructor overwrites moments later. + // + // It does NOT hold for the private BRAND. The stub copies private + // METHOD and accessor names verbatim (it needs them to resolve dispatch + // symbols), and `has_private_instance_brand` is defined purely over + // `#`-prefixed method/getter/setter names — so a stub answers `true` and + // this site emitted `js_private_brand_add` at the importing module's + // `new`, on top of the one the defining module's constructor emits. + // Installing a class's brand twice on one object is the observable + // error PrivateMethodOrAccessorAdd requires, so the runtime threw + // "Cannot initialize private elements twice on the same object" out of + // `new Hono()` — any imported class with a private method or accessor, + // whether constructed directly or reached as an ancestor through + // `AncestorsOnly`. + // + // Suppressing BOTH flags (not just the brand) is what restores the + // `continue` below for a stub whose only private elements are methods: + // for a stub the two predicates are the same question, since its fields + // are never private. let (class_has_private_elements, class_has_private_brand) = ctx .classes .get(&class_name_in_chain) .copied() + .filter(|class| !class.is_imported_stub()) .map(|class| { ( class.has_private_instance_elements(), diff --git a/crates/perry-hir/src/ir/decl.rs b/crates/perry-hir/src/ir/decl.rs index 4683d1b220..dbbe74e3cc 100644 --- a/crates/perry-hir/src/ir/decl.rs +++ b/crates/perry-hir/src/ir/decl.rs @@ -284,6 +284,27 @@ pub struct Class { } impl Class { + /// True for the metadata-only stub `compile_module` synthesizes for a class + /// IMPORTED from another module (`perry-codegen/src/codegen/mod.rs`, "Build + /// a stub Class with the minimum fields the codegen needs"). + /// + /// A stub is a NAME TABLE, not a class: it carries member names so the + /// importing module can resolve dispatch symbols, and carries no bodies, no + /// field initializers and no constructor. Everything a construction + /// actually *does* — field initializers, private-field adds, the private + /// brand — is baked into the defining module's standalone + /// `___constructor` instead (`codegen/method.rs`, + /// `is_constructor_method`), precisely because the stub has none of it. + /// + /// `id == 0` is the marker: the driver hands out class ids from 1 + /// (`run_pipeline.rs`: "Start at 1, 0 is reserved for \"no parent\"") and + /// every local class takes its id from `LoweringContext::fresh_class`, so + /// the stub built at `codegen/mod.rs` ("id: 0, // imported — no local + /// ClassId") is the only `Class` in a module's class table with id 0. + pub fn is_imported_stub(&self) -> bool { + self.id == 0 + } + /// Whether construction installs any instance-private element. pub fn has_private_instance_elements(&self) -> bool { self.fields.iter().any(|field| field.is_private) diff --git a/crates/perry/tests/issue_8962_imported_class_private_brand.rs b/crates/perry/tests/issue_8962_imported_class_private_brand.rs new file mode 100644 index 0000000000..58eea312a1 --- /dev/null +++ b/crates/perry/tests/issue_8962_imported_class_private_brand.rs @@ -0,0 +1,284 @@ +//! Regression for #8962: constructing a class IMPORTED from another module +//! threw `TypeError: Cannot initialize private elements twice on the same +//! object` when that class declared a private method or accessor. +//! +//! `import { Hono } from "hono"; new Hono()` was the report. The importing +//! module sees the class only as the metadata-only stub `compile_module` +//! builds — a name table with no bodies and no initializers — but the stub +//! copies private METHOD names verbatim (it needs them to resolve dispatch +//! symbols), and `Class::has_private_instance_brand` is defined purely over +//! `#`-prefixed member names. So the `new` site emitted `js_private_brand_add` +//! for a brand it does not own, on top of the one the DEFINING module's +//! standalone `___constructor` emits — and installing a class's +//! brand twice on one object is the error PrivateMethodOrAccessorAdd requires. +//! +//! Every case here calls the private member after construction, so a fix that +//! merely dropped the second install without leaving the first one standing +//! would fail these too: the brand check inside the private-member access +//! throws when no brand is present. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Compile `files` (relative path -> source) with `entry` as the entry point +/// and return the binary's stdout. Panics with the compiler's or the program's +/// output on any failure. +fn compile_and_run(files: &[(&str, &str)], entry: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + for (name, source) in files { + let path = root.join(name); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("mkdir"); + } + std::fs::write(&path, source).expect("write source"); + } + let entry_path = root.join(entry); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(&entry_path) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + assert!( + run.status.success(), + "binary failed (status {:?})\nstdout:\n{}\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_string() +} + +/// The reduced `new Hono()`: a class with a private METHOD, declared in one +/// module and constructed in another. No inheritance is needed to trigger it. +#[test] +fn imported_class_with_private_method_constructs_once() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +"#, + ), + ( + "main.ts", + r#"import { BaseX } from "./base"; +console.log(new BaseX().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// A private ACCESSOR carries the same brand as a private method, and the stub +/// copies getter/setter names the same way. +#[test] +fn imported_class_with_private_getter_constructs_once() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + v = 41; + get #g(): number { return this.v + 1; } + call(): number { return this.#g; } +} +"#, + ), + ( + "main.ts", + r#"import { BaseX } from "./base"; +console.log(new BaseX().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// A private-method class reached as an ANCESTOR: the leaf is local, so the +/// brand came from the `AncestorsOnly` walk at the `new` site rather than from +/// the leaf's own entry. Both spellings of the subclass — with and without an +/// explicit constructor — take different paths through `lower_new`. +#[test] +fn local_subclass_of_imported_private_method_class() { + let base = r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +"#; + let with_ctor = compile_and_run( + &[ + ("base.ts", base), + ( + "main.ts", + r#"import { BaseX } from "./base"; +class D extends BaseX { constructor() { super(); } } +console.log(new D().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(with_ctor, "42"); + + let without_ctor = compile_and_run( + &[ + ("base.ts", base), + ( + "main.ts", + r#"import { BaseX } from "./base"; +class D extends BaseX {} +console.log(new D().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(without_ctor, "42"); +} + +/// hono's own shape: the base with the private members is in one module, the +/// subclass that `super()`s into it is an anonymous class expression in a +/// second, and the `new` is in a third. Every link in the chain is an imported +/// stub at the site that constructs it. +#[test] +fn imported_subclass_of_imported_private_method_class() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"const notFound = (x: string): string => "nf:" + x; +export class BaseX { + pub: number; + #path = "/"; + #nf = notFound; + constructor(options: any = {}) { + this.pub = 1; + } + #addRoute(m: string): string { return m + this.#path; } + route(m: string): string { return this.#addRoute(m) + this.#nf("!"); } +} +"#, + ), + ( + "mid.ts", + r#"import { BaseX } from "./base"; +export const DerivedX = class extends BaseX { + constructor(options: any = {}) { super(options); } +}; +"#, + ), + ( + "main.ts", + r#"import { DerivedX } from "./mid"; +const a = new DerivedX(); +console.log(a.route("GET") + "|" + a.pub); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "GET/nf:!|1"); +} + +/// The same class constructed INSIDE its defining module never had the bug — +/// there the `new` site owns the field-initializer phase and installs the +/// brand itself. Pin it, so a fix that suppressed the install unconditionally +/// (rather than only where another module already performs it) fails here. +#[test] +fn same_module_construction_still_installs_the_brand() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +export function make(): BaseX { return new BaseX(); } +"#, + ), + ( + "main.ts", + r#"import { make } from "./base"; +console.log(make().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// Double initialization must still be observable where the spec requires it: +/// a base constructor that returns an object the derived class has already +/// branded. This is the case `js_private_brand_add`'s duplicate check exists +/// for, and #8962's fix must not silence it. +#[test] +fn genuine_double_initialization_still_throws() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::write( + root.join("main.ts"), + r#"const recycled: any = {}; +class Base { + constructor() { return recycled; } +} +class Derived extends Base { + #m(): number { return 1; } + call(): number { return this.#m(); } +} +new Derived(); +new Derived(); +console.log("no throw"); +"#, + ) + .expect("write entry"); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(root.join("main.ts")) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstderr:\n{}", + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + let stderr = String::from_utf8_lossy(&run.stderr); + let stdout = String::from_utf8_lossy(&run.stdout); + assert!( + !run.status.success() && stderr.contains("private elements twice"), + "expected the second construction to throw the duplicate-brand \ + TypeError\nstatus: {:?}\nstdout:\n{stdout}\nstderr:\n{stderr}", + run.status.code() + ); +} From 5a6a5c5f7105291f462d6b81a9af3cf792fdb460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 00:23:01 +0200 Subject: [PATCH 2/2] chore: PR-key the fragment --- changelog.d/8986-imported-private-brands.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog.d/8986-imported-private-brands.md diff --git a/changelog.d/8986-imported-private-brands.md b/changelog.d/8986-imported-private-brands.md new file mode 100644 index 0000000000..5025ed4354 --- /dev/null +++ b/changelog.d/8986-imported-private-brands.md @@ -0,0 +1,5 @@ +Imported private brands are installed once, by the defining module's standalone constructor. + +A metadata-only imported class stub is now identified explicitly, so importing a class that uses private elements no longer re-runs brand installation in the importing module. Re-branding produced a second brand for the same class, so a private access that had been valid through one import path failed through the other. + +Covers direct imports, accessors, local and imported subclasses, same-module branding, and genuine duplicate initialization.