Skip to content

fix(runtime): honor defineProperty prototype index setters - #9339

Closed
proggeramlug wants to merge 5 commits into
PerryTS:mainfrom
proggeramlug:fix/9249-array-prototype-define-property
Closed

fix(runtime): honor defineProperty prototype index setters#9339
proggeramlug wants to merge 5 commits into
PerryTS:mainfrom
proggeramlug:fix/9249-array-prototype-define-property

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Indexed array assignments now honor inherited accessors and non-writable data
properties installed on Array.prototype or Object.prototype through the
descriptor APIs.

Changes

  • raise the array-index invalidation latch when Object.defineProperty,
    Object.defineProperties, or Reflect.defineProperty installs a canonical
    index on Array.prototype
  • preserve strict source-assignment semantics through the typed-feedback cold
    fallback and walk only the default Array.prototype / Object.prototype
    chain
  • decline the dense numeric store lane for an in-bounds hole after prototype
    index invalidation, addressing the review finding without restoring the
    reverted custom-prototype path
  • add end-to-end coverage for out-of-bounds stores, in-bounds holes,
    Object.prototype, boolean arrays, setters, and non-writable descriptors

No version files changed.

Scope note

The typed-feedback continuation retains its pre-existing non-strict behavior
for unrelated guard failures, including existing own elements on frozen arrays
and missing elements on non-extensible arrays. Broadening every fallback to
strict assignment is outside #9249 and failed the affected runtime suite; this
change is deliberately limited to a missing canonical index under the default
prototype-index latches.

Related issue

Fixes #9249

Test plan

  • cargo test -p perry --test issue_9249_array_prototype_define_property -- --nocapture --test-threads=1 (5 passed)
  • cargo test -p perry-runtime strict_dense -- --nocapture --test-threads=1 (3 passed)
  • cargo test -p perry --test issue_5898_array_pop_prototype -- --test-threads=1
  • cargo test -p perry --test issue_5898_array_reverse_exotic -- --test-threads=1
  • RUSTFLAGS="-D warnings" cargo check -p perry-runtime -p perry --all-targets
  • SKIP_COMPILE_GATES=1 scripts/run_lint_gates.sh (all 58 runnable gates passed; 2 CI-only gates skipped)
  • cargo build --release -p perry
  • cc --help A/B on perrymaster (5/5 deterministic branch runs matched current main's known-bad cc --help parity gate down on main: defineProperty-on-non-object at startup — window pins #9326 (indexed writes with a recorded custom array prototype) #9341 baseline byte-for-byte: rc=1, 95 bytes; Node and the preserved known-good Perry build both returned rc=0 with identical 9,175-byte output)
  • ./scripts/test_affected_crates.sh --base origin/main (runtime 2,888 passed / 4 ignored; Perry 1,062 passed; FFI 30 passed; UI target clean)

Checklist

  • No workspace version, CLAUDE.md, or CHANGELOG.md changes
  • Commit messages use the repository's conventional prefixes
  • Read CONTRIBUTING.md and the Code of Conduct

@coderabbitai

coderabbitai Bot commented Aug 31, 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: 73816fc0-95c7-49c1-88c0-d0b2f82d8dc3

📥 Commits

Reviewing files that changed from the base of the PR and between 4d0f527 and 8dc81ba.

📒 Files selected for processing (6)
  • crates/perry-runtime/src/array/indexing.rs
  • crates/perry-runtime/src/array/indexing_keyed.rs
  • crates/perry-runtime/src/array/mod.rs
  • crates/perry-runtime/src/array/strict_dense_test_helpers.rs
  • crates/perry-runtime/src/typed_feedback.rs
  • crates/perry/tests/issue_9249_array_prototype_define_property.rs

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


📝 Walkthrough

Walkthrough

Indexed descriptors defined on Array.prototype or Object.prototype now invalidate array store fast paths. Strict indexed stores consult the default prototype chain. Regression tests cover descriptor APIs, accessors, and non-writable properties.

Changes

Array prototype index descriptors

Layer / File(s) Summary
Descriptor invalidation wiring
crates/perry-runtime/src/array/mod.rs, crates/perry-runtime/src/object/array_object_ops.rs, crates/perry-runtime/src/array/strict_dense_test_helpers.rs, crates/perry-runtime/src/array/indexing.rs
Indexed property definitions call note_array_index_write. Shared test helpers provide counters used by array indexing tests.
Inherited store fallback
crates/perry-runtime/src/array/indexing_keyed.rs, crates/perry-runtime/src/array/indexing.rs, crates/perry-runtime/src/typed_feedback.rs
Strict indexed stores route missing canonical indices through inherited [[Set]] handling when default prototypes carry indexed properties. Dense stores fall back when invalidation exposes a hole.
Descriptor API regression coverage
crates/perry/tests/issue_9249_array_prototype_define_property.rs, changelog.d/9339-array-prototype-define-property-index.md
Tests cover Object.defineProperty, Object.defineProperties, Reflect.defineProperty, setters, getters, prototype interception, holes, and non-writable descriptors.

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

Merge Risk: 🟡 Moderate · up to 8dc81

Array assignment handling may fail to throw required TypeErrors for some strict-mode writes to frozen or non-extensible arrays, allowing execution to continue when it should stop. The PR should resolve or explicitly accept this bounded correctness risk before merging.

Sequence Diagram(s)

sequenceDiagram
  participant TypeScriptTest
  participant define_array_property
  participant js_typed_feedback_array_index_set_fallback_boxed
  participant js_array_set_index_or_string_inherited_strict
  participant array_spec_set
  participant PrototypeDescriptor
  TypeScriptTest->>define_array_property: define indexed prototype descriptor
  define_array_property->>js_typed_feedback_array_index_set_fallback_boxed: raise index invalidation state
  TypeScriptTest->>js_typed_feedback_array_index_set_fallback_boxed: assign array index
  js_typed_feedback_array_index_set_fallback_boxed->>js_array_set_index_or_string_inherited_strict: route fallback store
  js_array_set_index_or_string_inherited_strict->>array_spec_set: walk inherited [[Set]]
  array_spec_set->>PrototypeDescriptor: inspect descriptor
  PrototypeDescriptor-->>TypeScriptTest: invoke setter or throw TypeError
Loading

Suggested reviewers: thehypnoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #9249 by tracking descriptor-defined prototype indices, invalidating array-store fast paths, and applying inherited strict-set behavior for Array.prototype and Object.prototy…
Out of Scope Changes check ✅ Passed The code, changelog, regression tests, and test helpers directly support the prototype-index descriptor fix and its validation. No unrelated changes are identified.
Title check ✅ Passed The title clearly and concisely describes the main runtime fix: honoring prototype index setters installed with defineProperty.
Description check ✅ Passed The description includes all required sections, explains the implementation and scope, references issue #9249, and provides detailed test results and checklist completion.
Full details: Linked Issues check

Explanation

The changes satisfy issue #9249 by tracking descriptor-defined prototype indices, invalidating array-store fast paths, and applying inherited strict-set behavior for Array.prototype and Object.prototype descriptors. Regression tests cover setters, non-writable descriptors, holes, out-of-bounds stores, boolean arrays, and related descriptor APIs.

  • Fix all pre-merge checks with AI
✨ 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

Held back from this batch: with #9326 reverted (#9345, restoring the cc --help parity gate — see #9341), this branch no longer applies to main. It conflicts on cherry-pick, and it touches the same defineProperty/prototype-index area as the reverted change and as the TypeError that motivated the revert.

Worth rebasing onto current main and re-checking against cc --help specifically before it lands — a green gap fixture and a green runtime suite did not catch the last regression in this area, which is the whole lesson of #9341.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

I tried to rebase and land this myself and could not clear it. The blocker is not a conflict — it is that the evidence needed to merge it safely does not exist locally, and I would rather say that than merge on hope.

Why it does not rebase mechanically. The new hunk is guarded by !prototype_already_checked, and that binding does not exist on main — it came in with #9326 and left with the revert (#9345). More to the point, array_spec_set currently has no callers on main: this PR would reintroduce the sole call site of exactly the path that was reverted for taking down the cc --help parity gate (#9341).

Why I could not validate it. I built the pre-revert commit (6bc775d9b9, containing #9326) specifically to get a known-bad binary, then tried to catch the regression with everything available to me:

check known-bad build
perry-runtime --lib green (2904, per #9341)
targeted defineProperty probes I wrote byte-identical to node
full 609-test gap suite 604 pass / 5 fail — identical to the known-good run, "Gap snapshot OK", exit 0

That last row is the finding: the gap suite does not detect this class at all. Its result on the build that breaks cc --help is indistinguishable from its result on main. So a green gap run on this PR would be worth nothing as evidence, and the cc --help bundle is not something I can run here — the local claude is a native binary, not the cli_2.1.112.js the gate uses.

I have merged everything else in the queue, including fixing two ceiling violations myself along the way. This is the one I am handing back, and only because merging it would mean re-landing a release-gate regression with no way to see it.

What would unblock it, in rough order of value:

  1. Run this branch against cc --help on perrymaster, where the known-good/known-bad binaries at /root/claude-shapedesc/ already are. That is the only gate that has ever caught this.
  2. Reduce cc --help parity gate down on main: defineProperty-on-non-object at startup — window pins #9326 (indexed writes with a recorded custom array prototype) #9341 to a fixture. The gap suite's blind spot here is worth closing on its own merits — right now a whole class of prototype/[[Set]] regressions can land green, which is how fix(runtime): indexed writes honour a custom array prototype (from #9297) #9326 got in.
  3. Rebase onto the post-revert shape, since prototype_already_checked and the call-site wiring need rebuilding regardless.

I have posted the gap-suite blind-spot result on #9341 too, since it matters beyond this PR.

@proggeramlug
proggeramlug force-pushed the fix/9249-array-prototype-define-property branch from 4d0f527 to 8dc81ba Compare September 1, 2026 07:54
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Rebased and reworked this on current main (0f95fbc9f). The branch is now conflict-free locally and deliberately does not restore the custom-prototype routing reverted in #9345.

The CodeRabbit merge-risk finding is addressed: once a prototype-index condition invalidates fast stores, the numeric dense lane now declines an in-bounds TAG_HOLE, so the inherited setter/non-writable descriptor walk runs. I added an end-to-end new Array(3) hole case that verifies the setter fires and no own property is created.

I also narrowed the typed-feedback continuation after the full affected runtime suite caught an over-broad strict fallback. Only a missing canonical index under the default prototype latches now enters strict inherited [[Set]]; existing own elements and unrelated guard failures retain their prior behavior. The helper uses the audited RuntimeHandle::across_mut reload around potentially allocating key classification.

Validation on perrymaster at 8dc81ba57:

No version files changed.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

CodeRabbit's refreshed review generated no actionable comments and now passes.
For clarity, its remaining moderate-risk summary refers to pre-existing strict-write behavior outside #9249: unrelated typed-feedback guard failures retain the historical non-strict continuation. An earlier broad strict fallback made typed_feedback_array_set_guards_reject_frozen_arrays fail, so this PR deliberately limits strict inherited [[Set]] to a missing canonical index under the default prototype latches. The 63.64% docstring item is a non-blocking warning over touched private/test helpers; all repository lint gates pass.

proggeramlug added a commit that referenced this pull request Sep 1, 2026
… (#9392)

* fix(runtime): honor defineProperty prototype index setters

* chore: key changelog fragment to PR 9339

---------

Co-authored-by: Ralph Küpper <ralph@skelpo.com>
@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via #9392 with your commits preserved — your branch conflicts against current main on GitHub's three-way merge, though it cherry-picks cleanly.

Worth noting why this was stuck: it depended on the array_spec_set call-site wiring and prototype_already_checked that the #9326 revert removed. #9370 reapplied that, so this became applicable again — the block was never about this change's own merits.

Verified before merge: both relevant gap fixtures byte-identical to the pinned oracle, 8 runtime suites green, and an independent probe confirming the prototype index setter runs with no own property created. Details on #9392, including a module-goal observation I hit while probing that is independent of your change.

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.

Array.prototype index setters installed with defineProperty are bypassed by array element stores

1 participant