Fix napi_get_property_names conformance on JSC, ChakraCore and QuickJS

[bkaradzic-microsoft]

Fixes #216.

napi_get_property_names is specified to return the enumerable string-keyed properties of an object and of its prototype chain — the same set a for...in loop visits. Only the V8 backend did that. Every other backend diverged:

backend	enumerable-only	includes prototypes	works at all
V8	yes	yes	yes
JavaScriptCore	n/a	n/a	no — always threw
ChakraCore	no	no	yes
QuickJS	yes	no	yes
JSI	n/a	n/a	no — TODO stub

• JavaScriptCore called Object.getOwnPropertyNames with argc 0, so the object argument was never used and the call always threw TypeError: undefined is not an object.
• ChakraCore used JsGetOwnPropertyNames, which is own-only and also reports non-enumerable properties.
• QuickJS used JS_GetOwnPropertyNames with JS_GPN_ENUM_ONLY — enumerable-only, but still own-only.
• JSI (found while fixing the above) had Object::GetPropertyNames as an unimplemented stub that threw std::runtime_error{"TODO"}.

Approach

None of JSC, ChakraCore or QuickJS exposes a native equivalent of V8's kIncludePrototypes | ONLY_ENUMERABLE | SKIP_SYMBOLS key collection, so rather than write three subtly different prototype walks, this adds one shared implementation (Source/js_native_api_shared.{h,cc}) written purely against the public napi_* surface, and has all three delegate to it.

Per prototype level it takes:

• Object.keys — the properties for...in reports at that level, already in specification order;
• Object.getOwnPropertyNames — the shadowing set. A non-enumerable own property is not reported itself, but it does hide a same-named enumerable property further up the chain.

JSObjectCopyPropertyNames was considered for JavaScriptCore since it walks the chain natively, but it drops that shadowing rule, so the shared walk is used there too and all backends stay consistent. The input is coerced with napi_coerce_to_object, matching V8's CHECK_TO_OBJECT.

JSI keeps its own path: jsi::Object::getPropertyNames already has exactly the specified semantics, so Object::GetPropertyNames just forwards to it.

Two further bugs this uncovered

1. napi_get_prototype on JavaScriptCore (fixed here — the shared walk depends on it). It ran the result of JSObjectGetPrototype through JSValueToObject. At the top of a prototype chain that value is null, so the conversion threw TypeError: null is not an object instead of reporting the end of the chain, making the chain impossible to walk. It now returns the raw prototype value, as V8 does. It had no other callers in this repository — Blob.cpp deliberately uses Object.getPrototypeOf instead.

2. Hermes' for...in does not implement the shadowing rule (not fixed — engine bug). CI showed napi_get_property_names returning the correct ['own', 'middle'] while Hermes' own for...in returned ['own', 'middle', 'deep'], i.e. it reports an inherited property that a non-enumerable own property is supposed to hide. The test therefore probes for that behaviour at runtime rather than naming engines, and only uses for...in as an oracle where it holds; the explicit expected-value assertion still runs everywhere. Happy to file this separately if useful.

Testing

Nine script tests, exercised through a new napiGetPropertyNames global that calls Napi::Object::GetPropertyNames:

• own enumerable string keys
• enumerable properties inherited from the prototype chain
• non-enumerable own properties excluded
• symbol keys excluded
• a shadowed inherited property reported only once
• an inherited property shadowed by a non-enumerable own property omitted
• class methods excluded (they are non-enumerable)
• array indices as strings, length omitted
• equality with for...in over a multi-level prototype chain

Verified locally on ChakraCore, QuickJS and JSI — 225 passing, 0 failing on each. JavaScriptCore, V8 and Hermes are covered by CI; all 25 legs are green.

As a negative control, restoring the old ChakraCore implementation makes five of the nine fail, confirming the tests are load-bearing rather than vacuous:

napi_get_property_names (#216)
  ✅ returns own enumerable string keys
  1) includes enumerable properties inherited from the prototype chain
  2) excludes non-enumerable own properties
  ✅ excludes symbol keys
  ✅ reports a shadowed inherited property only once
  3) omits an inherited property shadowed by a non-enumerable own property
  ✅ excludes class methods, which are non-enumerable
  4) reports array indices as strings and omits the non-enumerable length
  5) matches for...in over a multi-level prototype chain

Note on scope

napi_get_all_property_names is still only implemented by the V8 backend; the others would fail to link if a consumer called it. That is a separate gap and is left alone here.

[Copilot]

Pull request overview

This PR aligns napi_get_property_names behavior across the JavaScriptCore, ChakraCore, and QuickJS backends to match the Node-API/V8 semantics: enumerable, string-keyed property names including the prototype chain (i.e., matching for...in, including the “non-enumerable own property shadows inherited enumerable” rule).

Changes:

• Added an engine-agnostic implementation (napi_shared::GetEnumerablePropertyNames) that walks the prototype chain using Object.keys plus Object.getOwnPropertyNames to enforce for...in-equivalent semantics.
• Updated JavaScriptCore, ChakraCore, and QuickJS napi_get_property_names implementations to delegate to the shared helper (and added missing CHECK_ARG(env, object) on JSC/Chakra).
• Added script-level regression tests and a test harness global (napiGetPropertyNames) to validate behavior against for...in.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
Tests/UnitTests/Shared/Shared.cpp	Exposes napiGetPropertyNames to script tests via the C++ harness.
Tests/UnitTests/Scripts/tests.ts	Adds regression tests for napi_get_property_names semantics (prototype chain, enumerability, symbols, shadowing, arrays).
Core/Node-API/Source/js_native_api_shared.h	Declares shared helper for napi_get_property_names semantics.
Core/Node-API/Source/js_native_api_shared.cc	Implements the shared prototype-chain walk using only public napi_* APIs.
Core/Node-API/Source/js_native_api_quickjs.cc	Replaces own-only QuickJS implementation with shared helper.
Core/Node-API/Source/js_native_api_javascriptcore.cc	Fixes JSC implementation (previously throwing) by delegating to shared helper and validating object.
Core/Node-API/Source/js_native_api_chakra.cc	Replaces own-only / non-enumerable-including Chakra implementation with shared helper and validates object.
Core/Node-API/CMakeLists.txt	Adds shared helper sources to non-V8 backend builds.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bkaradzic-microsoft]

Fair point — that was untested, and you're right that the harness structurally could not test it: Napi::Object::GetPropertyNames can only be called on an already-constructed Napi::Object, so the coercion path was unreachable from C++.

Fixed in c3def88 by exposing the C entry point directly as napiGetPropertyNamesRaw and passing the raw value through, plus six tests.

One caveat on your suggestion to just call the C API in the shared harness: Core/Node-API-JSI implements the Napi:: C++ surface directly on top of JSI and has no js_native_api_*.cc at all, so there is no C API to call there. The new global is therefore compiled out under the existing JSRUNTIMEHOST_NAPI_ENGINE_JSI define and the coercion tests skip themselves on that backend (225 passing / 6 pending) while running everywhere else.

Writing the tests also turned up a real divergence for null and undefined, which have no object wrapper: V8 reports napi_object_expected, QuickJS's napi_coerce_to_object uses Object(value) and cheerfully returns an empty object, and JavaScriptCore's throws. The shared implementation now checks napi_typeof explicitly so all three match V8.

Local results: ChakraCore 231 passing / 0 failing, QuickJS 231 / 0, JSI 225 / 0 + 6 pending. Negative control: with the null/undefined check removed, QuickJS fails exactly the two tests that cover it.

[bkaradzic-microsoft]

Fair point — that was untested, and you're right that the harness structurally could not test it: Napi::Object::GetPropertyNames can only be called on an already-constructed Napi::Object, so the coercion path was unreachable from C++.

Fixed in c3def88 by exposing the C entry point directly as napiGetPropertyNamesRaw and passing the raw value through, plus six tests.

One caveat on your suggestion to just call the C API in the shared harness: Core/Node-API-JSI implements the Napi:: C++ surface directly on top of JSI and has no js_native_api_*.cc at all, so there is no C API to call there. The new global is therefore compiled out under the existing JSRUNTIMEHOST_NAPI_ENGINE_JSI define and the coercion tests skip themselves on that backend (225 passing / 6 pending) while running everywhere else.

Writing the tests also turned up a real divergence for null and undefined, which have no object wrapper: V8 reports napi_object_expected, QuickJS's napi_coerce_to_object uses Object(value) and cheerfully returns an empty object, and JavaScriptCore's throws. The shared implementation now checks napi_typeof explicitly so all three match V8.

Local results: ChakraCore 231 passing / 0 failing, QuickJS 231 / 0, JSI 225 / 0 + 6 pending. Negative control: with the null/undefined check removed, QuickJS fails exactly the two tests that cover it.