From fee63367d4abee4809ef5ba2dee079ea59279957 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 28 Aug 2026 21:55:25 +0000 Subject: [PATCH 1/2] fix: preserve private field update semantics --- crates/perry-codegen/src/codegen/mod.rs | 40 ++++-- .../perry-codegen/src/lower_call/new_alloc.rs | 25 ++-- crates/perry-codegen/src/typed_shape.rs | 5 +- crates/perry-hir/src/lower/mod.rs | 4 +- crates/perry-hir/src/lower_patterns.rs | 15 ++- .../src/object/field_get_set/enumeration.rs | 7 +- ..._gap_8969_private_field_compound_update.ts | 119 ++++++++++++++++++ 7 files changed, 187 insertions(+), 28 deletions(-) create mode 100644 test-files/test_gap_8969_private_field_compound_update.ts diff --git a/crates/perry-codegen/src/codegen/mod.rs b/crates/perry-codegen/src/codegen/mod.rs index 85d4a803d7..0e8ca294ff 100644 --- a/crates/perry-codegen/src/codegen/mod.rs +++ b/crates/perry-codegen/src/codegen/mod.rs @@ -1164,15 +1164,17 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> // first (walking from deepest ancestor down) so the slot // order matches `class_field_global_index`'s assumption. let mut packed_keys = String::new(); - // Skip computed-key fields (`[Symbol.for("k")] = …`): their key is an - // expression evaluated at runtime, not a stable string, so they don't - // get an inline slot. Including their synthetic `__computed_field_*` - // names in the packed keys would surface them as enumerable own - // properties via Object.keys() and inflate the inline-slot count. - // Their values are stored via `apply_field_initializers_recursive`'s - // IndexSet path → js_object_set_field / js_object_set_symbol_property. + // Skip computed-key fields (`[Symbol.for("k")] = …`) and private + // fields. Computed keys are evaluated at construction time; private + // fields live in class-id-qualified runtime storage installed by + // `js_private_field_add`. Neither is a public inline shape key. + // Including either synthetic/source spelling in packed keys leaks it + // through reflection and inflates/misaligns the inline-slot layout. let count_keyable = |fields: &[perry_hir::ClassField]| -> u32 { - fields.iter().filter(|f| f.key_expr.is_none()).count() as u32 + fields + .iter() + .filter(|f| f.key_expr.is_none() && !f.is_private) + .count() as u32 }; let mut total_field_count = count_keyable(&c.fields); // (parent_name, resolved_fields) captured during the chain walk so we @@ -1251,7 +1253,7 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> // (which would risk re-picking the wrong same-named stub). for (_parent_name, parent_fields) in parent_chain.iter().rev() { for f in parent_fields { - if f.key_expr.is_some() { + if f.key_expr.is_some() || f.is_private { continue; } packed_keys.push_str(&f.name); @@ -1259,7 +1261,7 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> } } for f in &c.fields { - if f.key_expr.is_some() { + if f.key_expr.is_some() || f.is_private { continue; } packed_keys.push_str(&f.name); @@ -1351,7 +1353,13 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> ); class_keys_globals_map.insert(c.name.clone(), global_name.clone()); let mut packed_keys = String::new(); - let mut total_field_count = c.fields.len() as u32; + let keyable_count = |fields: &[perry_hir::ClassField]| -> u32 { + fields + .iter() + .filter(|f| f.key_expr.is_none() && !f.is_private) + .count() as u32 + }; + let mut total_field_count = keyable_count(&c.fields); // Issue #485: imported subclass stubs also need their parent's // fields prepended to the packed-keys, so allocations on this // importing side reserve enough inline slots for parent + @@ -1379,12 +1387,12 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> resolve_parent(&parent_name, child_prefix.as_deref()) { parent_chain.push((parent_name.clone(), parent_fields.clone())); - total_field_count += parent_fields.len() as u32; + total_field_count += keyable_count(&parent_fields); p = parent_extends; child_prefix = Some(parent_prefix); } else if let Some(parent) = hir.classes.iter().find(|cls| cls.name == parent_name) { parent_chain.push((parent_name.clone(), parent.fields.clone())); - total_field_count += parent.fields.len() as u32; + total_field_count += keyable_count(&parent.fields); p = parent.extends_name.clone(); child_prefix = Some(module_prefix.clone()); } else { @@ -1393,11 +1401,17 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result> } for (_parent_name, parent_fields) in parent_chain.iter().rev() { for f in parent_fields { + if f.key_expr.is_some() || f.is_private { + continue; + } packed_keys.push_str(&f.name); packed_keys.push('\0'); } } for f in &c.fields { + if f.key_expr.is_some() || f.is_private { + continue; + } packed_keys.push_str(&f.name); packed_keys.push('\0'); } diff --git a/crates/perry-codegen/src/lower_call/new_alloc.rs b/crates/perry-codegen/src/lower_call/new_alloc.rs index f62d2b1301..2a43b655fa 100644 --- a/crates/perry-codegen/src/lower_call/new_alloc.rs +++ b/crates/perry-codegen/src/lower_call/new_alloc.rs @@ -173,7 +173,13 @@ fn emit_instance_alloc_inner( // Compute total field count including inherited parent fields. // The runtime allocates at least 8 inline slots regardless, so this // mostly matters for shapes >8 fields. - let mut field_count = class.fields.len() as u32; + let public_keyable_count = |fields: &[perry_hir::ClassField]| -> u32 { + fields + .iter() + .filter(|field| field.key_expr.is_none() && !field.is_private) + .count() as u32 + }; + let mut field_count = public_keyable_count(&class.fields); // Imported classes now carry their real field_names from the source // module. If the field count is still 0 (no fields info available), // use a generous default as a safety net. @@ -183,7 +189,7 @@ fn emit_instance_alloc_inner( let mut parent = class.extends_name.as_deref(); while let Some(parent_name) = parent { if let Some(p) = ctx.classes.get(parent_name).copied() { - field_count += p.fields.len() as u32; + field_count += public_keyable_count(&p.fields); parent = p.extends_name.as_deref(); } else { break; @@ -306,7 +312,7 @@ fn emit_instance_alloc_inner( // inline bump-alloc fast path (which would bake the wrong layout). let mut packed_keys = String::new(); for f in &class.fields { - if f.key_expr.is_some() { + if f.key_expr.is_some() || f.is_private { continue; } packed_keys.push_str(&f.name); @@ -688,15 +694,12 @@ fn emit_instance_alloc_inner( break; } } - // Skip computed-key fields: their key is an expression evaluated at - // construction time, not a stable string, so they don't get an inline - // slot. The runtime stores them via IndexSet → js_object_set_field / - // js_object_set_symbol_property paths in `apply_field_initializers_recursive`. - // Including their synthetic `__computed_field_*` names in packed_keys - // would surface them as enumerable own properties on Object.keys(). + // Skip computed and private fields: both are initialized through + // dedicated runtime paths and neither belongs in the public inline + // shape exposed by Object.keys/getOwnPropertyNames. for pc in parent_chain.iter().rev() { for f in &pc.fields { - if f.key_expr.is_some() { + if f.key_expr.is_some() || f.is_private { continue; } packed_keys.push_str(&f.name); @@ -704,7 +707,7 @@ fn emit_instance_alloc_inner( } } for f in &class.fields { - if f.key_expr.is_some() { + if f.key_expr.is_some() || f.is_private { continue; } packed_keys.push_str(&f.name); diff --git a/crates/perry-codegen/src/typed_shape.rs b/crates/perry-codegen/src/typed_shape.rs index 17d89af661..d45b6b2cfa 100644 --- a/crates/perry-codegen/src/typed_shape.rs +++ b/crates/perry-codegen/src/typed_shape.rs @@ -298,7 +298,10 @@ fn typed_layout_from_fields<'a>( let mut pointer_mask_words = Vec::new(); let mut slot_count = 0u32; for field in fields { - if field.key_expr.is_some() { + // Computed fields and private fields are initialized through runtime + // storage, not the public inline slots represented by this descriptor. + // Keep the mask indices in lockstep with the packed class keys. + if field.key_expr.is_some() || field.is_private { continue; } let slot = slot_count as usize; diff --git a/crates/perry-hir/src/lower/mod.rs b/crates/perry-hir/src/lower/mod.rs index 6ce65947ac..7a2cc5d62c 100644 --- a/crates/perry-hir/src/lower/mod.rs +++ b/crates/perry-hir/src/lower/mod.rs @@ -42,7 +42,9 @@ mod expr_call; pub(crate) mod expr_function; pub(crate) use expr_function::capture_function_source; mod expr_member; -pub(crate) use expr_member::{private_storage_property, wrap_private_guard, PRIV_OP_WRITE}; +pub(crate) use expr_member::{ + private_storage_property, wrap_private_guard, PRIV_OP_READ, PRIV_OP_WRITE, +}; mod expr_misc; mod expr_new; mod expr_new_builtins; diff --git a/crates/perry-hir/src/lower_patterns.rs b/crates/perry-hir/src/lower_patterns.rs index 217044e333..06c0f3f805 100644 --- a/crates/perry-hir/src/lower_patterns.rs +++ b/crates/perry-hir/src/lower_patterns.rs @@ -244,7 +244,20 @@ pub(crate) fn lower_assign_target_to_expr( Ok(Expr::IndexGet { object, index }) } ast::MemberProp::PrivateName(private) => { - let property = format!("#{}", private.name); + // A compound/logical assignment reads the target before + // writing it back. Private fields do not live under their + // source spelling (`#n`): use the same guarded, class-id- + // qualified storage lookup as an ordinary `this.#n` read. + // Reading `#n` as a public property returns `undefined`, + // which made `this.#n += 1` store NaN in the real slot. + let private_name = format!("#{}", private.name); + let object = crate::lower::wrap_private_guard( + ctx, + object, + &private_name, + crate::lower::PRIV_OP_READ, + ); + let property = crate::lower::private_storage_property(ctx, &private_name); Ok(Expr::PropertyGet { byte_offset: 0, object, diff --git a/crates/perry-runtime/src/object/field_get_set/enumeration.rs b/crates/perry-runtime/src/object/field_get_set/enumeration.rs index b86c326272..001b9ddd0a 100644 --- a/crates/perry-runtime/src/object/field_get_set/enumeration.rs +++ b/crates/perry-runtime/src/object/field_get_set/enumeration.rs @@ -1368,6 +1368,12 @@ pub(crate) unsafe fn instance_private_key_hidden( /// prefix test would wrongly hide legitimate user properties whose name happens /// to begin with `__perry_` (e.g. `this.__perry_user = 1`). /// +/// `#` is deliberately NOT in this list. It is a +/// transient compiler routing key for private method/accessor operations; the +/// runtime consumes it only when a matching private-access hint is pending and +/// never installs it as private object storage. Without a hint, that spelling +/// is ordinary user data and must remain visible to reflection. +/// /// The one prefix family is `__perry_native_super__` (#6316): the native /// base method a subclass override displaced. Its key set is parameterized by /// method name, so an exact allowlist cannot enumerate it. The prefix is a @@ -1386,7 +1392,6 @@ pub(crate) fn is_internal_runtime_key_bytes(b: &[u8]) -> bool { || b == b"#" || b == b"#" || b.starts_with(b"#