Skip to content

fix(runtime): honor inherited descriptors when filling array holes - #9797

Closed
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9787-array-hole-inherited-setters
Closed

fix(runtime): honor inherited descriptors when filling array holes#9797
proggeramlug wants to merge 2 commits into
PerryTS:mainfrom
proggeramlug:fix/9787-array-hole-inherited-setters

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closes #9787.

A numeric assignment into an in-bounds array hole could create an own element instead of invoking an inherited setter. The issue's fixture installs one accessor: its out-of-bounds write works, while the subsequent new Array(5)[3] = 37 bypasses it.

Use the existing array-index invalidation byte in the runtime's numeric store fast path. It covers indexed properties on Array.prototype and Object.prototype, as well as custom array prototypes. Unmodified prototype chains still take the one-load fast path, and existing own elements continue to bypass inherited descriptors.

Regression coverage includes new and deleted holes, an own undefined value, removal of the inherited accessor, and Object.prototype setters/getter-only/read-only properties. Both new fixtures fail on the original runtime and their expected output was verified with Node.

Validation:

  • Focused runtime fast-path test passed; full runtime suite: 3,137 passed, 4 ignored.
  • scripts/pre-tag-check.sh --quick passed.
  • All six prototype-descriptor compiler integration tests passed.
  • Both new fixtures passed the repository Node parity runner; the original issue fixture also matches Node byte for byte.
  • scripts/test_affected_crates.sh --base origin/main: runtime suite passed; CLI had 1,083 passes and the existing PERRY_CONCAT_SITE_CACHE cache-audit failure addressed separately by fix(cache): register concat switch and explain codegen inputs #9748.

No version bump.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed numeric assignments to sparse array holes so inherited setters and read-only properties are respected.
    • Corrected behavior for properties defined on Array.prototype and Object.prototype, including appropriate errors for getter-only and non-writable properties.
    • Preserved existing behavior for arrays with own elements and unchanged prototype chains.
  • Tests

    • Added regression coverage for inherited setters, deleted elements, newly created holes, and prototype descriptors.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: c6817930-71da-4572-9885-15e783050498

📥 Commits

Reviewing files that changed from the base of the PR and between 12efed1 and 5c092e3.

📒 Files selected for processing (6)
  • changelog.d/9797-array-hole-inherited-setters.md
  • crates/perry-runtime/src/array/indexing.rs
  • crates/perry-runtime/src/array/strict_store_tests.rs
  • crates/perry/tests/issue_9249_array_prototype_define_property.rs
  • test-files/test_gap_9787_array_hole_inherited_setter.ts
  • test-files/test_gap_9787_object_prototype_hole_setter.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The array hole numeric-store fast path now detects indexed descriptors on default prototypes and falls back to the general [[Set]] behavior. Runtime tests and regression tests cover inherited setters, getter-only accessors, non-writable properties, existing elements, and prototype cleanup.

Changes

Array hole write semantics

Layer / File(s) Summary
Fast-path invalidation and runtime coverage
crates/perry-runtime/src/array/indexing.rs, crates/perry-runtime/src/array/strict_store_tests.rs, changelog.d/9797-array-hole-inherited-setters.md
The hole-fill guard now checks PERRY_ARRAY_INDEX_FAST_PATH_INVALIDATED. Runtime tests cover both invalidated and unchanged prototype summaries, while existing own elements retain the fast path.
Prototype descriptor regression coverage
crates/perry/tests/issue_9249_array_prototype_define_property.rs, test-files/test_gap_9787_array_hole_inherited_setter.ts, test-files/test_gap_9787_object_prototype_hole_setter.ts
Regression tests cover setters on Array.prototype and Object.prototype, getter-only accessors, non-writable properties, deleted holes, and post-cleanup writes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 5c092

Array writes into holes now honor inherited indexed setters and read-only descriptors rather than creating incorrect own elements. The intended behavior and relevant descriptor cases are covered, with no concrete current-head merge risk identified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary runtime fix: honoring inherited descriptors when filling array holes.
Description check ✅ Passed The description explains the bug, implementation, related issue, regression coverage, validation results, and no version bump. It provides the required information even without reproducing every templ…
Linked Issues check ✅ Passed The runtime change addresses issue #9787 by preventing the fast path from creating an own element over an inherited Array.prototype accessor. The description and regression tests state that the affect…
Out of Scope Changes check ✅ Passed The changes are limited to the array numeric-store fast path, related runtime tests, compiler integration tests, fixtures, and the changelog entry. These changes support the linked issue and stated ob…
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files. (1 skipped: 1 u…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed on main via merge train #9804 (rebase-merged, so your commits keep their authorship). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A second accessor installed on Array.prototype is never invoked

1 participant