diff --git a/changelog.d/8984-private-field-updates.md b/changelog.d/8984-private-field-updates.md new file mode 100644 index 0000000000..81d0c5f2a7 --- /dev/null +++ b/changelog.d/8984-private-field-updates.md @@ -0,0 +1,7 @@ +Private class fields now preserve their value under compound and logical +assignments instead of reading `undefined` and storing `NaN`. + +Private fields no longer occupy public class-shape keys, so they stay absent +from `Object.keys`, `Object.getOwnPropertyNames`, `for...in`, spread, and JSON +serialization. An ordinary property whose name matches Perry's transient +private-member routing spelling is now retained as ordinary user data. 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_patterns.rs b/crates/perry-hir/src/lower_patterns.rs index 204f860179..b7f9e4bd39 100644 --- a/crates/perry-hir/src/lower_patterns.rs +++ b/crates/perry-hir/src/lower_patterns.rs @@ -246,9 +246,12 @@ pub(crate) fn lower_assign_target_to_expr( Ok(Expr::IndexGet { object, index }) } ast::MemberProp::PrivateName(private) => { - // Compound and logical assignments lower the read and write - // halves separately. Match ordinary private-member reads: - // guard the receiver and use the class-mangled storage key. + // 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 = wrap_private_guard(ctx, object, &private_name, PRIV_OP_READ); let property = private_storage_property(ctx, &private_name); 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"#