Object.getOwnPropertyDescriptor(obj, "key") returns undefined when key is a string literal

[proggeramlug]

Summary

Object.getOwnPropertyDescriptor(obj, "x") returns undefined when the key is passed as a string literal. The same call works correctly when the key is passed via a variable binding. Discovered while doing runtime-parity work for anon-shape classes (Phase 3 of the Static Hermes object-layout plan) — this bug is pre-existing, not related to that work. It reproduces on plain object literals with the same code shape.

Repro

const p = { x: 1 };

// FAILS — returns undefined
console.log(Object.getOwnPropertyDescriptor(p, "x"));

// WORKS — returns { value: 1, writable: true, enumerable: true, configurable: true }
const k = "x";
console.log(Object.getOwnPropertyDescriptor(p, k));

Also works if Object.keys(p) is called first (any call that exercises the keys path seems to "prime" subsequent descriptor lookups).

Root cause (sketched)

Runtime-side instrumentation of js_object_get_own_property_descriptor showed:

• Literal-key call: obj_value_bits = 0x7FFF_... (STRING_TAG), extract_obj_ptr returns null → function returns undefined.
• Variable-key call: obj_value_bits = 0x7FFD_... (POINTER_TAG), extracts the real object pointer, returns the descriptor.

So the value bound as obj_value in the FFI call is being filled with the string pointer, not the object pointer. Either the compile-time dispatch is swapping arg positions, or the call-site codegen is computing the wrong NaN-box layout when the second arg is a literal string.

Expected path:

• HIR lowering at crates/perry-hir/src/lower.rs:5403-5407 ("getOwnPropertyDescriptor" arm): correctly binds obj = args[0], key = args[1] into Expr::ObjectGetOwnPropertyDescriptor(obj, key).
• Codegen at crates/perry-codegen/src/expr.rs:5107-5116: correctly calls js_object_get_own_property_descriptor(o, k) in that order.

Both sites read correct. The bug is upstream — likely in how args is assembled from call.args before the Object-static-method dispatch, or how the literal-string arg is lowered in the codegen string pool.

Why this is not blocking Phase 3

With Phase 3 (anon-class synthesis for object literals) active, the bug reproduces identically. With Phase 3 disabled, same behavior. The gap test regressions I initially attributed to Phase 3 (test_gap_array_methods, test_gap_error_extensions, test_gap_fetch_response, test_gap_object_methods, test_gap_proxy_reflect) are all pre-existing failures that trace back to this or related runtime-side bugs, not Phase 3 itself.

Impact

• test_gap_object_methods fails on the getOwnPropertyDescriptor block (lines 82-100 of the gap test).
• Any user code doing Object.getOwnPropertyDescriptor(o, "literal") silently gets undefined — no error, just wrong result.
• Probably affects other Object.* methods that take string-literal keys too; bears a wider audit.

Suggested investigation

1. Add a temporary eprintln! in js_object_get_own_property_descriptor to confirm arg bits across (a) literal key, (b) variable key, (c) computed key.
2. Examine the LLVM IR emitted at the failing call site vs the working one to see where the arg tagging diverges.
3. Walk the compile path from Expr::Call to Expr::ObjectGetOwnPropertyDescriptor to see if the string literal is being handled differently (e.g. inlined as a raw pointer without NaN-box wrapping on one path).

[proggeramlug]

Narrowing update (investigation, not fix)

Spent a session instrumenting the runtime + codegen to trace where args diverge. Key findings:

Repro shape:

const p = { xx: 1 };
const d = Object.getOwnPropertyDescriptor(p, "xx");
console.log("d:", d);

→ d: undefined (expected: { value: 1, writable: true, enumerable: true, configurable: true })

Runtime-side instrumentation at the top of js_object_get_own_property_descriptor shows the obj arg arriving with STRING_TAG, not POINTER_TAG. Decoding the string pointer: when the call is inside console.log(...), obj arrives as the preceding log arg (e.g. "desc:"). Bare-call form arrives with obj_bits = 0x0000_0000_0000_0002. Either way extract_obj_ptr returns null → function returns undefined.

HIR is correct (ObjectGetOwnPropertyDescriptor(LocalGet(0), String("xx")) — LocalGet(0) is p).

Codegen arm at crates/perry-codegen/src/expr.rs:5108 is never hit — I added eprintln! at the arm entry, at the function-entry pattern match, and at the arm's inner body; zero fire despite the HIR clearly containing the variant and lower_stmts being the entry point. The binary contains the debug strings (verified via strings), but the code path never executes them.

Strong lead: crates/perry-codegen/src/stmt.rs:253-319 — the collect_non_escaping_news scalar-replacement path catches Stmt::Let { init: Some(Expr::New { class_name, args, .. }) } and inlines the allocation into stack allocas, bypassing lower_expr entirely. Phase 3 rewrites object literals to Expr::New, so any closed-shape literal at let-binding time goes through this path. The local gets a dummy_slot (stmt.rs:278) that is never populated — subsequent LocalGet(id) loads from it returns garbage. p.x works because PropertyGet has a scalar-replaced fast path that reads directly from field_slots; external operations that need the object identity (Object.getOwnPropertyDescriptor, Object.keys, etc.) load the dummy slot and get whatever's there.

Same bug applies to Expr::Object at stmt.rs:206-241 — it's not Phase 3 specific. Reproduces with Phase 3 fully disabled. Any non-escaping object literal whose identity later flows to an introspection API breaks.

Real fix direction:

• Option A: check_object_literal_escapes_in_expr / check_new_escapes_in_expr must recognize ObjectGetOwnPropertyDescriptor, ObjectKeys, ObjectEntries, ObjectValues, JSON.stringify, for-in, {...obj} spread, Proxy/Reflect as escapes. Currently at least ObjectGetOwnPropertyDescriptor is not marked as an escape (the local survives the filter).
• Option B: Don't scalar-replace locals whose identity is ever read opaquely (passed to any non-whitelisted function, flows through Type::Any binding, etc.).

Option A is the surgical fix. Needs an audit of the escape-check visitor in collectors.rs:3872 (check_object_literal_escapes_in_expr) and collectors.rs:~2600 (the News equivalent). Every HIR variant that takes an object and does something "identity-observing" with it needs to mark the operand as escaped.

Phase 3 impact: Phase 3's anon-class synthesis does not cause this bug, but does widen its footprint — it converts a broader set of object literals into Expr::New which hits the News-escape-check path instead of the Object-escape-check path. If the News path is less defensive about escape tracking than the Object path, Phase 3 indirectly exposes more broken cases. Audit both paths together.

Session's Phase 3 work is independently shippable — gap test count unchanged at 17/28 (same as before Phase 3), no new failures, microbenchmark shows parity with legacy path. Leaving this bug open for a dedicated session.

[proggeramlug]

Fixed in v0.5.168 by making mark_all_candidate_refs_in_expr conservative (see crates/perry-codegen/src/collectors.rs:3353).

Root cause: the News-scalar-replacement escape check at collectors.rs:~2764 (check_escapes_in_expr) falls to a catch-all for HIR variants it doesn't enumerate. The catch-all delegates to mark_all_candidate_refs_in_expr, which used collect_ref_ids_in_expr to find LocalGet references — but that function has a silent _ => {} at its tail for un-enumerated variants and returns an empty ref set. Identity-observing variants like Expr::ObjectGetOwnPropertyDescriptor(LocalGet(p), String("x")) hit both catch-alls, so p was never marked escaped, scalar replacement fired, the local got a dummy_slot at stmt.rs:278 that was never stored, and the FFI call loaded garbage.

Fix: mark_all_candidate_refs_in_expr now ALSO unconditionally marks every candidate as escaped when the catch-all fires. Matches the existing defensive pattern that check_object_literal_escapes_in_expr has always had for Object-literal candidates. Conservative but correct — the alternative would be to enumerate every HIR variant that might embed a LocalGet (there are dozens, and new ones land regularly).

Verification:

• const p = new Point(1); Object.getOwnPropertyDescriptor(p, "x") → now returns {value:1, writable:true, enumerable:true, configurable:true} (was undefined)
• const p = {xx:1}; Object.getOwnPropertyDescriptor(p, "xx") (Phase 3 anon-shape path) → same fix applies
• Gap tests: 17/28 (unchanged — the fix neither regresses nor newly passes; the other gap-test failures trace to different bugs)
• HIR tests: 28/28
• Microbenchmarks: object_create 2-4ms, method_calls 1-4ms, {x,y} literal access 30ms/5M iter — no regression

Follow-up (not blocking): the surgical alternative is to enumerate every identity-observing variant (ObjectGetOwnPropertyDescriptor, ObjectGetOwnPropertyNames, ObjectGetOwnPropertySymbols, JSON.stringify, for-in, spread, Proxy/Reflect, etc.) in both check_escapes_in_expr and collect_ref_ids_in_expr so the conservative catch-all never fires. That keeps scalar replacement coverage higher. Not urgent — current fix is sound, just coarse.