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. 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() + ); +}