fix(runtime): class-prototype own-keys enumeration — accessors listed, symbols real, spec order (36 diverging fixture lines → 0) - #9315
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThis change updates class lowering, code generation, and runtime reflection. Class methods and accessors retain source order across registries. Computed symbols use symbol keys. Own-key and descriptor checks now include accessors, symbols, dynamic properties, and ordered prototype members. ChangesClass prototype own-key ordering
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to The PR corrects class reflection keys, descriptors, ownership checks, and ordering. It is mergeable with owner awareness that an exceptional registration failure could preserve the member while losing its source-order metadata, causing incorrect reflection ordering. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a clear summary, detailed changes, related issue reference, test results, before-and-after output, residual limitations, and validation context. It does not reproduce every template heading or checklist item, but the required information is mostly present. Full details: Linked Issues checkExplanation The implementation satisfies the primary requirements in Full details: Out of Scope Changes checkExplanation The changes remain related to class-prototype own-key behavior. Ordering tables, deletion invalidation, symbol handling, stable-hash updates, FFI arity changes, regression tests, and changelog documentation support the stated objectives and do not introduce unrelated code. Full details: Docstring CoverageExplanation Docstring coverage is 42.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 25 files. (1 skipped: 1 unsupported.)
✨ 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 |
* chore(gc): classify the two order side-tables in the root-holder inventory CLASS_SYMBOL_MEMBER_ORDERS is keyed by SymbolHeader::id (stable, not an address) with a u32 order value; CLASS_DYNAMIC_PROP_ORDER holds owned Rust strings. Neither stores a JSValue, so neither is a GC root. * refactor(hir): split class member-registration helpers out of class_decl.rs class_decl.rs was at 1989 lines; #9315's additions take it over the 2000-line gate. The non-computed member registration and Symbol.iterator wrapper helpers move to a sibling module unchanged. * fix(runtime): drop unnecessary parens in the numeric-range header read `-D warnings` in CI treats unused_parens as an error. * chore: changelog fragment for the train16 follow-up --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
… an instance A class carrying a generic computed member lost `this.prototype` (and every other static-`this` read, and static-`this` writes) inside all of its static methods, while the same property read through the class's own binding answered correctly — one function, two answers. `class_has_computed_runtime_members` says the class's INSTANCES have keys the packed shape does not describe, so an instance access must go by name. The by-name helper strips the receiver NaN-box to a raw `ObjectHeader*`. But `receiver_class_name` answers with the owning class for `Expr::This` in a static body just as in an instance body (both read `class_stack`), and a static body's `this` is the class CONSTRUCTOR — an INT32 class ref. Masking it handed the runtime the bare class id as a pointer, below the handle band, so every such read answered `undefined`; `this.name` survived only because `js_object_get_field_by_name_f64` already reads a small-integer receiver back as a class id for that one key. `FnCtx::in_static_member` records what the receiver-class answer cannot, and the computed-member read/store routes consult it before treating a proven class name as a claim about the receiver's layout. Static bodies fall through to the general dispatch tower, which classifies the receiver tag and has a class-ref arm — the same lowering the identical static method already got when its class carried no computed member. This is what took `cc --help` down (PerryTS#9341): PerryTS#9315 routed well-known-symbol computed members onto the generic path, and axios's `AxiosHeaders` pairs `[Symbol.iterator]()` with `static accessor(){ let z = this.prototype; … }`. Refs PerryTS#9369, PerryTS#9341. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp
1 participant
Fixes #9226. Own-keys enumeration on class prototypes was wrong three independent ways: accessors missing from
getOwnPropertyNames, a symbol key leaking into the string list as the literal"@@iterator", andgetOwnPropertySymbolsreturning nothing — whilehasOwnPropertyandgetOwnPropertyDescriptordisagreed with all of it.Every number was produced twice — once on
6642990812, then re-run from scratch after rebasing past #9291 (which landed a Bloom filter over exactly these registries). Both rounds agree exactly.Three causes, all in the enumeration/lowering, none in the data
vtable.methods."@@iterator"string was a lowering artifact:generic_computed_member_keydiverted every well-known-symbol class member off the computed-key path, so the member registered under a synthetic string name and no real Symbol key was ever installed wheregetOwnPropertySymbolscould see it. The exclusion is narrowed to the three forms that genuinely need special lowering; the synthetic alias stays in the vtable for fast dispatch but is filtered from enumeration. This fixes the enumeration, not the spelling — checked explicitly: a source method literally named"@@iterator"still lists (it carries a definition-order record; an alias does not).ClassBody, so it orders across the separate method/getter/setter/Symbol registries — andsort_property_names_ecmapartitions rather than sorts, preserving it. Verified byte-identical to node on the discriminating case: integer-like ascending, then strings interleaving methods with accessors in source order, then symbols in insertion order; statics listlength|name|prototypefirst, as node does.A fourth defect the issue didn't name, also fixed:
hasOwnProperty(C.prototype, someSymbol)returned false whilegetOwnPropertyDescriptorreturned a descriptor for the same key.The consistency triple now holds everywhere probed: every reported key answers
trueto bothhasOwnPropertyandgetOwnPropertyDescriptor; inherited members are correctly not own; delete-then-recreate moves a key to the end; expandos append. Stable-hash and FFI arity updates are included so the module cache cannot serve stale objects.Tests
cargo test -p perry-runtime --lib -- --test-threads=1: 2,873 passed, 0 failed, 4 ignored.cargo fmtclean on all 27 touched files.Residual, pre-existing, byte-identical on base and this branch
static [Symbol.hasInstance]andget [Symbol.toStringTag]are lifted to top-level functions during lowering and never become class members at all, so they are absent from own keys (node lists them ingetOwnPropertySymbols). A different and milder defect than the"@@"leak; not regressed here, and untracked as an issue.A finding for #9273 discovered during verification
Five of the gap suite's "false regressions" —
backoff_options,cron_cronjob,dayjs_factory_arg,moment_methods,ratelimiter_memory— have a concrete cause: missing npm dependencies, not a stale snapshot. They compare perry's native package emulation against the real package under node; the packages are inpackage.jsonbutrun_gap_tests.shnever provisionsnode_modules, so the node oracle emittedERR_MODULE_NOT_FOUNDand the mismatch was scored against the branch. Afternpm ci, all five pass inside the gate with the same binaries. The gate should fail loudly on a missing oracle dependency rather than scoring it.Base
Deliberately left on
d20fb4fd25— the base actually verified — rather than speculatively rebased onto a base that wasn't. Arange-diffprobe shows the rebase onto current main is clean, all three patches carrying identically; the single overlapping file is a declaration-arity table with no interaction with the enumeration logic.Summary by CodeRabbit
Object.getOwnPropertyNames,Object.getOwnPropertySymbols, andReflect.ownKeysreturn consistent, spec-compliant results.Symbol.iterator.