fix(runtime): Array-subclass fill honours start and end - #8972
Conversation
`js_array_subclass_init` installs `fill` on the instance, and that stub had arity 1: every `start` / `end` argument was dropped, so `sub.fill(8, 1)` overwrote the whole array instead of the tail from index 1 (node prints `7|8|8`, perry printed `8|8|8`). The stub now takes both, treating an omitted (`undefined`) argument as the spec default. e2e test covers value-only, a start, a start and end, and a negative start against node's output. Claude-Session: https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughArray subclass initialization now installs a three-argument ChangesArray subclass fill behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to This change makes Array-subclass fill honor start and end, but proxy receivers can run user code while reading length before those range objects are coerced. Because the current path does not preserve start and end across that moving-memory boundary, it carries a bounded runtime memory-safety risk; merge should wait for the rooting fix or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 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 |
Fragment filenames are PR-keyed so in-flight PRs cannot collide; 0000 collides with every other placeholder.
|
Merged. A clean bug and a clean fix — arity 1 meant The part worth checking was the omitted-argument path, since an absent arg arrives as NaN-boxed The new test also covers the shapes that matter rather than just the reported one: One fix pushed: the fragment was Validation — runtime 2779/0 ( |
js_array_subclass_initinstallsfillas an own method on the instance (node inherits it fromArray.prototype; perry has no such prototype object for these classes), and the stub was registered with arity 1 — sostartandendnever reachedjs_array_fill_generic:The stub now takes
(value, start, end); an omitted argument arrives asundefinedand selects the spec default (0/length).Verified against node on all four forms (value only, start, start+end, negative start) —
array_subclass_fill_args.rs, and the compiled output matches node exactly.Found while probing Array-subclass semantics for the elements-store work (#8966).
https://claude.ai/code/session_019WVcWKmYsUBnnFB7nBgbBJ
Summary by CodeRabbit
Bug Fixes
Array.prototype.fillon classes extendingArrayto respect optionalstartandendarguments.fill(value, start), bounded ranges, and negative indices now produce the expected results while preserving default behavior.Tests
fillargument combinations on array subclasses.Documentation