fix(runtime): unify [[OwnPropertyKeys]] enumerators into one canonical op (#336)#442
Conversation
…l op (#336) Collapse the three divergent runtime own-key enumerators (object_own_property_keys_snapshot, own_string_property_names/ own_symbol_properties, and collect_target_own_keys) into a single canonical Interpreter::own_property_keys implementing ES 10.1.11 OrdinaryOwnPropertyKeys plus the Array/String/Module/TypedArray/Proxy exotic variants. Every consumer (getOwnPropertyNames/Symbols/ Descriptors, defineProperties, Reflect.ownKeys, Proxy ownKeys target enumeration) now routes through it. Behavior changes (all spec-correctness fixes, ES 10.4.2.1 / 10.1.11): - Array holes are omitted everywhere (getOwnPropertyNames and Reflect.ownKeys previously emitted hole indices). - Reflect.ownKeys / Proxy ownKeys on arrays now include named string keys and symbol keys (previously only 0..length + "length"). - TypedArray integer indices now surface for getOwnPropertyDescriptors / defineProperties (previously enumerated as an ordinary bag, missing the buffer-backed indices). The stale hidden-internal-slot divergence noted in #336 no longer applies: engine [[...]] slots moved to PropertyBag.internal_slots (#337), so they are not in bag.properties. Known pre-existing limitations left out of scope (documented in the op): array index keys beyond Int range emit after "length"; proxy ownKeys invariant validation treats all symbols as configurable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 45 minutes and 17 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e82f2380
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @runtime.proxy_own_property_keys(interp, proxy_data) | ||
| Object(_) | Value::Array(_) => | ||
| @runtime.make_array(@runtime.collect_target_own_keys(target, interp)) | ||
| @runtime.make_array(interp.own_property_keys(target)) |
There was a problem hiding this comment.
On ordinary targets this now routes through Interpreter::own_property_keys, but that helper's ordinary-object path only walks bag.properties. The removed collect_target_own_keys also appended bag.descriptors entries that have no value entry, and ordinary_get_own_property still reports those descriptor-only entries as own properties. When runtime/native code constructs such a target, for example via make_object with a descriptor map, Reflect.ownKeys drops the key and proxy ownKeys invariant validation can miss required non-configurable keys. Please merge descriptor-only string keys into the canonical list before using it here.
Useful? React with 👍 / 👎.
Benchmark ResultsRun: https://github.com/dowdiness/js_engine/actions/runs/28098923401
Stage summary
Focused bytecode base-vs-head comparison
Base-vs-head comparison
Mean-time chart (log scale)
Closure-conversion comparison
|
…y symbol keys in proxy [[OwnPropertyKeys]] (#445) Two #336 follow-ups left out of scope by the canonical-enumerator unification (PR #442): - #444: Reflect.ownKeys (§28.1.11) threw TypeError on Map/Set/Promise. The canonical Interpreter::own_property_keys already handles those variants; widen the dispatch arm to match instead of falling through to the non-object TypeError. - #443: Proxy [[OwnPropertyKeys]] (§10.5.11) step 16 must classify every non-configurable own key of the target so the trap cannot hide one. The keys-split loop assumed all symbol keys were configurable; classify them via bag.symbol_descriptors, mirroring target_has_non_configurable_key for strings. Fixes the invariant check for symbol keys (built-ins/Object/getOwnPropertyNames/proxy-invariant-absent-not-configurable-symbol-key). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Closes #336.
What
Collapses the three divergent runtime own-key enumerators into a single canonical
Interpreter::own_property_keysimplementing ES §10.1.11OrdinaryOwnPropertyKeysplus the Array §10.4.2.1 / String / Module / TypedArray §10.4.5 / Proxy §10.5.11 exotic variants. Every consumer now routes through it:Object.getOwnPropertyNames/getOwnPropertySymbols(filter the canonical list by key type)Object.getOwnPropertyDescriptors/Object.defineProperties(full canonical list)Reflect.ownKeys(Object/Array arm)ownKeystarget-key enumeration (no-trap path + invariant validation)Deleted
object_own_property_keys_snapshotandcollect_target_own_keys(now identical to the canonical op); net −1 public symbol.Behavior changes (all spec-correctness fixes)
getOwnPropertyNamesandReflect.ownKeyspreviously emitted hole indices.Reflect.ownKeys/ ProxyownKeyson arrays now include named string keys and symbol keys — previously only0..length+"length".getOwnPropertyDescriptors/defineProperties— previously enumerated as an ordinary bag, missing the buffer-backed indices.The stale hidden-internal-slot divergence noted in #336 no longer applies: engine
[[…]]slots moved toPropertyBag.internal_slots(#337), so they are not inbag.properties;is_hidden_internal_property_keyis gone.Verification
moon checkclean;moon info/moon fmtapplied..mbtidiff = exactly the symbol changes above.stage8 ownkeys … hole handlingtest updated to the spec-correct expectation; 2 new tests added).main(per-mode, both strict & non-strict) acrossbuilt-ins/{Reflect/ownKeys, Proxy/ownKeys, Object/getOwnProperty{Names,Symbols,Descriptors}, Object/defineProperties, TypedArrayConstructors/internals/OwnPropertyKeys, Object/assign, keys, seal, freeze}:Reflect/ownKeys/return-non-enumerable-keys, 2×Object/getOwnPropertyNames/order-after-define-property— both modes each)Out of scope (pre-existing, documented in the op + tracked as follow-ups)
"length"rather than merged into the ascending integer run (Reflect/ownKeys/return-on-corresponding-order-large-indexstill fails — same onmain).ownKeysinvariant validation treats all Symbol keys as configurable.Reflect.ownKeysstill throws onMap/Set/Promise(its type guard, unchanged here); now trivially widenable since the canonical op handles them.🤖 Generated with Claude Code