fix(runtime): honor defineProperty prototype index setters - #9339
fix(runtime): honor defineProperty prototype index setters#9339proggeramlug wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughIndexed descriptors defined on ChangesArray prototype index descriptors
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to 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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue
✨ 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 |
|
Held back from this batch: with #9326 reverted (#9345, restoring the Worth rebasing onto current |
|
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 Why I could not validate it. I built the pre-revert commit (
That last row is the finding: the gap suite does not detect this class at all. Its result on the build that breaks 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:
I have posted the gap-suite blind-spot result on #9341 too, since it matters beyond this PR. |
4d0f527 to
8dc81ba
Compare
|
Rebased and reworked this on current The CodeRabbit merge-risk finding is addressed: once a prototype-index condition invalidates fast stores, the numeric dense lane now declines an in-bounds 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 Validation on perrymaster at
No version files changed. |
|
CodeRabbit's refreshed review generated no actionable comments and now passes. |
… (#9392) * fix(runtime): honor defineProperty prototype index setters * chore: key changelog fragment to PR 9339 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
|
Landed via #9392 with your commits preserved — your branch conflicts against current Worth noting why this was stuck: it depended on the 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. |
Summary
Indexed array assignments now honor inherited accessors and non-writable data
properties installed on
Array.prototypeorObject.prototypethrough thedescriptor APIs.
Changes
Object.defineProperty,Object.defineProperties, orReflect.definePropertyinstalls a canonicalindex on
Array.prototypefallback and walk only the default
Array.prototype/Object.prototypechain
index invalidation, addressing the review finding without restoring the
reverted custom-prototype path
Object.prototype, boolean arrays, setters, and non-writable descriptorsNo 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=1cargo test -p perry --test issue_5898_array_reverse_exotic -- --test-threads=1RUSTFLAGS="-D warnings" cargo check -p perry-runtime -p perry --all-targetsSKIP_COMPILE_GATES=1 scripts/run_lint_gates.sh(all 58 runnable gates passed; 2 CI-only gates skipped)cargo build --release -p perrycc --helpA/B onperrymaster(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 returnedrc=0with 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
CLAUDE.md, orCHANGELOG.mdchangesCONTRIBUTING.mdand the Code of Conduct