fix: preserve private field update and reflection semantics

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.

crates/perry-codegen/src/codegen/mod.rs

@@ -1164,15 +1164,17 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result<Vec<u8>>
         // 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,15 +1253,15 @@ pub fn compile_module(hir: &HirModule, opts: CompileOptions) -> Result<Vec<u8>>
         // (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);
                 packed_keys.push('\0');
             }
         }
         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<Vec<u8>>
         );
         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<Vec<u8>>
                 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<Vec<u8>>
         }
         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');
         }

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,23 +694,20 @@ 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);
                 packed_keys.push('\0');
             }
         }
         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);

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;

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);

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`).
 ///
+/// `#<perry:private-member:…>` 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__<method>` (#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"#<perry:class-evaluation-prototype>"
         || b == b"#<perry:private-class-lexical-binding>"
         || b.starts_with(b"#<perry:private-brand:")
-        || b.starts_with(b"#<perry:private-member:")
         || b.starts_with(b"#<perry:private-field:")
         || b.starts_with(b"#<perry:private-value:")
         || b.starts_with(b"#<perry:class-evaluation-method:")

test-files/test_gap_8969_private_field_compound_update.ts

@@ -0,0 +1,119 @@
+class Counter {
+    #count = 0;
+
+    explicit(): number {
+        this.#count = this.#count + 1;
+        return this.#count;
+    }
+
+    readInExpr(): number {
+        const value = this.#count;
+        return value + 1;
+    }
+
+    compound(): number {
+        this.#count += 1;
+        return this.#count;
+    }
+
+    logical(): number {
+        this.#count ||= 10;
+        this.#count &&= 4;
+        this.#count ??= 20;
+        return this.#count;
+    }
+
+    postfix(): number {
+        return this.#count++;
+    }
+
+    prefix(): number {
+        return ++this.#count;
+    }
+
+    read(): number {
+        return this.#count;
+    }
+}
+
+const compound = new Counter();
+console.log("explicit", compound.explicit());
+console.log("readInExpr", compound.readInExpr());
+console.log("compound", compound.compound());
+console.log("logical", compound.logical());
+
+// Exercise update lowering on a fresh instance so a failed compound write
+// cannot poison the observation.
+const update = new Counter();
+console.log("postfix-result", update.postfix());
+console.log("postfix-value", update.read());
+console.log("prefix-result", update.prefix());
+console.log("prefix-value", update.read());
+
+class Hidden {
+    #value = 5;
+
+    read(): number {
+        return this.#value;
+    }
+}
+
+const hidden = new Hidden();
+console.log("hidden-read", hidden.read());
+console.log("own-names", JSON.stringify(Object.getOwnPropertyNames(hidden)));
+console.log("keys", JSON.stringify(Object.keys(hidden)));
+console.log("json", JSON.stringify(hidden));
+console.log("spread", JSON.stringify({ ...hidden }));
+let forIn = "";
+for (const key in hidden) {
+    forIn += key;
+}
+console.log("for-in", forIn);
+
+// A compiler routing spelling used as ordinary user data must remain an
+// ordinary property when no private-access hint accompanies it.
+const collisionKey = "#<perry:private-member:1:x>";
+const collision: Record<string, number> = {};
+collision[collisionKey] = 8;
+console.log("collision-json", JSON.stringify(collision));
+console.log("collision-keys", JSON.stringify(Object.keys(collision)));
+console.log("collision-names", JSON.stringify(Object.getOwnPropertyNames(collision)));
+console.log("collision-read", collision[collisionKey]);
+console.log("collision-in", collisionKey in collision);
+console.log("collision-own", Object.hasOwn(collision, collisionKey));
+
+// Private fields must not consume or shift public shape slots, including
+// across an inheritance chain.
+class Parent {
+    parent = 1;
+    #parentSecret = 2;
+
+    parentTotal(): number {
+        return this.parent + this.#parentSecret;
+    }
+}
+
+class Child extends Parent {
+    child = 3;
+    #childSecret = 4;
+
+    total(): number {
+        return this.parentTotal() + this.child + this.#childSecret;
+    }
+}
+
+const mixed = new Child();
+console.log("mixed-total", mixed.total());
+console.log("mixed-keys", JSON.stringify(Object.keys(mixed)));
+console.log("mixed-names", JSON.stringify(Object.getOwnPropertyNames(mixed)));
+
+class StaticCounter {
+    static #count = 0;
+
+    static increment(): number {
+        this.#count += 1;
+        return this.#count;
+    }
+}
+
+console.log("static-compound", StaticCounter.increment());